linux-kernel - Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9c7c794e-f6d7-4092-90f1-13d44c2111f8@amd.com>
Date:   Thu, 16 Nov 2023 12:15:52 +0530
From:   "Mukunda,Vijendar" <vijendar.mukunda@....com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Su Hui <suhui@...china.com>, vkoul@...nel.org,
        yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com,
        nathan@...nel.org, ndesaulniers@...gle.com, trix@...hat.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, kernel-janitors@...r.kernel.org,
        "Dommati, Sunil-kumar" <Sunil-kumar.Dommati@....com>,
        "Katragadda, Mastan" <mastan.katragadda@....com>
Subject: Re: [PATCH] soundwire: amd: add an error code check in
 amd_sdw_clock_stop_exit

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