[PATCH 0/7] ASoC: merge soc_pcm_open() rollback and soc_pcm_close()

[PATCH 0/7] ASoC: merge soc_pcm_open() rollback and soc_pcm_close()

[Kuninori Morimoto]

Hi Mark

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

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

Kuninori Morimoto (7):
  ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()
  ASoC: soc-link: add mark for snd_soc_link_startup/shutdown()
  ASoC: soc-component: add mark for soc_pcm_components_open/close()
  ASoC: soc-component: add mark for snd_soc_pcm_component_pm_runtime_get/put()
  ASoC: soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()
  ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_dai_startup()
  ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_component_module/open()

 include/sound/soc-component.h |  28 +++++---
 include/sound/soc-dai.h       |   5 +-
 include/sound/soc-link.h      |   3 +-
 include/sound/soc.h           |   3 +
 sound/soc/soc-component.c     |  73 ++++++++++++++++++++-
 sound/soc/soc-compress.c      |  30 +++------
 sound/soc/soc-dai.c           |  21 +++++-
 sound/soc/soc-dapm.c          |   4 +-
 sound/soc/soc-link.c          |  21 +++++-
 sound/soc/soc-pcm.c           | 120 ++++++++++++----------------------
 10 files changed, 190 insertions(+), 118 deletions(-)

-- 
2.25.1

[PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
=>	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

This patch is for 1) snd_soc_dai_startup/shutdown(),
and adds new substream mark.
It will mark substream when startup was suceeded.
If rollback happen *after* that, it will check rollback flag
and marked substream.

It cares *previous* startup() only now,
but we might want to check *whole* marked substream in the future.
This patch is using macro so that it can be easily adjust to it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-dai.h |  5 ++++-
 sound/soc/soc-dai.c     | 21 ++++++++++++++++++++-
 sound/soc/soc-dapm.c    |  4 ++--
 sound/soc/soc-pcm.c     |  4 ++--
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 776a60529e70..86411f503c1c 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai,
 int snd_soc_dai_startup(struct snd_soc_dai *dai,
 			struct snd_pcm_substream *substream);
 void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
-			  struct snd_pcm_substream *substream);
+			  struct snd_pcm_substream *substream, int rollback);
 snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
 				    struct snd_pcm_substream *substream);
 void snd_soc_dai_suspend(struct snd_soc_dai *dai);
@@ -388,6 +388,9 @@ struct snd_soc_dai {
 
 	struct list_head list;
 
+	/* function mark */
+	struct snd_pcm_substream *mark_startup;
+
 	/* bit field */
 	unsigned int probed:1;
 };
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 693893420bf0..4f9f73800ab0 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai *dai,
 	return ret;
 }
 
+/*
+ * We might want to check substream by using list.
+ * In such case, we can update these macros.
+ */
+#define soc_dai_mark_push(dai, substream, tgt)	((dai)->mark_##tgt = substream)
+#define soc_dai_mark_pop(dai, substream, tgt)	((dai)->mark_##tgt = NULL)
+#define soc_dai_mark_match(dai, substream, tgt)	((dai)->mark_##tgt == substream)
+
 /**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
@@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
 	    dai->driver->ops->startup)
 		ret = dai->driver->ops->startup(substream, dai);
 
+	/* mark substream if succeeded */
+	if (ret == 0)
+		soc_dai_mark_push(dai, substream, startup);
+
 	return soc_dai_ret(dai, ret);
 }
 
 void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
-			 struct snd_pcm_substream *substream)
+			  struct snd_pcm_substream *substream,
+			  int rollback)
 {
+	if (rollback && !soc_dai_mark_match(dai, substream, startup))
+		return;
+
 	if (dai->driver->ops &&
 	    dai->driver->ops->shutdown)
 		dai->driver->ops->shutdown(substream, dai);
+
+	/* remove marked substream */
+	soc_dai_mark_pop(dai, substream, startup);
 }
 
 snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 3273161e2787..980f2c330b87 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3968,14 +3968,14 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 		snd_soc_dapm_widget_for_each_source_path(w, path) {
 			source = path->source->priv;
 			snd_soc_dai_deactivate(source, substream->stream);
-			snd_soc_dai_shutdown(source, substream);
+			snd_soc_dai_shutdown(source, substream, 0);
 		}
 
 		substream->stream = SNDRV_PCM_STREAM_PLAYBACK;
 		snd_soc_dapm_widget_for_each_sink_path(w, path) {
 			sink = path->sink->priv;
 			snd_soc_dai_deactivate(sink, substream->stream);
-			snd_soc_dai_shutdown(sink, substream);
+			snd_soc_dai_shutdown(sink, substream, 0);
 		}
 		break;
 
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 10f703986be3..ae7e648cd21c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -682,7 +682,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	snd_soc_runtime_deactivate(rtd, substream->stream);
 
 	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream);
+		snd_soc_dai_shutdown(dai, substream, 0);
 
 	snd_soc_link_shutdown(substream);
 
@@ -813,7 +813,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 config_err:
 	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream);
+		snd_soc_dai_shutdown(dai, substream, 1);
 
 	snd_soc_link_shutdown(substream);
 rtd_startup_err:
-- 
2.25.1

[PATCH 2/7] ASoC: soc-link: add mark for snd_soc_link_startup/shutdown()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
	1) snd_soc_dai_startup/shutdown()
=>	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

This patch is for 2) snd_soc_link_startup/shutdown(),
and adds new substream mark.
It will mark substream when startup was suceeded.
If rollback happen *after* that, it will check rollback flag
and marked substream.

