From 4c9ee14572313ae2c9b2117dffdd210633c6cbc3 Mon Sep 17 00:00:00 2001 From: Pavel Vasilyev Date: Thu, 1 Feb 2024 11:38:18 +0100 Subject: [PATCH] Bluetooth: Mesh: Allow to suspend mesh from bt_mesh_send_cb callbacks This commit allows to suspend the mesh stack from `bt_mesh_send_cb` callbacks by removing the deadlock caused by `k_work_flush` in the extended advertiser. In case of the extended advertiser there are 2 cases: - when the `bt_mesh_adv_disable` is called from any of `bt_mesh_send_cb` callbacks which are called from the advertiser work item, or - when it is called from any other context. When it is called from `bt_mesh_send_cb` callbacks, since these callbacks are called from the delayable work which is running on the system workqueue, the advertiser can check the current context and its work state. If the function is called from the advertiser work, it can disable the advertising set straight away because all ble host APIs have already been called in `adv_start` function. Before sending anything else, the advertiser checks the `instance` value in `adv_start` function, which is also reset to NULL in `bt_mesh_adv_disable` call, and aborts all next advertisements. The `ADV_FLAG_SUSPENDING` tells the advertiser work to abort processing while `bt_mesh_adv_disable` function didn't finish stopping advertising set. This can happen if the work has been already scheduled and the schedler ran it while sleeping inside the `bt_le_ext_adv_stop` or `bt_le_ext_adv_disable` functions. When `bt_mesh_adv_disable` is called from any other context or from the system workqueue but not from the advertiser work, then `k_work_flush` can be called safely as it won't cause any deadlocks. The `adv_sent` function is inside the `bt_mesh_adv_disable` function to schedule the advertiser work (`send_pending_adv`) and abort all pending advertisements that have been already added to the pool. In case of the legacy advertiser, if the `bt_mesh_adv_disable` is called form the advertiser thread (this happens when it is called from `bt_mesh_send_cb.start` or `bt_mesh_send_cb.end` callbacks), then `k_thread_join` returns `-EDEADLK`. But the `enabled` flag is set to false and the thread will abort the current advertisement and the pending advertisements. Signed-off-by: Pavel Vasilyev --- include/zephyr/bluetooth/mesh/main.h | 4 ---- subsys/bluetooth/mesh/adv.h | 1 - subsys/bluetooth/mesh/adv_ext.c | 23 ++++++++++++++++++++++- subsys/bluetooth/mesh/adv_legacy.c | 12 +++++++++--- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/include/zephyr/bluetooth/mesh/main.h b/include/zephyr/bluetooth/mesh/main.h index 1622ccebbd8..dfcd1d0bd7c 100644 --- a/include/zephyr/bluetooth/mesh/main.h +++ b/include/zephyr/bluetooth/mesh/main.h @@ -607,10 +607,6 @@ void bt_mesh_reset(void); * If at all possible, the Friendship feature should be used instead, to * make the node into a Low Power Node. * - * @note Should not be called from work queue due to undefined behavior. - * This is due to k_work_flush_delayable() being used in disabling of the - * extended advertising. - * * @return 0 on success, or (negative) error code on failure. */ int bt_mesh_suspend(void); diff --git a/subsys/bluetooth/mesh/adv.h b/subsys/bluetooth/mesh/adv.h index 6595badc9f9..da86f697c2d 100644 --- a/subsys/bluetooth/mesh/adv.h +++ b/subsys/bluetooth/mesh/adv.h @@ -94,7 +94,6 @@ int bt_mesh_scan_disable(void); int bt_mesh_adv_enable(void); -/* Should not be called from work queue due to undefined behavior */ int bt_mesh_adv_disable(void); void bt_mesh_adv_local_ready(void); diff --git a/subsys/bluetooth/mesh/adv_ext.c b/subsys/bluetooth/mesh/adv_ext.c index 5e428283bf9..f6367a05258 100644 --- a/subsys/bluetooth/mesh/adv_ext.c +++ b/subsys/bluetooth/mesh/adv_ext.c @@ -49,6 +49,9 @@ enum { */ ADV_FLAG_UPDATE_PARAMS, + /** The advertiser is suspending. */ + ADV_FLAG_SUSPENDING, + /* Number of adv flags. */ ADV_FLAGS_NUM }; @@ -246,6 +249,11 @@ static void send_pending_adv(struct k_work *work) ext_adv = CONTAINER_OF(work, struct bt_mesh_ext_adv, work); + if (atomic_test_bit(ext_adv->flags, ADV_FLAG_SUSPENDING)) { + LOG_DBG("Advertiser is suspending"); + return; + } + if (atomic_test_and_clear_bit(ext_adv->flags, ADV_FLAG_SENT)) { LOG_DBG("Advertising stopped after %u ms for %s", k_uptime_get_32() - ext_adv->timestamp, @@ -284,6 +292,11 @@ static void send_pending_adv(struct k_work *work) } } + if (ext_adv->instance == NULL) { + LOG_DBG("Advertiser is suspended or deleted"); + return; + } + if (IS_ENABLED(CONFIG_BT_MESH_PROXY_SOLICITATION) && !bt_mesh_sol_send()) { return; @@ -488,7 +501,12 @@ int bt_mesh_adv_disable(void) struct k_work_sync sync; for (int i = 0; i < ARRAY_SIZE(advs); i++) { - k_work_flush(&advs[i].work, &sync); + atomic_set_bit(advs[i].flags, ADV_FLAG_SUSPENDING); + + if (k_current_get() != &k_sys_work_q.thread || + (k_work_busy_get(&advs[i].work) & K_WORK_RUNNING) == 0) { + k_work_flush(&advs[i].work, &sync); + } err = bt_le_ext_adv_stop(advs[i].instance); if (err) { @@ -506,7 +524,10 @@ int bt_mesh_adv_disable(void) LOG_ERR("Failed to delete adv %d", err); return err; } + advs[i].instance = NULL; + + atomic_clear_bit(advs[i].flags, ADV_FLAG_SUSPENDING); } return 0; diff --git a/subsys/bluetooth/mesh/adv_legacy.c b/subsys/bluetooth/mesh/adv_legacy.c index 867c91bbf8e..2e60ff423c1 100644 --- a/subsys/bluetooth/mesh/adv_legacy.c +++ b/subsys/bluetooth/mesh/adv_legacy.c @@ -104,7 +104,9 @@ static int bt_data_send(uint8_t num_events, uint16_t adv_int, bt_mesh_adv_send_start(duration, err, ctx); } - k_sleep(K_MSEC(duration)); + if (enabled) { + k_sleep(K_MSEC(duration)); + } err = bt_le_adv_stop(); if (err) { @@ -239,9 +241,13 @@ int bt_mesh_adv_enable(void) int bt_mesh_adv_disable(void) { + int err; + enabled = false; - k_thread_join(&adv_thread_data, K_FOREVER); - LOG_DBG("Advertising disabled"); + + err = k_thread_join(&adv_thread_data, K_FOREVER); + LOG_DBG("Advertising disabled: %d", err); + return 0; }