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

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

[Kuninori Morimoto]

Hi Mark

These are soc-pcm cleanup step2.
Very random cleanup...

Kuninori Morimoto (7):
  ASoC: soc-pcm: add snd_soc_runtime_action()
  ASoC: soc-pcm: adjustment for DAI member 0 reset
  ASoC: soc-pcm: add for_each_dapm_widgets() macro
  ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai
  ASoC: soc-pcm: goto error after trying all component open
  ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open()
  ASoC: soc-pcm: tidyup soc_pcm_open() order

 include/sound/soc-dapm.h |   5 +
 sound/soc/soc-dapm.c     |   8 +-
 sound/soc/soc-pcm.c      | 266 ++++++++++++++++++++---------------------------
 3 files changed, 118 insertions(+), 161 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 1/7] 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 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>
---
 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 e66ac9c..8bc6983 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] [PATCH 2/7] 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>
---
 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 8bc6983..690a912 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] [PATCH 3/7] 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>
---
 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 690a912..f11c15f 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] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Kuninori Morimoto]

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

soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai.
In such case, fallback process need to care about operated/non-operated
codec dai.

But, if it goto error process *after* loop even though error happen
during loop, it don't need to care about operated/non-operated.
In such case code can be more simple.
This patch do it. And this is prepare for soc_snd_open() cleanup

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index f11c15f..57d2f00 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		goto component_err;
 
 	for_each_rtd_codec_dai(rtd, i, codec_dai) {
-		ret = snd_soc_dai_startup(codec_dai, substream);
-		if (ret < 0) {
-			dev_err(codec_dai->dev,
-				"ASoC: can't open codec %s: %d\n",
-				codec_dai->name, ret);
-			goto codec_dai_err;
-		}
+		ret |= snd_soc_dai_startup(codec_dai, substream);
 
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			codec_dai->tx_mask = 0;
 		else
 			codec_dai->rx_mask = 0;
 	}
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n",
+			codec_dai->name, ret);
+		goto codec_dai_err;
+	}
 
 	ret = soc_rtd_startup(rtd, substream);
 	if (ret < 0) {
 		pr_err("ASoC: %s startup failed: %d\n",
 		       rtd->dai_link->name, ret);
-		goto machine_err;
+		goto codec_dai_err;
 	}
 
 	/* Dynamic PCM DAI links compat checks use dynamic capabilities */
@@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 config_err:
 	soc_rtd_shutdown(rtd, substream);
 
-machine_err:
-	i = rtd->num_codecs;
-
 codec_dai_err:
-	for_each_rtd_codec_dai_rollback(rtd, i, codec_dai)
+	for_each_rtd_codec_dai(rtd, i, codec_dai)
 		snd_soc_dai_shutdown(codec_dai, substream);
 
 component_err:
-- 
2.7.4

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

Re: [alsa-devel] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Takashi Iwai]

On Mon, 27 Jan 2020 02:49:22 +0100,
Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai.
> In such case, fallback process need to care about operated/non-operated
> codec dai.
> 
> But, if it goto error process *after* loop even though error happen
> during loop, it don't need to care about operated/non-operated.
> In such case code can be more simple.
> This patch do it. And this is prepare for soc_snd_open() cleanup

This would mean that snd_soc_dai_shutdown() is called even for the
stream that returned the error.  This isn't the expected behavior.

Also, bit-OR-ing the multiple error codes isn't wise, they may return
different error codes, and you'll mixed up to a different number.


thanks,

Takashi

> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-pcm.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index f11c15f..57d2f00 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>  		goto component_err;
>  
>  	for_each_rtd_codec_dai(rtd, i, codec_dai) {
> -		ret = snd_soc_dai_startup(codec_dai, substream);
> -		if (ret < 0) {
> -			dev_err(codec_dai->dev,
> -				"ASoC: can't open codec %s: %d\n",
> -				codec_dai->name, ret);
> -			goto codec_dai_err;
> -		}
> +		ret |= snd_soc_dai_startup(codec_dai, substream);
>  
>  		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  			codec_dai->tx_mask = 0;
>  		else
>  			codec_dai->rx_mask = 0;
>  	}
> +	if (ret < 0) {
> +		dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n",
> +			codec_dai->name, ret);
> +		goto codec_dai_err;
> +	}
>  
>  	ret = soc_rtd_startup(rtd, substream);
>  	if (ret < 0) {
>  		pr_err("ASoC: %s startup failed: %d\n",
>  		       rtd->dai_link->name, ret);
> -		goto machine_err;
> +		goto codec_dai_err;
>  	}
>  
>  	/* Dynamic PCM DAI links compat checks use dynamic capabilities */
> @@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>  config_err:
>  	soc_rtd_shutdown(rtd, substream);
>  
> -machine_err:
> -	i = rtd->num_codecs;
> -
>  codec_dai_err:
> -	for_each_rtd_codec_dai_rollback(rtd, i, codec_dai)
> +	for_each_rtd_codec_dai(rtd, i, codec_dai)
>  		snd_soc_dai_shutdown(codec_dai, substream);
>  
>  component_err:
> -- 
> 2.7.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Kuninori Morimoto]

Hi Takashi

Thank you for your feedback

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai.
> > In such case, fallback process need to care about operated/non-operated
> > codec dai.
> > 
> > But, if it goto error process *after* loop even though error happen
> > during loop, it don't need to care about operated/non-operated.
> > In such case code can be more simple.
> > This patch do it. And this is prepare for soc_snd_open() cleanup
> 
> This would mean that snd_soc_dai_shutdown() is called even for the
> stream that returned the error.  This isn't the expected behavior.

Yeah.
Actually, I have plan to add such flag by other patch.
But indeed order was reversed.
Will fixup.

> Also, bit-OR-ing the multiple error codes isn't wise, they may return
> different error codes, and you'll mixed up to a different number.

It is used at some architecture.
But yes, let's think about better idea.
Will return 1st error.

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

Re: [alsa-devel] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Pierre-Louis Bossart]

>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>
>>> soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai.
>>> In such case, fallback process need to care about operated/non-operated
>>> codec dai.
>>>
>>> But, if it goto error process *after* loop even though error happen
>>> during loop, it don't need to care about operated/non-operated.
>>> In such case code can be more simple.
>>> This patch do it. And this is prepare for soc_snd_open() cleanup
>>
>> This would mean that snd_soc_dai_shutdown() is called even for the
>> stream that returned the error.  This isn't the expected behavior.
> 
> Yeah.
> Actually, I have plan to add such flag by other patch.
> But indeed order was reversed.
> Will fixup.
> 
>> Also, bit-OR-ing the multiple error codes isn't wise, they may return
>> different error codes, and you'll mixed up to a different number.
> 
> It is used at some architecture.
> But yes, let's think about better idea.
> Will return 1st error.

I also cringed on the bit-OR'ed error codes, but I saw it's already used 
in soc-pcm.c so thought it was an agreed precedent?

		ret |= snd_soc_component_close(component, substream);
		ret |= snd_soc_component_hw_free(component, substream);

I also don't like the idea of not stopping loops on errors, or releasing 
things that haven't been properly allocated. It does simplify error 
handling but it's asking for trouble.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Takashi Iwai]

On Tue, 28 Jan 2020 17:31:05 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>
> >>> soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai.
> >>> In such case, fallback process need to care about operated/non-operated
> >>> codec dai.
> >>>
> >>> But, if it goto error process *after* loop even though error happen
> >>> during loop, it don't need to care about operated/non-operated.
> >>> In such case code can be more simple.
> >>> This patch do it. And this is prepare for soc_snd_open() cleanup
> >>
> >> This would mean that snd_soc_dai_shutdown() is called even for the
> >> stream that returned the error.  This isn't the expected behavior.
> >
> > Yeah.
> > Actually, I have plan to add such flag by other patch.
> > But indeed order was reversed.
> > Will fixup.
> >
> >> Also, bit-OR-ing the multiple error codes isn't wise, they may return
> >> different error codes, and you'll mixed up to a different number.
> >
> > It is used at some architecture.
> > But yes, let's think about better idea.
> > Will return 1st error.
> 
> I also cringed on the bit-OR'ed error codes, but I saw it's already
> used in soc-pcm.c so thought it was an agreed precedent?
> 
> 		ret |= snd_soc_component_close(component, substream);
> 		ret |= snd_soc_component_hw_free(component, substream);

I think it's just an overlook.  The driver may return arbitrary error
codes so they can conflict.  The bit-OR works only if the return code
is always consistent.


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

Re: [alsa-devel] [PATCH 4/7] ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai

[Kuninori Morimoto]

Hi

