[Bluez PATCH v3 1/2] device: Don't browse SDP if HIDSDPDisable is set

[Bluez PATCH v3 1/2] device: 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>
---

Changes in v3: None
Changes in v2:
* Renaming passive_sdp_discovery to refresh_discovery

 profiles/input/device.c |  3 +++
 src/device.c            | 11 +++++++++--
 src/device.h            |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 6ec0a4c63..5e47b88f2 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1373,6 +1373,9 @@ static struct input_device *input_device_new(struct btd_service *service)
 	/* Initialize device properties */
 	extract_hid_props(idev, rec);
 
+	if (idev->disable_sdp)
+		device_set_refresh_discovery(device, false);
+
 	return idev;
 }
 
diff --git a/src/device.c b/src/device.c
index 2237a7670..52dfea18f 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		refresh_discovery;
 
 	/* Manage whether this device can wake the system from suspend.
 	 * - wake_support: Requires a profile that supports wake (i.e. HID)
@@ -1472,7 +1473,6 @@ static gboolean dev_property_wake_allowed_exist(
 	return device_get_wake_support(device);
 }
 
-
 static gboolean disconnect_all(gpointer user_data)
 {
 	struct btd_device *device = user_data;
@@ -1805,7 +1805,7 @@ 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->refresh_discovery)
 			device_browse_sdp(dev, NULL);
 		g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
 	}
@@ -2572,6 +2572,11 @@ done:
 		browse_request_free(req);
 }
 
+void device_set_refresh_discovery(struct btd_device *dev, bool refresh)
+{
+	dev->refresh_discovery = refresh;
+}
+
 static void device_set_svc_refreshed(struct btd_device *device, bool value)
 {
 	if (device->svc_refreshed == value)
@@ -4071,6 +4076,8 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
 	device->db_id = gatt_db_register(device->db, gatt_service_added,
 					gatt_service_removed, device, NULL);
 
+	device->refresh_discovery = true;
+
 	return btd_device_ref(device);
 }
 
diff --git a/src/device.h b/src/device.h
index cb8d884e8..5ba2d7fe0 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_refresh_discovery(struct btd_device *dev, bool refresh);
 
 typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
 					void *user_data);
-- 
2.28.0.220.ged08abb693-goog

[Bluez PATCH v3 2/2] main: add configurable refresh_discovery parameter

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

This is to configure the default behavior of issuing SDP query
to update the services upon profile connection.
---

Changes in v3:
* Add main_opts.refresh_discovery

Changes in v2: None

 src/device.c  | 2 +-
 src/hcid.h    | 1 +
 src/main.c    | 8 ++++++++
 src/main.conf | 4 ++++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index 52dfea18f..a91eda027 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4076,7 +4076,7 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
 	device->db_id = gatt_db_register(device->db, gatt_service_added,
 					gatt_service_removed, device, NULL);
 
-	device->refresh_discovery = true;
+	device->refresh_discovery = main_opts.refresh_discovery;
 
 	return btd_device_ref(device);
 }
diff --git a/src/hcid.h b/src/hcid.h
index 5f249ebf9..3624ba6ea 100644
--- a/src/hcid.h
+++ b/src/hcid.h
@@ -100,6 +100,7 @@ struct main_opts {
 	gboolean	name_resolv;
 	gboolean	debug_keys;
 	gboolean	fast_conn;
+	gboolean	refresh_discovery;
 
 	uint16_t	did_source;
 	uint16_t	did_vendor;
diff --git a/src/main.c b/src/main.c
index ec7a9fbd7..b205575f6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -627,6 +627,13 @@ static void parse_config(GKeyFile *config)
 	else
 		main_opts.fast_conn = boolean;
 
+	boolean = g_key_file_get_boolean(config, "General",
+						"RefreshDiscovery", &err);
+	if (err)
+		g_clear_error(&err);
+	else
+		main_opts.refresh_discovery = boolean;
+
 	str = g_key_file_get_string(config, "GATT", "Cache", &err);
 	if (err) {
 		DBG("%s", err->message);
@@ -688,6 +695,7 @@ static void init_defaults(void)
 	main_opts.reverse_discovery = TRUE;
 	main_opts.name_resolv = TRUE;
 	main_opts.debug_keys = FALSE;
+	main_opts.refresh_discovery = TRUE;
 
 	main_opts.default_params.num_entries = 0;
 	main_opts.default_params.br_page_scan_type = 0xFFFF;
diff --git a/src/main.conf b/src/main.conf
index f41203b96..42f7e41c5 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -82,6 +82,10 @@
 # 0 = disable timer, i.e. never keep temporary devices
 #TemporaryTimeout = 30
 
+# Enables the device to issue an SDP request to update known services when
+# profile is connected. Defaults to true.
+#RefreshDiscovery = true
+
 [Controller]
 # The following values are used to load default adapter parameters.  BlueZ loads
 # the values into the kernel before the adapter is powered if the kernel
-- 
2.28.0.220.ged08abb693-goog

Re: [Bluez PATCH v3 1/2] device: Don't browse SDP if HIDSDPDisable is set

[Luiz Augusto von Dentz]

Hi Archie,

On Tue, Aug 18, 2020 at 12:34 AM 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>
> ---
>
> Changes in v3: None
> Changes in v2:
> * Renaming passive_sdp_discovery to refresh_discovery
>
>  profiles/input/device.c |  3 +++
>  src/device.c            | 11 +++++++++--
>  src/device.h            |  1 +
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 6ec0a4c63..5e47b88f2 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -1373,6 +1373,9 @@ static struct input_device *input_device_new(struct btd_service *service)
>         /* Initialize device properties */
>         extract_hid_props(idev, rec);
>
> +       if (idev->disable_sdp)
> +               device_set_refresh_discovery(device, false);
> +
>         return idev;
>  }
>
> diff --git a/src/device.c b/src/device.c
> index 2237a7670..52dfea18f 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            refresh_discovery;
>
>         /* Manage whether this device can wake the system from suspend.
>          * - wake_support: Requires a profile that supports wake (i.e. HID)
> @@ -1472,7 +1473,6 @@ static gboolean dev_property_wake_allowed_exist(
>         return device_get_wake_support(device);
>  }
>
> -
>  static gboolean disconnect_all(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -1805,7 +1805,7 @@ 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->refresh_discovery)
>                         device_browse_sdp(dev, NULL);
>                 g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID);
>         }
> @@ -2572,6 +2572,11 @@ done:
>                 browse_request_free(req);
>  }
>
> +void device_set_refresh_discovery(struct btd_device *dev, bool refresh)
> +{
> +       dev->refresh_discovery = refresh;
> +}
> +
>  static void device_set_svc_refreshed(struct btd_device *device, bool value)
>  {
>         if (device->svc_refreshed == value)
> @@ -4071,6 +4076,8 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
>         device->db_id = gatt_db_register(device->db, gatt_service_added,
>                                         gatt_service_removed, device, NULL);
>
> +       device->refresh_discovery = true;
> +
>         return btd_device_ref(device);
>  }
>
> diff --git a/src/device.h b/src/device.h
> index cb8d884e8..5ba2d7fe0 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_refresh_discovery(struct btd_device *dev, bool refresh);
>
>  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
>                                         void *user_data);
> --
> 2.28.0.220.ged08abb693-goog

Applied, thanks.

-- 
Luiz Augusto von Dentz