[alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2
Hi Mark
These are v3 of soc-pcm cleanup. Original [5/7] [6/7] patch call open / start function for all DAI / component. And, it calls close / stop function for all if some of them got error. But in such case, error occured DAI / component don't need to call close / stop.
This issue can be solved if it had flag, and my local tree has such patch. But it was planed to post to ML a little later. This time, I merged original patch and this new flag patch on [5/7] [6/7].
These mention that "it is prepare for soc-pcm-open() cleanup". But it will be happen later.
These are based on v5.6-rc1
Kuninori Morimoto (8): 1) ASoC: soc-pcm: add snd_soc_runtime_action() 2) ASoC: soc-pcm: adjustment for DAI member 0 reset 3) ASoC: soc-pcm: add for_each_dapm_widgets() macro 4) ASoC: soc-pcm: don't use bit-OR'ed error 5) ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once 6) ASoC: soc-pcm: call snd_soc_component_open/close() once 7) ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open() 8) ASoC: soc-pcm: tidyup soc_pcm_open() order
include/sound/soc-component.h | 7 +- include/sound/soc-dai.h | 5 +- include/sound/soc-dapm.h | 5 + sound/soc/soc-component.c | 35 ++++-- sound/soc/soc-dai.c | 11 +- sound/soc/soc-dapm.c | 8 +- sound/soc/soc-pcm.c | 246 +++++++++++++++++++----------------------- 7 files changed, 161 insertions(+), 156 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has snd_soc_runtime_activate() / snd_soc_runtime_deactivate(). These increment or decrement DAI/Component activity, but the code difference is only +1 or -1. This patch adds common snd_soc_runtime_action() which can get +1 or -1 as parameter, and use it from snd_soc_runtime_activate/deactivate() to avoid duplicate implementation.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- v2 -> v3
- add Ranjani's Reviewed-by
sound/soc/soc-pcm.c | 67 +++++++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ff1b7c7..4d26558 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -82,17 +82,8 @@ static int soc_rtd_trigger(struct snd_soc_pcm_runtime *rtd, return 0; }
-/** - * snd_soc_runtime_activate() - Increment active count for PCM runtime components - * @rtd: ASoC PCM runtime that is activated - * @stream: Direction of the PCM stream - * - * Increments the active count for all the DAIs and components attached to a PCM - * runtime. Should typically be called when a stream is opened. - * - * Must be called with the rtd->card->pcm_mutex being held - */ -void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +static void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd, + int stream, int action) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; @@ -101,24 +92,39 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active++; + cpu_dai->playback_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active++; + codec_dai->playback_active += action; } else { - cpu_dai->capture_active++; + cpu_dai->capture_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active++; + codec_dai->capture_active += action; }
- cpu_dai->active++; - cpu_dai->component->active++; + cpu_dai->active += action; + cpu_dai->component->active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->active++; - codec_dai->component->active++; + codec_dai->active += action; + codec_dai->component->active += action; } }
/** + * snd_soc_runtime_activate() - Increment active count for PCM runtime components + * @rtd: ASoC PCM runtime that is activated + * @stream: Direction of the PCM stream + * + * Increments the active count for all the DAIs and components attached to a PCM + * runtime. Should typically be called when a stream is opened. + * + * Must be called with the rtd->card->pcm_mutex being held + */ +void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +{ + snd_soc_runtime_action(rtd, stream, 1); +} + +/** * snd_soc_runtime_deactivate() - Decrement active count for PCM runtime components * @rtd: ASoC PCM runtime that is deactivated * @stream: Direction of the PCM stream @@ -130,28 +136,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) */ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream) { - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - lockdep_assert_held(&rtd->card->pcm_mutex); - - if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active--; - } else { - cpu_dai->capture_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active--; - } - - cpu_dai->active--; - cpu_dai->component->active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->component->active--; - codec_dai->active--; - } + snd_soc_runtime_action(rtd, stream, -1); }
/**
The patch
ASoC: soc-pcm: add snd_soc_runtime_action()
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From 7a5aaba4a4f45acc8192beb8a4b1bd4a58b67ce3 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:12 +0900 Subject: [PATCH] ASoC: soc-pcm: add snd_soc_runtime_action()
ALSA SoC has snd_soc_runtime_activate() / snd_soc_runtime_deactivate(). These increment or decrement DAI/Component activity, but the code difference is only +1 or -1. This patch adds common snd_soc_runtime_action() which can get +1 or -1 as parameter, and use it from snd_soc_runtime_activate/deactivate() to avoid duplicate implementation.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/87blq7ceyq.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 67 ++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ff1b7c7078e5..4d26558fcbfc 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -82,17 +82,8 @@ static int soc_rtd_trigger(struct snd_soc_pcm_runtime *rtd, return 0; }
-/** - * snd_soc_runtime_activate() - Increment active count for PCM runtime components - * @rtd: ASoC PCM runtime that is activated - * @stream: Direction of the PCM stream - * - * Increments the active count for all the DAIs and components attached to a PCM - * runtime. Should typically be called when a stream is opened. - * - * Must be called with the rtd->card->pcm_mutex being held - */ -void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +static void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd, + int stream, int action) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; @@ -101,23 +92,38 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active++; + cpu_dai->playback_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active++; + codec_dai->playback_active += action; } else { - cpu_dai->capture_active++; + cpu_dai->capture_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active++; + codec_dai->capture_active += action; }
- cpu_dai->active++; - cpu_dai->component->active++; + cpu_dai->active += action; + cpu_dai->component->active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->active++; - codec_dai->component->active++; + codec_dai->active += action; + codec_dai->component->active += action; } }
+/** + * snd_soc_runtime_activate() - Increment active count for PCM runtime components + * @rtd: ASoC PCM runtime that is activated + * @stream: Direction of the PCM stream + * + * Increments the active count for all the DAIs and components attached to a PCM + * runtime. Should typically be called when a stream is opened. + * + * Must be called with the rtd->card->pcm_mutex being held + */ +void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +{ + snd_soc_runtime_action(rtd, stream, 1); +} + /** * snd_soc_runtime_deactivate() - Decrement active count for PCM runtime components * @rtd: ASoC PCM runtime that is deactivated @@ -130,28 +136,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) */ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream) { - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - lockdep_assert_held(&rtd->card->pcm_mutex); - - if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active--; - } else { - cpu_dai->capture_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active--; - } - - cpu_dai->active--; - cpu_dai->component->active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->component->active--; - codec_dai->active--; - } + snd_soc_runtime_action(rtd, stream, -1); }
/**
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
commit 3635bf09a89cf ("ASoC: soc-pcm: add symmetry for channels and sample bits") set 0 not only to dai->rate but also to dai->channels and dai->sample_bits if DAI was not active at soc_pcm_close().
and
commit d3383420c969c ("ASoC: soc-pcm: move DAIs parameters cleaning into hw_free()") moved it from soc_pcm_close() to soc_pcm_hw_free().
These happen at v3.14. But, maybe because of branch merge conflict or something similar happen then, soc_pcm_close() still has old settings (care only dai->rate, doesn't care dai->channels/sample_bits). This is 100% duplicated operation.
This patch removes soc_pcm_close() side operation which supposed to already moved to soc_pcm_hw_free().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- v2 -> v3
- add Ranjani's Reviewed-by
sound/soc/soc-pcm.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 4d26558..2a4f7ac 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -687,15 +687,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
snd_soc_runtime_deactivate(rtd, substream->stream);
- /* clear the corresponding DAIs rate when inactive */ - if (!cpu_dai->active) - cpu_dai->rate = 0; - - for_each_rtd_codec_dai(rtd, i, codec_dai) { - if (!codec_dai->active) - codec_dai->rate = 0; - } - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
snd_soc_dai_shutdown(cpu_dai, substream);
The patch
ASoC: soc-pcm: adjustment for DAI member 0 reset
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From 5c25bd641a7b195b5ed71ce9d6955618bae7b7d3 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:18 +0900 Subject: [PATCH] ASoC: soc-pcm: adjustment for DAI member 0 reset
commit 3635bf09a89cf ("ASoC: soc-pcm: add symmetry for channels and sample bits") set 0 not only to dai->rate but also to dai->channels and dai->sample_bits if DAI was not active at soc_pcm_close().
and
commit d3383420c969c ("ASoC: soc-pcm: move DAIs parameters cleaning into hw_free()") moved it from soc_pcm_close() to soc_pcm_hw_free().
These happen at v3.14. But, maybe because of branch merge conflict or something similar happen then, soc_pcm_close() still has old settings (care only dai->rate, doesn't care dai->channels/sample_bits). This is 100% duplicated operation.
This patch removes soc_pcm_close() side operation which supposed to already moved to soc_pcm_hw_free().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/87a75rceyl.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 4d26558fcbfc..2a4f7ac5f563 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -687,15 +687,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
snd_soc_runtime_deactivate(rtd, substream->stream);
- /* clear the corresponding DAIs rate when inactive */ - if (!cpu_dai->active) - cpu_dai->rate = 0; - - for_each_rtd_codec_dai(rtd, i, codec_dai) { - if (!codec_dai->active) - codec_dai->rate = 0; - } - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
snd_soc_dai_shutdown(cpu_dai, substream);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch adds new for_each_dapm_widgets() macro and use it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- v2 -> v3
- add Ranjani's Reviewed-by
include/sound/soc-dapm.h | 5 +++++ sound/soc/soc-dapm.c | 8 ++------ sound/soc/soc-pcm.c | 17 +++++++++-------- 3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 2a306c6..9439e75 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -693,6 +693,11 @@ struct snd_soc_dapm_widget_list { struct snd_soc_dapm_widget *widgets[0]; };
+#define for_each_dapm_widgets(list, i, widget) \ + for ((i) = 0; \ + (i) < list->num_widgets && (widget = list->widgets[i]); \ + (i)++) + struct snd_soc_dapm_stats { int power_checks; int path_checks; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index bc20ad9..cc17a37 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1724,9 +1724,7 @@ static void dapm_widget_update(struct snd_soc_card *card)
wlist = dapm_kcontrol_get_wlist(update->kcontrol);
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_PRE_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_PRE_REG); if (ret != 0) @@ -1753,9 +1751,7 @@ static void dapm_widget_update(struct snd_soc_card *card) w->name, ret); }
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_POST_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG); if (ret != 0) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 2a4f7ac..7a490c0 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1306,12 +1306,12 @@ static inline struct snd_soc_dapm_widget * static int widget_in_list(struct snd_soc_dapm_widget_list *list, struct snd_soc_dapm_widget *widget) { + struct snd_soc_dapm_widget *w; int i;
- for (i = 0; i < list->num_widgets; i++) { - if (widget == list->widgets[i]) + for_each_dapm_widgets(list, i, w) + if (widget == w) return 1; - }
return 0; } @@ -1422,12 +1422,13 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_card *card = fe->card; struct snd_soc_dapm_widget_list *list = *list_; struct snd_soc_pcm_runtime *be; + struct snd_soc_dapm_widget *widget; int i, new = 0, err;
/* Create any new FE <--> BE connections */ - for (i = 0; i < list->num_widgets; i++) { + for_each_dapm_widgets(list, i, widget) {
- switch (list->widgets[i]->id) { + switch (widget->id) { case snd_soc_dapm_dai_in: if (stream != SNDRV_PCM_STREAM_PLAYBACK) continue; @@ -1441,10 +1442,10 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, }
/* is there a valid BE rtd for this widget */ - be = dpcm_get_be(card, list->widgets[i], stream); + be = dpcm_get_be(card, widget, stream); if (!be) { dev_err(fe->dev, "ASoC: no BE found for %s\n", - list->widgets[i]->name); + widget->name); continue; }
@@ -1460,7 +1461,7 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, err = dpcm_be_connect(fe, be, stream); if (err < 0) { dev_err(fe->dev, "ASoC: can't connect %s\n", - list->widgets[i]->name); + widget->name); break; } else if (err == 0) /* already connected */ continue;
The patch
ASoC: soc-pcm: add for_each_dapm_widgets() macro
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From 09e88f8a5c56ac5258935a5a543868c20a55d4dd Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:22 +0900 Subject: [PATCH] ASoC: soc-pcm: add for_each_dapm_widgets() macro
This patch adds new for_each_dapm_widgets() macro and use it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/878slbceyg.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- include/sound/soc-dapm.h | 5 +++++ sound/soc/soc-dapm.c | 8 ++------ sound/soc/soc-pcm.c | 17 +++++++++-------- 3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 2a306c6f3fbc..9439e75945f6 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -693,6 +693,11 @@ struct snd_soc_dapm_widget_list { struct snd_soc_dapm_widget *widgets[0]; };
+#define for_each_dapm_widgets(list, i, widget) \ + for ((i) = 0; \ + (i) < list->num_widgets && (widget = list->widgets[i]); \ + (i)++) + struct snd_soc_dapm_stats { int power_checks; int path_checks; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index bc20ad9abf8b..cc17a3730d3d 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1724,9 +1724,7 @@ static void dapm_widget_update(struct snd_soc_card *card)
wlist = dapm_kcontrol_get_wlist(update->kcontrol);
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_PRE_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_PRE_REG); if (ret != 0) @@ -1753,9 +1751,7 @@ static void dapm_widget_update(struct snd_soc_card *card) w->name, ret); }
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_POST_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG); if (ret != 0) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 2a4f7ac5f563..7a490c05d4e9 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1306,12 +1306,12 @@ static inline struct snd_soc_dapm_widget * static int widget_in_list(struct snd_soc_dapm_widget_list *list, struct snd_soc_dapm_widget *widget) { + struct snd_soc_dapm_widget *w; int i;
- for (i = 0; i < list->num_widgets; i++) { - if (widget == list->widgets[i]) + for_each_dapm_widgets(list, i, w) + if (widget == w) return 1; - }
return 0; } @@ -1422,12 +1422,13 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_card *card = fe->card; struct snd_soc_dapm_widget_list *list = *list_; struct snd_soc_pcm_runtime *be; + struct snd_soc_dapm_widget *widget; int i, new = 0, err;
/* Create any new FE <--> BE connections */ - for (i = 0; i < list->num_widgets; i++) { + for_each_dapm_widgets(list, i, widget) {
- switch (list->widgets[i]->id) { + switch (widget->id) { case snd_soc_dapm_dai_in: if (stream != SNDRV_PCM_STREAM_PLAYBACK) continue; @@ -1441,10 +1442,10 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, }
/* is there a valid BE rtd for this widget */ - be = dpcm_get_be(card, list->widgets[i], stream); + be = dpcm_get_be(card, widget, stream); if (!be) { dev_err(fe->dev, "ASoC: no BE found for %s\n", - list->widgets[i]->name); + widget->name); continue; }
@@ -1460,7 +1461,7 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, err = dpcm_be_connect(fe, be, stream); if (err < 0) { dev_err(fe->dev, "ASoC: can't connect %s\n", - list->widgets[i]->name); + widget->name); break; } else if (err == 0) /* already connected */ continue;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc-pcm is using bit-OR'ed error
ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream);
The driver may return arbitrary error codes so they can conflict. The bit-OR'ed error works only if the return code is always consistent. This patch fixup it, and use *last* ret value.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v2 -> v3
- new patch
sound/soc/soc-pcm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7a490c0..8d8ed47 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -498,13 +498,16 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; - int i, ret = 0; + int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { if (component == last) break;
- ret |= snd_soc_component_close(component, substream); + r = snd_soc_component_close(component, substream); + if (r < 0) + ret = r; /* use last ret */ + snd_soc_component_module_put_when_close(component); }
@@ -798,13 +801,15 @@ static int soc_pcm_components_hw_free(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; - int i, ret = 0; + int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { if (component == last) break;
- ret |= snd_soc_component_hw_free(component, substream); + r = snd_soc_component_hw_free(component, substream); + if (r < 0) + ret = r; /* use last ret */ }
return ret;
The patch
ASoC: soc-pcm: don't use bit-OR'ed error
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From e82ebffce3ec07584bcc2fc4c4d33a43fd9515f5 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:26 +0900 Subject: [PATCH] ASoC: soc-pcm: don't use bit-OR'ed error
Current soc-pcm is using bit-OR'ed error
ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream);
The driver may return arbitrary error codes so they can conflict. The bit-OR'ed error works only if the return code is always consistent. This patch fixup it, and use *last* ret value.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/877e0vceyc.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7a490c05d4e9..8d8ed4774e9c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -498,13 +498,16 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; - int i, ret = 0; + int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { if (component == last) break;
- ret |= snd_soc_component_close(component, substream); + r = snd_soc_component_close(component, substream); + if (r < 0) + ret = r; /* use last ret */ + snd_soc_component_module_put_when_close(component); }
@@ -798,13 +801,15 @@ static int soc_pcm_components_hw_free(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; - int i, ret = 0; + int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { if (component == last) break;
- ret |= snd_soc_component_hw_free(component, substream); + r = snd_soc_component_hw_free(component, substream); + if (r < 0) + ret = r; /* use last ret */ }
return ret;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_pcm_open() calls snd_soc_dai_startup() under loop. Thus, it needs to care about started/not-yet-started codec DAI.
But, if soc-dai.c is handling it, soc-pcm.c don't need to care about it. This patch adds started flag to soc-dai.h, and simplify soc-pcm.c. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v2 -> v3
- use flag - Startuped DAI only can call Shutdown, and it can't Start again without Shutdown.
include/sound/soc-dai.h | 5 ++++- sound/soc/soc-dai.c | 11 +++++++++-- sound/soc/soc-pcm.c | 7 ++----- 3 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index eaaeb00..04c23ac 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -324,7 +324,6 @@ struct snd_soc_dai { /* DAI runtime info */ unsigned int capture_active; /* stream usage count */ unsigned int playback_active; /* stream usage count */ - unsigned int probed:1;
unsigned int active;
@@ -348,6 +347,10 @@ struct snd_soc_dai { unsigned int rx_mask;
struct list_head list; + + /* bit field */ + unsigned int probed:1; + unsigned int started:1; };
static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 51031e33..73a8293 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -295,17 +295,24 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, { int ret = 0;
- if (dai->driver->ops->startup) + if (!dai->started && + dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai);
+ if (ret == 0) + dai->started = 1; + return ret; }
void snd_soc_dai_shutdown(struct snd_soc_dai *dai, struct snd_pcm_substream *substream) { - if (dai->driver->ops->shutdown) + if (dai->started && + dai->driver->ops->shutdown) dai->driver->ops->shutdown(substream, dai); + + dai->started = 0; }
int snd_soc_dai_prepare(struct snd_soc_dai *dai, diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8d8ed47..d53afb9 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -568,7 +568,7 @@ static int soc_pcm_open(struct snd_pcm_substream *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 */ @@ -637,11 +637,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:
The patch
ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From b56be800f1292c9b79c4f66571c701551bdf9e12 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:33 +0900 Subject: [PATCH] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once
Current soc_pcm_open() calls snd_soc_dai_startup() under loop. Thus, it needs to care about started/not-yet-started codec DAI.
But, if soc-dai.c is handling it, soc-pcm.c don't need to care about it. This patch adds started flag to soc-dai.h, and simplify soc-pcm.c. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/875zgfcey5.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- include/sound/soc-dai.h | 5 ++++- sound/soc/soc-dai.c | 11 +++++++++-- sound/soc/soc-pcm.c | 7 ++----- 3 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index eaaeb00e9e84..04c23ac0dfff 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -324,7 +324,6 @@ struct snd_soc_dai { /* DAI runtime info */ unsigned int capture_active; /* stream usage count */ unsigned int playback_active; /* stream usage count */ - unsigned int probed:1;
unsigned int active;
@@ -348,6 +347,10 @@ struct snd_soc_dai { unsigned int rx_mask;
struct list_head list; + + /* bit field */ + unsigned int probed:1; + unsigned int started:1; };
static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 51031e330179..73a829393652 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -295,17 +295,24 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, { int ret = 0;
- if (dai->driver->ops->startup) + if (!dai->started && + dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai);
+ if (ret == 0) + dai->started = 1; + return ret; }
void snd_soc_dai_shutdown(struct snd_soc_dai *dai, struct snd_pcm_substream *substream) { - if (dai->driver->ops->shutdown) + if (dai->started && + dai->driver->ops->shutdown) dai->driver->ops->shutdown(substream, dai); + + dai->started = 0; }
int snd_soc_dai_prepare(struct snd_soc_dai *dai, diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8d8ed4774e9c..d53afb96b05b 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -568,7 +568,7 @@ static int soc_pcm_open(struct snd_pcm_substream *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 */ @@ -637,11 +637,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:
On 2/10/2020 4:14 AM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_pcm_open() calls snd_soc_dai_startup() under loop. Thus, it needs to care about started/not-yet-started codec DAI.
But, if soc-dai.c is handling it, soc-pcm.c don't need to care about it. This patch adds started flag to soc-dai.h, and simplify soc-pcm.c. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
(...)
static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 51031e33..73a8293 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -295,17 +295,24 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, { int ret = 0;
- if (dai->driver->ops->startup)
if (!dai->started &&
dai->driver->ops->startup)ret = dai->driver->ops->startup(substream, dai);
if (ret == 0)
dai->started = 1;return ret; }
Hi,
the above change breaks simultaneous playback and capture on single DAI in more complicated use cases. With above change when one runs playback first, startup callback is skipped when running capture while playback is still running.
With snd_soc_skl it leads to null pointer dereference, because we didn't initialize streams properly:
[ 78.901574] dpcm_be_dai_hw_params:2219: Analog Playback and Capture: ASoC: hw_params BE Analog Playback and Capture [ 78.901582] dapm_update_dai_unlocked:2638: snd_hda_codec_realtek ehdaudio0D0: Update DAI routes for Analog Codec DAI capture [ 78.901585] dapm_update_dai_chan:2612: snd_hda_codec_realtek ehdaudio0D0: Connecting DAI route AIF3TX -> Analog Codec Capture [ 78.901590] dapm_update_dai_chan:2612: snd_hda_codec_realtek ehdaudio0D0: Connecting DAI route AIF1TX -> Analog Codec Capture [ 78.901608] dapm_update_dai_unlocked:2638: snd_soc_skl 0000:00:1f.3: Update DAI routes for Analog CPU DAI capture [ 78.901612] dapm_update_dai_chan:2612: snd_soc_skl 0000:00:1f.3: Connecting DAI route Analog CPU Capture -> codec0_in [ 78.901615] dpcm_fe_dai_hw_params:2277: Analog HDA DSP: ASoC: hw_params FE Analog HDA DSP rate 48000 chan 2 fmt 2 [ 78.901622] skl_pcm_hw_params:307: snd_soc_skl 0000:00:1f.3: skl_pcm_hw_params: hda-dsp-analog-dai [ 78.901624] ================================================================== [ 78.907515] BUG: KASAN: null-ptr-deref in skl_pcm_hw_params+0x102/0x3d0 [snd_soc_skl] [ 78.914003] Write of size 4 at addr 0000000000000044 by task arecord/2119
Amadeusz
static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai, diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 51031e33..73a8293 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -295,17 +295,24 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, { int ret = 0; - if (dai->driver->ops->startup) + if (!dai->started && + dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai); + if (ret == 0) + dai->started = 1;
return ret; }
Hi,
the above change breaks simultaneous playback and capture on single DAI in more complicated use cases. With above change when one runs playback first, startup callback is skipped when running capture while playback is still running.
Should the 'started' bitfield should be an array for capture and playback cases respectively? e.g.
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 78bac995db15..d4825b82c7a3 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -351,7 +351,7 @@ struct snd_soc_dai {
/* bit field */ unsigned int probed:1; - unsigned int started:1; + unsigned int started[SNDRV_PCM_STREAM_LAST + 1]; };
static inline struct snd_soc_pcm_stream * diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index 19142f6e533c..8f3cad8db89a 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -295,12 +295,12 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, { int ret = 0;
- if (!dai->started && + if (!dai->started[substream->stream] && dai->driver->ops->startup) ret = dai->driver->ops->startup(substream, dai);
if (ret == 0) - dai->started = 1; + dai->started[substream->stream] = 1;
return ret; } @@ -308,11 +308,11 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai, void snd_soc_dai_shutdown(struct snd_soc_dai *dai, struct snd_pcm_substream *substream) { - if (dai->started && + if (dai->started[substream->stream] && dai->driver->ops->shutdown) dai->driver->ops->shutdown(substream, dai);
- dai->started = 0; + dai->started[substream->stream] = 0; }
int snd_soc_dai_prepare(struct snd_soc_dai *dai,
Hi Amadeusz, Pierre-Louis
Thank you for your feedback, and sorry to bother you.
the above change breaks simultaneous playback and capture on single DAI in more complicated use cases. With above change when one runs playback first, startup callback is skipped when running capture while playback is still running.
Similar issue had been happened on component open before. https://lore.kernel.org/alsa-devel/20200219182650.1416-1-kai.vehmanen@linux....
I'm so sorry but this is bug. In my quick check, this patch is not related to other patches. So, just reverting it is nice idea, I think.
Should the 'started' bitfield should be an array for capture and playback cases respectively? e.g.
Yeah. But, I will re-try this issue (for DAI, for Component) again. Let's just revert it so far. Is it OK for you ?
Thank you for your help !!
Best regards --- Kuninori Morimoto
Thank you for your help !!
Best regards --- Kuninori Morimoto
On 3/30/2020 3:10 AM, Kuninori Morimoto wrote:
Hi Amadeusz, Pierre-Louis
Thank you for your feedback, and sorry to bother you.
the above change breaks simultaneous playback and capture on single DAI in more complicated use cases. With above change when one runs playback first, startup callback is skipped when running capture while playback is still running.
Similar issue had been happened on component open before. https://lore.kernel.org/alsa-devel/20200219182650.1416-1-kai.vehmanen@linux....
I'm so sorry but this is bug. In my quick check, this patch is not related to other patches. So, just reverting it is nice idea, I think.
Should the 'started' bitfield should be an array for capture and playback cases respectively? e.g.
Yeah. But, I will re-try this issue (for DAI, for Component) again. Let's just revert it so far. Is it OK for you ?
Thank you for your help !!
Hi,
I tested patch from Pierre and it works for me, I'm also ok with revert.
Thanks, Amadeusz
On Mon, Mar 30, 2020 at 09:25:31AM +0200, Amadeusz Sławiński wrote:
On 3/30/2020 3:10 AM, Kuninori Morimoto wrote:
But, I will re-try this issue (for DAI, for Component) again. Let's just revert it so far. Is it OK for you ?
I tested patch from Pierre and it works for me, I'm also ok with revert.
Pierre's patch makes sense to me and this is the only reported issue so I'd be happy with that. Whichever way can one (or both!) of you please send a patch?
Hi Mark
I tested patch from Pierre and it works for me, I'm also ok with revert.
Pierre's patch makes sense to me and this is the only reported issue so I'd be happy with that. Whichever way can one (or both!) of you please send a patch?
I have no objection about Pierre's patch. But I want to re-make it in future.
Thank you for your help !!
Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_pcm_open() calls snd_soc_component_open() under loop. Thus, it needs to care about opened/not-yet-opened Component.
But, if soc-component.c is handling it, soc-pcm.c don't need to care about it. This patch adds opened flag to soc-component.h, and simplify soc-pcm.c. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v2 -> v3
- use flag - Opened Component only can call Close, and it can't open again without close.
include/sound/soc-component.h | 7 +++++-- sound/soc/soc-component.c | 35 ++++++++++++++++++++++++++++------- sound/soc/soc-pcm.c | 19 ++++++------------- 3 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 154d02f..1866ecc 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -147,8 +147,6 @@ struct snd_soc_component {
unsigned int active;
- unsigned int suspended:1; /* is in suspend PM state */ - struct list_head list; struct list_head card_aux_list; /* for auxiliary bound components */ struct list_head card_list; @@ -182,6 +180,11 @@ struct snd_soc_component { struct dentry *debugfs_root; const char *debugfs_prefix; #endif + + /* bit field */ + unsigned int suspended:1; /* is in suspend PM state */ + unsigned int opened:1; + unsigned int module:1; };
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 14e175c..ee00c09 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,34 +297,55 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { + if (component->module) + return 0; + if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
+ component->module = 1; + return 0; }
void snd_soc_component_module_put(struct snd_soc_component *component, int upon_open) { - if (component->driver->module_get_upon_open == !!upon_open) + if (component->module && + component->driver->module_get_upon_open == !!upon_open) module_put(component->dev->driver->owner); + + component->module = 0; }
int snd_soc_component_open(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - if (component->driver->open) - return component->driver->open(component, substream); - return 0; + int ret = 0; + + if (!component->opened && + component->driver->open) + ret = component->driver->open(component, substream); + + if (ret == 0) + component->opened = 1; + + return ret; }
int snd_soc_component_close(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - if (component->driver->close) - return component->driver->close(component, substream); - return 0; + int ret = 0; + + if (component->opened && + component->driver->close) + ret = component->driver->close(component, substream); + + component->opened = 0; + + return ret; }
int snd_soc_component_prepare(struct snd_soc_component *component, diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d53afb9..ae94d8a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -463,16 +463,13 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); }
-static int soc_pcm_components_open(struct snd_pcm_substream *substream, - struct snd_soc_component **last) +static int soc_pcm_components_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) { - *last = component; - ret = snd_soc_component_module_get_when_open(component); if (ret < 0) { dev_err(component->dev, @@ -489,21 +486,17 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return ret; } } - *last = NULL; + return 0; }
-static int soc_pcm_components_close(struct snd_pcm_substream *substream, - struct snd_soc_component *last) +static int soc_pcm_components_close(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { - if (component == last) - break; - r = snd_soc_component_close(component, substream); if (r < 0) ret = r; /* use last ret */ @@ -545,7 +538,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto out; }
- ret = soc_pcm_components_open(substream, &component); + ret = soc_pcm_components_open(substream); if (ret < 0) goto component_err;
@@ -642,7 +635,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) snd_soc_dai_shutdown(codec_dai, substream);
component_err: - soc_pcm_components_close(substream, component); + soc_pcm_components_close(substream);
snd_soc_dai_shutdown(cpu_dai, substream); out: @@ -696,7 +689,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
soc_rtd_shutdown(rtd, substream);
- soc_pcm_components_close(substream, NULL); + soc_pcm_components_close(substream);
snd_soc_dapm_stream_stop(rtd, substream->stream);
Hi Morimoto-san,
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 14e175c..ee00c09 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,34 +297,55 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) {
if (component->module)
return 0;if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
component->module = 1;
Maybe a red-herring but is there a potential for race conditions here if that function is called twice from different places? Don't we need some sort of lock for all the new flags introduced here?
Thanks -Pierre
On Mon, Feb 10, 2020 at 08:41:07AM -0600, Pierre-Louis Bossart wrote:
Maybe a red-herring but is there a potential for race conditions here if that function is called twice from different places? Don't we need some sort of lock for all the new flags introduced here?
The probe stuff is all going to get pretty upset if it's called from multiple paths already.
Hi Pierre-Louis, Mark
Maybe a red-herring but is there a potential for race conditions here if that function is called twice from different places? Don't we need some sort of lock for all the new flags introduced here?
The probe stuff is all going to get pretty upset if it's called from multiple paths already.
Hmm.. indeed we need to consider about it. But I think it can/should/will be incremental patch. Thank you for pointing it.
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis, Mark
Maybe a red-herring but is there a potential for race conditions here if that function is called twice from different places? Don't we need some sort of lock for all the new flags introduced here?
The probe stuff is all going to get pretty upset if it's called from multiple paths already.
Hmm.. indeed we need to consider about it. But I think it can/should/will be incremental patch. Thank you for pointing it.
Is it OK for you ? Or, do we need v4 patch ?
Thank you for your help !! Best regards --- Kuninori Morimoto
On 2/11/20 7:16 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis, Mark
Maybe a red-herring but is there a potential for race conditions here if that function is called twice from different places? Don't we need some sort of lock for all the new flags introduced here?
The probe stuff is all going to get pretty upset if it's called from multiple paths already.
Hmm.. indeed we need to consider about it. But I think it can/should/will be incremental patch. Thank you for pointing it.
Is it OK for you ? Or, do we need v4 patch ?
I can't prove that the code is broken, this was a question. If there is a follow-up that looks into potential conflicts that's fine, I can see the benefits of the series as is so
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Thank you Morimoto-san for all this work, much appreciated.
The patch
ASoC: soc-pcm: call snd_soc_component_open/close() once
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From dd03907bf129b42e9e3203fdf405ea9873b28dd3 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:37 +0900 Subject: [PATCH] ASoC: soc-pcm: call snd_soc_component_open/close() once
Current soc_pcm_open() calls snd_soc_component_open() under loop. Thus, it needs to care about opened/not-yet-opened Component.
But, if soc-component.c is handling it, soc-pcm.c don't need to care about it. This patch adds opened flag to soc-component.h, and simplify soc-pcm.c. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/874kvzcey1.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- include/sound/soc-component.h | 7 +++++-- sound/soc/soc-component.c | 35 ++++++++++++++++++++++++++++------- sound/soc/soc-pcm.c | 19 ++++++------------- 3 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 154d02fbbfed..1866ecc8e94b 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -147,8 +147,6 @@ struct snd_soc_component {
unsigned int active;
- unsigned int suspended:1; /* is in suspend PM state */ - struct list_head list; struct list_head card_aux_list; /* for auxiliary bound components */ struct list_head card_list; @@ -182,6 +180,11 @@ struct snd_soc_component { struct dentry *debugfs_root; const char *debugfs_prefix; #endif + + /* bit field */ + unsigned int suspended:1; /* is in suspend PM state */ + unsigned int opened:1; + unsigned int module:1; };
#define for_each_component_dais(component, dai)\ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 14e175cdeeb8..ee00c09df5e7 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -297,34 +297,55 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack); int snd_soc_component_module_get(struct snd_soc_component *component, int upon_open) { + if (component->module) + return 0; + if (component->driver->module_get_upon_open == !!upon_open && !try_module_get(component->dev->driver->owner)) return -ENODEV;
+ component->module = 1; + return 0; }
void snd_soc_component_module_put(struct snd_soc_component *component, int upon_open) { - if (component->driver->module_get_upon_open == !!upon_open) + if (component->module && + component->driver->module_get_upon_open == !!upon_open) module_put(component->dev->driver->owner); + + component->module = 0; }
int snd_soc_component_open(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - if (component->driver->open) - return component->driver->open(component, substream); - return 0; + int ret = 0; + + if (!component->opened && + component->driver->open) + ret = component->driver->open(component, substream); + + if (ret == 0) + component->opened = 1; + + return ret; }
int snd_soc_component_close(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - if (component->driver->close) - return component->driver->close(component, substream); - return 0; + int ret = 0; + + if (component->opened && + component->driver->close) + ret = component->driver->close(component, substream); + + component->opened = 0; + + return ret; }
int snd_soc_component_prepare(struct snd_soc_component *component, diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d53afb96b05b..ae94d8a86992 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -463,16 +463,13 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); }
-static int soc_pcm_components_open(struct snd_pcm_substream *substream, - struct snd_soc_component **last) +static int soc_pcm_components_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) { - *last = component; - ret = snd_soc_component_module_get_when_open(component); if (ret < 0) { dev_err(component->dev, @@ -489,21 +486,17 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, return ret; } } - *last = NULL; + return 0; }
-static int soc_pcm_components_close(struct snd_pcm_substream *substream, - struct snd_soc_component *last) +static int soc_pcm_components_close(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, r, ret = 0;
for_each_rtd_components(rtd, i, component) { - if (component == last) - break; - r = snd_soc_component_close(component, substream); if (r < 0) ret = r; /* use last ret */ @@ -545,7 +538,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto out; }
- ret = soc_pcm_components_open(substream, &component); + ret = soc_pcm_components_open(substream); if (ret < 0) goto component_err;
@@ -642,7 +635,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) snd_soc_dai_shutdown(codec_dai, substream);
component_err: - soc_pcm_components_close(substream, component); + soc_pcm_components_close(substream);
snd_soc_dai_shutdown(cpu_dai, substream); out: @@ -696,7 +689,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
soc_rtd_shutdown(rtd, substream);
- soc_pcm_components_close(substream, NULL); + soc_pcm_components_close(substream);
snd_soc_dapm_stream_stop(rtd, substream->stream);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves soc_pcm_close() next to soc_pcm_open(). This is prepare for soc_pcm_open() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- v2 -> v3
- add Ranjani's Reviewed-by
sound/soc/soc-pcm.c | 88 ++++++++++++++++++++++++++--------------------------- 1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ae94d8a..8aa775e0 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -508,6 +508,50 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream) }
/* + * Called by ALSA when a PCM substream is closed. Private data can be + * freed here. The cpu DAI, codec DAI, machine and components are also + * shutdown. + */ +static int soc_pcm_close(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_component *component; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct snd_soc_dai *codec_dai; + int i; + + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); + + snd_soc_runtime_deactivate(rtd, substream->stream); + + snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); + + snd_soc_dai_shutdown(cpu_dai, substream); + + for_each_rtd_codec_dai(rtd, i, codec_dai) + snd_soc_dai_shutdown(codec_dai, substream); + + soc_rtd_shutdown(rtd, substream); + + soc_pcm_components_close(substream); + + snd_soc_dapm_stream_stop(rtd, substream->stream); + + mutex_unlock(&rtd->card->pcm_mutex); + + for_each_rtd_components(rtd, i, component) { + pm_runtime_mark_last_busy(component->dev); + pm_runtime_put_autosuspend(component->dev); + } + + for_each_rtd_components(rtd, i, component) + if (!component->active) + pinctrl_pm_select_sleep_state(component->dev); + + return 0; +} + +/* * Called by ALSA when a PCM substream is opened, the runtime->hw record is * then initialized and any private data can be allocated. This also calls * startup for the cpu DAI, component, machine and codec DAI. @@ -664,50 +708,6 @@ static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd) }
/* - * Called by ALSA when a PCM substream is closed. Private data can be - * freed here. The cpu DAI, codec DAI, machine and components are also - * shutdown. - */ -static int soc_pcm_close(struct snd_pcm_substream *substream) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_component *component; - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); - - snd_soc_runtime_deactivate(rtd, substream->stream); - - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); - - snd_soc_dai_shutdown(cpu_dai, substream); - - for_each_rtd_codec_dai(rtd, i, codec_dai) - snd_soc_dai_shutdown(codec_dai, substream); - - soc_rtd_shutdown(rtd, substream); - - soc_pcm_components_close(substream); - - snd_soc_dapm_stream_stop(rtd, substream->stream); - - mutex_unlock(&rtd->card->pcm_mutex); - - for_each_rtd_components(rtd, i, component) { - pm_runtime_mark_last_busy(component->dev); - pm_runtime_put_autosuspend(component->dev); - } - - for_each_rtd_components(rtd, i, component) - if (!component->active) - pinctrl_pm_select_sleep_state(component->dev); - - return 0; -} - -/* * Called by ALSA when the PCM substream is prepared, can set format, sample * rate, etc. This function is non atomic and can be called multiple times, * it can refer to the runtime info.
The patch
ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From 62c86d1d5fd942c741791be94f670b99ffedfb5c Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:41 +0900 Subject: [PATCH] ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()
This patch moves soc_pcm_close() next to soc_pcm_open(). This is prepare for soc_pcm_open() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/8736bjcexx.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 88 ++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ae94d8a86992..8aa775e0eb0d 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -507,6 +507,50 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream) return ret; }
+/* + * Called by ALSA when a PCM substream is closed. Private data can be + * freed here. The cpu DAI, codec DAI, machine and components are also + * shutdown. + */ +static int soc_pcm_close(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_component *component; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct snd_soc_dai *codec_dai; + int i; + + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); + + snd_soc_runtime_deactivate(rtd, substream->stream); + + snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); + + snd_soc_dai_shutdown(cpu_dai, substream); + + for_each_rtd_codec_dai(rtd, i, codec_dai) + snd_soc_dai_shutdown(codec_dai, substream); + + soc_rtd_shutdown(rtd, substream); + + soc_pcm_components_close(substream); + + snd_soc_dapm_stream_stop(rtd, substream->stream); + + mutex_unlock(&rtd->card->pcm_mutex); + + for_each_rtd_components(rtd, i, component) { + pm_runtime_mark_last_busy(component->dev); + pm_runtime_put_autosuspend(component->dev); + } + + for_each_rtd_components(rtd, i, component) + if (!component->active) + pinctrl_pm_select_sleep_state(component->dev); + + return 0; +} + /* * Called by ALSA when a PCM substream is opened, the runtime->hw record is * then initialized and any private data can be allocated. This also calls @@ -663,50 +707,6 @@ static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd) */ }
-/* - * Called by ALSA when a PCM substream is closed. Private data can be - * freed here. The cpu DAI, codec DAI, machine and components are also - * shutdown. - */ -static int soc_pcm_close(struct snd_pcm_substream *substream) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_component *component; - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); - - snd_soc_runtime_deactivate(rtd, substream->stream); - - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); - - snd_soc_dai_shutdown(cpu_dai, substream); - - for_each_rtd_codec_dai(rtd, i, codec_dai) - snd_soc_dai_shutdown(codec_dai, substream); - - soc_rtd_shutdown(rtd, substream); - - soc_pcm_components_close(substream); - - snd_soc_dapm_stream_stop(rtd, substream->stream); - - mutex_unlock(&rtd->card->pcm_mutex); - - for_each_rtd_components(rtd, i, component) { - pm_runtime_mark_last_busy(component->dev); - pm_runtime_put_autosuspend(component->dev); - } - - for_each_rtd_components(rtd, i, component) - if (!component->active) - pinctrl_pm_select_sleep_state(component->dev); - - return 0; -} - /* * Called by ALSA when the PCM substream is prepared, can set format, sample * rate, etc. This function is non atomic and can be called multiple times,
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() operation order is not good. At first, soc_pcm_open() operation order is
1) CPU DAI startup 2) Component open 3) Codec DAI startup 4) rtd startup
But here, 2) will call try_module_get() if component has module_get_upon_open flags. This means 1) CPU DAI startup will be operated *before* its module was loaded. DAI should be called *after* Component.
Second, soc_pcm_close() operation order is 1) CPU DAI shutdown 2) Codec DAI shutdown 3) rtd shutdown 4) Component close
soc_pcm_open() and soc_pcm_close() are paired function, but, its operation order is unbalance. This patch tidyup soc_pcm_open() order to Component -> rtd -> DAI. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- v2 -> v3
- add Ranjani's Reviewed-by
sound/soc/soc-pcm.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8aa775e0..6630fad 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -574,25 +574,32 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ ret = soc_pcm_components_open(substream); + if (ret < 0) + goto component_err; + + ret = soc_rtd_startup(rtd, substream); + if (ret < 0) { + pr_err("ASoC: %s startup failed: %d\n", + rtd->dai_link->name, ret); + goto component_err; + } + /* startup the audio subsystem */ ret = snd_soc_dai_startup(cpu_dai, substream); if (ret < 0) { dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n", cpu_dai->name, ret); - goto out; + goto cpu_dai_err; }
- ret = soc_pcm_components_open(substream); - if (ret < 0) - 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; + goto config_err; }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) @@ -601,13 +608,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) codec_dai->rx_mask = 0; }
- ret = soc_rtd_startup(rtd, substream); - if (ret < 0) { - pr_err("ASoC: %s startup failed: %d\n", - rtd->dai_link->name, ret); - goto codec_dai_err; - } - /* Dynamic PCM DAI links compat checks use dynamic capabilities */ if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) goto dynamic; @@ -672,17 +672,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) return 0;
config_err: - soc_rtd_shutdown(rtd, substream); - -codec_dai_err: for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream); +cpu_dai_err: + snd_soc_dai_shutdown(cpu_dai, substream);
+ soc_rtd_shutdown(rtd, substream); component_err: soc_pcm_components_close(substream);
- snd_soc_dai_shutdown(cpu_dai, substream); -out: mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtd_components(rtd, i, component) {
The patch
ASoC: soc-pcm: tidyup soc_pcm_open() order
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.7
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
From 5d9fa03e6c3514266fa94851ab1b6dd6e0595a13 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Mon, 10 Feb 2020 12:14:45 +0900 Subject: [PATCH] ASoC: soc-pcm: tidyup soc_pcm_open() order
soc_pcm_open() operation order is not good. At first, soc_pcm_open() operation order is
1) CPU DAI startup 2) Component open 3) Codec DAI startup 4) rtd startup
But here, 2) will call try_module_get() if component has module_get_upon_open flags. This means 1) CPU DAI startup will be operated *before* its module was loaded. DAI should be called *after* Component.
Second, soc_pcm_close() operation order is 1) CPU DAI shutdown 2) Codec DAI shutdown 3) rtd shutdown 4) Component close
soc_pcm_open() and soc_pcm_close() are paired function, but, its operation order is unbalance. This patch tidyup soc_pcm_open() order to Component -> rtd -> DAI. This is one of prepare for cleanup soc-pcm-open()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/871rr3cext.wl-kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8aa775e0eb0d..6630fadd6e09 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -574,25 +574,32 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ ret = soc_pcm_components_open(substream); + if (ret < 0) + goto component_err; + + ret = soc_rtd_startup(rtd, substream); + if (ret < 0) { + pr_err("ASoC: %s startup failed: %d\n", + rtd->dai_link->name, ret); + goto component_err; + } + /* startup the audio subsystem */ ret = snd_soc_dai_startup(cpu_dai, substream); if (ret < 0) { dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n", cpu_dai->name, ret); - goto out; + goto cpu_dai_err; }
- ret = soc_pcm_components_open(substream); - if (ret < 0) - 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; + goto config_err; }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) @@ -601,13 +608,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) codec_dai->rx_mask = 0; }
- ret = soc_rtd_startup(rtd, substream); - if (ret < 0) { - pr_err("ASoC: %s startup failed: %d\n", - rtd->dai_link->name, ret); - goto codec_dai_err; - } - /* Dynamic PCM DAI links compat checks use dynamic capabilities */ if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) goto dynamic; @@ -672,17 +672,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) return 0;
config_err: - soc_rtd_shutdown(rtd, substream); - -codec_dai_err: for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream); +cpu_dai_err: + snd_soc_dai_shutdown(cpu_dai, substream);
+ soc_rtd_shutdown(rtd, substream); component_err: soc_pcm_components_close(substream);
- snd_soc_dai_shutdown(cpu_dai, substream); -out: mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtd_components(rtd, i, component) {
10.02.2020 06:13, Kuninori Morimoto пишет:
Hi Mark
These are v3 of soc-pcm cleanup. Original [5/7] [6/7] patch call open / start function for all DAI / component. And, it calls close / stop function for all if some of them got error. But in such case, error occured DAI / component don't need to call close / stop.
This issue can be solved if it had flag, and my local tree has such patch. But it was planed to post to ML a little later. This time, I merged original patch and this new flag patch on [5/7] [6/7].
These mention that "it is prepare for soc-pcm-open() cleanup". But it will be happen later.
These are based on v5.6-rc1
Kuninori Morimoto (8):
- ASoC: soc-pcm: add snd_soc_runtime_action()
- ASoC: soc-pcm: adjustment for DAI member 0 reset
- ASoC: soc-pcm: add for_each_dapm_widgets() macro
- ASoC: soc-pcm: don't use bit-OR'ed error
- ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once
- ASoC: soc-pcm: call snd_soc_component_open/close() once
- ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()
- ASoC: soc-pcm: tidyup soc_pcm_open() order
include/sound/soc-component.h | 7 +- include/sound/soc-dai.h | 5 +- include/sound/soc-dapm.h | 5 + sound/soc/soc-component.c | 35 ++++-- sound/soc/soc-dai.c | 11 +- sound/soc/soc-dapm.c | 8 +- sound/soc/soc-pcm.c | 246 +++++++++++++++++++----------------------- 7 files changed, 161 insertions(+), 156 deletions(-)
Hello,
I'm observing a NULL dereference on NVIDIA Tegra20/30 once PulseAudio is loaded.
The offending patch is:
ASoC: soc-pcm: call snd_soc_component_open/close() once
Please fix, thanks in advance.
[ 61.860826] 8<--- cut here --- [ 61.860965] Unable to handle kernel NULL pointer dereference at virtual address 00000000 [ 61.861037] pgd = ef2eab54 [ 61.861155] [00000000] *pgd=00000000 [ 61.861228] Internal error: Oops: 5 [#1] SMP THUMB2 [ 61.861298] Modules linked in: [ 61.861427] CPU: 2 PID: 599 Comm: pulseaudio Not tainted 5.6.0-rc2-next-20200218-00168-g1e584fed87b9 #1275 [ 61.861546] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 61.861626] PC is at snd_dmaengine_pcm_close+0x1c/0x3c [ 61.861756] LR is at snd_soc_component_close+0x1d/0x3c [ 61.861823] pc : [<c072a36c>] lr : [<c0751b51>] psr: 60000033 [ 61.861944] sp : dc01bc88 ip : 00000000 fp : ffffffea [ 61.862013] r10: 00000010 r9 : dd81a840 r8 : de318e00 [ 61.862080] r7 : dd81adfc r6 : 00000000 r5 : 00000003 r4 : 00000000 [ 61.862199] r3 : dc19f800 r2 : 00000000 r1 : 00000447 r0 : c0e2f438 [ 61.862322] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment none [ 61.862390] Control: 50c5387d Table: 9db0c04a DAC: 00000051 [ 61.862510] Process pulseaudio (pid: 599, stack limit = 0xcfc4cd60) [ 61.862576] Stack: (0xdc01bc88 to 0xdc01c000) [ 61.862700] bc80: c0756611 de31b60c 00000003 c0751b51 de31b60c c07525ff ... [ 61.865643] bfe0: 00000142 beb9b7e8 b6c35f0d b6bbcd56 00000030 ffffff9c 00000000 00000000 [ 61.865773] [<c072a36c>] (snd_dmaengine_pcm_close) from [<c0751b51>] (snd_soc_component_close+0x1d/0x3c) [ 61.865920] [<c0751b51>] (snd_soc_component_close) from [<c07525ff>] (soc_pcm_components_close+0x27/0x54) [ 61.865993] [<c07525ff>] (soc_pcm_components_close) from [<c0752c27>] (soc_pcm_close+0x73/0xf0) [ 61.866130] [<c0752c27>] (soc_pcm_close) from [<c072370d>] (snd_pcm_release_substream.part.0+0x29/0x6c) [ 61.866258] [<c072370d>] (snd_pcm_release_substream.part.0) from [<c0723d5f>] (snd_pcm_open_substream+0x5f7/0x638) [ 61.866383] [<c0723d5f>] (snd_pcm_open_substream) from [<c0723e45>] (snd_pcm_open+0xa5/0x184) [ 61.866510] [<c0723e45>] (snd_pcm_open) from [<c0723f51>] (snd_pcm_capture_open+0x2d/0x40) [ 61.866589] [<c0723f51>] (snd_pcm_capture_open) from [<c025fb69>] (chrdev_open+0xa9/0x148) [ 61.866724] [<c025fb69>] (chrdev_open) from [<c02588a5>] (do_dentry_open+0xd5/0x2e8) [ 61.866855] [<c02588a5>] (do_dentry_open) from [<c0267a8b>] (path_openat+0x203/0xd44) [ 61.866981] [<c0267a8b>] (path_openat) from [<c02690d5>] (do_filp_open+0x49/0x90) [ 61.867055] [<c02690d5>] (do_filp_open) from [<c0258e4b>] (do_sys_openat2+0x1a7/0x210) [ 61.867180] [<c0258e4b>] (do_sys_openat2) from [<c0259ca9>] (do_sys_open+0x6d/0x98) [ 61.867310] [<c0259ca9>] (do_sys_open) from [<c01011cb>] (__sys_trace_return+0x1/0x16) [ 61.867377] Exception stack(0xdc01bfa8 to 0xdc01bff0) [ 61.867498] bfa0: 00000000 b6fac2e0 ffffff9c beb9b964 00080802 00000000 [ 61.867624] bfc0: 00000000 b6fac2e0 ffffffff 00000142 b22f57a8 beb9b840 beb9b964 81204101 [ 61.867692] bfe0: 00000142 beb9b7e8 b6c35f0d b6bbcd56 [ 61.867819] Code: f2cc 00e2 f8d3 40e0 (6825) f615 [ 61.867959] ---[ end trace 335017ef9fedfd8c ]---
Hi Dmitry
Thank you for reporting
I'm observing a NULL dereference on NVIDIA Tegra20/30 once PulseAudio is loaded.
The offending patch is:
ASoC: soc-pcm: call snd_soc_component_open/close() once
Please fix, thanks in advance.
[ 61.860826] 8<--- cut here --- [ 61.860965] Unable to handle kernel NULL pointer dereference at virtual address 00000000 [ 61.861037] pgd = ef2eab54 [ 61.861155] [00000000] *pgd=00000000 [ 61.861228] Internal error: Oops: 5 [#1] SMP THUMB2 [ 61.861298] Modules linked in: [ 61.861427] CPU: 2 PID: 599 Comm: pulseaudio Not tainted 5.6.0-rc2-next-20200218-00168-g1e584fed87b9 #1275 [ 61.861546] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 61.861626] PC is at snd_dmaengine_pcm_close+0x1c/0x3c [ 61.861756] LR is at snd_soc_component_close+0x1d/0x3c [ 61.861823] pc : [<c072a36c>] lr : [<c0751b51>] psr: 60000033 [ 61.861944] sp : dc01bc88 ip : 00000000 fp : ffffffea [ 61.862013] r10: 00000010 r9 : dd81a840 r8 : de318e00 [ 61.862080] r7 : dd81adfc r6 : 00000000 r5 : 00000003 r4 : 00000000 [ 61.862199] r3 : dc19f800 r2 : 00000000 r1 : 00000447 r0 : c0e2f438 [ 61.862322] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment none [ 61.862390] Control: 50c5387d Table: 9db0c04a DAC: 00000051 [ 61.862510] Process pulseaudio (pid: 599, stack limit = 0xcfc4cd60) [ 61.862576] Stack: (0xdc01bc88 to 0xdc01c000) [ 61.862700] bc80: c0756611 de31b60c 00000003 c0751b51 de31b60c c07525ff ... [ 61.865643] bfe0: 00000142 beb9b7e8 b6c35f0d b6bbcd56 00000030 ffffff9c 00000000 00000000 [ 61.865773] [<c072a36c>] (snd_dmaengine_pcm_close) from [<c0751b51>] (snd_soc_component_close+0x1d/0x3c) [ 61.865920] [<c0751b51>] (snd_soc_component_close) from [<c07525ff>] (soc_pcm_components_close+0x27/0x54) [ 61.865993] [<c07525ff>] (soc_pcm_components_close) from [<c0752c27>] (soc_pcm_close+0x73/0xf0)
But, hmm... This is strange...
I checked this patch and your Oops trace.
This patch protects kernel from "duplicate close" or "close without open", and your Oops happen in snd_dmaengine_pcm_close(). This means it is really opened, and was closed correctly, if my understanding was correct.
I guess the NULL is on substream or substream_to_prtd(substream) in snd_dmaengine_pcm_close(). I guess it has same issue without this patch ?
Can you debug that this component .close() was called twice or more ? # but, I don't think so... I think "component->name" can help you ?
int snd_soc_component_close(struct snd_soc_component *component, struct snd_pcm_substream *substream) { - if (component->driver->close) - return component->driver->close(component, substream); - return 0; + int ret = 0; + + if (component->opened && + component->driver->close) + ret = component->driver->close(component, substream); + + component->opened = 0; + + return ret; }
Thank you for your help !! Best regards --- Kuninori Morimoto
I'm observing a NULL dereference on NVIDIA Tegra20/30 once PulseAudio is loaded.
The offending patch is:
ASoC: soc-pcm: call snd_soc_component_open/close() once
We also see a regression Kai bisected to the same patch
https://github.com/thesofproject/linux/pull/1805#issuecomment-588266014
Hi,
On Wed, 19 Feb 2020, Pierre-Louis Bossart wrote:
I'm observing a NULL dereference on NVIDIA Tegra20/30 once PulseAudio is loaded. The offending patch is:
ASoC: soc-pcm: call snd_soc_component_open/close() once
We also see a regression Kai bisected to the same patch
https://github.com/thesofproject/linux/pull/1805#issuecomment-588266014
issue rootcaused -- this relates to the new "opened" tracking, I'll send a patch shortly after a few more test rounds.
Br, Kai
On Wed, Feb 19, 2020 at 07:01:33PM +0200, Kai Vehmanen wrote:
On Wed, 19 Feb 2020, Pierre-Louis Bossart wrote:
We also see a regression Kai bisected to the same patch
https://github.com/thesofproject/linux/pull/1805#issuecomment-588266014
issue rootcaused -- this relates to the new "opened" tracking, I'll send a patch shortly after a few more test rounds.
Oh, excellent thanks - I was just starting to poke at this a little!
participants (6)
-
Amadeusz Sławiński -
Dmitry Osipenko -
Kai Vehmanen -
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart