drivers: dai: dmic: do not call k_sleep() in PM callbacks #95352
Conversation
zephyrbot
requested review from
dbaluta,
iuliana-prodan,
juimonen,
LaurentiuM1234 and
lgirdwood
| @@ -315,7 +315,7 @@ static inline void dai_dmic_en_power(const struct dai_intel_dmic *dmic) | |||
| #if defined(CONFIG_SOC_INTEL_ACE20_LNL) || defined(CONFIG_SOC_INTEL_ACE30) || \ | |||
| defined(CONFIG_SOC_INTEL_ACE40) | |||
| while (!(sys_read32(base + DMICLCTL_OFFSET) & DMICLCTL_CPA)) { | |||
| k_sleep(K_USEC(100)); | |||
| k_busy_wait(100); | |||
There was a problem hiding this comment.
I don't understand this sentence in commit message:
"And even on platforms where this code is used, k_sleep() is not always called, so hitting real issues requires multiple conditions to be met at runtime."
drivers/dai/intel/dmic/dmic.c
Outdated
| k_busy_wait(100); | ||
| if (!WAIT_FOR((sys_read32(base + DMICLCTL_OFFSET) & DMICLCTL_CPA) != 0, 1000, | ||
| k_busy_wait(100))) { | ||
| LOG_ERR("power-up timeout"); |
There was a problem hiding this comment.
Isn't it an error if we timing out waiting for CPA to be set? I see, the dai_dmic_en_power() is void, cannot fail.
The call to k_sleep() is not safe as dai_dmic_probe() is called from PM dmic_pm_action() and k_can_yield() may be false. Problem was caught on Intel WCL ADSP platform with SOF as application and a Zephyr build with asserts enabled. Fix the issue by using k_busy_wait() instead. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
force-pushed
the
202508-dmic-power-up-fix
branch
from
d16cc7f to
21013a3
Compare
|
V2:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes unsafe usage of k_sleep() in PM (Power Management) callbacks by replacing it with k_busy_wait(). The sleep function cannot be called in interrupt or non-blocking contexts, which PM callbacks typically execute in.
- Replaces
k_sleep(K_USEC(100))withk_busy_wait(100)in power enable function - Ensures proper synchronization without blocking in PM callback context
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| while (!(sys_read32(base + DMICLCTL_OFFSET) & DMICLCTL_CPA)) { | ||
| k_sleep(K_USEC(100)); | ||
| k_busy_wait(100); | ||
| } |
There was a problem hiding this comment.
The busy wait loop could potentially run indefinitely if the hardware never sets the CPA bit. Consider adding a timeout counter or maximum iteration limit to prevent infinite spinning and provide error handling for hardware failure scenarios.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Ack on that. I'll do a separate PR to add the a timeout, but I want to fix the error reporting in other places as well, so I think better done in a separate PR.
There was a problem hiding this comment.
why not fix it in one go? This loop is incorrect anyways.
There was a problem hiding this comment.
@ujfalusi this PR is urgent and I need more time to do the follow-up and test it
|
Fix unsafe usage of k_sleep() in interrupt/nonblocking context.