Re: [PATCH 1/3] ASoC: component: Propagate result of suspend and resume callbacks

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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.