It cares *previous* startup() only now,
but we might want to check *whole* marked substream in the future.
This patch is using macro so that it can be easily adjust to it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-link.h |  3 ++-
 include/sound/soc.h      |  3 +++
 sound/soc/soc-link.c     | 21 ++++++++++++++++++++-
 sound/soc/soc-pcm.c      |  4 ++--
 4 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc-link.h b/include/sound/soc-link.h
index 337ac5666757..dac6c0ce6ede 100644
--- a/include/sound/soc-link.h
+++ b/include/sound/soc-link.h
@@ -14,7 +14,8 @@ int snd_soc_link_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
 				    struct snd_pcm_hw_params *params);
 
 int snd_soc_link_startup(struct snd_pcm_substream *substream);
-void snd_soc_link_shutdown(struct snd_pcm_substream *substream);
+void snd_soc_link_shutdown(struct snd_pcm_substream *substream,
+			   int rollback);
 int snd_soc_link_prepare(struct snd_pcm_substream *substream);
 int snd_soc_link_hw_params(struct snd_pcm_substream *substream,
 			   struct snd_pcm_hw_params *params);
diff --git a/include/sound/soc.h b/include/sound/soc.h
index acbb5efb28ef..fddb89db1f8e 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1158,6 +1158,9 @@ struct snd_soc_pcm_runtime {
 	unsigned int num; /* 0-based and monotonic increasing */
 	struct list_head list; /* rtd list of the soc card */
 
+	/* function mark */
+	struct snd_pcm_substream *mark_startup;
+
 	/* bit field */
 	unsigned int pop_wait:1;
 	unsigned int fe_compr:1; /* for Dynamic PCM */
diff --git a/sound/soc/soc-link.c b/sound/soc/soc-link.c
index cec70b19863e..2a8881978930 100644
--- a/sound/soc/soc-link.c
+++ b/sound/soc/soc-link.c
@@ -30,6 +30,14 @@ static inline int _soc_link_ret(struct snd_soc_pcm_runtime *rtd,
 	return ret;
 }
 
+/*
+ * We might want to check substream by using list.
+ * In such case, we can update these macros.
+ */
+#define soc_link_mark_push(rtd, substream, tgt)		((rtd)->mark_##tgt = substream)
+#define soc_link_mark_pop(rtd, substream, tgt)		((rtd)->mark_##tgt = NULL)
+#define soc_link_mark_match(rtd, substream, tgt)	((rtd)->mark_##tgt == substream)
+
 int snd_soc_link_init(struct snd_soc_pcm_runtime *rtd)
 {
 	int ret = 0;
@@ -66,16 +74,27 @@ int snd_soc_link_startup(struct snd_pcm_substream *substream)
 	    rtd->dai_link->ops->startup)
 		ret = rtd->dai_link->ops->startup(substream);
 
+	/* mark substream if succeeded */
+	if (ret == 0)
+		soc_link_mark_push(rtd, substream, startup);
+
 	return soc_link_ret(rtd, ret);
 }
 
-void snd_soc_link_shutdown(struct snd_pcm_substream *substream)
+void snd_soc_link_shutdown(struct snd_pcm_substream *substream,
+			   int rollback)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 
+	if (rollback && !soc_link_mark_match(rtd, substream, startup))
+		return;
+
 	if (rtd->dai_link->ops &&
 	    rtd->dai_link->ops->shutdown)
 		rtd->dai_link->ops->shutdown(substream);
+
+	/* remove marked substream */
+	soc_link_mark_pop(rtd, substream, startup);
 }
 
 int snd_soc_link_prepare(struct snd_pcm_substream *substream)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index ae7e648cd21c..7e809aa56e8d 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -684,7 +684,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	for_each_rtd_dais(rtd, i, dai)
 		snd_soc_dai_shutdown(dai, substream, 0);
 
-	snd_soc_link_shutdown(substream);
+	snd_soc_link_shutdown(substream, 0);
 
 	soc_pcm_components_close(substream);
 
@@ -815,7 +815,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	for_each_rtd_dais(rtd, i, dai)
 		snd_soc_dai_shutdown(dai, substream, 1);
 
-	snd_soc_link_shutdown(substream);
+	snd_soc_link_shutdown(substream, 1);
 rtd_startup_err:
 	soc_pcm_components_close(substream);
 component_err:
-- 
2.25.1

[PATCH 3/7] ASoC: soc-component: add mark for soc_pcm_components_open/close()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
=>	3) snd_soc_component_module_get/put()
=>	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

This patch is for 3) snd_soc_component_module_get/put()
4) snd_soc_component_open/close(),
and adds new substream mark.
It will mark substream when open was suceeded.
If rollback happen *after* that, it will check rollback flag
and marked substream.

