| Message ID | 871rrloehp.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: soc-pcm cleanup step2 | expand |
On Mon, 27 Jan 2020 02:49:22 +0100, Kuninori Morimoto wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. > In such case, fallback process need to care about operated/non-operated > codec dai. > > But, if it goto error process *after* loop even though error happen > during loop, it don't need to care about operated/non-operated. > In such case code can be more simple. > This patch do it. And this is prepare for soc_snd_open() cleanup This would mean that snd_soc_dai_shutdown() is called even for the stream that returned the error. This isn't the expected behavior. Also, bit-OR-ing the multiple error codes isn't wise, they may return different error codes, and you'll mixed up to a different number. thanks, Takashi > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index f11c15f..57d2f00 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) > goto component_err; > > for_each_rtd_codec_dai(rtd, i, codec_dai) { > - ret = snd_soc_dai_startup(codec_dai, substream); > - if (ret < 0) { > - dev_err(codec_dai->dev, > - "ASoC: can't open codec %s: %d\n", > - codec_dai->name, ret); > - goto codec_dai_err; > - } > + ret |= snd_soc_dai_startup(codec_dai, substream); > > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > codec_dai->tx_mask = 0; > else > codec_dai->rx_mask = 0; > } > + if (ret < 0) { > + dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n", > + codec_dai->name, ret); > + goto codec_dai_err; > + } > > ret = soc_rtd_startup(rtd, substream); > if (ret < 0) { > pr_err("ASoC: %s startup failed: %d\n", > rtd->dai_link->name, ret); > - goto machine_err; > + goto codec_dai_err; > } > > /* Dynamic PCM DAI links compat checks use dynamic capabilities */ > @@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) > config_err: > soc_rtd_shutdown(rtd, substream); > > -machine_err: > - i = rtd->num_codecs; > - > codec_dai_err: > - for_each_rtd_codec_dai_rollback(rtd, i, codec_dai) > + for_each_rtd_codec_dai(rtd, i, codec_dai) > snd_soc_dai_shutdown(codec_dai, substream); > > component_err: > -- > 2.7.4 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi Takashi Thank you for your feedback > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > > > soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. > > In such case, fallback process need to care about operated/non-operated > > codec dai. > > > > But, if it goto error process *after* loop even though error happen > > during loop, it don't need to care about operated/non-operated. > > In such case code can be more simple. > > This patch do it. And this is prepare for soc_snd_open() cleanup > > This would mean that snd_soc_dai_shutdown() is called even for the > stream that returned the error. This isn't the expected behavior. Yeah. Actually, I have plan to add such flag by other patch. But indeed order was reversed. Will fixup. > Also, bit-OR-ing the multiple error codes isn't wise, they may return > different error codes, and you'll mixed up to a different number. It is used at some architecture. But yes, let's think about better idea. Will return 1st error. Thank you for your help !! Best regards --- Kuninori Morimoto
>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >>> >>> soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. >>> In such case, fallback process need to care about operated/non-operated >>> codec dai. >>> >>> But, if it goto error process *after* loop even though error happen >>> during loop, it don't need to care about operated/non-operated. >>> In such case code can be more simple. >>> This patch do it. And this is prepare for soc_snd_open() cleanup >> >> This would mean that snd_soc_dai_shutdown() is called even for the >> stream that returned the error. This isn't the expected behavior. > > Yeah. > Actually, I have plan to add such flag by other patch. > But indeed order was reversed. > Will fixup. > >> Also, bit-OR-ing the multiple error codes isn't wise, they may return >> different error codes, and you'll mixed up to a different number. > > It is used at some architecture. > But yes, let's think about better idea. > Will return 1st error. I also cringed on the bit-OR'ed error codes, but I saw it's already used in soc-pcm.c so thought it was an agreed precedent? ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream); I also don't like the idea of not stopping loops on errors, or releasing things that haven't been properly allocated. It does simplify error handling but it's asking for trouble.
On Tue, 28 Jan 2020 17:31:05 +0100, Pierre-Louis Bossart wrote: > > > > >>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > >>> > >>> soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. > >>> In such case, fallback process need to care about operated/non-operated > >>> codec dai. > >>> > >>> But, if it goto error process *after* loop even though error happen > >>> during loop, it don't need to care about operated/non-operated. > >>> In such case code can be more simple. > >>> This patch do it. And this is prepare for soc_snd_open() cleanup > >> > >> This would mean that snd_soc_dai_shutdown() is called even for the > >> stream that returned the error. This isn't the expected behavior. > > > > Yeah. > > Actually, I have plan to add such flag by other patch. > > But indeed order was reversed. > > Will fixup. > > > >> Also, bit-OR-ing the multiple error codes isn't wise, they may return > >> different error codes, and you'll mixed up to a different number. > > > > It is used at some architecture. > > But yes, let's think about better idea. > > Will return 1st error. > > I also cringed on the bit-OR'ed error codes, but I saw it's already > used in soc-pcm.c so thought it was an agreed precedent? > > ret |= snd_soc_component_close(component, substream); > ret |= snd_soc_component_hw_free(component, substream); I think it's just an overlook. The driver may return arbitrary error codes so they can conflict. The bit-OR works only if the return code is always consistent. Takashi
Hi > > I also cringed on the bit-OR'ed error codes, but I saw it's already > > used in soc-pcm.c so thought it was an agreed precedent? > > > > ret |= snd_soc_component_close(component, substream); > > ret |= snd_soc_component_hw_free(component, substream); > > I think it's just an overlook. The driver may return arbitrary error > codes so they can conflict. The bit-OR works only if the return code > is always consistent. OK. Let's cleanup it, too. If you ara OK, I will do it. > I also don't like the idea of not stopping loops on errors, or > releasing things that haven't been properly allocated. It does > simplify error handling but it's asking for trouble. Actually, my local patch which will be posted will solve it. I will re-order patch-set, and post it. # But, this re-order patch will have conflict in my local tree. # I will solve it, too. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index f11c15f..57d2f00 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto component_err; for_each_rtd_codec_dai(rtd, i, codec_dai) { - ret = snd_soc_dai_startup(codec_dai, substream); - if (ret < 0) { - dev_err(codec_dai->dev, - "ASoC: can't open codec %s: %d\n", - codec_dai->name, ret); - goto codec_dai_err; - } + ret |= snd_soc_dai_startup(codec_dai, substream); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_dai->tx_mask = 0; else codec_dai->rx_mask = 0; } + if (ret < 0) { + dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n", + codec_dai->name, ret); + goto codec_dai_err; + } ret = soc_rtd_startup(rtd, substream); if (ret < 0) { pr_err("ASoC: %s startup failed: %d\n", rtd->dai_link->name, ret); - goto machine_err; + goto codec_dai_err; } /* Dynamic PCM DAI links compat checks use dynamic capabilities */ @@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) config_err: soc_rtd_shutdown(rtd, substream); -machine_err: - i = rtd->num_codecs; - codec_dai_err: - for_each_rtd_codec_dai_rollback(rtd, i, codec_dai) + for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream); component_err: