Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200316123914.Bluez.v1.1.I2c83372de789a015c1ee506690bb795ee0b0b0d9@changeid>
 <CABBYNZL1D44M4z2c+4zbjLgZ=PMHsTwy1VpYZpsJbpFJ9POZug@mail.gmail.com> <CAJQfnxEFM81DSvZb+ULG7+nwQuy-GWj26GWn4OP+Bp--24N1CQ@mail.gmail.com>
In-Reply-To: <CAJQfnxEFM81DSvZb+ULG7+nwQuy-GWj26GWn4OP+Bp--24N1CQ@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 17 Mar 2020 14:31:11 -0700
Message-ID: <CABBYNZ+=wMNuvjJ5VK2zpd6euqeEZ8JXbd67RmPgxDfdCgce9w@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel
To:     Archie Pusaka <apusaka@google.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Archie,

On Mon, Mar 16, 2020 at 11:53 PM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> Luckily you asked, because I found out that actually the patch didn't
> wait for the disconnection response for any case. It does delay the
> disconnection of the ctrl channel slightly though, but that doesn't
> guarantee a proper order of disconnection. Therefore, as of now, this
> patch is not fixing anything.
>
> Digging more into this matter, I found out by removing this condition
> (sk->sk_shutdown == SHUTDOWN_MASK) in [1], it makes intr_watch_cb()
> called after truly receiving the interrupt disconnection response.
> However, I haven't checked whether removal of such condition will
> break other things.
> Do you have any suggestions?

I see, we shutdown the socket immediately since the socket API itself
don't seem to have a concept of disconnect syscall not sure if other
values could be passed to shutdown second parameter to indicate we
want to actually wait it to be disconnected.

> With this patch and removal of that condition, I confirm that it works
> with situations where the device is being removed and/or just being
> disconnected. It also works with virtual cable unplug when UHID is
> used.
> * Virtual cable unplug has another problem which doesn't adhere to the
> specs, but it is unrelated to disconnection.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/af_bluetooth.c#n470
>
> Thanks,
> Archie
>
> On Tue, 17 Mar 2020 at 04:58, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Sun, Mar 15, 2020 at 9:40 PM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > According to bluetooth HID Profile spec Ver 1.0, section 7.2.2, A
> > > host or device shall always complete the disconnection of the
> > > interrupt channel before disconnecting the control channel.
> > > However, the current implementation disconnects them both
> > > simultaneously.
> > >
> > > This patch postpone the disconnection of control channel to the
> > > callback of interrupt watch, which shall be called upon receiving
> > > interrupt channel disconnection response.
> > > ---
> > >
> > >  profiles/input/device.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/profiles/input/device.c b/profiles/input/device.c
> > > index 8ada1b4ff..8ef3714c9 100644
> > > --- a/profiles/input/device.c
> > > +++ b/profiles/input/device.c
> > > @@ -1010,14 +1010,19 @@ static bool is_connected(struct input_device *idev)
> > >
> > >  static int connection_disconnect(struct input_device *idev, uint32_t flags)
> > >  {
> > > +       int sock;
> > > +
> > >         if (!is_connected(idev))
> > >                 return -ENOTCONN;
> > >
> > > -       /* Standard HID disconnect */
> > > -       if (idev->intr_io)
> > > -               g_io_channel_shutdown(idev->intr_io, TRUE, NULL);
> > > -       if (idev->ctrl_io)
> > > -               g_io_channel_shutdown(idev->ctrl_io, TRUE, NULL);
> > > +       /* Standard HID disconnect
> > > +        * Intr channel must be disconnected before ctrl channel, so only
> > > +        * disconnect intr here, ctrl is disconnected in intr_watch_cb.
> > > +        */
> > > +       if (idev->intr_io) {
> > > +               sock = g_io_channel_unix_get_fd(idev->intr_io);
> > > +               shutdown(sock, SHUT_RDWR);
> > > +       }
> > >
> > >         if (idev->uhid)
> > >                 return 0;
> > > --
> > > 2.25.1.481.gfbce0eb801-goog
> >
> > Just to confirm, have you checked if this works with both situation
> > where the device is being removed or just being disconnected,
> > specially the case of HIDP_CTRL_VIRTUAL_CABLE_UNPLUG which perhaps was
> > not working before as well since we disconnect the ctrl channel before
> > transmitting it.
> >
> > --
> > Luiz Augusto von Dentz


-- 
Luiz Augusto von Dentz