It cares *previous* open() only now,
but we might want to check *whole* marked substream in the future.
This patch is using macro so that it can be easily adjust to it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h | 23 +++++++++++++++--------
 sound/soc/soc-component.c     | 35 +++++++++++++++++++++++++++++++++--
 sound/soc/soc-pcm.c           | 28 +++++++---------------------
 3 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 8917b15eccae..8aabadfbdebc 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -217,6 +217,10 @@ struct snd_soc_component {
 	/* machine specific init */
 	int (*init)(struct snd_soc_component *component);
 
+	/* function mark */
+	struct snd_pcm_substream *mark_module;
+	struct snd_pcm_substream *mark_open;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_root;
 	const char *debugfs_prefix;
@@ -373,17 +377,19 @@ void snd_soc_component_exit_regmap(struct snd_soc_component *component);
 #endif
 
 #define snd_soc_component_module_get_when_probe(component)\
-	snd_soc_component_module_get(component, 0)
-#define snd_soc_component_module_get_when_open(component)	\
-	snd_soc_component_module_get(component, 1)
+	snd_soc_component_module_get(component, NULL, 0)
+#define snd_soc_component_module_get_when_open(component, substream)	\
+	snd_soc_component_module_get(component, substream, 1)
 int snd_soc_component_module_get(struct snd_soc_component *component,
+				 struct snd_pcm_substream *substream,
 				 int upon_open);
 #define snd_soc_component_module_put_when_remove(component)	\
-	snd_soc_component_module_put(component, 0)
-#define snd_soc_component_module_put_when_close(component)	\
-	snd_soc_component_module_put(component, 1)
+	snd_soc_component_module_put(component, NULL, 0, 0)
+#define snd_soc_component_module_put_when_close(component, substream, rollback) \
+	snd_soc_component_module_put(component, substream, 1, rollback)
 void snd_soc_component_module_put(struct snd_soc_component *component,
-				  int upon_open);
+				  struct snd_pcm_substream *substream,
+				  int upon_open, int rollback);
 
 static inline void snd_soc_component_set_drvdata(struct snd_soc_component *c,
 						 void *data)
@@ -427,7 +433,8 @@ int snd_soc_component_force_enable_pin_unlocked(
 int snd_soc_component_open(struct snd_soc_component *component,
 			   struct snd_pcm_substream *substream);
 int snd_soc_component_close(struct snd_soc_component *component,
-			    struct snd_pcm_substream *substream);
+			    struct snd_pcm_substream *substream,
+			    int rollback);
 void snd_soc_component_suspend(struct snd_soc_component *component);
 void snd_soc_component_resume(struct snd_soc_component *component);
 int snd_soc_component_is_suspended(struct snd_soc_component *component);
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index dcc89fa8913a..6544e74cf7f7 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -33,6 +33,14 @@ static inline int _soc_component_ret(struct snd_soc_component *component,
 	return ret;
 }
 
+/*
+ * We might want to check substream by using list.
+ * In such case, we can update these macros.
+ */
+#define soc_component_mark_push(component, substream, tgt)	((component)->mark_##tgt = substream)
+#define soc_component_mark_pop(component, substream, tgt)	((component)->mark_##tgt = NULL)
+#define soc_component_mark_match(component, substream, tgt)	((component)->mark_##tgt == substream)
+
 int snd_soc_component_initialize(struct snd_soc_component *component,
 				 const struct snd_soc_component_driver *driver,
 				 struct device *dev, const char *name)
@@ -254,6 +262,7 @@ int snd_soc_component_set_jack(struct snd_soc_component *component,
 EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
 
 int snd_soc_component_module_get(struct snd_soc_component *component,
+				 struct snd_pcm_substream *substream,
 				 int upon_open)
 {
 	int ret = 0;
@@ -262,14 +271,25 @@ int snd_soc_component_module_get(struct snd_soc_component *component,
 	    !try_module_get(component->dev->driver->owner))
 		ret = -ENODEV;
 
+	/* mark substream if succeeded */
+	if (ret == 0)
+		soc_component_mark_push(component, substream, module);
+
 	return soc_component_ret(component, ret);
 }
 
 void snd_soc_component_module_put(struct snd_soc_component *component,
-				  int upon_open)
+				  struct snd_pcm_substream *substream,
+				  int upon_open, int rollback)
 {
+	if (rollback && !soc_component_mark_match(component, substream, module))
+		return;
+
 	if (component->driver->module_get_upon_open == !!upon_open)
 		module_put(component->dev->driver->owner);
+
+	/* remove marked substream */
+	soc_component_mark_pop(component, substream, module);
 }
 
 int snd_soc_component_open(struct snd_soc_component *component,
@@ -280,17 +300,28 @@ int snd_soc_component_open(struct snd_soc_component *component,
 	if (component->driver->open)
 		ret = component->driver->open(component, substream);
 
+	/* mark substream if succeeded */
+	if (ret == 0)
+		soc_component_mark_push(component, substream, open);
+
 	return soc_component_ret(component, ret);
 }
 
 int snd_soc_component_close(struct snd_soc_component *component,
-			    struct snd_pcm_substream *substream)
+			    struct snd_pcm_substream *substream,
+			    int rollback)
 {
 	int ret = 0;
 
+	if (rollback && !soc_component_mark_match(component, substream, open))
+		return 0;
+
 	if (component->driver->close)
 		ret = component->driver->close(component, substream);
 
+	/* remove marked substream */
+	soc_component_mark_pop(component, substream, open);
+
 	return soc_component_ret(component, ret);
 }
 
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7e809aa56e8d..c034b8a1e6f9 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -609,14 +609,11 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 static int soc_pcm_components_open(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_soc_component *last = NULL;
 	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_module_get_when_open(component, substream);
 		if (ret < 0) {
 			dev_err(component->dev,
 				"ASoC: can't get module %s\n",
@@ -626,7 +623,6 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream)
 
 		ret = snd_soc_component_open(component, substream);
 		if (ret < 0) {
-			snd_soc_component_module_put_when_close(component);
 			dev_err(component->dev,
 				"ASoC: can't open component %s: %d\n",
 				component->name, ret);
@@ -634,32 +630,22 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream)
 		}
 	}
 
-	if (ret < 0) {
-		/* rollback on error */
-		for_each_rtd_components(rtd, i, component) {
-			if (component == last)
-				break;
-
-			snd_soc_component_close(component, substream);
-			snd_soc_component_module_put_when_close(component);
-		}
-	}
-
 	return ret;
 }
 
-static int soc_pcm_components_close(struct snd_pcm_substream *substream)
+static int soc_pcm_components_close(struct snd_pcm_substream *substream,
+				    int rollback)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_component *component;
 	int i, r, ret = 0;
 
 	for_each_rtd_components(rtd, i, component) {
-		r = snd_soc_component_close(component, substream);
+		r = snd_soc_component_close(component, substream, rollback);
 		if (r < 0)
 			ret = r; /* use last ret */
 
-		snd_soc_component_module_put_when_close(component);
+		snd_soc_component_module_put_when_close(component, substream, rollback);
 	}
 
 	return ret;
@@ -686,7 +672,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	snd_soc_link_shutdown(substream, 0);
 
-	soc_pcm_components_close(substream);
+	soc_pcm_components_close(substream, 0);
 
 	snd_soc_dapm_stream_stop(rtd, substream->stream);
 
@@ -817,7 +803,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	snd_soc_link_shutdown(substream, 1);
 rtd_startup_err:
-	soc_pcm_components_close(substream);
+	soc_pcm_components_close(substream, 1);
 component_err:
 	mutex_unlock(&rtd->card->pcm_mutex);
 
-- 
2.25.1

[PATCH 4/7] ASoC: soc-component: add mark for snd_soc_pcm_component_pm_runtime_get/put()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
=>	5) pm_runtime_put/get()

This patch is for 5) pm_runtime_put/get(),
and adds new substream mark.
It will mark substream when get() was suceeded.
If rollback happen *after* that, it will check rollback flag
and marked substream.

It cares *previous* get() only now,
but we might want to check *whole* marked substream in the future.
This patch is using macro so that it can be easily adjust to it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h |  5 +++++
 sound/soc/soc-component.c     | 38 +++++++++++++++++++++++++++++++++++
 sound/soc/soc-compress.c      | 30 ++++++++-------------------
 sound/soc/soc-pcm.c           | 17 ++++++----------
 4 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 8aabadfbdebc..ae112f53143a 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -220,6 +220,7 @@ struct snd_soc_component {
 	/* function mark */
 	struct snd_pcm_substream *mark_module;
 	struct snd_pcm_substream *mark_open;
+	void *mark_pm;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_root;
@@ -467,5 +468,9 @@ void snd_soc_pcm_component_hw_free(struct snd_pcm_substream *substream,
 				   struct snd_soc_component *last);
 int snd_soc_pcm_component_trigger(struct snd_pcm_substream *substream,
 				  int cmd);
+int snd_soc_pcm_component_pm_runtime_get(struct snd_soc_pcm_runtime *rtd,
+					 void *stream);
+void snd_soc_pcm_component_pm_runtime_put(struct snd_soc_pcm_runtime *rtd,
+					  void *stream, int rollback);
 
 #endif /* __SOC_COMPONENT_H */
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index 6544e74cf7f7..30e5599b24d4 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -9,6 +9,7 @@
 // Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 //
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <sound/soc.h>
 
 #define soc_component_ret(dai, ret) _soc_component_ret(dai, __func__, ret)
@@ -852,3 +853,40 @@ int snd_soc_pcm_component_trigger(struct snd_pcm_substream *substream,
 
 	return 0;
 }
+
+int snd_soc_pcm_component_pm_runtime_get(struct snd_soc_pcm_runtime *rtd,
+					 void *stream)
+{
+	struct snd_soc_component *component;
+	int i, ret;
+
+	for_each_rtd_components(rtd, i, component) {
+		ret = pm_runtime_get_sync(component->dev);
+		if (ret < 0 && ret != -EACCES) {
+			pm_runtime_put_noidle(component->dev);
+			return soc_component_ret(component, ret);
+		}
+		/* mark stream if succeeded */
+		soc_component_mark_push(component, stream, pm);
+	}
+
+	return 0;
+}
+
+void snd_soc_pcm_component_pm_runtime_put(struct snd_soc_pcm_runtime *rtd,
+					  void *stream, int rollback)
+{
+	struct snd_soc_component *component;
+	int i;
+
+	for_each_rtd_components(rtd, i, component) {
+		if (rollback && !soc_component_mark_match(component, stream, pm))
+			continue;
+
+		pm_runtime_mark_last_busy(component->dev);
+		pm_runtime_put_autosuspend(component->dev);
+
+		/* remove marked stream */
+		soc_component_mark_pop(component, stream, pm);
+	}
+}
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 415510909a82..3a6a60215e81 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -73,18 +73,13 @@ static int soc_compr_components_free(struct snd_compr_stream *cstream,
 static int soc_compr_open(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_component *component = NULL, *save = NULL;
+	struct snd_soc_component *component = NULL;
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
-	int ret, i;
+	int ret;
 
-	for_each_rtd_components(rtd, i, component) {
-		ret = pm_runtime_get_sync(component->dev);
-		if (ret < 0 && ret != -EACCES) {
-			pm_runtime_put_noidle(component->dev);
-			save = component;
-			goto pm_err;
-		}
-	}
+	ret = snd_soc_pcm_component_pm_runtime_get(rtd, cstream);
+	if (ret < 0)
+		goto pm_err;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
@@ -113,12 +108,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 out:
 	mutex_unlock(&rtd->card->pcm_mutex);
 pm_err:
-	for_each_rtd_components(rtd, i, component) {
-		if (component == save)
-			break;
-		pm_runtime_mark_last_busy(component->dev);
-		pm_runtime_put_autosuspend(component->dev);
-	}
+	snd_soc_pcm_component_pm_runtime_put(rtd, cstream, 1);
 
 	return ret;
 }
@@ -205,10 +195,9 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream)
 static int soc_compr_free(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_component *component;
 	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
-	int stream, i;
+	int stream;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
@@ -237,10 +226,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 
 	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);
-	}
+	snd_soc_pcm_component_pm_runtime_put(rtd, cstream, 0);
 
 	return 0;
 }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index c034b8a1e6f9..6243e1a0561c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -678,10 +678,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	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);
