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