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.