-	}
+	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 0);
 
 	for_each_rtd_components(rtd, i, component)
 		if (!snd_soc_component_active(component))
@@ -708,8 +705,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	for_each_rtd_components(rtd, i, component)
 		pinctrl_pm_select_default_state(component->dev);
 
-	for_each_rtd_components(rtd, i, component)
-		pm_runtime_get_sync(component->dev);
+	ret = snd_soc_pcm_component_pm_runtime_get(rtd, substream);
+	if (ret < 0)
+		goto pm_err;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
@@ -806,11 +804,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	soc_pcm_components_close(substream, 1);
 component_err:
 	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);
-	}
+pm_err:
+	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 1);
 
 	for_each_rtd_components(rtd, i, component)
 		if (!snd_soc_component_active(component))
-- 
2.25.1

[PATCH 5/7] ASoC: soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

Now, 1) to 5) are handled.
This patch adds new soc_pcm_clean() and call it from
soc_pcm_open() as rollback, and from soc_pcm_close() as
normal close handler.

One note here is that it don't need to call snd_soc_runtime_deactivate()
when rollback case, because it will be called without
snd_soc_runtime_activate().
It also don't need to call snd_soc_dapm_stream_stop() when rollback case.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 6243e1a0561c..25e368e2b74b 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -651,12 +651,7 @@ 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)
+static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_component *component;
@@ -665,20 +660,22 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
-	snd_soc_runtime_deactivate(rtd, substream->stream);
+	if (!rollback)
+		snd_soc_runtime_deactivate(rtd, substream->stream);
 
 	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream, 0);
+		snd_soc_dai_shutdown(dai, substream, rollback);
 
-	snd_soc_link_shutdown(substream, 0);
+	snd_soc_link_shutdown(substream, rollback);
 
-	soc_pcm_components_close(substream, 0);
+	soc_pcm_components_close(substream, rollback);
 
-	snd_soc_dapm_stream_stop(rtd, substream->stream);
+	if (!rollback)
+		snd_soc_dapm_stream_stop(rtd, substream->stream);
 
 	mutex_unlock(&rtd->card->pcm_mutex);
 
-	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 0);
+	snd_soc_pcm_component_pm_runtime_put(rtd, substream, rollback);
 
 	for_each_rtd_components(rtd, i, component)
 		if (!snd_soc_component_active(component))
@@ -687,6 +684,16 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+/*
+ * 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)
+{
+	return soc_pcm_clean(substream, 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
@@ -707,17 +714,17 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	ret = snd_soc_pcm_component_pm_runtime_get(rtd, substream);
 	if (ret < 0)
-		goto pm_err;
+		goto err;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
 	ret = soc_pcm_components_open(substream);
 	if (ret < 0)
-		goto component_err;
+		goto err;
 
 	ret = snd_soc_link_startup(substream);
 	if (ret < 0)
-		goto rtd_startup_err;
+		goto err;
 
 	/* startup the audio subsystem */
 	for_each_rtd_dais(rtd, i, dai) {
@@ -726,7 +733,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 			dev_err(dai->dev,
 				"ASoC: can't open DAI %s: %d\n",
 				dai->name, ret);
-			goto config_err;
+			goto err;
 		}
 
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
@@ -755,18 +762,18 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	if (!runtime->hw.rates) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n",
 			codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 	if (!runtime->hw.formats) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching formats\n",
 			codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 	if (!runtime->hw.channels_min || !runtime->hw.channels_max ||
 	    runtime->hw.channels_min > runtime->hw.channels_max) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching channels\n",
 				codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 
 	soc_pcm_apply_msb(substream);
@@ -776,7 +783,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		if (snd_soc_dai_active(dai)) {
 			ret = soc_pcm_apply_symmetry(substream, dai);
 			if (ret != 0)
-				goto config_err;
+				goto err;
 		}
 	}
 
@@ -787,29 +794,13 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		 runtime->hw.channels_max);
 	pr_debug("ASoC: min rate %d max rate %d\n", runtime->hw.rate_min,
 		 runtime->hw.rate_max);
-
 dynamic:
-
 	snd_soc_runtime_activate(rtd, substream->stream);
-
-	mutex_unlock(&rtd->card->pcm_mutex);
-	return 0;
-
-config_err:
-	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream, 1);
-
-	snd_soc_link_shutdown(substream, 1);
-rtd_startup_err:
-	soc_pcm_components_close(substream, 1);
-component_err:
+err:
 	mutex_unlock(&rtd->card->pcm_mutex);
-pm_err:
-	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 1);
 
-	for_each_rtd_components(rtd, i, component)
-		if (!snd_soc_component_active(component))
-			pinctrl_pm_select_sleep_state(component->dev);
+	if (ret < 0)
+		soc_pcm_clean(substream, 1);
 
 	return ret;
 }
-- 
2.25.1

[PATCH 6/7] ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_dai_startup()

[Kuninori Morimoto]

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

snd_soc_dai_startup() itself will indicate error message,
thus, soc_pcm_open() don't need to handle it.
This patch removes it.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 25e368e2b74b..aff84fdb493e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -729,12 +729,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	/* startup the audio subsystem */
 	for_each_rtd_dais(rtd, i, dai) {
 		ret = snd_soc_dai_startup(dai, substream);
-		if (ret < 0) {
-			dev_err(dai->dev,
-				"ASoC: can't open DAI %s: %d\n",
-				dai->name, ret);
+		if (ret < 0)
 			goto err;
-		}
 
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			dai->tx_mask = 0;
-- 
2.25.1

[PATCH 7/7] ASoC: soc-pcm: remove unneeded dev_err() for snd_soc_component_module/open()

[Kuninori Morimoto]

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

snd_soc_component_module_get(), snd_soc_component_open() itself will indicate
error message, thus, soc_pcm_components_open() don't need to handle it.
This patch removes these.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index aff84fdb493e..884e99971464 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -614,20 +614,12 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream)
 
 	for_each_rtd_components(rtd, i, component) {
 		ret = snd_soc_component_module_get_when_open(component, substream);
-		if (ret < 0) {
-			dev_err(component->dev,
-				"ASoC: can't get module %s\n",
-				component->name);
+		if (ret < 0)
 			break;
-		}
 
 		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);
+		if (ret < 0)
 			break;
-		}
 	}
 
 	return ret;
-- 
2.25.1

Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Pierre-Louis Bossart]

Hi Morimoto-san,

> +#define soc_dai_mark_push(dai, substream, tgt)	((dai)->mark_##tgt = substream)

we may want a check that detects if the pointer is NULL before assigning 
it, otherwise we won't be able to detect bad configuration where a 
pointer is overwritten by 2 mark_push() calls on the same object?

> +#define soc_dai_mark_pop(dai, substream, tgt)	((dai)->mark_##tgt = NULL)
> +#define soc_dai_mark_match(dai, substream, tgt)	((dai)->mark_##tgt == substream)
> +
>   /**
>    * snd_soc_dai_set_sysclk - configure DAI system or master clock.
>    * @dai: DAI
> @@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
>   	    dai->driver->ops->startup)
>   		ret = dai->driver->ops->startup(substream, dai);
>   
> +	/* mark substream if succeeded */
> +	if (ret == 0)
> +		soc_dai_mark_push(dai, substream, startup);
> +

I am a bit concerned here about the case of a bi-directional DAI, it's 
my understanding that the .startup() callback could be called for each 
direction?

soc-dapm.c:		ret = snd_soc_dai_startup(source, substream);
soc-dapm.c:		ret = snd_soc_dai_startup(sink, substream);

To convince myself of this, I added a dummy startup routine and I do see 
it called when I do playback and capture at the same time:

[  179.057494] plb: ssp2 startup stream 0
[  183.976963] plb: ssp2 startup stream 1

That makes me nervous about having a single pointer and unbalanced calls 
between startup and shutdown.

We had such issues in the past so I may be on the paranoid side here...

Thanks
-Pierre

Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Ranjani Sridharan]

On Tue, 2020-07-28 at 15:57 +0900, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> soc_pcm_open() does rollback when failed (A),
> but, it is almost same as soc_pcm_close().
> 
> 	static int soc_pcm_open(xxx)
> 	{
> 		...
> 		if (ret < 0)
> 			goto xxx_err;
> 		...
> 		return 0;
> 
>  ^	config_err:
>  |		...
>  |	rtd_startup_err:
> (A)		...
>  |	component_err:
>  |		...
>  v		return ret;
> 	}
> 
> The difference is
> soc_pcm_close() is for all dai/component/substream,
> rollback        is for succeeded part only.
> 
> This kind of duplicated code can be a hotbed of bugs,
> thus, we want to share soc_pcm_close() and rollback.
> 
> Now, soc_pcm_open/close() are handling
> =>	1) snd_soc_dai_startup/shutdown()
> 	2) snd_soc_link_startup/shutdown()
> 	3) snd_soc_component_module_get/put()
> 	4) snd_soc_component_open/close()
> 	5) pm_runtime_put/get()
> 
> This patch is for 1) snd_soc_dai_startup/shutdown(),
> and adds new substream mark.
> It will mark substream when startup was suceeded.
> If rollback happen *after* that, it will check rollback flag
> and marked substream.
> 
> It cares *previous* startup() only now,
> but we might want to check *whole* marked substream in the future.
> This patch is using macro so that it can be easily adjust to it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc-dai.h |  5 ++++-
>  sound/soc/soc-dai.c     | 21 ++++++++++++++++++++-
>  sound/soc/soc-dapm.c    |  4 ++--
>  sound/soc/soc-pcm.c     |  4 ++--
>  4 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index 776a60529e70..86411f503c1c 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai,
>  int snd_soc_dai_startup(struct snd_soc_dai *dai,
>  			struct snd_pcm_substream *substream);
>  void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
> -			  struct snd_pcm_substream *substream);
> +			  struct snd_pcm_substream *substream, int
> rollback);
>  snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
>  				    struct snd_pcm_substream
> *substream);
>  void snd_soc_dai_suspend(struct snd_soc_dai *dai);
> @@ -388,6 +388,9 @@ struct snd_soc_dai {
>  
>  	struct list_head list;
>  
> +	/* function mark */
> +	struct snd_pcm_substream *mark_startup;
> +
>  	/* bit field */
>  	unsigned int probed:1;
>  };
> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
> index 693893420bf0..4f9f73800ab0 100644
> --- a/sound/soc/soc-dai.c
> +++ b/sound/soc/soc-dai.c
> @@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai
> *dai,
>  	return ret;
>  }
>  
> +/*
> + * We might want to check substream by using list.
> + * In such case, we can update these macros.
> + */
> +#define soc_dai_mark_push(dai, substream, tgt)	((dai)-
> >mark_##tgt = substream)
> +#define soc_dai_mark_pop(dai, substream, tgt)	((dai)-
> >mark_##tgt = NULL)
> +#define soc_dai_mark_match(dai, substream, tgt)	((dai)-
> >mark_##tgt == substream)
> +
>  /**
>   * snd_soc_dai_set_sysclk - configure DAI system or master clock.
>   * @dai: DAI
> @@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai
> *dai,
>  	    dai->driver->ops->startup)
>  		ret = dai->driver->ops->startup(substream, dai);
>  
> +	/* mark substream if succeeded */
> +	if (ret == 0)
> +		soc_dai_mark_push(dai, substream, startup);
> +
>  	return soc_dai_ret(dai, ret);
>  }
>  
>  void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
> -			 struct snd_pcm_substream *substream)
> +			  struct snd_pcm_substream *substream,
> +			  int rollback)
>  {
> +	if (rollback && !soc_dai_mark_match(dai, substream, startup))
> +		return;
Morimoto-san,
Im having a hard time undersdtanding why we need the second check? When
will it ever be the case that this match will fail? 

I think if we want to roll-back in case of errors , we could simply
have a bool in snd_soc_dai to indicate that the dai has been started up
already and check that here to decide whether we should shut it down or
not. Ideally, a helper function like snd_soc_dai_shutdown_all() which
iterates through all the rtd dai's and shuts all of them that are
marked as started up should suffice and will be more intuitive.

Also, push/pop are associated with stacks and we're only really dealing
with one object here. So it is a bit misleading nomemclature-wise. 

What do you think?

Thanks,
Ranjani

Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Kuninori Morimoto]

