Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit

[Mukunda,Vijendar]

On 16/11/23 01:21, Pierre-Louis Bossart wrote:
>>>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>>>> index 3a99f6dcdfaf..f391b541f4b7 100644
>>>> --- a/drivers/soundwire/amd_manager.c
>>>> +++ b/drivers/soundwire/amd_manager.c
>>>> @@ -1029,6 +1029,10 @@ static int amd_sdw_clock_stop_exit(struct amd_sdw_manager *amd_manager)
>>>>  		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 (ret)
>>>> +			dev_err(amd_manager->dev, "%s: timed out: %pe\n", __func__,
>>>> +				ERR_PTR(ret));
>>> Is this really the desired behavior?
>>>
>>> This patch fixes the static analysis issue by logging the error code,
>>> but does it make sense to continue resuming here and trying to exit from
>>> the clock stop mode?
>>>
>>> At this point a bus reset might be a more relevant behavior...
>> As per earlier discussion, when we sent the initial patch series,
>> It was communicated that even clock stop sequence fails,
>> return '0' in suspend/resume callbacks that why we returned
>> status as zero.
> clock stop is for suspend and clock stop exit for resume. Different
> problems.
>
>> In this scenario, it's not continuing resume when clock stop exit
>> sequence fails. Even In Intel's case, if the clock stop sequence fails,
>> just code is exiting from that sequence.
> that's right, in the Intel SoundWire drivers we never prevent the
> pm_runtime suspend from happening, and discard any errors. In the resume
> step we do a bus reset anyways.
>
> But that's different here, this is the clock stop exit which happens on
> resume and IIRC there is no bus reset. If the resume fails, what is the
> expected behavior? If you keep going then you are going to have other
> issues down the road.
We will check and come back on clock stop exit sequence failure
adding bus reset sequence.
>>>>  		if (val & AMD_SDW_CLK_RESUME_DONE) {
>>>>  			writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
>>>>  			ret = sdw_bus_exit_clk_stop(&amd_manager->bus);