Re: [PATCH] ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret()

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 12-Dec-24 3:23 AM, Kuninori Morimoto wrote:
> commit 1f5664351410 ("ASoC: lower "no backend DAIs enabled for ... Port"
> log severity") ignores -EINVAL error message on common soc_pcm_ret().
> It is used from many functions, ignoring -EINVAL is not good idea.
> 
> The reason why -EINVAL was ignored is only for ignoring
> dpcm_fe_dai_prepare() error flood.
> It should be handled at dpcm_fe_dai_prepare() side, not soc_pcm_ret()
> side.

-EINVAL really should only be used upon invalid parameters coming from
userspace and in that case we don't want to log an error since we do
not want to give userspace a way to do a denial-of-service attack
on the syslog / diskspace.

I'm not convinced that this change is a good idea, but I also
have no strong objections.

There also is a bug in this patch which needs to be fixed before this
can be accepted, see my inline comment below.

> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
>  sound/soc/soc-pcm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 1150455619aa4..7ea580f0a6f7f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -38,7 +38,6 @@ static inline int _soc_pcm_ret(struct snd_soc_pcm_runtime *rtd,
>  	switch (ret) {
>  	case -EPROBE_DEFER:
>  	case -ENOTSUPP:
> -	case -EINVAL:
>  		break;
>  	default:
>  		dev_err(rtd->dev,
> @@ -2560,8 +2559,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>  			     fe->dai_link->name);
>  		dev_dbg(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
>  			fe->dai_link->name);
> -		ret = -EINVAL;
> -		goto out;
> +		/* don't use soc_pcm_ret() to lower error log severity */
> +		return -EINVAL;

You cannot just do a return here, you are now missing these 2 lines from
the "goto out" path:

        dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
        snd_soc_dpcm_mutex_unlock(fe);


>  	}
>  
>  	ret = dpcm_be_dai_prepare(fe, stream);

Regards,

Hans