[alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[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):
  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(-)

-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 1/8] ASoC: soc-pcm: add snd_soc_runtime_action()

[Kuninori Morimoto]

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);
 }
 
 /**
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: add snd_soc_runtime_action()" to the asoc tree

[Mark Brown]

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);
 }
 
 /**
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 2/8] ASoC: soc-pcm: adjustment for DAI member 0 reset

[Kuninori Morimoto]

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);
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: adjustment for DAI member 0 reset" to the asoc tree

[Mark Brown]

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);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 3/8] ASoC: soc-pcm: add for_each_dapm_widgets() macro

[Kuninori Morimoto]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: add for_each_dapm_widgets() macro" to the asoc tree

[Mark Brown]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 4/8] ASoC: soc-pcm: don't use bit-OR'ed error

[Kuninori Morimoto]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: don't use bit-OR'ed error" to the asoc tree

[Mark Brown]

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Kuninori Morimoto]

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:
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once" to the asoc tree

[Mark Brown]

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:
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Amadeusz Sławiński]

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

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Pierre-Louis Bossart]

>>   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,

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Kuninori Morimoto]

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.intel.com/

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

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Amadeusz Sławiński]

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.intel.com/
> 
> 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

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH v3 5/8] ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once

[Kuninori Morimoto]

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

[alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[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);
 
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[Pierre-Louis Bossart]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 368 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[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.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 6/8] ASoC: soc-pcm: call snd_soc_component_open/close() once

[Pierre-Louis Bossart]

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: call snd_soc_component_open/close() once" to the asoc tree

[Mark Brown]

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);
 
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 7/8] ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()

[Kuninori Morimoto]

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.
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()" to the asoc tree

[Mark Brown]

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,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v3 8/8] ASoC: soc-pcm: tidyup soc_pcm_open() order

[Kuninori Morimoto]

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) {
-- 
2.7.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: soc-pcm: tidyup soc_pcm_open() order" to the asoc tree

[Mark Brown]

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) {
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Dmitry Osipenko]

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

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 ]---

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Dmitry Osipenko]

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

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 ]---

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Pierre-Louis Bossart]

> 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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Pierre-Louis Bossart]

> 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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Kai Vehmanen]

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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Kai Vehmanen]

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

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH v3 0/7] ASoC: soc-pcm cleanup step2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]