Bluetooth: Controller: Fix nRF CCM disable on connection event abort #88184
Conversation
github-actions
bot
requested review from
Tronil,
carlescufi,
erbr-ot,
thoh-ot and
wopu-ot
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue where the nRF CCM is not disabled on connection event abort, which could lead to data corruption when switching from encrypted to cleartext receptions.
- Added calls to radio_ccm_disable() in various abort callbacks across connection and ISO contexts.
- Declared and implemented the radio_ccm_disable() function in the radio HAL layer to properly stop the CCM module.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c | Added encryption check for broadcast ISO and CCM disable call. |
| subsys/bluetooth/controller/ll_sw/nordic/lll/lll_peripheral_iso.c | Added encryption check on abort callback with ACL context. |
| subsys/bluetooth/controller/ll_sw/nordic/lll/lll_conn.c | Added encryption check with conditional CCM disable call. |
| subsys/bluetooth/controller/ll_sw/nordic/lll/lll_central_iso.c | Added encryption check on abort callback with ACL context. |
| subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h | Declared the new radio_ccm_disable() API. |
| subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c | Implemented the radio_ccm_disable() function. |
| const struct lll_sync_iso *lll = param; | ||
|
|
||
| if (lll->enc) { | ||
| radio_ccm_disable(); | ||
| } |
There was a problem hiding this comment.
[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.
| const struct lll_sync_iso *lll = param; | |
| if (lll->enc) { | |
| radio_ccm_disable(); | |
| } | |
| disable_ccm_if_encrypted(param); |
There was a problem hiding this comment.
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.
| /* 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(); | ||
| } |
There was a problem hiding this comment.
[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.
| /* 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); |
There was a problem hiding this comment.
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.
| if (lll->enc_rx) { | ||
| radio_ccm_disable(); | ||
| } |
There was a problem hiding this comment.
[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.
| if (lll->enc_rx) { | |
| radio_ccm_disable(); | |
| } | |
| disable_ccm_if_encrypted(lll); |
There was a problem hiding this comment.
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.
| /* 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(); | ||
| } |
There was a problem hiding this comment.
[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.
| /* 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); |
There was a problem hiding this comment.
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.
|
|
||
| void radio_ccm_disable(void) | ||
| { | ||
| if (IS_ENABLED(CONFIG_SOC_SERIES_BSIM_NRFXX)) { |
There was a problem hiding this comment.
[nitpick] Consider adding inline comments within radio_ccm_disable() to explain the purpose of the IS_ENABLED(CONFIG_SOC_SERIES_BSIM_NRFXX) check and clarify the function’s behavior across different nRF SoC configurations.
subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c
Outdated
Show resolved
Hide resolved
subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c
Outdated
Show resolved
Hide resolved
Fix missing nRF CCM disable on connection event abort. There can be a problem on nRF SoC for example when a S8 "encrypted" reception is aborted, and a 2M "cleartext" reception starts; slow CCM (that is not stopped as part of radio disable) will corrupt a fast received "cleartext" when the same current free rx buffer is reused in the Controller. This is not a problem when the connection being abort-ee is on a faster PHY than the abort-er. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
cvinayak
force-pushed
the
github_radio_ccm_disable
branch
from
caa1a24 to
b487dfc
Compare
|
4 participants
Fix missing nRF CCM disable on connection event abort.
There can be a problem on nRF SoC for example when a S8 "encrypted" reception is aborted, and a 2M "cleartext" reception starts; slow CCM (that is not stopped as part of radio disable) will corrupt a fast received "cleartext" when the same current free rx buffer is reused in the Controller. This is not a problem when the connection being abort-ee is on a faster PHY than the abort-er.
Fixes #87906