* [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit @ 2023-11-15 9:54 Su Hui 2023-11-15 14:44 ` Pierre-Louis Bossart 0 siblings, 1 reply; 5+ messages in thread From: Su Hui @ 2023-11-15 9:54 UTC (permalink / raw) To: vkoul, yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale, nathan, ndesaulniers, trix Cc: Su Hui, alsa-devel, linux-kernel, llvm, kernel-janitors 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)); + 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); -- 2.30.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit 2023-11-15 9:54 [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit Su Hui @ 2023-11-15 14:44 ` Pierre-Louis Bossart 2023-11-15 17:27 ` Mukunda,Vijendar 0 siblings, 1 reply; 5+ messages in thread From: Pierre-Louis Bossart @ 2023-11-15 14:44 UTC (permalink / raw) To: Su Hui, vkoul, yung-chuan.liao, sanyog.r.kale, nathan, ndesaulniers, trix, Vijendar Mukunda Cc: alsa-devel, linux-kernel, llvm, kernel-janitors 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit 2023-11-15 14:44 ` Pierre-Louis Bossart @ 2023-11-15 17:27 ` Mukunda,Vijendar 2023-11-15 19:51 ` Pierre-Louis Bossart 0 siblings, 1 reply; 5+ messages in thread From: Mukunda,Vijendar @ 2023-11-15 17:27 UTC (permalink / raw) To: Pierre-Louis Bossart, Su Hui, vkoul, yung-chuan.liao, sanyog.r.kale, nathan, ndesaulniers, trix Cc: alsa-devel, linux-kernel, llvm, kernel-janitors On 15/11/23 20:14, Pierre-Louis Bossart wrote: > > 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... 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. 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. > >> 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit 2023-11-15 17:27 ` Mukunda,Vijendar @ 2023-11-15 19:51 ` Pierre-Louis Bossart 2023-11-16 6:45 ` Mukunda,Vijendar 0 siblings, 1 reply; 5+ messages in thread From: Pierre-Louis Bossart @ 2023-11-15 19:51 UTC (permalink / raw) To: Mukunda,Vijendar, Su Hui, vkoul, yung-chuan.liao, sanyog.r.kale, nathan, ndesaulniers, trix Cc: alsa-devel, linux-kernel, llvm, kernel-janitors >>> 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); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit 2023-11-15 19:51 ` Pierre-Louis Bossart @ 2023-11-16 6:45 ` Mukunda,Vijendar 0 siblings, 0 replies; 5+ messages in thread From: Mukunda,Vijendar @ 2023-11-16 6:45 UTC (permalink / raw) To: Pierre-Louis Bossart, Su Hui, vkoul, yung-chuan.liao, sanyog.r.kale, nathan, ndesaulniers, trix Cc: alsa-devel, linux-kernel, llvm, kernel-janitors, Dommati, Sunil-kumar, Katragadda, Mastan 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-16 6:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-15 9:54 [PATCH] soundwire: amd: add an error code check in amd_sdw_clock_stop_exit Su Hui 2023-11-15 14:44 ` Pierre-Louis Bossart 2023-11-15 17:27 ` Mukunda,Vijendar 2023-11-15 19:51 ` Pierre-Louis Bossart 2023-11-16 6:45 ` Mukunda,Vijendar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox