Re: [PATCH 20/21] soundwire: amd: Check clock resume timeout in amd_resume_runtime()
From: Mukunda,Vijendar
Date: Sat Nov 07 2026 - 06:18:07 EST
On 10/30/26 18:25, Mukunda,Vijendar wrote:
Correction to my earlier reply.
On 10/30/26 18:21, Mukunda,Vijendar wrote:
On 10/30/26 16:16, Lee Jones wrote:
When resuming from AMD_SDW_POWER_OFF_MODE in amd_resume_runtime(), the
return value of readl_poll_timeout() when polling for
AMD_SDW_CLK_RESUME_DONE is stored in ret but never checked, allowing
resume to proceed even when clock resume times out.
Return ret if polling for AMD_SDW_CLK_RESUME_DONE fails.
After further review, the existing timeout handling is intentional.
Please disregard my earlier statement that the timeout should be
propagated.
A similar CLK_RESUME_DONE timeout issue was discussed earlier:
https://lore.kernel.org/all/fbaae15c-f3f7-4125-96aa-41b6bb4f174b@xxxxxxx/#t
As discussed there, propagating -ETIMEDOUT would abort the PM resume
path rather than allowing the remaining recovery sequence to proceed.
That discussion also noted the same timeout-handling pattern in the
POWER_OFF_MODE path.
For POWER_OFF_MODE, returning an error at this point would skip the
subsequent slave-state reset and SoundWire manager re-initialization,
preventing the bus recovery and codec re-enumeration.
The preferred long-term approach is to recover the bus on such failures
rather than propagate the error to the PM framework. This remains a
TODO.
Therefore, we intend to retain the existing timeout behavior and will
not change it in the upcoming respin.
Please drop this patch. Sorry for the confusion caused by my earlier
reply.
Slight correction. Ignore comment 1.++
Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
Closes: https://sashiko.dev/#/bug/linux-c6360b30-11b9-40a1-ab04-528462af313c
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
---
drivers/soundwire/amd_manager.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index ba8e6d42c467..faff3e9eefad 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1494,10 +1494,10 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,
(val & AMD_SDW_CLK_RESUME_DONE), ACP_DELAY_US,
AMD_SDW_TIMEOUT);
- if (val & AMD_SDW_CLK_RESUME_DONE) {
- writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
- amd_manager->clk_stopped = false;
- }
+ if (ret)
+ return ret;
+ writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
+ amd_manager->clk_stopped = false;
The bug is valid. On clock resume timeout the code falls through to
sdw_clear_slave_status() and full bus re-init without the clock
being active, which is incorrect.
However, we have identified additional issues in this code path that
need to be addressed together:
1. writel(0, ACP_SW_CLK_RESUME_CTRL) is incorrect — this register
is not a plain zero-clear register. Only the Clk_Resume_Req bit
should be cleared using a read-modify-write on the success path.
2. The same unchecked ret pattern exists in
amd_acp63_sdw_clock_stop_exit() and needs to be fixed together.
We are planning to address all of these as part of our upcoming v4
patch series which will be posted shortly. Could you please hold
this patch until v4 is on-list to avoid conflicts?
}
sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET);
ret = amd_sdw_clk_init_ctrl(amd_manager);