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

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

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)