Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <05147111-e46b-42a5-9e99-ec289e61730f@linux.intel.com>
Date:   Wed, 15 Nov 2023 13:51:14 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] soundwire: amd: add an error code check in
 amd_sdw_clock_stop_exit
Content-Language: en-US
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>,
        Su Hui <suhui@nfschina.com>, vkoul@kernel.org,
        yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
        nathan@kernel.org, ndesaulniers@google.com, trix@redhat.com
Cc:     alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev, kernel-janitors@vger.kernel.org
References: <20231115095407.1059704-1-suhui@nfschina.com>
 <3e461bb5-fa6b-4729-8ff5-ccc1d2887066@linux.intel.com>
 <5729c613-182b-43d2-a2ed-caff046b68e7@amd.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <5729c613-182b-43d2-a2ed-caff046b68e7@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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

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