Hi Archie,
On Mon, Aug 17, 2020 at 12:23 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> Hi Luiz,
>
>
> On Sat, 15 Aug 2020 at 02:59, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Aug 11, 2020 at 9:21 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> > >
> > > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > ---
> > >
> > > 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
