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

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

On 10/30/26 18:25, Mukunda,Vijendar wrote:
>
>
> 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.
Correction to my earlier reply.
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@amd.com/#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.
>>>
>>> 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@vger.kernel.org
>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>> ---
>>>   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.
> Slight correction.  Ignore comment 1.
>
>>
>> 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);
>>
>