Bluetooth: Controller: Fix assert on role stop/abort
Call to ticker_stop/update can fail under the condition
where in a role is being stopped but at the same time it is
preempted by the role event that also uses ticker_stop/
update.
Also if a role closes graceful while it is being stopped,
the radio ISR will process the stop state with no active
role at that instance in time. In this case just reset the
state to none, the role has already been gracefully closed
before this ISR execution. The above applies to aborting a
role event too.
This commit adds code to detect these conditions and
deterministically recover from it.
This commit fixes the assert observed while stopping
advertiser in the Bluetooth sample scan_adv.
Jira: ZEP-1852
Change-id: I51c8d6e212ef43e3526a199cf7b666a79729c732
Signed-off-by: Vinayak Chettimada <vinayak.kariappa.chettimada@nordicsemi.no>
diff --git a/subsys/bluetooth/controller/ll/ctrl.c b/subsys/bluetooth/controller/ll/ctrl.c
index 687d053..d65f302 100644
--- a/subsys/bluetooth/controller/ll/ctrl.c
+++ b/subsys/bluetooth/controller/ll/ctrl.c
@@ -134,6 +134,8 @@
uint8_t volatile ticker_id_prepare;
uint8_t volatile ticker_id_event;
+ uint8_t volatile ticker_id_stop;
+
enum role volatile role;
enum state state;
@@ -214,6 +216,10 @@
static void common_init(void);
static void ticker_success_assert(uint32_t status, void *params);
+static void ticker_stop_adv_assert(uint32_t status, void *params);
+static void ticker_stop_obs_assert(uint32_t status, void *params);
+static void ticker_update_adv_assert(uint32_t status, void *params);
+static void ticker_update_slave_assert(uint32_t status, void *params);
static void event_inactive(uint32_t ticks_at_expire, uint32_t remainder,
uint16_t lazy, void *context);
static void adv_setup(void);
@@ -694,23 +700,21 @@
ticker_status = ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
RADIO_TICKER_USER_ID_WORKER,
RADIO_TICKER_ID_ADV,
- ticker_success_assert, (void *)__LINE__);
- LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ ticker_stop_adv_assert,
+ (void *)__LINE__);
+ ticker_stop_adv_assert(ticker_status, (void *)__LINE__);
/* Stop Direct Adv Stopper */
_pdu_adv = (struct pdu_adv *)&_radio.advertiser.adv_data.data
[_radio.advertiser.adv_data.first][0];
if (_pdu_adv->type == PDU_ADV_TYPE_DIRECT_IND) {
- ticker_status =
- ticker_stop(
- RADIO_TICKER_INSTANCE_ID_RADIO,
- RADIO_TICKER_USER_ID_WORKER,
- RADIO_TICKER_ID_ADV_STOP,
- 0, /* @todo ticker_success_assert */
- 0 /* @todo (void *) __LINE__*/);
- LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ /* Advertiser stop can expire while here in this ISR.
+ * Deferred attempt to stop can fail as it would have
+ * expired, hence ignore failure.
+ */
+ ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
+ RADIO_TICKER_USER_ID_WORKER,
+ RADIO_TICKER_ID_ADV_STOP, NULL, NULL);
}
/* Start Slave */
@@ -901,21 +905,23 @@
conn->hdr.ticks_xtal_to_start :
conn->hdr.ticks_active_to_start;
- /* Stop Observer and start Master */
+ /* Stop Observer */
ticker_status = ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
RADIO_TICKER_USER_ID_WORKER,
RADIO_TICKER_ID_OBS,
- ticker_success_assert,
+ ticker_stop_obs_assert,
(void *)__LINE__);
- LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
- ticker_status = ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
- RADIO_TICKER_USER_ID_WORKER,
- RADIO_TICKER_ID_OBS_STOP,
- 0, /* @todo ticker_success_assert */
- 0 /* @todo (void *) __LINE__ */);
- LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ ticker_stop_obs_assert(ticker_status, (void *)__LINE__);
+
+ /* Observer stop can expire while here in this ISR.
+ * Deferred attempt to stop can fail as it would have
+ * expired, hence ignore failure.
+ */
+ ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
+ RADIO_TICKER_USER_ID_WORKER,
+ RADIO_TICKER_ID_OBS_STOP, NULL, NULL);
+
+ /* Start master */
ticker_status =
ticker_start(RADIO_TICKER_INSTANCE_ID_RADIO,
RADIO_TICKER_USER_ID_WORKER,
@@ -2102,16 +2108,25 @@
/** @todo use random 0-10 */
random_delay = 10;
+ /* Call to ticker_update can fail under the race
+ * condition where in the Adv role is being stopped but
+ * at the same time it is preempted by Adv event that
+ * gets into close state. Accept failure when Adv role
+ * is being stopped.
+ */
ticker_status =
ticker_update(RADIO_TICKER_INSTANCE_ID_RADIO,
RADIO_TICKER_USER_ID_WORKER,
RADIO_TICKER_ID_ADV,
- TICKER_US_TO_TICKS(random_delay * 1000),
+ TICKER_US_TO_TICKS(random_delay *
+ 1000),
0, 0, 0, 0, 0,
- ticker_success_assert,
+ ticker_update_adv_assert,
(void *)__LINE__);
LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ (ticker_status == TICKER_STATUS_BUSY) ||
+ (_radio.ticker_id_stop ==
+ RADIO_TICKER_ID_ADV));
}
}
@@ -2139,20 +2154,16 @@
radio_tmr_end_capture();
} else {
- uint32_t ticker_status;
-
radio_filter_disable();
if (_radio.state == STATE_ABORT) {
- ticker_status =
- ticker_stop(
- RADIO_TICKER_INSTANCE_ID_RADIO,
- RADIO_TICKER_USER_ID_WORKER,
- RADIO_TICKER_ID_OBS_STOP,
- 0 /** @todo ticker_success_assert */,
- 0 /** @todo (void *) __LINE__ */);
- LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ /* Observer stop can expire while here in this ISR.
+ * Deferred attempt to stop can fail as it would have
+ * expired, hence ignore failure.
+ */
+ ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
+ RADIO_TICKER_USER_ID_WORKER,
+ RADIO_TICKER_ID_OBS_STOP, NULL, NULL);
}
}
@@ -2423,18 +2434,25 @@
if ((ticks_drift_plus != 0) || (ticks_drift_minus != 0) ||
(lazy != 0) || (force != 0)) {
uint32_t ticker_status;
+ uint8_t ticker_id = RADIO_TICKER_ID_FIRST_CONNECTION +
+ _radio.conn_curr->handle;
+ /* Call to ticker_update can fail under the race
+ * condition where in the Slave role is being stopped but
+ * at the same time it is preempted by Slave event that
+ * gets into close state. Accept failure when Slave role
+ * is being stopped.
+ */
ticker_status =
ticker_update(RADIO_TICKER_INSTANCE_ID_RADIO,
RADIO_TICKER_USER_ID_WORKER,
- RADIO_TICKER_ID_FIRST_CONNECTION +
- _radio.conn_curr->handle,
+ ticker_id,
ticks_drift_plus, ticks_drift_minus, 0, 0,
- lazy, force,
- 0 /** @todo ticker_success_assert */,
- 0 /** @todo (void *) __LINE__ */);
+ lazy, force, ticker_update_slave_assert,
+ (void *)(uint32_t)ticker_id);
LL_ASSERT((ticker_status == TICKER_STATUS_SUCCESS) ||
- (ticker_status == TICKER_STATUS_BUSY));
+ (ticker_status == TICKER_STATUS_BUSY) ||
+ (_radio.ticker_id_stop == ticker_id));
}
}
@@ -2457,6 +2475,20 @@
break;
case ROLE_NONE:
+ /* If a role closes graceful while it is being stopped, then
+ * Radio ISR will be triggered to process the stop state with
+ * no active role at that instance in time.
+ * Just reset the state to none. The role has gracefully closed
+ * before this ISR run.
+ * The above applies to aborting a role event too.
+ */
+ LL_ASSERT((_radio.state == STATE_STOP) ||
+ (_radio.state == STATE_ABORT));
+
+ _radio.state = STATE_NONE;
+
+ return;
+
default:
LL_ASSERT(0);
break;
@@ -2569,6 +2601,60 @@
LL_ASSERT(status == TICKER_STATUS_SUCCESS);
}
+static void ticker_stop_adv_assert(uint32_t status, void *params)
+{
+ ARG_UNUSED(params);
+
+ if (status == TICKER_STATUS_FAILURE) {
+ if (_radio.ticker_id_stop == RADIO_TICKER_ID_ADV) {
+ /* ticker_stop failed due to race condition
+ * while in role_disable. Let the role_disable
+ * be made aware of, so it can return failure
+ * (to stop Adv role as it is now transitioned
+ * to Slave role).
+ */
+ _radio.ticker_id_stop = 0;
+ } else {
+ LL_ASSERT(0);
+ }
+ }
+}
+
+static void ticker_stop_obs_assert(uint32_t status, void *params)
+{
+ ARG_UNUSED(params);
+
+ if (status == TICKER_STATUS_FAILURE) {
+ if (_radio.ticker_id_stop == RADIO_TICKER_ID_OBS) {
+ /* ticker_stop failed due to race condition
+ * while in role_disable. Let the role_disable
+ * be made aware of, so it can return failure
+ * (to stop Obs role as it is now transitioned
+ * to Master role).
+ */
+ _radio.ticker_id_stop = 0;
+ } else {
+ LL_ASSERT(0);
+ }
+ }
+}
+
+static void ticker_update_adv_assert(uint32_t status, void *params)
+{
+ ARG_UNUSED(params);
+
+ LL_ASSERT((status == TICKER_STATUS_SUCCESS) ||
+ (_radio.ticker_id_stop == RADIO_TICKER_ID_ADV));
+}
+
+static void ticker_update_slave_assert(uint32_t status, void *params)
+{
+ uint8_t ticker_id = (uint32_t)params & 0xFF;
+
+ LL_ASSERT((status == TICKER_STATUS_SUCCESS) ||
+ (_radio.ticker_id_stop == ticker_id));
+}
+
static void work_radio_active(void *params)
{
static uint8_t s_active;
@@ -3534,7 +3620,7 @@
#endif
#undef RADIO_DEFERRED_PREEMPT
- /** Handle change in _ticks_active_to_start */
+ /** Handle change in _ticks_active_to_start */
if (_radio.ticks_active_to_start != _ticks_active_to_start) {
uint32_t ticks_to_start_new =
((_radio.ticks_active_to_start <
@@ -6585,6 +6671,10 @@
break;
}
+ LL_ASSERT(!_radio.ticker_id_stop);
+
+ _radio.ticker_id_stop = ticker_id_primary;
+
/* Step 1: Is Primary started? Stop the Primary ticker */
ticker_status =
ticker_stop(RADIO_TICKER_INSTANCE_ID_RADIO,
@@ -6605,7 +6695,7 @@
}
if (ticker_status != TICKER_STATUS_SUCCESS) {
- return 1;
+ goto role_disable_cleanup;
}
/* Inside our event, gracefully handle XTAL and Radio actives */
@@ -6616,7 +6706,14 @@
ticks_xtal_to_start, ticks_active_to_start);
}
- return 0;
+ if (!_radio.ticker_id_stop) {
+ ticker_status = TICKER_STATUS_FAILURE;
+ }
+
+role_disable_cleanup:
+ _radio.ticker_id_stop = 0;
+
+ return ticker_status;
}
uint32_t radio_adv_enable(uint16_t interval, uint8_t chl_map,