> > I also cringed on the bit-OR'ed error codes, but I saw it's already
> > used in soc-pcm.c so thought it was an agreed precedent?
> > 
> > 		ret |= snd_soc_component_close(component, substream);
> > 		ret |= snd_soc_component_hw_free(component, substream);
> 
> I think it's just an overlook.  The driver may return arbitrary error
> codes so they can conflict.  The bit-OR works only if the return code
> is always consistent.

OK. Let's cleanup it, too.
If you ara OK, I will do it.

> I also don't like the idea of not stopping loops on errors, or
> releasing things that haven't been properly allocated. It does
> simplify error handling but it's asking for trouble.

Actually, my local patch which will be posted will solve it.
I will re-order patch-set, and post it.

# But, this re-order patch will have conflict in my local tree.
# I will solve it, too.

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

[alsa-devel] [PATCH 5/7] ASoC: soc-pcm: goto error after trying all component open

[Kuninori Morimoto]

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

soc_pcm_components_open() might goto error process *during* opening
component loop.
In such case, fallback process need to care about operated/non-operated
component.

But, if it goto error process *after* loop even though error happen
during loop, it don't need to care about operated/non-operated.
In such case code can be more simple.
This patch do it. And this is prepare for soc_snd_open() cleanup

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 57d2f00..1e370ef 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -463,47 +463,32 @@ 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);
+		ret |= snd_soc_component_open(component, substream);
+	}
 
-		ret = snd_soc_component_module_get_when_open(component);
-		if (ret < 0) {
-			dev_err(component->dev,
-				"ASoC: can't get module %s\n",
-				component->name);
-			return ret;
-		}
+	if (ret < 0)
+		dev_err(component->dev,
+			"ASoC: error happen during open component %s: %d\n",
+			component->name, ret);
 
-		ret = snd_soc_component_open(component, substream);
-		if (ret < 0) {
-			dev_err(component->dev,
-				"ASoC: can't open component %s: %d\n",
-				component->name, ret);
-			return ret;
-		}
-	}
-	*last = NULL;
-	return 0;
+	return ret;
 }
 
-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, ret = 0;
 
 	for_each_rtd_components(rtd, i, component) {
-		if (component == last)
-			break;
-
 		ret |= snd_soc_component_close(component, substream);
 		snd_soc_component_module_put_when_close(component);
 	}
@@ -542,7 +527,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;
 
@@ -638,7 +623,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:
@@ -692,7 +677,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 5/7] ASoC: soc-pcm: goto error after trying all component open

[Sridharan, Ranjani]

On Sun, Jan 26, 2020 at 5:54 PM Kuninori Morimoto <
kuninori.morimoto.gx@renesas.com> wrote:

>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> soc_pcm_components_open() might goto error process *during* opening
> component loop.
> In such case, fallback process need to care about operated/non-operated
> component.
>
> But, if it goto error process *after* loop even though error happen
> during loop, it don't need to care about operated/non-operated.
> In such case code can be more simple.
> This patch do it. And this is prepare for soc_snd_open() cleanup
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-pcm.c | 41 +++++++++++++----------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 57d2f00..1e370ef 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -463,47 +463,32 @@ 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);
> +               ret |= snd_soc_component_open(component, substream);
> +       }
>
> -               ret = snd_soc_component_module_get_when_open(component);
> -               if (ret < 0) {
> -                       dev_err(component->dev,
> -                               "ASoC: can't get module %s\n",
> -                               component->name);
> -                       return ret;
> -               }
> +       if (ret < 0)
> +               dev_err(component->dev,
> +                       "ASoC: error happen during open component %s:
> %d\n",
> +                       component->name, ret);
>
Hi Morimoto-san,

Wouldn't the component here always be the last component in the list of rtd
components? Should this error log be moved inside
the for_each_rtd_components() {} above?

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

Re: [alsa-devel] [PATCH 5/7] ASoC: soc-pcm: goto error after trying all component open

[Kuninori Morimoto]

Hi Sridharan

Thank you for your feedback

>     @@ -463,47 +463,32 @@ 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);
>     +               ret |= snd_soc_component_open(component, substream);
>     +       }
>    
>     -               ret = snd_soc_component_module_get_when_open(component);
>     -               if (ret < 0) {
>     -                       dev_err(component->dev,
>     -                               "ASoC: can't get module %s\n",
>     -                               component->name);
>     -                       return ret;
>     -               }
>     +       if (ret < 0)
>     +               dev_err(component->dev,
>     +                       "ASoC: error happen during open component %s: %d\n",
>     +                       component->name, ret);
> 
> Hi Morimoto-san,
> 
> Wouldn't the component here always be the last component in the list of rtd components? Should this error log be moved inside
> the for_each_rtd_components() {} above?

