Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <3e461bb5-fa6b-4729-8ff5-ccc1d2887066@linux.intel.com>
Date:   Wed, 15 Nov 2023 08:44:16 -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
To:     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,
        Vijendar Mukunda <Vijendar.Mukunda@amd.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>
Content-Language: en-US
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20231115095407.1059704-1-suhui@nfschina.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/15/23 03:54, Su Hui wrote:
> Clang static analyzer complains that value stored to 'ret' is never read.
> Add an error code check and print an error message if 'readl_poll_timeout'
> failed.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  drivers/soundwire/amd_manager.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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...

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