[PATCH 0/6] ASoC: merge soc_pcm_hw_param() rollback and soc_pcm_hw_free()

newer
[PATCH 1/2] ASoC: dt-bindings:...

older
[PATCH v2 00/17] ASoC:...

Kuninori Morimoto

29 Sep 2020 29 Sep '20

6:30 a.m.

Hi Mark

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; }

This kind of duplicated code can be a hotbed of bugs, thus, this patch-set share soc_pcm_hw_free() and rollback.

Kuninori Morimoto (6): ASoC: soc.h: remove for_each_rtd_dais_rollback() ASoC: soc-pcm: move soc_pcm_hw_free() next to soc_pcm_hw_params() ASoC: soc-link: add mark for snd_soc_link_hw_params/free() ASoC: soc-component: add mark for snd_soc_pcm_component_hw_params/free() ASoC: soc-dai: add mark for snd_soc_dai_hw_params/free() ASoC: soc-pcm: add soc_pcm_hw_clean() and call it from soc_pcm_hw_params/free()

-- 2.25.1

Show replies by date

Kuninori Morimoto

29 Sep 29 Sep

6:31 a.m.

New subject: [PATCH 1/6] ASoC: soc.h: remove for_each_rtd_dais_rollback()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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 kuninori.morimoto.gx@renesas.com --- 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);

-- 2.25.1

Kuninori Morimoto

6:31 a.m.

New subject: [PATCH 2/6] ASoC: soc-pcm: move soc_pcm_hw_free() next to soc_pcm_hw_params()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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 kuninori.morimoto.gx@renesas.com --- 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 09e8d703a502..65a6eebafefc 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -859,6 +859,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 @@ -990,49 +1033,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;

-- 2.25.1

Kuninori Morimoto

6:31 a.m.

New subject: [PATCH 3/6] ASoC: soc-link: add mark for snd_soc_link_hw_params/free()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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 kuninori.morimoto.gx@renesas.com --- 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 65a6eebafefc..969f5774cd00 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -885,7 +885,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); @@ -1027,7 +1027,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;

-- 2.25.1

Kuninori Morimoto

6:31 a.m.

New subject: [PATCH 4/6] ASoC: soc-component: add mark for snd_soc_pcm_component_hw_params/free()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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().

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 kuninori.morimoto.gx@renesas.com --- 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 969f5774cd00..5d4d0a95891a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -888,7 +888,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) { @@ -911,7 +911,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; @@ -994,7 +993,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;

@@ -1003,7 +1002,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;

-- 2.25.1

Kuninori Morimoto

6:31 a.m.

New subject: [PATCH 5/6] ASoC: soc-dai: add mark for snd_soc_dai_hw_params/free()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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().

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.

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- 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 5d4d0a95891a..80337d0cd0d2 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -895,7 +895,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); @@ -1011,7 +1011,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; }

@@ -1022,7 +1022,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; }

-- 2.25.1

Kuninori Morimoto

6:32 a.m.

New subject: [PATCH 6/6] ASoC: soc-pcm: add soc_pcm_hw_clean() and call it from soc_pcm_hw_params/free()

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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 kuninori.morimoto.gx@renesas.com --- 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 80337d0cd0d2..8b51f9b8a271 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -859,10 +859,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; @@ -885,23 +882,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 @@ -962,7 +967,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); @@ -982,7 +987,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); @@ -994,41 +999,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); - - 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); + if (ret < 0) + soc_pcm_hw_clean(substream, 1);

- mutex_unlock(&rtd->card->pcm_mutex); return ret; }

-- 2.25.1

Mark Brown

27 Oct 27 Oct

12:46 a.m.

On 29 Sep 2020 13:30:41 +0900, Kuninori Morimoto wrote:

...

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;

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/6] ASoC: soc.h: remove for_each_rtd_dais_rollback() commit: 5560d8c6053c98e3ce17b988dde743792ae613c8 [2/6] ASoC: soc-pcm: move soc_pcm_hw_free() next to soc_pcm_hw_params() commit: ab49436eecf5cc779a1ff659ba59c88779685b47 [3/6] ASoC: soc-link: add mark for snd_soc_link_hw_params/free() commit: 918ad772c4e47f26bc1b5040a79336b7063626cf [4/6] ASoC: soc-component: add mark for snd_soc_pcm_component_hw_params/free() commit: 3a36a64a2de4699ef4a2479a7fb2564b85c8fb4e [5/6] ASoC: soc-dai: add mark for snd_soc_dai_hw_params/free() commit: c304c9acb6e60bcc5c4d4b5a72763ca3bdf7d76b [6/6] ASoC: soc-pcm: add soc_pcm_hw_clean() and call it from soc_pcm_hw_params/free() commit: 4662c59688b8db8834aab14f0d37a4f26fc0dd20

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks, Mark

1518

Age (days ago)

1545

Last active (days ago)

List overview

Download

7 comments

2 participants

tags (0)

participants (2)

Kuninori Morimoto
Mark Brown