Skip to content

Bluetooth: controller: Prevent ticker_job re-entrance #31026

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

Closed
wants to merge 1 commit into from
+12 −4
Closed

Bluetooth: controller: Prevent ticker_job re-entrance #31026

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
16 changes: 12 additions & 4 deletions subsys/bluetooth/controller/ticker/ticker.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,11 +2276,19 @@ void ticker_job(void *param)

DEBUG_TICKER_JOB(1);

/* Defer worker, as job is now running */
/* Prevent running job if worker is still running */
if (instance->worker_trigger) {
DEBUG_TICKER_JOB(0);
return;
}

/* Prevent running job if already running (re-entrance) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the need for running ticker_job from more than one execution/ISR/thread/task context? What is making ticker_job to pre-empt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is the need for running ticker_job from more than one execution/ISR/thread/task context? What is making ticker_job to pre-empt?

We observe it with meta-IRQ threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct my understanding, a meta-IRQ thread should not pre-empt itself, right?

If you agree to the changes here being a workaround for issue with meta-IRQ, then the changes here can be under a workaround Kconfig option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, meta-IRQs run to completion - but pre-empts HCI thread (the debugging is a few months back, so I may need to brush up on the actual flow of things). A workaround config is OK with me, if we can make sure this is not a more general problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, the issue occurs when ticker operations are called from thread context and ticker_job is executed inline instead of being chained. An example for this would be a call to ticker_stop from thread context that is preempted by an ISR that then calls, e.g., ticker_update.

Copy link
Contributor

Choose a reason for hiding this comment

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

ticker operations are called from thread context and ticker_job is executed inline instead of being chained

In this case, TICKER_USER_ID_THREAD is equal to TICKER_USER_ID_ULL_LOW to execute the ticker_job inline.

ticker_stop from thread context that is preempted by an ISR that then calls, e.g., ticker_update

Here ISR context is not equal to TICKER_USER_ID_ULL_LOW (ISR context has higher priority in comparison to the thread), and hence ticker_job shall not be a inline function when called by ticker_update.

What is important is, there shall only be one thread (or meta-IRQ) calling mayfly_run(TICKER_USER_ID_ULL_LOW); in the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are investigating whether changing the TICKER_USER_ID_XX "priorities" will solve the problem. This may take a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mtpr-ot I see that I have faced the similar issue to which I propose this: #32752

if (instance->job_guard) {
DEBUG_TICKER_JOB(0);
return;
}

/* Defer worker, as job is now running */
instance->job_guard = 1U;

/* Back up the previous known tick */
Expand Down Expand Up @@ -2357,14 +2365,14 @@ void ticker_job(void *param)
ticker_job_list_inquire(instance);
}

/* Permit worker job to run */
instance->job_guard = 0U;

/* update compare if head changed */
if (flag_compare_update) {
ticker_job_compare_update(instance, ticker_id_old_head);
}

/* Permit worker job to run */
instance->job_guard = 0U;

/* trigger worker if deferred */
if (instance->worker_trigger) {
instance->sched_cb(TICKER_CALL_ID_JOB, TICKER_CALL_ID_WORKER, 1,
Expand Down