Hi Ranjani

Thank you for your review

> >  void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
> > -			 struct snd_pcm_substream *substream)
> > +			  struct snd_pcm_substream *substream,
> > +			  int rollback)
> >  {
> > +	if (rollback && !soc_dai_mark_match(dai, substream, startup))
> > +		return;
> Morimoto-san,
> Im having a hard time undersdtanding why we need the second check? When
> will it ever be the case that this match will fail?

	for_each_rtd_dais(...) {
		ret = snd_soc_dai_startup(dai, substream);
		if (ret < 0)
			goto config_err;
		...
	}
	...
config_err:
        for_each_rtd_dais(rtd, i, dai)
		snd_soc_dai_shutdown(dai, substream, 1);

For example, if rtd had 10 DAIs, and .startup() failed at 3rd DAI,
the mark will be
     
	/* xxx here means unknown */
	dai[0].mark_startup = substream;
	dai[1].mark_startup = substream;
	dai[2].mark_startup = substream;
	dai[3].mark_startup = xxx;
	...
	dai[7].mark_startup = xxx;
	dai[8].mark_startup = xxx;
	dai[9].mark_startup = xxx;

Then, we need to call .shutdown() is for dai[0] - dai[2],
In such case, .shutdown() will be called with rollback flag.
if's second check will be failed on dai[3] - dai[9].
But please double check.

> I think if we want to roll-back in case of errors , we could simply
> have a bool in snd_soc_dai to indicate that the dai has been started up
> already and check that here to decide whether we should shut it down or
> not.

Yes, my v1 patch was that.
But someone (I don't remember who was he) indicated that
it is not enough if same DAI was called many times.

In my understanding, for example, if same DAI is used for 2xPlaybacks,
and 1st Playback was successed, 2nd Playback was failed,
and if it was using bool instead of pointer,

2nd Playback rollback doesn't need to call shutdown,
but it has successed bit of 1st Playback.
Then, 2nd Playback rollback will call unneeded shutdown,
and 1st Playback's necessary shutdown will not be called.

We can avoid such things if we use pointer instead of bool.
But please double check.

> Also, push/pop are associated with stacks and we're only really dealing
> with one object here. So it is a bit misleading nomemclature-wise. 

Yes maybe.
The reason why I used push/pop was that it might be updated to handle
pointer list instead of single pointer (and maybe Pierre-Louis want it :).
I think I indicated it on git log and comment.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Kuninori Morimoto]

Hi Pierre-Louis

Thank you for your review.

> > +#define soc_dai_mark_push(dai, substream, tgt)	((dai)->mark_##tgt = substream)
> 
> we may want a check that detects if the pointer is NULL before
> assigning it, otherwise we won't be able to detect bad configuration
> where a pointer is overwritten by 2 mark_push() calls on the same
> object?

One assumption here is that open() / close() pair are called same number of times.
open() / close() unbalance is not mentioned here, it is other problem.

My expectation for this mark is that it will be used only for rollback.
The necessary things in such case is that marked pointer is match to
current pointer, or not when rollback case.
Thus, overwritten is not problem in my understanding.

> I am a bit concerned here about the case of a bi-directional DAI, it's
> my understanding that the .startup() callback could be called for each
> direction?
> 
> soc-dapm.c:		ret = snd_soc_dai_startup(source, substream);
> soc-dapm.c:		ret = snd_soc_dai_startup(sink, substream);
> 
> To convince myself of this, I added a dummy startup routine and I do
> see it called when I do playback and capture at the same time:
> 
> [  179.057494] plb: ssp2 startup stream 0
> [  183.976963] plb: ssp2 startup stream 1
> 
> That makes me nervous about having a single pointer and unbalanced
> calls between startup and shutdown.
> 
> We had such issues in the past so I may be on the paranoid side here...

Thank you for sharing your experience.
As I mentioned above, this mark is used only for rollback of open(),
not related to close().

But hmm.. I now double checked the code, 1 concern is mutex_lock().
In final step, the soc_pcm_open() will be

	static int soc_pcm_open()
	{
		...
(1)		mutex_lock_nested(...);
		...
		for_each_rtd_dais(rtd, i, dai) {
(A)			ret = snd_soc_dai_startup(dai, substream);
			...
		}
		...
	err:
(2)		mutex_unlock(&rtd->card->pcm_mutex);

		if (ret < 0)
(B)			soc_pcm_clean(substream, 1);
		...
	}

(A) is called under (1) lock, but it will be unlocked at (2),
and (B) is called if rollback.

If
	- 2 x soc_pcm_open() were called in the same time
	- 1st soc_pcm_open() was failed
	- 2nd soc_pcm_open() was called between 1st soc_pcm_open()'s (2) and (B)

Indeed single pointer is not good...
!?
But, soc_pcm_open() itself is called under othere mutex_lock() of pcm_native.c
Above issue never happen ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[PATCH 1/7] ASoC: soc-dai: add mark for snd_soc_dai_startup/shutdown()

[Kuninori Morimoto]

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

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
=>	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

This patch is for 1) snd_soc_dai_startup/shutdown().

