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.

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

Bluetooth: controller: split: Fix radio in use during flash op #22946

Merged

Conversation

cvinayak
Copy link
Contributor

Fix a race condition in radio abort requested by flash
driver. It is possible that during abort function execution,
PPI setup to start radio fires. Hence, check explicitly in
cleanup function for radio being in use and disable it.

Fixes #22945.

Signed-off-by: Vinayak Kariappa Chettimada vich@nordicsemi.no

Fix a race condition in radio abort requested by flash
driver. It is possible that during abort function execution,
PPI setup to start radio fires. Hence, check explicitly in
cleanup function for radio being in use and disable it.

Fixes zephyrproject-rtos#22945.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak cvinayak added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug area: Bluetooth labels Feb 19, 2020
@cvinayak cvinayak self-assigned this Feb 19, 2020
Fix ticker resolve collision implementation for incorrect
ticks accumulation and the calculation of next period.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@jhedberg
Copy link
Member

Is #22926 related to this? (it also deals with Bluetooth + flash coexistence)

@cvinayak
Copy link
Contributor Author

This is not related to #22926, I stumbled on the issue in this PR during analysis of #22926.

Copy link
Collaborator

@thoh-ot thoh-ot left a comment

Choose a reason for hiding this comment

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

We need time to review the impact of the ticker changes.

@cvinayak
Copy link
Contributor Author

Ticker fixes are related, when the radio assert is handled, the flashing times out as ticker is not scheduling both the connection and flash instances. There is periodic Worker and Job executions but both the instances are skipped always.

image

And this leads to:

...
Write OK.
Erase OK.
Write OK.
Erase OK.
Write internal ERROR!

/* Accumulate ticks_to_expire for each node */
acc_ticks_to_expire += ticker_next->ticks_to_expire;
if (acc_ticks_to_expire > ticker->ticks_slot) {
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I right in saying that this is an optimization - breaking here? (instead of possibly traversing more tickers)

@@ -1400,8 +1402,9 @@ static inline void ticker_job_worker_bh(struct ticker_instance *instance,
/* Reload ticks_to_expire with one period */
ticker->ticks_to_expire =
ticker->ticks_periodic;
ticker->ticks_to_expire +=
ticker_remainder_inc(ticker);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct, this is needed for Nordic!

Copy link
Collaborator

@mtpr-ot mtpr-ot left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for fixing. All automated tests pass on our side (ticker unittests, EDTT tests and target integration tests)

@mtpr-ot mtpr-ot requested a review from thoh-ot February 20, 2020 18:42
@jhedberg
Copy link
Member

@mtpr-ot I'm tempted to merge this, but does your addition of @thoh-ot as reviewer mean that I should wait for feedback from him before merging?

@jhedberg
Copy link
Member

@mtpr-ot I'm tempted to merge this, but does your addition of @thoh-ot as reviewer mean that I should wait for feedback from him before merging?

Nevermind. I see that an earlier review from @thoh-ot is blocking merging this either way.

@aescolar aescolar dismissed thoh-ot’s stale review February 21, 2020 09:47

MTPR has reviewed the ciker changes and is ok with them

@aescolar aescolar merged commit cc468e8 into zephyrproject-rtos:master Feb 21, 2020
@cvinayak cvinayak deleted the github_radio_on_flash_fix branch March 1, 2021 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bluetooth: controller: ASSERTION FAIL Radio is on during flash operation
5 participants