Hi Archie, On Mon, Mar 16, 2020 at 11:53 PM Archie Pusaka <apusaka@xxxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > Hi Archie, > > > > On Sun, Mar 15, 2020 at 9:40 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote: > > > > > > From: Archie Pusaka <apusaka@xxxxxxxxxxxx> > > > > > > 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
