Skip to content

Commit

Permalink
Bluetooth: Controller: Fix scanner window close by using lll_disable
Browse files Browse the repository at this point in the history 
Fix race condition in setting up ISR callback and parameter
caused between ULL_HIGH and LLL context. As LLL IRQ is not
disabled the parameter and ISR callback would get out of
sync causing incorrect parameter supplied to callback and
hence leading to development assert in ull_scan_done().

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
  • Loading branch information
@cvinayak
cvinayak committed Feb 2, 2022
1 parent 456418c commit 9749514
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 83 deletions.
9 changes: 7 additions & 2 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,10 @@ void lll_done_ull_inc(void)
}
#endif /* CONFIG_BT_CTLR_LOW_LAT_ULL_DONE */

bool lll_is_done(void *param)
bool lll_is_done(void *param, bool *is_resume)
{
/* FIXME: use param to check */
*is_resume = (param != event.curr.param);

return !event.curr.abort_cb;
}

Expand Down Expand Up @@ -731,6 +732,10 @@ static struct lll_event *resume_enqueue(lll_prepare_cb_t resume_cb)
{
struct lll_prepare_param prepare_param = {0};

/* Enqueue into prepare pipeline as resume radio event, and remove
* parameter assignment from currently active radio event so that
* done event is not generated.
*/
prepare_param.param = event.curr.param;
event.curr.param = NULL;

Expand Down
2 changes: 1 addition & 1 deletion subsys/bluetooth/controller/ll_sw/nordic/lll/lll_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

int lll_prepare_done(void *param);
int lll_done(void *param);
bool lll_is_done(void *param);
bool lll_is_done(void *param, bool *is_resume);
int lll_is_abort_cb(void *next, void *curr, lll_prepare_cb_t *resume_cb);
void lll_abort_cb(struct lll_prepare_param *prepare_param, void *param);

Expand Down
119 changes: 39 additions & 80 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_scan.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ static void isr_abort(void *param);
* (EVENT_OVERHEAD_PREEMPT_US <= EVENT_OVERHEAD_PREEMPT_MIN_US)
*/
static void isr_done_cleanup(void *param);
static void isr_cleanup(void *param);

static inline int isr_rx_pdu(struct lll_scan *lll, struct pdu_adv *pdu_adv_rx,
uint8_t devmatch_ok, uint8_t devmatch_id,
Expand Down Expand Up @@ -620,11 +619,8 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
cpu_sleep();
}
#endif /* CONFIG_BT_CENTRAL */
} else if (IS_ENABLED(CONFIG_BT_CTLR_LOW_LAT)) {
radio_isr_set(isr_done_cleanup, param);
radio_disable();
} else {
radio_isr_set(isr_cleanup, param);
radio_isr_set(isr_done_cleanup, param);
radio_disable();
}
return;
Expand All @@ -640,10 +636,17 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
}

static void ticker_stop_cb(uint32_t ticks_at_expire, uint32_t ticks_drift,
uint32_t remainder, uint16_t lazy, uint8_t force, void *param)
uint32_t remainder, uint16_t lazy, uint8_t force,
void *param)
{
radio_isr_set(isr_done_cleanup, param);
radio_disable();
static memq_link_t link;
static struct mayfly mfy = {0, 0, &link, NULL, lll_disable};
uint32_t ret;

mfy.param = param;
ret = mayfly_enqueue(TICKER_USER_ID_ULL_HIGH, TICKER_USER_ID_LLL, 0,
&mfy);
LL_ASSERT(!ret);
}

