Bluetooth: GATT DM: Schedule host cbs in work q

Moves host calls in discovery manager callbacks to be scheduled in
either a GATT DM-specific work queue or the system work queue depending
on a new Kconfig option `BT_GATT_DM_WORKQ_CHOICE`. This is done to
improve robustness and avoid possible concurrency issues.

Using the system work queue will not increase the memory footprint, but
is marked as experimental as the callbacks can potentially block, which
is not recommended in the syswq.

Signed-off-by: Håvard Reierstad <[email protected]>

Author: HaavardRei

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

@@ -191,6 +191,10 @@ Bluetooth® LE
   The SoftDevice Controller driver uses a devicetree node named ``bt_hci_sdc`` with a devicetree binding compatible with ``nordic,bt-hci-sdc``.
   The Zephyr Bluetooth LE Controller uses a devicetree node named ``bt_hci_controller`` with a devicetree binding compatible with ``zephyr,bt-hci-ll-sw-split``.
   Applications using the Zephyr Bluetooth Controller need to be updated (see the :ref:`migration guide <migration_2.8>`).
+* Host calls in GATT Discovery Manager (DM) callbacks are now scheduled in a workqueue.
+  The :kconfig:option:`BT_GATT_DM_WORKQ_CHOICE` Kconfig option allows you to select the workqueue implementation.
+  You can select either a workqueue specific to GATT DM (default) or the system workqueue.
+  You can use the system workqueue if creating a new thread is not viable due to memory constraints, but it is not recommended to have potential blocking calls in the system workqueue.
 
 Bluetooth Mesh
 --------------

subsys/bluetooth/Kconfig.discovery

@@ -11,6 +11,51 @@ menuconfig BT_GATT_DM
 
 if BT_GATT_DM
 
+choice BT_GATT_DM_WORKQ_CHOICE
+	prompt "GATT Discovery Manager workqueue selection"
+	default BT_GATT_DM_WORKQ_OWN
+	help
+	  Select the workqueue implementation for GATT Discovery Manager.
+
+config BT_GATT_DM_WORKQ_OWN
+	bool "Use own workqueue"
+	help
+	  Use own workqueue for GATT Discovery Manager.
+
+config BT_GATT_DM_WORKQ_SYS
+	bool "Use system workqueue"
+	select EXPERIMENTAL
+	help
+	  Use system workqueue for GATT Discovery Manager. Can be used if creating a new thread
+	  is not viable due to memory constraints, but it is not recommended to have potential
+	  blocking calls in the system workqueue.
+
+endchoice
+
+if BT_GATT_DM_WORKQ_OWN
+
+config BT_GATT_DM_WORKQ_STACK_SIZE
+	# Hidden option for workqueue stack size. Should be derived from system
+	# requirements.
+	int
+	default 1300 if BT_GATT_CACHING
+	default 1140 if BT_TINYCRYPT_ECC
+	default 1024
+
+config BT_GATT_DM_WORKQ_PRIO
+	int "GATT Discovery Manager workqueue priority."
+	default 10
+	help
+	  Priority level for the GATT Discovery Manager workqueue.
+
+config BT_GATT_DM_WORKQ_INIT_PRIO
+	int "GATT Discovery Manager workqueue init priority"
+	default 50
+	help
+	  Init priority level to setup the GATT Discovery Manager workqueue.
+
+endif # BT_GATT_DM_WORKQ_OWN
+
 config BT_GATT_DM_MAX_ATTRS
 	int "Maximum number of attributes that can be present in the discovered service"
 	default 35

subsys/bluetooth/gatt_dm.c

@@ -22,6 +22,24 @@ LOG_MODULE_REGISTER(bt_gatt_dm, CONFIG_BT_GATT_DM_LOG_LEVEL);
 BUILD_ASSERT(sizeof(struct bt_gatt_service_val) % DATA_ALIGN == 0);
 BUILD_ASSERT(sizeof(struct bt_gatt_chrc) % DATA_ALIGN == 0);
 
