Bluetooth: Controller: Fix empty PDU buffer overrun under ISR latency #74916
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cvinayak
force-pushed
the
github_radio_rx_overrun_fix2
branch
from
f4e26fb to
8b65ca6
Compare
|
Gave a quick try and it seems to support a connection on the board here now, although it does continually give this: Have not tried with exchanging any data, just a base connection |
cvinayak
force-pushed
the
github_radio_rx_overrun_fix2
branch
4 times, most recently
from
2391722 to
51fdb1a
Compare
|
@thedjnK Could you please retry with this updated PR? discovered that there was a regression (in how Kconfig hidden menu and a choice inside did not get its default assigned) in use of speed optimization for Controller code. (Related issue: #39756) You should try both the samples/bluetooth/peripheral_hr (cleartext connection) and samples/bluetooth/peripheral (subscribing to a vendor service, for pairing and encrypted connection). If the changes fix the issues observed, then please review and approve the PR. Thank you in advance. @carlescufi Hope you will be able to test a bbc:microbit, if you manage to fit it, for peripheral connections (cleartext or encrypted)... |
cvinayak
force-pushed
the
github_radio_rx_overrun_fix2
branch
from
51fdb1a to
0bdfd29
Compare
Fix in-system ISR profiling for advertiser and connection role for the missing implementation when there is CRC error. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Use uint16_t to store ISR profiling value to avoid overflow in case of higher latencies. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Add ISR profiling using ticker ticks, hence profile ISR CPU use outside radio events. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Provide Radio ISR latency in microseconds when asserting due to high ISR latency. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_radio_rx_overrun_fix2
branch
from
0bdfd29 to
7a368ca
Compare
zephyrbot
requested review from
carlescufi,
erbr-ot,
kruithofa,
mtpr-ot,
thoh-ot and
wopu-ot
kruithofa
previously approved these changes
Jul 29, 2024
Thalley
reviewed
Aug 13, 2024
Fix ISR profiling when using single timer for tIFS radio switching wherein in the timer is cleared on every radio end. Hence, the captured timer value is the latency and does not required the radio end timestamp to be subtracted. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Only 3 bytes (PDU_EM_LL_SIZE_MAX) is required for empty PDU transmission, but in case of Radio ISR latency if rx packet pointer is not setup then Radio DMA will use previously assigned buffer which can be this empty PDU buffer. Radio DMA will overrun this buffer and cause memory corruption. Any detection of ISR latency will not happen if the ISR function pointer in RAM is corrupted by this overrun. Increasing ISR latencies in OS and CPU usage in the ULL_HIGH priority if it is same as LLL priority in Controller implementation then it is making it tight to execute Controller code in the tIFS between Tx-Rx PDU's Radio ISRs. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
In case of ISR latencies, if packet pointer has not been set on time then we do not want to check uninitialized length in rx buffer that did not get used by Radio DMA. This would help us in detecting radio ready event being set? We can not detect radio ready if it happens twice before Radio ISR executes after latency. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Fix regression in encrypted connection introduced in commit f3deccd ("Bluetooth: Controller: CCM read data to early when DF enabled on PHY 1M"). Due to this nRF51x SoC hang waiting to encrypt and/or check MIC. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Fix regression using speed optimization introduced in commit 1b7fe79 ("Bluetooth: Controller: Support Link Time Optimizations (LTO)"). Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Relax the radio packet pointer assignment deadline assertion until access address being transmitted. The PDU buffer is probably only needed just after access address is being transmitted or received by the radio. This will give some more breathing room for slow CPUs like in nRF51x SoCs. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_radio_rx_overrun_fix2
branch
from
7a368ca to
6a44568
Compare
kruithofa
approved these changes
Aug 14, 2024
Thalley
approved these changes
Aug 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix regression using speed optimization introduced in commit cvinayak@1b7fe79 ("Bluetooth: Controller: Support Link Time Optimizations (LTO)").
Fix regression in encrypted connection introduced in commit cvinayak@f3deccd ("Bluetooth: Controller: CCM read data to early when DF enabled on PHY 1M").
Due to this nRF51x SoC hang waiting to encrypt and/or
check MIC.
Only 3 bytes (PDU_EM_LL_SIZE_MAX) is required for empty PDU
transmission, but in case of Radio ISR latency if rx packet
pointer is not setup then Radio DMA will use previously
assigned buffer which can be this empty PDU buffer. Radio
DMA will overrun this buffer and cause memory corruption.
Any detection of ISR latency will not happen if the ISR
function pointer in RAM is corrupted by this overrun.
Increasing ISR latencies in OS and CPU usage in the
ULL_HIGH priority if it is same as LLL priority in Controller
implementation then it is making it tight to execute
Controller code in the tIFS between Tx-Rx PDU's Radio ISRs.
Other fixes to ISR profiling needed as part of analysis of increased ISR latencies.
Related to #74345.
Fixes #76427.
Signed-off-by: Vinayak Kariappa Chettimada [email protected]