The idea of having bit-flag or counter is not enough for this purpose.
For example if one DAI is used for 2xPlaybacks for some reasons,
and if 1st Playback was succeeded but 2nd Playback was failed,
2nd Playback rollback doesn't need to call shutdown.
But it has succeeded bit-flag or counter via 1st Playback,
thus, 2nd Playback rollback will call unneeded shutdown.
And 1st Playback's necessary shutdown will not be called,
because bit-flag or counter was cleared by wrong 2nd Playback rollback.

To avoid such case, this patch marks substream pointer when startup() was
succeeded. If rollback needed, it will check rollback flag and marked
substream pointer.

One note here is that it cares *current* startup() only now.
but we might want to check *whole* marked substream in the future.
This patch is using macro named "push/pop", so that it can be easily
update.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-dai.h |  5 ++++-
 sound/soc/soc-dai.c     | 21 ++++++++++++++++++++-
 sound/soc/soc-dapm.c    |  4 ++--
 sound/soc/soc-pcm.c     |  4 ++--
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 8b693dade9c6..2150bd4c7a05 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -153,7 +153,7 @@ void snd_soc_dai_hw_free(struct snd_soc_dai *dai,
 int snd_soc_dai_startup(struct snd_soc_dai *dai,
 			struct snd_pcm_substream *substream);
 void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
-			  struct snd_pcm_substream *substream);
+			  struct snd_pcm_substream *substream, int rollback);
 snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
 				    struct snd_pcm_substream *substream);
 void snd_soc_dai_suspend(struct snd_soc_dai *dai);
@@ -388,6 +388,9 @@ struct snd_soc_dai {
 
 	struct list_head list;
 
+	/* function mark */
+	struct snd_pcm_substream *mark_startup;
+
 	/* bit field */
 	unsigned int probed:1;
 };
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 0dbd312aad08..4705c3da6280 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -32,6 +32,14 @@ static inline int _soc_dai_ret(struct snd_soc_dai *dai,
 	return ret;
 }
 
+/*
+ * We might want to check substream by using list.
+ * In such case, we can update these macros.
+ */
+#define soc_dai_mark_push(dai, substream, tgt)	((dai)->mark_##tgt = substream)
+#define soc_dai_mark_pop(dai, substream, tgt)	((dai)->mark_##tgt = NULL)
+#define soc_dai_mark_match(dai, substream, tgt)	((dai)->mark_##tgt == substream)
+
 /**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
@@ -348,15 +356,26 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
 	    dai->driver->ops->startup)
 		ret = dai->driver->ops->startup(substream, dai);
 
+	/* mark substream if succeeded */
+	if (ret == 0)
+		soc_dai_mark_push(dai, substream, startup);
+
 	return soc_dai_ret(dai, ret);
 }
 
 void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
-			 struct snd_pcm_substream *substream)
+			  struct snd_pcm_substream *substream,
+			  int rollback)
 {
+	if (rollback && !soc_dai_mark_match(dai, substream, startup))
+		return;
+
 	if (dai->driver->ops &&
 	    dai->driver->ops->shutdown)
 		dai->driver->ops->shutdown(substream, dai);
+
+	/* remove marked substream */
+	soc_dai_mark_pop(dai, substream, startup);
 }
 
 snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 3273161e2787..980f2c330b87 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3968,14 +3968,14 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 		snd_soc_dapm_widget_for_each_source_path(w, path) {
 			source = path->source->priv;
 			snd_soc_dai_deactivate(source, substream->stream);
-			snd_soc_dai_shutdown(source, substream);
+			snd_soc_dai_shutdown(source, substream, 0);
 		}
 
 		substream->stream = SNDRV_PCM_STREAM_PLAYBACK;
 		snd_soc_dapm_widget_for_each_sink_path(w, path) {
 			sink = path->sink->priv;
 			snd_soc_dai_deactivate(sink, substream->stream);
-			snd_soc_dai_shutdown(sink, substream);
+			snd_soc_dai_shutdown(sink, substream, 0);
 		}
 		break;
 
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 4c9d4cd8cf0b..00ed55e40819 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -682,7 +682,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	snd_soc_runtime_deactivate(rtd, substream->stream);
 
 	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream);
+		snd_soc_dai_shutdown(dai, substream, 0);
 
 	snd_soc_link_shutdown(substream);
 
@@ -813,7 +813,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 config_err:
 	for_each_rtd_dais_rollback(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream);
+		snd_soc_dai_shutdown(dai, substream, 1);
 
 	snd_soc_link_shutdown(substream);
 rtd_startup_err:
-- 
2.25.1