+#if defined(CONFIG_BT_GATT_DM_WORKQ_OWN)
+K_THREAD_STACK_DEFINE(bt_gatt_dm_wq_stack_area, CONFIG_BT_GATT_DM_WORKQ_STACK_SIZE);
+static struct k_work_q bt_gatt_dm_wq;
+
+static int gatt_dm_wq_init(void)
+{
+	const struct k_work_queue_config cfg = {.name = "BT GATT DM WQ"};
+
+	k_work_queue_init(&bt_gatt_dm_wq);
+	k_work_queue_start(&bt_gatt_dm_wq, bt_gatt_dm_wq_stack_area,
+			   K_THREAD_STACK_SIZEOF(bt_gatt_dm_wq_stack_area),
+			   CONFIG_BT_GATT_DM_WORKQ_PRIO, &cfg);
+	return 0;
+}
+
+SYS_INIT(gatt_dm_wq_init, POST_KERNEL, CONFIG_BT_GATT_DM_WORKQ_INIT_PRIO);
+#endif
+
 /* Flags for parsed attribute array state */
 enum {
 	STATE_ATTRS_LOCKED,
@@ -71,6 +89,9 @@ struct bt_gatt_dm {
 
 	/* Indicates that services should be searched by the UUID. */
 	bool search_svc_by_uuid;
+
+	/* Work item used for discovery callbacks. */
+	struct k_work discover_work;
 };
 
 /* Currently only one instance is supported */
@@ -277,12 +298,27 @@ static void discovery_complete_error(struct bt_gatt_dm *dm, int err)
 	}
 }
 
+static void gatt_discover_work(struct k_work *work)
+{
+	struct bt_gatt_dm *dm = CONTAINER_OF(work, struct bt_gatt_dm, discover_work);
+
+	if (!atomic_test_bit(dm->state_flags, STATE_ATTRS_LOCKED)) {
+		LOG_WRN("Attributes not locked");
+		return;
+	}
+
+	int err = bt_gatt_discover(dm->conn, &(dm->discover_params));
+
+	if (err) {
+		LOG_ERR("GATT discover failed, error: %d.", err);
+		discovery_complete_error(dm, -ENOMEM);
+	}
+}
+
 static uint8_t discovery_process_service(struct bt_gatt_dm *dm,
 				      const struct bt_gatt_attr *attr,
 				      struct bt_gatt_discover_params *params)
 {
-	int err;
-
 	if (!attr) {
 		discovery_complete_not_found(dm);
 		return BT_GATT_ITER_STOP;
@@ -336,13 +372,12 @@ static uint8_t discovery_process_service(struct bt_gatt_dm *dm,
 	dm->discover_params.type         = BT_GATT_DISCOVER_ATTRIBUTE;
 	dm->discover_params.start_handle = cur_attr->handle + 1;
 	LOG_DBG("Starting descriptors discovery");
-	err = bt_gatt_discover(dm->conn, &(dm->discover_params));
 
-	if (err) {
-		LOG_ERR("Descriptor discover failed, error: %d.", err);
-		discovery_complete_error(dm, -ENOMEM);
-		return BT_GATT_ITER_STOP;
-	}
+#if defined(CONFIG_BT_GATT_DM_WORKQ_OWN)
+	k_work_submit_to_queue(&bt_gatt_dm_wq, &dm->discover_work);
+#else
+	k_work_submit(&dm->discover_work);
+#endif
 
 	return BT_GATT_ITER_STOP;
 }
@@ -360,15 +395,12 @@ static uint8_t discovery_process_attribute(struct bt_gatt_dm *dm,
 				dm->attrs[0].handle + 1;
 			dm->discover_params.type =
 				BT_GATT_DISCOVER_CHARACTERISTIC;
-			int err = bt_gatt_discover(dm->conn,
-						   &(dm->discover_params));
-
-			if (err) {
-				LOG_ERR("Characteristic discover failed,"
-					" error: %d.",
-					err);
-				discovery_complete_error(dm, err);
-			}
+
+#if defined(CONFIG_BT_GATT_DM_WORKQ_OWN)
+			k_work_submit_to_queue(&bt_gatt_dm_wq, &dm->discover_work);
+#else
+			k_work_submit(&dm->discover_work);
+#endif
 		} else {
 			discovery_complete(dm);
 		}
@@ -645,6 +677,7 @@ int bt_gatt_dm_start(struct bt_conn *conn,
 	dm->discover_params.start_handle = 0x0001;
 	dm->discover_params.end_handle = 0xffff;
 	dm->discover_params.type = BT_GATT_DISCOVER_PRIMARY;
+	k_work_init(&dm->discover_work, gatt_discover_work);
 
 	err = bt_gatt_discover(conn, &dm->discover_params);
 	if (err) {
@@ -706,6 +739,7 @@ int bt_gatt_dm_data_release(struct bt_gatt_dm *dm)
 		return -EALREADY;
 	}
 
+	k_work_cancel(&dm->discover_work);
 	svc_attr_memory_release(dm);
 	atomic_clear_bit(dm->state_flags, STATE_ATTRS_LOCKED);