static void ticker_op_start_cb(uint32_t status, void *param)
Expand Down Expand Up @@ -982,14 +985,15 @@ static void isr_abort(void *param)
static void isr_done_cleanup(void *param)
{
struct lll_scan *lll;
bool is_resume;

/* Clear radio status and events */
lll_isr_status_reset();

/* Under race between duration expire, is_stop is set in this function,
* and event preemption, prevent generating duplicate scan done events.
*/
if (lll_is_done(param)) {
if (lll_is_done(param, &is_resume)) {
return;
}

Expand All @@ -1009,67 +1013,19 @@ static void isr_done_cleanup(void *param)
ticker_stop(TICKER_INSTANCE_ID_CTLR, TICKER_USER_ID_LLL,
TICKER_ID_SCAN_STOP, NULL, NULL);

#if defined(CONFIG_BT_CTLR_ADV_EXT)
struct event_done_extra *extra;

/* Generate Scan done events so that duration and max expiry is
* detected in ULL.
*/
extra = ull_done_extra_type_set(EVENT_DONE_EXTRA_TYPE_SCAN);
LL_ASSERT(extra);

/* Prevent scan events in pipeline from being scheduled if duration has
* expired.
*/
if (unlikely(lll->duration_reload && !lll->duration_expire)) {
lll->is_stop = 1U;
}

if (lll->is_aux_sched) {
struct node_rx_pdu *node_rx;

lll->is_aux_sched = 0U;

node_rx = ull_pdu_rx_alloc();
LL_ASSERT(node_rx);
#if defined(CONFIG_BT_CTLR_SCAN_INDICATION)
struct node_rx_hdr *node_rx = ull_pdu_rx_alloc_peek(3);

node_rx->hdr.type = NODE_RX_TYPE_EXT_AUX_RELEASE;
if (node_rx) {
ull_pdu_rx_alloc();

node_rx->hdr.rx_ftr.param = lll;
/* TODO: add other info by defining a payload struct */
node_rx->type = NODE_RX_TYPE_SCAN_INDICATION;

ull_rx_put(node_rx->hdr.link, node_rx);
ull_rx_put(node_rx->link, node_rx);
ull_rx_sched();
}
#endif /* CONFIG_BT_CTLR_ADV_EXT */

lll_isr_cleanup(param);
}

static void isr_cleanup(void *param)
{
/* Clear radio status and events */
lll_isr_status_reset();

/* Do not generate done event for connection initiation, ULL will
* disable the event when establishing/setting up the connection.
* Also, do not generate done event when duration expire, as ULL
* will disable the event.
* As it was transmission of CONNECT_IND, there is not filters to
* disable either.
*/
if (lll_is_done(param)) {
return;
}

/* Disable Rx filters, if cleanup after being in Rx state */
radio_filter_disable();

/* Scanner stop can expire while here in this ISR.
* Deferred attempt to stop can fail as it would have
* expired, hence ignore failure.
*/
ticker_stop(TICKER_INSTANCE_ID_CTLR, TICKER_USER_ID_LLL,
TICKER_ID_SCAN_STOP, NULL, NULL);
#endif /* CONFIG_BT_CTLR_SCAN_INDICATION */

#if defined(CONFIG_BT_HCI_MESH_EXT)
if (_radio.advertiser.is_enabled && _radio.advertiser.is_mesh &&
Expand All @@ -1078,22 +1034,26 @@ static void isr_cleanup(void *param)
}
#endif /* CONFIG_BT_HCI_MESH_EXT */

#if defined(CONFIG_BT_CTLR_SCAN_INDICATION)
struct node_rx_hdr *node_rx = ull_pdu_rx_alloc_peek(3);

if (node_rx) {
ull_pdu_rx_alloc();

/* TODO: add other info by defining a payload struct */
node_rx->type = NODE_RX_TYPE_SCAN_INDICATION;
#if defined(CONFIG_BT_CTLR_ADV_EXT)
/* If continous scan then do not generate scan done when radio event
* has been placed in to prepare pipeline as resume radio event.
*/
if (!is_resume) {
struct event_done_extra *extra;

ull_rx_put(node_rx->link, node_rx);
ull_rx_sched();
/* Generate Scan done events so that duration and max expiry is
* detected in ULL.
*/
extra = ull_done_extra_type_set(EVENT_DONE_EXTRA_TYPE_SCAN);
LL_ASSERT(extra);
}
#endif /* CONFIG_BT_CTLR_SCAN_INDICATION */

#if defined(CONFIG_BT_CTLR_ADV_EXT)
struct lll_scan *lll = param;
/* Prevent scan events in pipeline from being scheduled if duration has
* expired.
*/
if (unlikely(lll->duration_reload && !lll->duration_expire)) {
lll->is_stop = 1U;
}

if (lll->is_aux_sched) {
struct node_rx_pdu *node_rx;
Expand All @@ -1112,7 +1072,6 @@ static void isr_cleanup(void *param)
}
#endif /* CONFIG_BT_CTLR_ADV_EXT */


lll_isr_cleanup(param);
}

Expand Down Expand Up @@ -1206,7 +1165,7 @@ static inline int isr_rx_pdu(struct lll_scan *lll, struct pdu_adv *pdu_adv_rx,
lll_prof_cputime_capture();
}

radio_isr_set(isr_cleanup, lll);
radio_isr_set(isr_done_cleanup, lll);

#if defined(HAL_RADIO_GPIO_HAVE_PA_PIN)
if (IS_ENABLED(CONFIG_BT_CTLR_PROFILE_ISR)) {
Expand Down

0 comments on commit 9749514

Please sign in to comment.