Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

cvinayak · 2024-11-02T05:00:17Z

Fix spurious ISO Sync Receiver stall due to uninitialised
value accessed due to regression introduced by
commit cvinayak@64facee ("Bluetooth: controller: Stop Sync ISO
ticker when establishment fails").

Align audio test Controller Kconfig value same as used with nRF53bsim.

Fixes #80734.

Align audio test Controller Kconfig value same as used with nRF53bsim. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Fix spurious ISO Sync Receiver stall due to uninitialised value accessed due to regression introduced by commit 64facee ("Bluetooth: controller: Stop Sync ISO ticker when establishment fails"). Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

aescolar

Thank you

aescolar · 2024-11-02T15:04:18Z

@dkalowsk @mmahadevan108 It would be very nice to get this for 4.0, otherwise we should disable the failing test case (it would be quite bad to release 4.0 with a failing BT testcase as that would prevent backporting any other BT related fix)

Thalley · 2024-11-02T21:12:59Z

Will try to test this with my PR that saw a similar problem

Thalley · 2024-11-02T21:23:03Z

tests/bsim/bluetooth/audio/overlay-bt_ll_sw_split.conf

Could we instead just use the nRF53 BSIM .conf file?

Will investigate as part of enabling some high reliability CAP tests. Reusing the target conf file is challenging as high reliability test need lot more tx and rx buffers to handle worst case retransmissions.

Trying out fixes and Kconfig changes here: #80788

Thalley · 2024-11-02T21:23:36Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

I had a feeling it was related to uninitialzed data, but valgrind didn't show anything for me. How did you find this?

Had to debug inside the lower link layer to check when the reception stopped, there were calls to ticker_stop and lead to the caller which used this uninitialized value (recently added struct member). Since the value is from inside the Rx buffer node allocated from pool, the overall content get assigned something sometime before when receiving other packets.

Thalley · 2024-11-03T22:24:54Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

@@ -1281,6 +1282,7 @@ static void isr_rx_done(void *param)

 	/* Calculate and place the drift information in done event */
 	e->type = EVENT_DONE_EXTRA_TYPE_SYNC_ISO;
+	e->estab_failed = 0U;


This value isn't assigned after line 1252 - Is that intentional?

Yes, intentional. Only relevant structure members are assigned for each type being passed around.

Controller has large Rx data flow (in struct node_rx) overheads (two parts: fixed overhead of unions, and configurable PDU length payload). Generally, not all members of the overhead part (respective to the type of rx node) are assigned.

When adding new struct member, typically all sources of the rx node type in the data flow need to be inspected :-(

On second thought i need to investigate further today...#80788 pass locally, but fails in CI :-(

Thalley · 2024-11-03T22:31:29Z

I think this fixes the stalling issue (passes those tests locally), but it will fail other tests from my PR #80691:

======== Running CAP AC_13 with 441_1_1 =========
...
d_01: @00:00:01.392685  [0]: Incoming audio on stream 0x84da700 len 97 and ts 1376271 flags 0x09
d_01: @00:00:01.392685  [0]: Incoming audio on stream 0x84da700 len 97 and ts 1376271
d_01: @00:00:01.392685  [0]: Incoming audio on stream 0x84da744 len 97 and ts 1383748 flags 0x09
d_01: @00:00:01.392685  [0]: Incoming audio on stream 0x84da744 len 97 and ts 1383748
d_01: @00:00:01.400187  [1]: Incoming audio on stream 0x84da700 len 0 and ts 1384434 flags 0x0c
d_01: @00:00:01.400187 ERROR: (WEST_TOPDIR/zephyr/tests/bsim/bluetooth/audio/src/cap_acceptor_test.c:321): ISO receive lost

This is a broadcast test, and the host always enqueues 2 SDUs per stream in the controller.

I think it's OK to merge, as it fixes the issues it claims, but it does not solve all issues. Will spend some more time tomorrow verifying that the host correctly enqueues the number of SDUs it expects.

dkalowsk · 2024-11-04T01:21:17Z

@dkalowsk @mmahadevan108 It would be very nice to get this for 4.0, otherwise we should disable the failing test case (it would be quite bad to release 4.0 with a failing BT testcase as that would prevent backporting any other BT related fix)

@aescolar thanks for bringing it to attention. It does have the associated Issue to fix so as far as I can tell this will work on RC2 provided maintainers are good with it (and it looks like Thally is)

cvinayak · 2024-11-05T04:40:37Z

@aescolar @Thalley any new PR fails CI with different audio tests failing for same commit HEAD with re-runs: https://github.com/zephyrproject-rtos/zephyr/actions/runs/11666290372?pr=80788
May be many tests fail in CI (that do not fail locally or vice versa), but CI stops on first failure of many running in parallel?

Comparing the bsim timestamps between CI and local run (local native posix toolchain), they do not appear same and i suspect timing changes in bsim run on CI too for re-runs.

Audio test need the bsim timing to be deterministic on every run otherwise the Controller will experience overlapping states/roles causing tests to fail.

Example, timing compare here: #80788 (comment)

cvinayak · 2024-11-05T09:19:34Z

I have only seen it happen on the nRF52 BSIM, and have not been able to reproduce with the nRF53 BSIM nor on target.

Refer to #80734 (comment)

The conf change in the commit in this PR is to address the inconsistency between nrf52 bsim and nrf53 bsim testing.

cvinayak · 2024-11-05T10:53:25Z

@aescolar I do not like that you have independently opened two new PRs (#80894, and ##80896) and marked them as hotfixes while this PR is made to wait for the merge timeout.

cvinayak · 2024-11-06T04:14:49Z

Replaced by #80894. Other change to be addressed as part of #80788.

cvinayak added 2 commits November 2, 2024 05:58

tests: bsim: Bluetooth: Align audio test Controller Kconfig

c9545f8

Align audio test Controller Kconfig value same as used with nRF53bsim. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak changed the title ~~tests: bsim: Bluetooth: Align audio test Controller Kconfig~~ Bluetooth: Controller: Fix spurious ISO Sync receiver stall Nov 2, 2024

cvinayak marked this pull request as ready for review November 2, 2024 09:45

zephyrbot added area: Bluetooth area: Bluetooth Audio area: Bluetooth Controller labels Nov 2, 2024

zephyrbot requested review from asbjornsabo, carlescufi, Casper-Bonde-Bose, erbr-ot, fredrikdanebjer, jhedberg, kruithofa, larsgk, MariuszSkamra, mtpr-ot, pin-zephyr, sjanc, Thalley, thoh-ot and wopu-ot November 2, 2024 09:46

zephyrbot assigned Thalley Nov 2, 2024

aescolar approved these changes Nov 2, 2024

View reviewed changes

aescolar added this to the v4.0.0 milestone Nov 2, 2024

aescolar added the bug The issue is a bug, or the PR is fixing a bug label Nov 2, 2024

Thalley reviewed Nov 2, 2024

View reviewed changes

Thalley reviewed Nov 3, 2024

View reviewed changes

Thalley approved these changes Nov 3, 2024

View reviewed changes

cvinayak mentioned this pull request Nov 4, 2024

tests: bsim: Bluetooth: Enable some high reliability CAP tests #80788

Merged

aescolar mentioned this pull request Nov 5, 2024

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80894

Merged

aescolar mentioned this pull request Nov 5, 2024

tests bsim bt audio: Increase execution timeout #80896

Merged

cvinayak closed this Nov 6, 2024

cvinayak removed this from the v4.0.0 milestone Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

cvinayak commented Nov 2, 2024 •

edited

Loading

aescolar left a comment

aescolar commented Nov 2, 2024 •

edited

Loading

Thalley commented Nov 2, 2024

Thalley Nov 2, 2024

cvinayak Nov 3, 2024

Thalley Nov 2, 2024

cvinayak Nov 3, 2024

Thalley Nov 3, 2024

cvinayak Nov 4, 2024

cvinayak Nov 4, 2024

Thalley commented Nov 3, 2024

dkalowsk commented Nov 4, 2024

cvinayak commented Nov 5, 2024 •

edited

Loading

cvinayak commented Nov 5, 2024

cvinayak commented Nov 5, 2024

cvinayak commented Nov 6, 2024 •

edited

Loading

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

Conversation

cvinayak commented Nov 2, 2024 • edited Loading

aescolar left a comment

Choose a reason for hiding this comment

aescolar commented Nov 2, 2024 • edited Loading

Thalley commented Nov 2, 2024

Thalley Nov 2, 2024

Choose a reason for hiding this comment

cvinayak Nov 3, 2024

Choose a reason for hiding this comment

Thalley Nov 2, 2024

Choose a reason for hiding this comment

cvinayak Nov 3, 2024

Choose a reason for hiding this comment

Thalley Nov 3, 2024

Choose a reason for hiding this comment

cvinayak Nov 4, 2024

Choose a reason for hiding this comment

cvinayak Nov 4, 2024

Choose a reason for hiding this comment

Thalley commented Nov 3, 2024

dkalowsk commented Nov 4, 2024

cvinayak commented Nov 5, 2024 • edited Loading

cvinayak commented Nov 5, 2024

cvinayak commented Nov 5, 2024

cvinayak commented Nov 6, 2024 • edited Loading

cvinayak commented Nov 2, 2024 •

edited

Loading

aescolar commented Nov 2, 2024 •

edited

Loading

cvinayak commented Nov 5, 2024 •

edited

Loading

cvinayak commented Nov 6, 2024 •

edited

Loading