| Message ID | 20221104131244.3920179-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: core: Suspend/resume error propagation | expand |
On 11/4/22 09:12, Cezary Rojewski wrote: > From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > > Both component->driver->suspend and ->resume() do return an int value > but it isn't propagated to the core later on. Update > snd_soc_component_suspend() and snd_soc_component_resume() so that the > possible errors are not squelched. This looks alright on paper but could break existing solutions. There are a number of cases where an error during suspend is not fatal and you don't want to prevent a system suspend if this is recoverable on resume. See for example the errors on clock-stop for SoundWire, which are squelched on purpose. See also Andy Ross' PR to precisely stop propagating errors in SOF https://github.com/thesofproject/linux/pull/3863 Maybe a less intrusive change would be to add a WARN_ON or something visible to make sure solutions are fixed, and only critical issues can prevent suspend? And in a second step the errors are propagated. > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > include/sound/soc-component.h | 4 ++-- > sound/soc/soc-component.c | 22 ++++++++++++++++------ > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h > index c26ffb033777..421f0fc4df3e 100644 > --- a/include/sound/soc-component.h > +++ b/include/sound/soc-component.h > @@ -456,8 +456,8 @@ int snd_soc_component_open(struct snd_soc_component *component, > int snd_soc_component_close(struct snd_soc_component *component, > struct snd_pcm_substream *substream, > int rollback); > -void snd_soc_component_suspend(struct snd_soc_component *component); > -void snd_soc_component_resume(struct snd_soc_component *component); > +int snd_soc_component_suspend(struct snd_soc_component *component); > +int snd_soc_component_resume(struct snd_soc_component *component); > int snd_soc_component_is_suspended(struct snd_soc_component *component); > int snd_soc_component_probe(struct snd_soc_component *component); > void snd_soc_component_remove(struct snd_soc_component *component); > diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c > index e12f8244242b..27b862ded846 100644 > --- a/sound/soc/soc-component.c > +++ b/sound/soc/soc-component.c > @@ -318,18 +318,28 @@ int snd_soc_component_close(struct snd_soc_component *component, > return soc_component_ret(component, ret); > } > > -void snd_soc_component_suspend(struct snd_soc_component *component) > +int snd_soc_component_suspend(struct snd_soc_component *component) > { > + int ret = 0; > + > if (component->driver->suspend) > - component->driver->suspend(component); > - component->suspended = 1; > + ret = component->driver->suspend(component); > + if (!ret) > + component->suspended = 1; > + > + return soc_component_ret(component, ret); > } > > -void snd_soc_component_resume(struct snd_soc_component *component) > +int snd_soc_component_resume(struct snd_soc_component *component) > { > + int ret = 0; > + > if (component->driver->resume) > - component->driver->resume(component); > - component->suspended = 0; > + ret = component->driver->resume(component); > + if (!ret) > + component->suspended = 0; > + > + return soc_component_ret(component, ret); > } > > int snd_soc_component_is_suspended(struct snd_soc_component *component)
On 11/4/2022 3:00 PM, Pierre-Louis Bossart wrote: > > > On 11/4/22 09:12, Cezary Rojewski wrote: >> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >> >> Both component->driver->suspend and ->resume() do return an int value >> but it isn't propagated to the core later on. Update >> snd_soc_component_suspend() and snd_soc_component_resume() so that the >> possible errors are not squelched. > > This looks alright on paper but could break existing solutions. > There are a number of cases where an error during suspend is not fatal > and you don't want to prevent a system suspend if this is recoverable on > resume. > > See for example the errors on clock-stop for SoundWire, which are > squelched on purpose. See also Andy Ross' PR to precisely stop > propagating errors in SOF https://github.com/thesofproject/linux/pull/3863 > > Maybe a less intrusive change would be to add a WARN_ON or something > visible to make sure solutions are fixed, and only critical issues can > prevent suspend? And in a second step the errors are propagated. > Do note that thread you've pointed out handles device suspend, by which I mean, it is modification of sof_suspend(), called by snd_sof_runtime_suspend() which is then registered as handler in: sound/soc/sof/sof-pci-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, sound/soc/sof/sof-acpi-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, sound/soc/sof/sof-of-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, and then taking TGL device for example there is: static struct pci_driver snd_sof_pci_intel_tgl_driver = { (...) .driver = { .pm = &sof_pci_pm, }, }; And what this patch set changes is handling of .suspend callback present in struct snd_soc_component_driver, which as evidenced by followup patches is handled in ASoC core while audio is being suspended. As far as I can tell SOF makes no direct use of this callback. I'm not negating that maybe there should be a bit of time when only warning is emitted, just making sure that we are on the same page, about what is being changed.
On 11/7/22 02:51, Amadeusz Sławiński wrote: > On 11/4/2022 3:00 PM, Pierre-Louis Bossart wrote: >> >> >> On 11/4/22 09:12, Cezary Rojewski wrote: >>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >>> >>> Both component->driver->suspend and ->resume() do return an int value >>> but it isn't propagated to the core later on. Update >>> snd_soc_component_suspend() and snd_soc_component_resume() so that the >>> possible errors are not squelched. >> >> This looks alright on paper but could break existing solutions. >> There are a number of cases where an error during suspend is not fatal >> and you don't want to prevent a system suspend if this is recoverable on >> resume. >> >> See for example the errors on clock-stop for SoundWire, which are >> squelched on purpose. See also Andy Ross' PR to precisely stop >> propagating errors in SOF >> https://github.com/thesofproject/linux/pull/3863 >> >> Maybe a less intrusive change would be to add a WARN_ON or something >> visible to make sure solutions are fixed, and only critical issues can >> prevent suspend? And in a second step the errors are propagated. >> > > Do note that thread you've pointed out handles device suspend, by which If by 'that thread' you are referring to PR #3863, then it's an excellent example of a desire NOT to propage suspend errors and at the same time an example of a configuration where suspend would not work without additional changes. > I mean, it is modification of sof_suspend(), called by > snd_sof_runtime_suspend() which is then registered as handler in: > sound/soc/sof/sof-pci-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, > snd_sof_runtime_resume, > sound/soc/sof/sof-acpi-dev.c: > SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, > sound/soc/sof/sof-of-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, > snd_sof_runtime_resume, > and then taking TGL device for example there is: > static struct pci_driver snd_sof_pci_intel_tgl_driver = { > (...) > .driver = { > .pm = &sof_pci_pm, > }, > }; > > And what this patch set changes is handling of .suspend callback present > in struct snd_soc_component_driver, which as evidenced by followup > patches is handled in ASoC core while audio is being suspended. > As far as I can tell SOF makes no direct use of this callback. > > I'm not negating that maybe there should be a bit of time when only > warning is emitted, just making sure that we are on the same page, about > what is being changed. I don't think there is an impact on SOF indeed. I was just making the point that well-intended changes to propagate error status can break platforms. we've had a similar case when trying to add checks on pm_runtime_get_sync() and saw multiple errors. Adding more error checks when they were not there from the very beginning is a difficult thing to achieve without regressions.
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index c26ffb033777..421f0fc4df3e 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -456,8 +456,8 @@ int snd_soc_component_open(struct snd_soc_component *component, int snd_soc_component_close(struct snd_soc_component *component, struct snd_pcm_substream *substream, int rollback); -void snd_soc_component_suspend(struct snd_soc_component *component); -void snd_soc_component_resume(struct snd_soc_component *component); +int snd_soc_component_suspend(struct snd_soc_component *component); +int snd_soc_component_resume(struct snd_soc_component *component); int snd_soc_component_is_suspended(struct snd_soc_component *component); int snd_soc_component_probe(struct snd_soc_component *component); void snd_soc_component_remove(struct snd_soc_component *component); diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index e12f8244242b..27b862ded846 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -318,18 +318,28 @@ int snd_soc_component_close(struct snd_soc_component *component, return soc_component_ret(component, ret); } -void snd_soc_component_suspend(struct snd_soc_component *component) +int snd_soc_component_suspend(struct snd_soc_component *component) { + int ret = 0; + if (component->driver->suspend) - component->driver->suspend(component); - component->suspended = 1; + ret = component->driver->suspend(component); + if (!ret) + component->suspended = 1; + + return soc_component_ret(component, ret); } -void snd_soc_component_resume(struct snd_soc_component *component) +int snd_soc_component_resume(struct snd_soc_component *component) { + int ret = 0; + if (component->driver->resume) - component->driver->resume(component); - component->suspended = 0; + ret = component->driver->resume(component); + if (!ret) + component->suspended = 0; + + return soc_component_ret(component, ret); } int snd_soc_component_is_suspended(struct snd_soc_component *component)