From 5560d8c6053c98e3ce17b988dde743792ae613c8 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:31:11 +0900 Subject: [PATCH 1/6] ASoC: soc.h: remove for_each_rtd_dais_rollback() commit 140a4532cdb8 ("ASoC: soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()") uses soc_pcm_clean() and then for_each_rtd_dais_rollback() is no longer used. This patch removes it. Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87o8lpgqbp.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- include/sound/soc.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/sound/soc.h b/include/sound/soc.h index 3b038c563ae1..7541c71c9eb8 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1196,8 +1196,6 @@ struct snd_soc_pcm_runtime { ((i) < (rtd)->num_cpus + (rtd)->num_codecs) && \ ((dai) = (rtd)->dais[i]); \ (i)++) -#define for_each_rtd_dais_rollback(rtd, i, dai) \ - for (; (--(i) >= 0) && ((dai) = (rtd)->dais[i]);) void snd_soc_close_delayed_work(struct snd_soc_pcm_runtime *rtd); From ab49436eecf5cc779a1ff659ba59c88779685b47 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:31:19 +0900 Subject: [PATCH 2/6] ASoC: soc-pcm: move soc_pcm_hw_free() next to soc_pcm_hw_params() This patch moves soc_pcm_hw_free() next to soc_pcm_hw_params(). This is prepare for soc_pcm_hw_params() cleanup Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87mu19gqbh.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- sound/soc/soc-pcm.c | 86 ++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index dcab9527ba3d..697c9cb4a127 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -860,6 +860,49 @@ static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params, interval->max = channels; } +/* + * Frees resources allocated by hw_params, can be called multiple times + */ +static int soc_pcm_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_soc_dai *dai; + int i; + + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); + + /* clear the corresponding DAIs parameters when going to be inactive */ + for_each_rtd_dais(rtd, i, dai) { + int active = snd_soc_dai_stream_active(dai, substream->stream); + + if (snd_soc_dai_active(dai) == 1) { + dai->rate = 0; + dai->channels = 0; + dai->sample_bits = 0; + } + + if (active == 1) + snd_soc_dai_digital_mute(dai, 1, substream->stream); + } + + /* free any machine hw params */ + snd_soc_link_hw_free(substream); + + /* free any component resources */ + snd_soc_pcm_component_hw_free(substream, NULL); + + /* now free hw params for the DAIs */ + for_each_rtd_dais(rtd, i, dai) { + if (!snd_soc_dai_stream_valid(dai, substream->stream)) + continue; + + snd_soc_dai_hw_free(dai, substream); + } + + mutex_unlock(&rtd->card->pcm_mutex); + return 0; +} + /* * Called by ALSA when the hardware params are set by application. This * function can also be called multiple times and can allocate buffers @@ -991,49 +1034,6 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, return ret; } -/* - * Frees resources allocated by hw_params, can be called multiple times - */ -static int soc_pcm_hw_free(struct snd_pcm_substream *substream) -{ - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); - struct snd_soc_dai *dai; - int i; - - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); - - /* clear the corresponding DAIs parameters when going to be inactive */ - for_each_rtd_dais(rtd, i, dai) { - int active = snd_soc_dai_stream_active(dai, substream->stream); - - if (snd_soc_dai_active(dai) == 1) { - dai->rate = 0; - dai->channels = 0; - dai->sample_bits = 0; - } - - if (active == 1) - snd_soc_dai_digital_mute(dai, 1, substream->stream); - } - - /* free any machine hw params */ - snd_soc_link_hw_free(substream); - - /* free any component resources */ - snd_soc_pcm_component_hw_free(substream, NULL); - - /* now free hw params for the DAIs */ - for_each_rtd_dais(rtd, i, dai) { - if (!snd_soc_dai_stream_valid(dai, substream->stream)) - continue; - - snd_soc_dai_hw_free(dai, substream); - } - - mutex_unlock(&rtd->card->pcm_mutex); - return 0; -} - static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { int ret = -EINVAL; From 918ad772c4e47f26bc1b5040a79336b7063626cf Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:31:25 +0900 Subject: [PATCH 3/6] ASoC: soc-link: add mark for snd_soc_link_hw_params/free() soc_pcm_hw_params() does rollback when failed (A), but, it is almost same as soc_pcm_hw_free(). static int soc_pcm_hw_params(xxx) { ... if (ret < 0) goto xxx_err; ... return ret; ^ component_err: | ... | interface_err: (A) ... | codec_err: | ... v return ret; } The difference is soc_pcm_hw_free() is for all dai/component/substream, rollback is for succeeded part only. This kind of duplicated code can be a hotbed of bugs, thus, we want to share soc_pcm_hw_free() and rollback. Now, soc_pcm_hw_params/free() are handling => 1) snd_soc_link_hw_params/free() 2) snd_soc_pcm_component_hw_params/free() 3) snd_soc_dai_hw_params/free() This patch is for 1) snd_soc_link_hw_params/free(). The idea of having bit-flag or counter is not enough for this purpose. For example if one DAI is used for 2xPlaybacks for some reasons, and if 1st Playback was succeeded but 2nd Playback was failed, 2nd Playback rollback doesn't need to call shutdown. But it has succeeded bit-flag or counter via 1st Playback, thus, 2nd Playback rollback will call unneeded shutdown. And 1st Playback's necessary shutdown will not be called, because bit-flag or counter was cleared by wrong 2nd Playback rollback. To avoid such case, this patch marks substream pointer when hw_params() was succeeded. If rollback needed, it will check rollback flag and marked substream pointer. One note here ist that it cares *previous* hw_params() only now, but we might want to check *whole* marked substream in the future. This patch is using macro named "push/pop", so that it can be easily update. Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87lfgtgqba.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- include/sound/soc-link.h | 3 ++- include/sound/soc.h | 1 + sound/soc/soc-link.c | 12 +++++++++++- sound/soc/soc-pcm.c | 4 ++-- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/include/sound/soc-link.h b/include/sound/soc-link.h index dac6c0ce6ede..eff34fc7d3d3 100644 --- a/include/sound/soc-link.h +++ b/include/sound/soc-link.h @@ -19,7 +19,8 @@ void snd_soc_link_shutdown(struct snd_pcm_substream *substream, int snd_soc_link_prepare(struct snd_pcm_substream *substream); int snd_soc_link_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params); -void snd_soc_link_hw_free(struct snd_pcm_substream *substream); +void snd_soc_link_hw_free(struct snd_pcm_substream *substream, + int rollback); int snd_soc_link_trigger(struct snd_pcm_substream *substream, int cmd); int snd_soc_link_compr_startup(struct snd_compr_stream *cstream); diff --git a/include/sound/soc.h b/include/sound/soc.h index 7541c71c9eb8..fa6ce936f899 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1161,6 +1161,7 @@ struct snd_soc_pcm_runtime { /* function mark */ struct snd_pcm_substream *mark_startup; + struct snd_pcm_substream *mark_hw_params; /* bit field */ unsigned int pop_wait:1; diff --git a/sound/soc/soc-link.c b/sound/soc/soc-link.c index 2a8881978930..409ae4940da3 100644 --- a/sound/soc/soc-link.c +++ b/sound/soc/soc-link.c @@ -119,16 +119,26 @@ int snd_soc_link_hw_params(struct snd_pcm_substream *substream, rtd->dai_link->ops->hw_params) ret = rtd->dai_link->ops->hw_params(substream, params); + /* mark substream if succeeded */ + if (ret == 0) + soc_link_mark_push(rtd, substream, hw_params); + return soc_link_ret(rtd, ret); } -void snd_soc_link_hw_free(struct snd_pcm_substream *substream) +void snd_soc_link_hw_free(struct snd_pcm_substream *substream, int rollback) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + if (rollback && !soc_link_mark_match(rtd, substream, hw_params)) + return; + if (rtd->dai_link->ops && rtd->dai_link->ops->hw_free) rtd->dai_link->ops->hw_free(substream); + + /* remove marked substream */ + soc_link_mark_pop(rtd, substream, hw_params); } int snd_soc_link_trigger(struct snd_pcm_substream *substream, int cmd) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 697c9cb4a127..8c69288f1bf5 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -886,7 +886,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) } /* free any machine hw params */ - snd_soc_link_hw_free(substream); + snd_soc_link_hw_free(substream, 0); /* free any component resources */ snd_soc_pcm_component_hw_free(substream, NULL); @@ -1028,7 +1028,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, codec_dai->rate = 0; } - snd_soc_link_hw_free(substream); + snd_soc_link_hw_free(substream, 1); mutex_unlock(&rtd->card->pcm_mutex); return ret; From 3a36a64a2de4699ef4a2479a7fb2564b85c8fb4e Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:31:41 +0900 Subject: [PATCH 4/6] ASoC: soc-component: add mark for snd_soc_pcm_component_hw_params/free() soc_pcm_hw_params() does rollback when failed (A), but, it is almost same as soc_pcm_hw_free(). static int soc_pcm_hw_params(xxx) { ... if (ret < 0) goto xxx_err; ... return ret; ^ component_err: | ... | interface_err: (A) ... | codec_err: | ... v return ret; } The difference is soc_pcm_hw_free() is for all dai/component/substream, rollback is for succeeded part only. This kind of duplicated code can be a hotbed of bugs, thus, we want to share soc_pcm_hw_free() and rollback. Now, soc_pcm_hw_params/free() are handling 1) snd_soc_link_hw_params/free() => 2) snd_soc_pcm_component_hw_params/free() 3) snd_soc_dai_hw_params/free() This patch is for 2) snd_soc_pcm_component_hw_params/free(). The idea of having bit-flag or counter is not enough for this purpose. For example if one DAI is used for 2xPlaybacks for some reasons, and if 1st Playback was succeeded but 2nd Playback was failed, 2nd Playback rollback doesn't need to call shutdown. But it has succeeded bit-flag or counter via 1st Playback, thus, 2nd Playback rollback will call unneeded shutdown. And 1st Playback's necessary shutdown will not be called, because bit-flag or counter was cleared by wrong 2nd Playback rollback. To avoid such case, this patch marks substream pointer when hw_params() was succeeded. If rollback needed, it will check rollback flag and marked substream pointer. One note here is that it cares *previous* hw_params() only now, but we might want to check *whole* marked substream in the future. This patch is using macro named "push/pop", so that it can be easily update. Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87k0wdgqav.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- include/sound/soc-component.h | 6 +++--- sound/soc/soc-component.c | 19 ++++++++++--------- sound/soc/soc-pcm.c | 7 +++---- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 2c790ce95259..21f1d120b68e 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -220,6 +220,7 @@ struct snd_soc_component { /* function mark */ struct snd_pcm_substream *mark_module; struct snd_pcm_substream *mark_open; + struct snd_pcm_substream *mark_hw_params; void *mark_pm; #ifdef CONFIG_DEBUG_FS @@ -459,10 +460,9 @@ int snd_soc_pcm_component_new(struct snd_soc_pcm_runtime *rtd); void snd_soc_pcm_component_free(struct snd_soc_pcm_runtime *rtd); int snd_soc_pcm_component_prepare(struct snd_pcm_substream *substream); int snd_soc_pcm_component_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_component **last); + struct snd_pcm_hw_params *params); void snd_soc_pcm_component_hw_free(struct snd_pcm_substream *substream, - struct snd_soc_component *last); + int rollback); int snd_soc_pcm_component_trigger(struct snd_pcm_substream *substream, int cmd); int snd_soc_pcm_component_pm_runtime_get(struct snd_soc_pcm_runtime *rtd, diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 728e93f35ffb..6d719c2db92e 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -779,8 +779,7 @@ int snd_soc_pcm_component_prepare(struct snd_pcm_substream *substream) } int snd_soc_pcm_component_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_component **last) + struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_component *component; @@ -790,33 +789,35 @@ int snd_soc_pcm_component_hw_params(struct snd_pcm_substream *substream, if (component->driver->hw_params) { ret = component->driver->hw_params(component, substream, params); - if (ret < 0) { - *last = component; + if (ret < 0) return soc_component_ret(component, ret); - } } + /* mark substream if succeeded */ + soc_component_mark_push(component, substream, hw_params); } - *last = NULL; return 0; } void snd_soc_pcm_component_hw_free(struct snd_pcm_substream *substream, - struct snd_soc_component *last) + int rollback) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_component *component; int i, ret; for_each_rtd_components(rtd, i, component) { - if (component == last) - break; + if (rollback && !soc_component_mark_match(component, substream, hw_params)) + continue; if (component->driver->hw_free) { ret = component->driver->hw_free(component, substream); if (ret < 0) soc_component_ret(component, ret); } + + /* remove marked substream */ + soc_component_mark_pop(component, substream, hw_params); } } diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8c69288f1bf5..6eec0f498090 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -889,7 +889,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) snd_soc_link_hw_free(substream, 0); /* free any component resources */ - snd_soc_pcm_component_hw_free(substream, NULL); + snd_soc_pcm_component_hw_free(substream, 0); /* now free hw params for the DAIs */ for_each_rtd_dais(rtd, i, dai) { @@ -912,7 +912,6 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); - struct snd_soc_component *component; struct snd_soc_dai *cpu_dai; struct snd_soc_dai *codec_dai; int i, ret = 0; @@ -995,7 +994,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, snd_soc_dapm_update_dai(substream, params, cpu_dai); } - ret = snd_soc_pcm_component_hw_params(substream, params, &component); + ret = snd_soc_pcm_component_hw_params(substream, params); if (ret < 0) goto component_err; @@ -1004,7 +1003,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, return ret; component_err: - snd_soc_pcm_component_hw_free(substream, component); + snd_soc_pcm_component_hw_free(substream, 1); i = rtd->num_cpus; From c304c9acb6e60bcc5c4d4b5a72763ca3bdf7d76b Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:31:53 +0900 Subject: [PATCH 5/6] ASoC: soc-dai: add mark for snd_soc_dai_hw_params/free() soc_pcm_hw_params() does rollback when failed (A), but, it is almost same as soc_pcm_hw_free(). static int soc_pcm_hw_params(xxx) { ... if (ret < 0) goto xxx_err; ... return ret; ^ component_err: | ... | interface_err: (A) ... | codec_err: | ... v return ret; } The difference is soc_pcm_hw_free() is for all dai/component/substream, rollback is for succeeded part only. This kind of duplicated code can be a hotbed of bugs, thus, we want to share soc_pcm_hw_free() and rollback. Now, soc_pcm_hw_params/free() are handling 1) snd_soc_link_hw_params/free() 2) snd_soc_pcm_component_hw_params/free() => 3) snd_soc_dai_hw_params/free() This patch is for 3) snd_soc_dai_hw_params/free(). The idea of having bit-flag or counter is not enough for this purpose. For example if one DAI is used for 2xPlaybacks for some reasons, and if 1st Playback was succeeded but 2nd Playback was failed, 2nd Playback rollback doesn't need to call shutdown. But it has succeeded bit-flag or counter via 1st Playback, thus, 2nd Playback rollback will call unneeded shutdown. And 1st Playback's necessary shutdown will not be called, because bit-flag or counter was cleared by wrong 2nd Playback rollback. To avoid such case, this patch marks substream pointer when hw_params() was succeeded. If rollback needed, it will check rollback flag and marked substream pointer. One note here is that it cares *previous* hw_params() only now, but we might want to check *whole* marked substream in the future. This patch is using macro named "push/pop", so that it can be easily update. Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87imbxgqai.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- include/sound/soc-dai.h | 4 +++- sound/soc/soc-dai.c | 13 ++++++++++++- sound/soc/soc-dapm.c | 4 ++-- sound/soc/soc-pcm.c | 6 +++--- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 2150bd4c7a05..7a85a6f83ca8 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -149,7 +149,8 @@ int snd_soc_dai_hw_params(struct snd_soc_dai *dai, struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params); void snd_soc_dai_hw_free(struct snd_soc_dai *dai, - struct snd_pcm_substream *substream); + struct snd_pcm_substream *substream, + int rollback); int snd_soc_dai_startup(struct snd_soc_dai *dai, struct snd_pcm_substream *substream); void snd_soc_dai_shutdown(struct snd_soc_dai *dai, @@ -390,6 +391,7 @@ struct snd_soc_dai { /* function mark */ struct snd_pcm_substream *mark_startup; + struct snd_pcm_substream *mark_hw_params; /* bit field */ unsigned int probed:1; diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 4705c3da6280..2686a566649b 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -335,16 +335,27 @@ int snd_soc_dai_hw_params(struct snd_soc_dai *dai, if (dai->driver->ops && dai->driver->ops->hw_params) ret = dai->driver->ops->hw_params(substream, params, dai); + + /* mark substream if succeeded */ + if (ret == 0) + soc_dai_mark_push(dai, substream, hw_params); end: return soc_dai_ret(dai, ret); } void snd_soc_dai_hw_free(struct snd_soc_dai *dai, - struct snd_pcm_substream *substream) + struct snd_pcm_substream *substream, + int rollback) { + if (rollback && !soc_dai_mark_match(dai, substream, hw_params)) + return; + if (dai->driver->ops && dai->driver->ops->hw_free) dai->driver->ops->hw_free(substream, dai); + + /* remove marked substream */ + soc_dai_mark_pop(dai, substream, hw_params); } int snd_soc_dai_startup(struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 980f2c330b87..471ac8d9b669 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3955,13 +3955,13 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w, substream->stream = SNDRV_PCM_STREAM_CAPTURE; snd_soc_dapm_widget_for_each_source_path(w, path) { source = path->source->priv; - snd_soc_dai_hw_free(source, substream); + snd_soc_dai_hw_free(source, substream, 0); } substream->stream = SNDRV_PCM_STREAM_PLAYBACK; snd_soc_dapm_widget_for_each_sink_path(w, path) { sink = path->sink->priv; - snd_soc_dai_hw_free(sink, substream); + snd_soc_dai_hw_free(sink, substream, 0); } substream->stream = SNDRV_PCM_STREAM_CAPTURE; diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 6eec0f498090..d4dd2dc1b00c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -896,7 +896,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) if (!snd_soc_dai_stream_valid(dai, substream->stream)) continue; - snd_soc_dai_hw_free(dai, substream); + snd_soc_dai_hw_free(dai, substream, 0); } mutex_unlock(&rtd->card->pcm_mutex); @@ -1012,7 +1012,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) continue; - snd_soc_dai_hw_free(cpu_dai, substream); + snd_soc_dai_hw_free(cpu_dai, substream, 1); cpu_dai->rate = 0; } @@ -1023,7 +1023,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, if (!snd_soc_dai_stream_valid(codec_dai, substream->stream)) continue; - snd_soc_dai_hw_free(codec_dai, substream); + snd_soc_dai_hw_free(codec_dai, substream, 1); codec_dai->rate = 0; } From 4662c59688b8db8834aab14f0d37a4f26fc0dd20 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto Date: Tue, 29 Sep 2020 13:32:01 +0900 Subject: [PATCH 6/6] ASoC: soc-pcm: add soc_pcm_hw_clean() and call it from soc_pcm_hw_params/free() soc_pcm_hw_params() does rollback when failed (A), but, it is almost same as soc_pcm_hw_free(). static int soc_pcm_hw_params(xxx) { ... if (ret < 0) goto xxx_err; ... return ret; ^ component_err: | ... | interface_err: (A) ... | codec_err: | ... v return ret; } The difference is soc_pcm_hw_free() is for all dai/component/substream, rollback is for succeeded part only. This kind of duplicated code can be a hotbed of bugs, thus, we want to share soc_pcm_hw_free() and rollback. Now, soc_pcm_hw_params/free() are handling 1) snd_soc_link_hw_params/free() 2) snd_soc_pcm_component_hw_params/free() 3) snd_soc_dai_hw_params/free() Now, 1) to 3) are handled. This patch adds new soc_pcm_hw_clean() and call it from soc_pcm_hw_params() as rollback, and from soc_pcm_hw_free() as normal close handler. Other difference is that soc_pcm_hw_free() handles digital mute if it was last user. Rollback also handles it by this patch. Signed-off-by: Kuninori Morimoto Link: https://lore.kernel.org/r/87h7rhgqab.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown --- include/sound/soc.h | 4 ---- sound/soc/soc-pcm.c | 56 +++++++++++++-------------------------------- 2 files changed, 16 insertions(+), 44 deletions(-) diff --git a/include/sound/soc.h b/include/sound/soc.h index fa6ce936f899..5ac578c9340c 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1184,14 +1184,10 @@ struct snd_soc_pcm_runtime { for ((i) = 0; \ ((i) < rtd->num_cpus) && ((dai) = asoc_rtd_to_cpu(rtd, i)); \ (i)++) -#define for_each_rtd_cpu_dais_rollback(rtd, i, dai) \ - for (; (--(i) >= 0) && ((dai) = asoc_rtd_to_cpu(rtd, i));) #define for_each_rtd_codec_dais(rtd, i, dai) \ for ((i) = 0; \ ((i) < rtd->num_codecs) && ((dai) = asoc_rtd_to_codec(rtd, i)); \ (i)++) -#define for_each_rtd_codec_dais_rollback(rtd, i, dai) \ - for (; (--(i) >= 0) && ((dai) = asoc_rtd_to_codec(rtd, i));) #define for_each_rtd_dais(rtd, i, dai) \ for ((i) = 0; \ ((i) < (rtd)->num_cpus + (rtd)->num_codecs) && \ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d4dd2dc1b00c..17ff3a369631 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -860,10 +860,7 @@ static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params, interval->max = channels; } -/* - * Frees resources allocated by hw_params, can be called multiple times - */ -static int soc_pcm_hw_free(struct snd_pcm_substream *substream) +static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *dai; @@ -886,23 +883,31 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) } /* free any machine hw params */ - snd_soc_link_hw_free(substream, 0); + snd_soc_link_hw_free(substream, rollback); /* free any component resources */ - snd_soc_pcm_component_hw_free(substream, 0); + snd_soc_pcm_component_hw_free(substream, rollback); /* now free hw params for the DAIs */ for_each_rtd_dais(rtd, i, dai) { if (!snd_soc_dai_stream_valid(dai, substream->stream)) continue; - snd_soc_dai_hw_free(dai, substream, 0); + snd_soc_dai_hw_free(dai, substream, rollback); } mutex_unlock(&rtd->card->pcm_mutex); return 0; } +/* + * Frees resources allocated by hw_params, can be called multiple times + */ +static int soc_pcm_hw_free(struct snd_pcm_substream *substream) +{ + return soc_pcm_hw_clean(substream, 0); +} + /* * Called by ALSA when the hardware params are set by application. This * function can also be called multiple times and can allocate buffers @@ -963,7 +968,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, ret = snd_soc_dai_hw_params(codec_dai, substream, &codec_params); if(ret < 0) - goto codec_err; + goto out; codec_dai->rate = params_rate(&codec_params); codec_dai->channels = params_channels(&codec_params); @@ -983,7 +988,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, ret = snd_soc_dai_hw_params(cpu_dai, substream, params); if (ret < 0) - goto interface_err; + goto out; /* store the parameters for each DAI */ cpu_dai->rate = params_rate(params); @@ -995,41 +1000,12 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, } ret = snd_soc_pcm_component_hw_params(substream, params); - if (ret < 0) - goto component_err; - out: mutex_unlock(&rtd->card->pcm_mutex); - return ret; -component_err: - snd_soc_pcm_component_hw_free(substream, 1); + if (ret < 0) + soc_pcm_hw_clean(substream, 1); - i = rtd->num_cpus; - -interface_err: - for_each_rtd_cpu_dais_rollback(rtd, i, cpu_dai) { - if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) - continue; - - snd_soc_dai_hw_free(cpu_dai, substream, 1); - cpu_dai->rate = 0; - } - - i = rtd->num_codecs; - -codec_err: - for_each_rtd_codec_dais_rollback(rtd, i, codec_dai) { - if (!snd_soc_dai_stream_valid(codec_dai, substream->stream)) - continue; - - snd_soc_dai_hw_free(codec_dai, substream, 1); - codec_dai->rate = 0; - } - - snd_soc_link_hw_free(substream, 1); - - mutex_unlock(&rtd->card->pcm_mutex); return ret; }