[Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

According to the HID1.1 spec, part 5.3.4.9:
The HIDSDPDisable attribute is a Boolean value, which indicates
whether connection to the SDP channel and Control or Interrupt
channels are mutually exclusive. This feature supports Bluetooth
HID devices that have minimal resources, and multiplex those
resources between servicing the initialization (SDP) and runtime
(Control and Interrupt) channels.

However, Bluez still tries to connect SDP upon HID connection,
regardless of the existence of the HIDSDPDisable attribute.

This patch prevents the connection of SDP after HID has been
established, if the device has HIDSDPDisable attribute.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---

 profiles/input/device.c | 2 ++
 src/device.c            | 8 +++++++-
 src/device.h            | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 6ec0a4c63..fac8c6896 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1373,6 +1373,8 @@ static struct input_device *input_device_new(struct btd_service *service)
 	/* Initialize device properties */
 	extract_hid_props(idev, rec);
 
+	device_set_skip_passive_sdp_discovery(device, idev->disable_sdp);
+
 	return idev;
 }
 
diff --git a/src/device.c b/src/device.c
index 2237a7670..a67787a2d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -195,6 +195,7 @@ struct btd_device {
 	bool		le;
 	bool		pending_paired;		/* "Paired" waiting for SDP */
 	bool		svc_refreshed;
+	bool		skip_passive_sdp_discovery;
 
 	/* Manage whether this device can wake the system from suspend.
 	 * - wake_support: Requires a profile that supports wake (i.e. HID)
@@ -1472,6 +1473,10 @@ static gboolean dev_property_wake_allowed_exist(
 	return device_get_wake_support(device);
 }
 
+void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip)
+{
+	dev->skip_passive_sdp_discovery = skip;
+}
 
 static gboolean disconnect_all(gpointer user_data)
 {
@@ -1805,7 +1810,8 @@ done:
 				btd_error_failed(dev->connect, strerror(-err)));
 	} else {
 		/* Start passive SDP discovery to update known services */
-		if (dev->bredr && !dev->svc_refreshed)
+		if (dev->bredr && !dev->svc_refreshed &&
+					!dev->skip_passive_sdp_discovery)
 			device_browse_sdp(dev, NULL);
 		g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
 	}
diff --git a/src/device.h b/src/device.h
index cb8d884e8..5348d2652 100644
--- a/src/device.h
+++ b/src/device.h
@@ -145,6 +145,7 @@ void device_set_wake_override(struct btd_device *device, bool wake_override);
 void device_set_wake_allowed(struct btd_device *device, bool wake_allowed,
 			     guint32 id);
 void device_set_wake_allowed_complete(struct btd_device *device);
+void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip);
 
 typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
 					void *user_data);
-- 
2.28.0.236.gb10cc79966-goog

Re: [Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Marcel Holtmann]

Hi Archie,

> According to the HID1.1 spec, part 5.3.4.9:
> The HIDSDPDisable attribute is a Boolean value, which indicates
> whether connection to the SDP channel and Control or Interrupt
> channels are mutually exclusive. This feature supports Bluetooth
> HID devices that have minimal resources, and multiplex those
> resources between servicing the initialization (SDP) and runtime
> (Control and Interrupt) channels.
> 
> However, Bluez still tries to connect SDP upon HID connection,
> regardless of the existence of the HIDSDPDisable attribute.
> 
> This patch prevents the connection of SDP after HID has been
> established, if the device has HIDSDPDisable attribute.

out of curiosity, is a qualification test failing or do you have devices that really enforce the Disable SDP part of the specification. A long long long time ago (we are talking 15+ years) some HID devices only allowed 2 L2CAP channels to be open at the same time. So the PSM 1 for SDP needed to be closed before opening the second HID data channel.

If this is failing devices, it would be good to include the btmon trace of failure in the commit message so that we archive this.

Regards

Marcel

Re: [Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Luiz Augusto von Dentz]

Hi Archie,

On Tue, Aug 11, 2020 at 9:21 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> According to the HID1.1 spec, part 5.3.4.9:
> The HIDSDPDisable attribute is a Boolean value, which indicates
> whether connection to the SDP channel and Control or Interrupt
> channels are mutually exclusive. This feature supports Bluetooth
> HID devices that have minimal resources, and multiplex those
> resources between servicing the initialization (SDP) and runtime
> (Control and Interrupt) channels.
>
> However, Bluez still tries to connect SDP upon HID connection,
> regardless of the existence of the HIDSDPDisable attribute.
>
> This patch prevents the connection of SDP after HID has been
> established, if the device has HIDSDPDisable attribute.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
>
>  profiles/input/device.c | 2 ++
>  src/device.c            | 8 +++++++-
>  src/device.h            | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 6ec0a4c63..fac8c6896 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -1373,6 +1373,8 @@ static struct input_device *input_device_new(struct btd_service *service)
>         /* Initialize device properties */
>         extract_hid_props(idev, rec);
>
> +       device_set_skip_passive_sdp_discovery(device, idev->disable_sdp);

Shouldn't you actually be checking for the presence of HIDSDPDisable,
I suppose the first time when you pair with it the SDP must be active
in order for us to be able to probe the drivers, then once we get the
SDP records stored we should inhibit the refresh of the records.

>         return idev;
>  }
>
> diff --git a/src/device.c b/src/device.c
> index 2237a7670..a67787a2d 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -195,6 +195,7 @@ struct btd_device {
>         bool            le;
>         bool            pending_paired;         /* "Paired" waiting for SDP */
>         bool            svc_refreshed;
> +       bool            skip_passive_sdp_discovery;
>
>         /* Manage whether this device can wake the system from suspend.
>          * - wake_support: Requires a profile that supports wake (i.e. HID)
> @@ -1472,6 +1473,10 @@ static gboolean dev_property_wake_allowed_exist(
>         return device_get_wake_support(device);
>  }
>
> +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip)
> +{
> +       dev->skip_passive_sdp_discovery = skip;
> +}
>
>  static gboolean disconnect_all(gpointer user_data)
>  {
> @@ -1805,7 +1810,8 @@ done:
>                                 btd_error_failed(dev->connect, strerror(-err)));
>         } else {
>                 /* Start passive SDP discovery to update known services */
> -               if (dev->bredr && !dev->svc_refreshed)
> +               if (dev->bredr && !dev->svc_refreshed &&
> +                                       !dev->skip_passive_sdp_discovery)
>                         device_browse_sdp(dev, NULL);
>                 g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
>         }
> diff --git a/src/device.h b/src/device.h
> index cb8d884e8..5348d2652 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -145,6 +145,7 @@ void device_set_wake_override(struct btd_device *device, bool wake_override);
>  void device_set_wake_allowed(struct btd_device *device, bool wake_allowed,
>                              guint32 id);
>  void device_set_wake_allowed_complete(struct btd_device *device);
> +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip);
>
>  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
>                                         void *user_data);
> --
> 2.28.0.236.gb10cc79966-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Archie Pusaka]

Hi Luiz,


