[Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Archie Pusaka]

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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Luiz Augusto von Dentz]

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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Archie Pusaka]

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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[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



-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Marcel Holtmann]

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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Archie Pusaka]

> > 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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Archie Pusaka]

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

Re: [Bluez PATCH v1] input: disconnect intr channel before ctrl channel

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz