Re: [PATCH 20/21] soundwire: amd: Check clock resume timeout in amd_resume_runtime()

[Mukunda,Vijendar]

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.
>
> 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);