| Message ID | 87v7vptzap.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret() | expand |
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@redhat.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > 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
Hi Hans Thank you for the feedback > -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. Thanks but hmm... In reality, -EINVAL is used not only from userspace, and many inside kernel functions are using it. So don't indicate -EINVAL for all case is over-kill in real world. But thank you for clarify the situation. I think indicating it on comment is good idea. > > @@ -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: Oh, yes, indeed. Thank you for pointing it. Will fix in v2 Thank you for your help !! Best regards --- Kuninori Morimoto
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; } ret = dpcm_be_dai_prepare(fe, stream);
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. Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- sound/soc/soc-pcm.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)