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

kernel: Update k_wakeup() #69255

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

peter-mitsis
Copy link
Collaborator

This commit does two things to k_wakeup():

  1. It locks the scheduler before marking the thread as not suspended. As the the clearing of the _THREAD_SUSPENDED bit is not atomic, this helps ensure that neither another thread nor ISR interrupts this action (resulting in a corrupted thread_state).

  2. The call to flag_ipi() has been removed as it is already being made within ready_thread().

Fixes #69148

This commit does two things to k_wakeup():

1. It locks the scheduler before marking the thread as not suspended.
As the the clearing of the _THREAD_SUSPENDED bit is not atomic, this
helps ensure that neither another thread nor ISR interrupts this
action (resulting in a corrupted thread_state).

2. The call to flag_ipi() has been removed as it is already being
made within ready_thread().

Signed-off-by: Peter Mitsis <[email protected]>
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Nice, was about to look at exactly the same bug. Looks correct to me.

Might be worth nothing that a root-ish cause of this messup is that (1) k_wakeup() is really obscure and (2) it really should have shared a backend implementation with k_thread_resume(), which is a much more common (and in this case, correct) path.

@nashif nashif added this to the v3.7.0 milestone Feb 21, 2024
@nashif nashif merged commit 51ae993 into zephyrproject-rtos:main Feb 26, 2024
27 checks passed
@kv2019i
Copy link
Collaborator

kv2019i commented Mar 5, 2024

FYI @peter-mitsis @ceolin -- we saw new cases of "__ASSERT_NO_MSG(!z_smp_cpu_mobile()" firing form zephyr/kernel/pm.c:pm_system_suspend(). Example fail log https://sof-ci.01.org/sofpr/PR8901/build3107/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=check-playback-10sec . It seems this PR fixes the problem. UPDATE: failures still seen, submitted #69807 to track

FYI to @lyakh as well. We are debugging this in context of thesofproject/sof#8818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kernel function z_impl_k_wakeup is not interrupt safe, can result in invalid thread state and assert
6 participants