On Sat, 15 Aug 2020 at 02:59, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Aug 11, 2020 at 9:21 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > According to the HID1.1 spec, part 5.3.4.9:
> > The HIDSDPDisable attribute is a Boolean value, which indicates
> > whether connection to the SDP channel and Control or Interrupt
> > channels are mutually exclusive. This feature supports Bluetooth
> > HID devices that have minimal resources, and multiplex those
> > resources between servicing the initialization (SDP) and runtime
> > (Control and Interrupt) channels.
> >
> > However, Bluez still tries to connect SDP upon HID connection,
> > regardless of the existence of the HIDSDPDisable attribute.
> >
> > This patch prevents the connection of SDP after HID has been
> > established, if the device has HIDSDPDisable attribute.
> >
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > ---
> >
> >  profiles/input/device.c | 2 ++
> >  src/device.c            | 8 +++++++-
> >  src/device.h            | 1 +
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/profiles/input/device.c b/profiles/input/device.c
> > index 6ec0a4c63..fac8c6896 100644
> > --- a/profiles/input/device.c
> > +++ b/profiles/input/device.c
> > @@ -1373,6 +1373,8 @@ static struct input_device *input_device_new(struct btd_service *service)
> >         /* Initialize device properties */
> >         extract_hid_props(idev, rec);
> >
> > +       device_set_skip_passive_sdp_discovery(device, idev->disable_sdp);
>
> Shouldn't you actually be checking for the presence of HIDSDPDisable,

Yes, the variable idev->disable_sdp stores the presence of
HIDSDPDisable attribute.

> I suppose the first time when you pair with it the SDP must be active
> in order for us to be able to probe the drivers, then once we get the
> SDP records stored we should inhibit the refresh of the records.

Correct, the first time we pair the device, SDP is still active as
usual. The additional check is only applied when refreshing the SDP
records.

>
> >         return idev;
> >  }
> >
> > diff --git a/src/device.c b/src/device.c
> > index 2237a7670..a67787a2d 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -195,6 +195,7 @@ struct btd_device {
> >         bool            le;
> >         bool            pending_paired;         /* "Paired" waiting for SDP */
> >         bool            svc_refreshed;
> > +       bool            skip_passive_sdp_discovery;
> >
> >         /* Manage whether this device can wake the system from suspend.
> >          * - wake_support: Requires a profile that supports wake (i.e. HID)
> > @@ -1472,6 +1473,10 @@ static gboolean dev_property_wake_allowed_exist(
> >         return device_get_wake_support(device);
> >  }
> >
> > +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip)
> > +{
> > +       dev->skip_passive_sdp_discovery = skip;
> > +}
> >
> >  static gboolean disconnect_all(gpointer user_data)
> >  {
> > @@ -1805,7 +1810,8 @@ done:
> >                                 btd_error_failed(dev->connect, strerror(-err)));
> >         } else {
> >                 /* Start passive SDP discovery to update known services */
> > -               if (dev->bredr && !dev->svc_refreshed)
> > +               if (dev->bredr && !dev->svc_refreshed &&
> > +                                       !dev->skip_passive_sdp_discovery)
> >                         device_browse_sdp(dev, NULL);
> >                 g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
> >         }
> > diff --git a/src/device.h b/src/device.h
> > index cb8d884e8..5348d2652 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -145,6 +145,7 @@ void device_set_wake_override(struct btd_device *device, bool wake_override);
> >  void device_set_wake_allowed(struct btd_device *device, bool wake_allowed,
> >                              guint32 id);
> >  void device_set_wake_allowed_complete(struct btd_device *device);
> > +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip);
> >
> >  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> >                                         void *user_data);
> > --
> > 2.28.0.236.gb10cc79966-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

Re: [Bluez PATCH v1] input: Don't browse SDP if HIDSDPDisable is set

[Luiz Augusto von Dentz]

Hi Archie,

