Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bluetooth: Controller: Fix multiple Extended Adv chain reception #79100

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Bluetooth: Controller: Fix multiple Extended Adv chain reception #79100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_scan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,6 +1549,11 @@ static int isr_rx_scan_report(struct lll_scan *lll, uint8_t devmatch_ok,
{
struct node_rx_ftr *ftr;

/* Reset Scan context association with any Aux context as a new
* extended advertising chain is being setup for reception here.
*/
lll->lll_aux = NULL;
Comment on lines +1552 to +1555
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if lll_aux is non-NULL before this, we don't need to do any cleanups or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is just an association do that received PDUs are added to a linked list in the aux context. By setting it to NULL, a new aux context will get associated to which the new chain of PDUs will be added to the linked list.

Copy link
Contributor Author

@cvinayak cvinayak Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found an issue in overnight testing :-(
This code has a race and is causing the LLL scheduled auxiliary PDU reception to leak aux contexts :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now... answers to the questions I had asked myself, avoid both set and reset from ULL execution context. Now association reset in LLL, and association set in ULL, this solves aux context memory leak.


ftr = &(node_rx->rx_ftr);
ftr->param = lll;
ftr->ticks_anchor = radio_tmr_start_get();
Expand Down
15 changes: 15 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_scan_aux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,17 +1241,32 @@ static int isr_rx_pdu(struct lll_scan *lll, struct lll_scan_aux *lll_aux,

ftr = &(node_rx->rx_ftr);
if (lll_aux) {
/* Auxiliary context was used in ULL scheduling in the
* reception of this current PDU.
*/
ftr->param = lll_aux;
ftr->scan_rsp = lll_aux->state;

/* Further auxiliary PDU reception will be chain PDUs */
lll_aux->is_chain_sched = 1U;

/* Reset auxiliary context association with scan context
* as ULL scheduling has been used and may switch to
* using LLL scheduling if the next auxiliary PDU in
* chain is below the threshold to use ULL scheduling.
*/
lll->lll_aux = NULL;

} else if (lll->lll_aux) {
/* Auxiliary context was allocated to Scan context in
* LLL scheduling in the reception of this current PDU.
*/
ftr->param = lll;
ftr->scan_rsp = lll->lll_aux->state;

/* Further auxiliary PDU reception will be chain PDUs */
lll->lll_aux->is_chain_sched = 1U;

} else {
/* Return -ECHILD, as ULL execution has not yet assigned
* an aux context. This can happen only under LLL
Expand Down
5 changes: 5 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,11 @@ static int isr_rx(struct lll_sync *lll, uint8_t node_type, uint8_t crc_ok,
* again a node_rx for periodic report incomplete.
*/
if (node_type != NODE_RX_TYPE_EXT_AUX_REPORT) {
/* Reset Sync context association with any Aux context
* as a new chain is being setup for reception here.
*/
lll->lll_aux = NULL;

node_rx = ull_pdu_rx_alloc_peek(4);
} else {
node_rx = ull_pdu_rx_alloc_peek(3);
Expand Down
30 changes: 12 additions & 18 deletions subsys/bluetooth/controller/ll_sw/ull_scan_aux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,9 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
*/
if (ftr->aux_lll_sched) {
if (IS_ENABLED(CONFIG_BT_CTLR_SYNC_PERIODIC) && sync_lll) {
/* Associate Sync context with the Aux context so that
* it can continue reception in LLL scheduling.
*/
sync_lll->lll_aux = lll_aux;

/* AUX_ADV_IND/AUX_CHAIN_IND PDU reception is being
Expand All @@ -661,8 +664,8 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
*/
LL_ASSERT(!lll->lll_aux || (lll->lll_aux == lll_aux));

/* scan context get the aux context so that it can
* continue reception in LLL scheduling.
/* Associate Scan context with the Aux context so that
* it can continue reception in LLL scheduling.
*/
lll->lll_aux = lll_aux;

Expand All @@ -688,11 +691,6 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
if (unlikely(scan->is_stop)) {
goto ull_scan_aux_rx_flush;
}

/* Remove auxiliary context association with scan context so
* that LLL can differentiate it to being ULL scheduling.
*/
lll->lll_aux = NULL;
} else {
struct ll_sync_set *sync_set;

Expand All @@ -705,7 +703,13 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
goto ull_scan_aux_rx_flush;
}

/* Associate the auxiliary context with sync context */
/* Associate the auxiliary context with sync context, we do this
* for ULL scheduling also in constrast to how extended
* advertising only associates when LLL scheduling is used.
* Each Periodic Advertising chain is received by unique sync
* context, hence LLL and ULL scheduling is always associated
* with same unique sync context.
*/
sync_lll->lll_aux = lll_aux;

/* Backup the node rx to be dispatch on successfully ULL
Expand Down Expand Up @@ -1280,19 +1284,9 @@ static void flush(void *param)
scan = HDR_LLL2ULL(lll);
scan = ull_scan_is_valid_get(scan);
if (!IS_ENABLED(CONFIG_BT_CTLR_SYNC_PERIODIC) || scan) {
lll->lll_aux = NULL;
#if defined(CONFIG_BT_CTLR_JIT_SCHEDULING)
lll->scan_aux_score = aux->lll.hdr.score;
#endif /* CONFIG_BT_CTLR_JIT_SCHEDULING */
} else {
struct lll_sync *sync_lll;
struct ll_sync_set *sync;

sync_lll = aux->parent;
sync = HDR_LLL2ULL(sync_lll);

LL_ASSERT(sync->is_stop || sync_lll->lll_aux);
sync_lll->lll_aux = NULL;
}

aux_release(aux);
Expand Down
Loading