Skip to content

Bluetooth: Controller: Fix nRF CCM disable on connection event abort #88184

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

Merged
kartben merged 1 commit into from
May 16, 2025
Merged

Bluetooth: Controller: Fix nRF CCM disable on connection event abort #88184

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
6 changes: 6 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2511,6 +2511,12 @@ uint32_t radio_ccm_mic_is_valid(void)
return (NRF_CCM->MICSTATUS != 0);
}

void radio_ccm_disable(void)
{
nrf_ccm_task_trigger(NRF_CCM, NRF_CCM_TASK_STOP);
nrf_ccm_disable(NRF_CCM);
}

#if defined(CONFIG_BT_CTLR_PRIVACY)
static uint8_t MALIGN(4) _aar_scratch[3];

Expand Down
1 change: 1 addition & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ void *radio_ccm_tx_pkt_set(struct ccm *ccm, void *pkt);
void *radio_ccm_iso_tx_pkt_set(struct ccm *ccm, uint8_t pdu_type, void *pkt);
uint32_t radio_ccm_is_done(void);
uint32_t radio_ccm_mic_is_valid(void);
void radio_ccm_disable(void);

void radio_ar_configure(uint32_t nirk, void *irk, uint8_t flags);
uint32_t radio_ar_match_get(void);
Expand Down
9 changes: 9 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_central_iso.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,15 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
radio_isr_set(isr_done, cis_lll);
radio_disable();

#if defined(CONFIG_BT_CTLR_LE_ENC)
/* Get reference to ACL context */
const struct lll_conn *conn_lll = ull_conn_lll_get(cis_lll->acl_handle);

if (conn_lll->enc_rx) {
radio_ccm_disable();
}
Comment on lines +433 to +438
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The similar pattern of checking the encryption state and disabling CCM appears across different modules. Consider consolidating this logic into a helper function to ease future maintenance.

Suggested change
/* Get reference to ACL context */
const struct lll_conn *conn_lll = ull_conn_lll_get(cis_lll->acl_handle);
if (conn_lll->enc_rx) {
radio_ccm_disable();
}
disable_ccm_if_encrypted(cis_lll->acl_handle);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@cvinayak cvinayak May 16, 2025

Choose a reason for hiding this comment

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

The abort_cb is kept role specific to make it easy to update its logic in future, hence not considering using common helper functions. Also, the context that stores the enc state is different for different roles in the Controller.

#endif /* CONFIG_BT_CTLR_LE_ENC */

return;
}

Expand Down
7 changes: 7 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,13 @@ void lll_conn_abort_cb(struct lll_prepare_param *prepare_param, void *param)
*/
radio_isr_set(isr_done, param);
radio_disable();

#if defined(CONFIG_BT_CTLR_LE_ENC)
if (lll->enc_rx) {
radio_ccm_disable();
}
Comment on lines +233 to +235
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] This encryption check and CCM disable logic is repeated in multiple abort callbacks. Refactoring this into a shared helper function could improve the code’s maintainability and reduce duplication.

Suggested change
if (lll->enc_rx) {
radio_ccm_disable();
}
disable_ccm_if_encrypted(lll);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@cvinayak cvinayak May 16, 2025

Choose a reason for hiding this comment

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

The abort_cb is kept role specific to make it easy to update its logic in future, hence not considering using common helper functions. Also, the context that stores the enc state is different for different roles in the Controller.

#endif /* CONFIG_BT_CTLR_LE_ENC */

return;
}

Expand Down
9 changes: 9 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_peripheral_iso.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,15 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
radio_isr_set(isr_done, cis_lll);
radio_disable();

#if defined(CONFIG_BT_CTLR_LE_ENC)
/* Get reference to ACL context */
const struct lll_conn *conn_lll = ull_conn_lll_get(cis_lll->acl_handle);

if (conn_lll->enc_rx) {
radio_ccm_disable();
}
Comment on lines +449 to +454
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The encryption check and CCM disable call here duplicate similar logic in other abort callbacks. Consider abstracting this into a common helper function to reduce code duplication.

Suggested change
/* Get reference to ACL context */
const struct lll_conn *conn_lll = ull_conn_lll_get(cis_lll->acl_handle);
if (conn_lll->enc_rx) {
radio_ccm_disable();
}
/* Disable CCM if encryption is enabled */
disable_ccm_if_encrypted(cis_lll->acl_handle);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@cvinayak cvinayak May 16, 2025

Choose a reason for hiding this comment

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

The abort_cb is kept role specific to make it easy to update its logic in future, hence not considering using common helper functions. Also, the context that stores the enc state is different for different roles in the Controller.

#endif /* CONFIG_BT_CTLR_LE_ENC */

return;
}

Expand Down
9 changes: 9 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,15 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
if (!prepare_param) {
radio_isr_set(isr_done, param);
radio_disable();

if (IS_ENABLED(CONFIG_BT_CTLR_BROADCAST_ISO_ENC)) {
const struct lll_sync_iso *lll = param;

if (lll->enc) {
radio_ccm_disable();
}
Comment on lines +487 to +491
Copy link

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The encryption check and radio_ccm_disable() call in this abort callback is similar to logic in other modules. Consider refactoring this repeated logic into a shared helper function to improve maintainability.

Suggested change
const struct lll_sync_iso *lll = param;
if (lll->enc) {
radio_ccm_disable();
}
disable_ccm_if_encrypted(param);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@cvinayak cvinayak May 16, 2025

Choose a reason for hiding this comment

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

The abort_cb is kept role specific to make it easy to update its logic in future, hence not considering using common helper functions. Also, the context that stores the enc state is different for different roles in the Controller.

}

return;
}

Expand Down