Yeah, indeed.
Will fix

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

[alsa-devel] [PATCH 6/7] 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>
---
 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 1e370ef..b5d2840 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -497,6 +497,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.
@@ -652,50 +696,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] [PATCH 7/7] 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.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index b5d2840..d916182 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -563,18 +563,25 @@ 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 rtd_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);
 
@@ -586,14 +593,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	if (ret < 0) {
 		dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n",
 			codec_dai->name, ret);
-		goto codec_dai_err;
-	}
-
-	ret = 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;
+		goto config_err;
 	}
 
 	/* Dynamic PCM DAI links compat checks use dynamic capabilities */
@@ -660,17 +660,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);
+rtd_err:
+	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

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

[Pierre-Louis Bossart]

Hi Morimoto-san,

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

Maybe a red-herring but while reviewing the other soc-pcm changes I 
started wondering if this order change is actually valid for all platforms.

If you look at the soc-pcm code, all the way to 2011

ddee627cf6bb60 ('ASoC: core - Separate out PCM operations into new
file')

you'll see that the intent was to
- start the cpu_dai
- open the platform driver to e.g handle DMAs
- start the codec_dai.

The second step is not really needed for Intel but might be needed on 
others where the DMA is centrally handled (Omap, etc).

My concern is that we've modified the order to deal with module 
dependencies, without necessarily taking into account that DMA setup part

That said I don't understand the initial sequence for soc_pcm_close() so 
I am not advocating for a revert, but more a clarification of what those 
component open/close steps are supposed to do.

Thanks
-Pierre

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

[Mark Brown]

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

On Mon, Feb 24, 2020 at 07:22:25PM -0600, Pierre-Louis Bossart wrote:

> you'll see that the intent was to
> - start the cpu_dai
> - open the platform driver to e.g handle DMAs
> - start the codec_dai.

> The second step is not really needed for Intel but might be needed on others
> where the DMA is centrally handled (Omap, etc).

> My concern is that we've modified the order to deal with module
> dependencies, without necessarily taking into account that DMA setup part

> That said I don't understand the initial sequence for soc_pcm_close() so I
> am not advocating for a revert, but more a clarification of what those
> component open/close steps are supposed to do.

It's possible we're going to shake some issues out here but the
ordering has always been a bit arbitrary, especially the CPU vs
CODEC ordering.  We're basically hoping that the ordering we've
picked is the one that causes fewest glitches and upsets on the
broad range of hardware.  My expectation/hope is that for most
hardware the flow control between the DAI and the DMA controller
(which has to be there anyway) will mean that it mostly doesn't
matter, if there's anything that is too sensitive to it we can
always revert and try something
else.

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

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

[Kuninori Morimoto]

Hi Pierre-Louis, Mark

> > you'll see that the intent was to
> > - start the cpu_dai
> > - open the platform driver to e.g handle DMAs
> > - start the codec_dai.
> 
> > The second step is not really needed for Intel but might be needed on others
> > where the DMA is centrally handled (Omap, etc).
> 
> > My concern is that we've modified the order to deal with module
> > dependencies, without necessarily taking into account that DMA setup part
> 
> > That said I don't understand the initial sequence for soc_pcm_close() so I
> > am not advocating for a revert, but more a clarification of what those
> > component open/close steps are supposed to do.
> 
> It's possible we're going to shake some issues out here but the
> ordering has always been a bit arbitrary, especially the CPU vs
> CODEC ordering.  We're basically hoping that the ordering we've
> picked is the one that causes fewest glitches and upsets on the
> broad range of hardware.  My expectation/hope is that for most
> hardware the flow control between the DAI and the DMA controller
> (which has to be there anyway) will mean that it mostly doesn't
> matter, if there's anything that is too sensitive to it we can
> always revert and try something
> else.

I'm sorry to not clarify the detail.
Yes, as you mentioned, the patch exchanged start/stop order,
and some platform *might* get damage (I hope not, of course).

But we can't say that the previous order was the best order for all platform.
I'm thinking that the current order is also not the best,
but is not random.
The *best* solution is that we need to have xxx_order flag like for_each_comp_order().

Thank you for your help !!
Best regards
---
Kuninori Morimoto