| Message ID | 20200316123914.Bluez.v1.1.I2c83372de789a015c1ee506690bb795ee0b0b0d9@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [Bluez,v1] input: disconnect intr channel before ctrl channel | expand |
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.
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? 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
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
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. in a blocking synchronous system call world we have SO_LINGER for that. In the world of asynchronous IO handling (what we do), we need to check what is the right way of handling this. Regards Marcel
> > 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. I don't think the second parameter matters, I tried every possible valid values and intr_watch_cb is still called without waiting for the response. > > in a blocking synchronous system call world we have SO_LINGER for that. In the world of asynchronous IO handling (what we do), we need to check what is the right way of handling this. > I spot this piece of code [1] which utilizes getsockopt to query socket connection information from the kernel space to the user space. We can use a similar method to query whether (sk->sk_state == BT_CLOSED), which is only true when we get the response. What do you think? [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/l2cap_sock.c#n476
Hi Marcel, Luiz, I found out that shutdown second parameter is passed as the "how" parameter in l2cap_sock_shutdown() [1]. Currently the value of the parameter is unused, but I think we can assign it to sk->sk_shutdown. Therefore, we can differentiate whether we are interested to wait for the disconnection reply or not, by supplying SHUT_RDWR and SHUT_WR, respectively. Do you think this is a sound idea? [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/l2cap_sock.c#n1267 Thanks, Archie On Wed, 18 Mar 2020 at 20:41, Archie Pusaka <apusaka@google.com> wrote: > > > > 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. > > I don't think the second parameter matters, I tried every possible > valid values and intr_watch_cb is still called without waiting for the > response. > > > > > in a blocking synchronous system call world we have SO_LINGER for that. In the world of asynchronous IO handling (what we do), we need to check what is the right way of handling this. > > > > I spot this piece of code [1] which utilizes getsockopt to query > socket connection information from the kernel space to the user space. > We can use a similar method to query whether (sk->sk_state == > BT_CLOSED), which is only true when we get the response. > What do you think? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/l2cap_sock.c#n476
Hi Archie, On Mon, Apr 13, 2020 at 6:48 AM Archie Pusaka <apusaka@google.com> wrote: > > Hi Marcel, Luiz, > > I found out that shutdown second parameter is passed as the "how" > parameter in l2cap_sock_shutdown() [1]. > Currently the value of the parameter is unused, but I think we can > assign it to sk->sk_shutdown. Therefore, we can differentiate whether > we are interested to wait for the disconnection reply or not, by > supplying SHUT_RDWR and SHUT_WR, respectively. > > Do you think this is a sound idea? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/l2cap_sock.c#n1267 That was what I meant on my reply, that way we can detect if the has been shutdown on both directions or not, so in case of SHUT_WR it shall on indicate POLL_HUP if the userspace is polling with POLL_OUT, thus POLL_IN would still wait for the disconnect to complete, I guess this is worth the shot since this does not introduce any new APIs, but we might want to introduce a test to l2cap-tester to cover this functionality. > Thanks, > Archie > > On Wed, 18 Mar 2020 at 20:41, Archie Pusaka <apusaka@google.com> wrote: > > > > > > 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. > > > > I don't think the second parameter matters, I tried every possible > > valid values and intr_watch_cb is still called without waiting for the > > response. > > > > > > > > in a blocking synchronous system call world we have SO_LINGER for that. In the world of asynchronous IO handling (what we do), we need to check what is the right way of handling this. > > > > > > > I spot this piece of code [1] which utilizes getsockopt to query > > socket connection information from the kernel space to the user space. > > We can use a similar method to query whether (sk->sk_state == > > BT_CLOSED), which is only true when we get the response. > > What do you think? > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/l2cap_sock.c#n476
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;
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(-)