On Mon, Aug 17, 2020 at 12:23 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
>
> On Sat, 15 Aug 2020 at 02:59, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Aug 11, 2020 at 9:21 PM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > According to the HID1.1 spec, part 5.3.4.9:
> > > The HIDSDPDisable attribute is a Boolean value, which indicates
> > > whether connection to the SDP channel and Control or Interrupt
> > > channels are mutually exclusive. This feature supports Bluetooth
> > > HID devices that have minimal resources, and multiplex those
> > > resources between servicing the initialization (SDP) and runtime
> > > (Control and Interrupt) channels.
> > >
> > > However, Bluez still tries to connect SDP upon HID connection,
> > > regardless of the existence of the HIDSDPDisable attribute.
> > >
> > > This patch prevents the connection of SDP after HID has been
> > > established, if the device has HIDSDPDisable attribute.
> > >
> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > > ---
> > >
> > >  profiles/input/device.c | 2 ++
> > >  src/device.c            | 8 +++++++-
> > >  src/device.h            | 1 +
> > >  3 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/profiles/input/device.c b/profiles/input/device.c
> > > index 6ec0a4c63..fac8c6896 100644
> > > --- a/profiles/input/device.c
> > > +++ b/profiles/input/device.c
> > > @@ -1373,6 +1373,8 @@ static struct input_device *input_device_new(struct btd_service *service)
> > >         /* Initialize device properties */
> > >         extract_hid_props(idev, rec);
> > >
> > > +       device_set_skip_passive_sdp_discovery(device, idev->disable_sdp);
> >
> > Shouldn't you actually be checking for the presence of HIDSDPDisable,
>
> Yes, the variable idev->disable_sdp stores the presence of
> HIDSDPDisable attribute.

I didn't realize it was already being stored, now it makes sense.

> > I suppose the first time when you pair with it the SDP must be active
> > in order for us to be able to probe the drivers, then once we get the
> > SDP records stored we should inhibit the refresh of the records.
>
> Correct, the first time we pair the device, SDP is still active as
> usual. The additional check is only applied when refreshing the SDP
> records.
>
> >
> > >         return idev;
> > >  }
> > >
> > > diff --git a/src/device.c b/src/device.c
> > > index 2237a7670..a67787a2d 100644
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -195,6 +195,7 @@ struct btd_device {
> > >         bool            le;
> > >         bool            pending_paired;         /* "Paired" waiting for SDP */
> > >         bool            svc_refreshed;
> > > +       bool            skip_passive_sdp_discovery;

Let's have it as refresh_discovery and I'd add support for setting it
globally on main.conf so  we can initialize it with
main_opts.refresh_discovery so we are consistent with the terminology
we have been using.

> > >
> > >         /* Manage whether this device can wake the system from suspend.
> > >          * - wake_support: Requires a profile that supports wake (i.e. HID)
> > > @@ -1472,6 +1473,10 @@ static gboolean dev_property_wake_allowed_exist(
> > >         return device_get_wake_support(device);
> > >  }
> > >
> > > +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip)
> > > +{
> > > +       dev->skip_passive_sdp_discovery = skip;
> > > +}
> > >
> > >  static gboolean disconnect_all(gpointer user_data)
> > >  {
> > > @@ -1805,7 +1810,8 @@ done:
> > >                                 btd_error_failed(dev->connect, strerror(-err)));
> > >         } else {
> > >                 /* Start passive SDP discovery to update known services */
> > > -               if (dev->bredr && !dev->svc_refreshed)
> > > +               if (dev->bredr && !dev->svc_refreshed &&
> > > +                                       !dev->skip_passive_sdp_discovery)
> > >                         device_browse_sdp(dev, NULL);

Then here just check for dev->refresh_discovery.

> > >                 g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
> > >         }
> > > diff --git a/src/device.h b/src/device.h
> > > index cb8d884e8..5348d2652 100644
> > > --- a/src/device.h
> > > +++ b/src/device.h
> > > @@ -145,6 +145,7 @@ void device_set_wake_override(struct btd_device *device, bool wake_override);
> > >  void device_set_wake_allowed(struct btd_device *device, bool wake_allowed,
> > >                              guint32 id);
> > >  void device_set_wake_allowed_complete(struct btd_device *device);
> > > +void device_set_skip_passive_sdp_discovery(struct btd_device *dev, bool skip);

And here we name it device_set_refresh_discovery so plugins can
actually set if the device should have refresh_discovery
enabled/disabled.

> > >
> > >  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> > >                                         void *user_data);
> > > --
> > > 2.28.0.236.gb10cc79966-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie



-- 
Luiz Augusto von Dentz