[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

ASoC component open/close and snd_soc_component_module_get/put are
called independently for each component-substream pair, so the logic
in the reverted patch was not sufficient and led to PCM playback and
module unload errors.

Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 include/sound/soc-component.h |  7 ++-----
 sound/soc/soc-component.c     | 35 +++++++----------------------------
 sound/soc/soc-pcm.c           | 19 +++++++++++++------
 3 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 1866ecc8e94b..154d02fbbfed 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -147,6 +147,8 @@ struct snd_soc_component {
 
 	unsigned int active;
 
+	unsigned int suspended:1; /* is in suspend PM state */
+
 	struct list_head list;
 	struct list_head card_aux_list; /* for auxiliary bound components */
 	struct list_head card_list;
@@ -180,11 +182,6 @@ struct snd_soc_component {
 	struct dentry *debugfs_root;
 	const char *debugfs_prefix;
 #endif
-
-	/* bit field */
-	unsigned int suspended:1; /* is in suspend PM state */
-	unsigned int opened:1;
-	unsigned int module:1;
 };
 
 #define for_each_component_dais(component, dai)\
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index ee00c09df5e7..14e175cdeeb8 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -297,55 +297,34 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
 int snd_soc_component_module_get(struct snd_soc_component *component,
 				 int upon_open)
 {
-	if (component->module)
-		return 0;
-
 	if (component->driver->module_get_upon_open == !!upon_open &&
 	    !try_module_get(component->dev->driver->owner))
 		return -ENODEV;
 
-	component->module = 1;
-
 	return 0;
 }
 
 void snd_soc_component_module_put(struct snd_soc_component *component,
 				  int upon_open)
 {
-	if (component->module &&
-	    component->driver->module_get_upon_open == !!upon_open)
+	if (component->driver->module_get_upon_open == !!upon_open)
 		module_put(component->dev->driver->owner);
-
-	component->module = 0;
 }
 
 int snd_soc_component_open(struct snd_soc_component *component,
 			   struct snd_pcm_substream *substream)
 {
-	int ret = 0;
-
-	if (!component->opened &&
-	    component->driver->open)
-		ret = component->driver->open(component, substream);
-
-	if (ret == 0)
-		component->opened = 1;
-
-	return ret;
+	if (component->driver->open)
+		return component->driver->open(component, substream);
+	return 0;
 }
 
 int snd_soc_component_close(struct snd_soc_component *component,
 			    struct snd_pcm_substream *substream)
 {
-	int ret = 0;
-
-	if (component->opened &&
-	    component->driver->close)
-		ret = component->driver->close(component, substream);
-
-	component->opened = 0;
-
-	return ret;
+	if (component->driver->close)
+		return component->driver->close(component, substream);
+	return 0;
 }
 
 int snd_soc_component_prepare(struct snd_soc_component *component,
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 63f67eb7c077..0c3a12d6290a 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -466,13 +466,16 @@ 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)
+static int soc_pcm_components_open(struct snd_pcm_substream *substream,
+				   struct snd_soc_component **last)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	int i, ret = 0;
 
 	for_each_rtd_components(rtd, i, component) {
+		*last = component;
+
 		ret = snd_soc_component_module_get_when_open(component);
 		if (ret < 0) {
 			dev_err(component->dev,
@@ -489,17 +492,21 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream)
 			return ret;
 		}
 	}
-
+	*last = NULL;
 	return 0;
 }
 
-static int soc_pcm_components_close(struct snd_pcm_substream *substream)
+static int soc_pcm_components_close(struct snd_pcm_substream *substream,
+				    struct snd_soc_component *last)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component;
 	int i, r, ret = 0;
 
 	for_each_rtd_components(rtd, i, component) {
+		if (component == last)
+			break;
+
 		r = snd_soc_component_close(component, substream);
 		if (r < 0)
 			ret = r; /* use last ret */
@@ -536,7 +543,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	soc_rtd_shutdown(rtd, substream);
 
-	soc_pcm_components_close(substream);
+	soc_pcm_components_close(substream, NULL);
 
 	snd_soc_dapm_stream_stop(rtd, substream->stream);
 
@@ -577,7 +584,7 @@ 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);
+	ret = soc_pcm_components_open(substream, &component);
 	if (ret < 0)
 		goto component_err;
 
@@ -682,7 +689,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	soc_rtd_shutdown(rtd, substream);
 component_err:
-	soc_pcm_components_close(substream);
+	soc_pcm_components_close(substream, component);
 
 	mutex_unlock(&rtd->card->pcm_mutex);
 
-- 
2.17.1

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hey,

On Wed, 19 Feb 2020, Kai Vehmanen wrote:

> ASoC component open/close and snd_soc_component_module_get/put are
> called independently for each component-substream pair, so the logic
> in the reverted patch was not sufficient and led to PCM playback and
> module unload errors.

I tried to keep part of the original patch at first, but I kept hitting 
new issues either in component load, or in module unload-reload flow.
Thanks to Pierre and Ranjani for early reviews.

So in the end I ended up with a full revert. This at least works on all 
SOF device topologies I tested with. 

At the root of the problem is that component open is called with multiple 
substreams and driver open (and close) should be called for each substream 
as well. This also caused problems to the added module refcounting logic.. 
so that is reverted as well.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Pierre-Louis Bossart]

On 2/19/20 12:53 PM, Kai Vehmanen wrote:
> Hey,
> 
> On Wed, 19 Feb 2020, Kai Vehmanen wrote:
> 
>> ASoC component open/close and snd_soc_component_module_get/put are
>> called independently for each component-substream pair, so the logic
>> in the reverted patch was not sufficient and led to PCM playback and
>> module unload errors.
> 
> I tried to keep part of the original patch at first, but I kept hitting
> new issues either in component load, or in module unload-reload flow.
> Thanks to Pierre and Ranjani for early reviews.
> 
> So in the end I ended up with a full revert. This at least works on all
> SOF device topologies I tested with.
> 
> At the root of the problem is that component open is called with multiple
> substreams and driver open (and close) should be called for each substream
> as well. This also caused problems to the added module refcounting logic..
> so that is reverted as well.

I also tried to find a fix in parallel with Kai's work, but no luck. 
First I am not sure why we need to add a 'module' state in addition to 
the existing module ref-counting, and the 'opened' state management 
looks too simple. revert-and-revisit seems like the best course of 
action indeed.

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Dmitry Osipenko]

19.02.2020 21:26, Kai Vehmanen пишет:
> ASoC component open/close and snd_soc_component_module_get/put are
> called independently for each component-substream pair, so the logic
> in the reverted patch was not sufficient and led to PCM playback and
> module unload errors.
> 
> Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---

Thanks,

Tested-by: Dmitry Osipenko <digetx@gmail.com>

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai, Pierre-Louis

Thank you for your test / review

> ASoC component open/close and snd_soc_component_module_get/put are
> called independently for each component-substream pair, so the logic
> in the reverted patch was not sufficient and led to PCM playback and
> module unload errors.
> 
> Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---

OK, some component (SOF, and maybe DPCM too ?) want to be opened for each substreams.
If so, indeed current code is not enough for it.

But, unfortunately I don't want spaghetti error handling code again.
I think we can solve it if we can *count* open / module ?
Can you please test this patch ?

--- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< --- 8< ---
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 1866ecc8e94b..4e78925858c0 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -181,10 +181,11 @@ struct snd_soc_component {
 	const char *debugfs_prefix;
 #endif
 
+	u8 opened;
+	u8 module;
+
 	/* bit field */
 	unsigned int suspended:1; /* is in suspend PM state */
-	unsigned int opened:1;
-	unsigned int module:1;
 };
 
 #define for_each_component_dais(component, dai)\
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index ee00c09df5e7..a2526a1ffcbe 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
 int snd_soc_component_module_get(struct snd_soc_component *component,
 				 int upon_open)
 {
-	if (component->module)
-		return 0;
+	if (unlikely(component->module == 0xff)) {
+		dev_warn(component->dev, "too many module get (%s)\n", component->name);
+		return -EBUSY;
+	}
 
 	if (component->driver->module_get_upon_open == !!upon_open &&
 	    !try_module_get(component->dev->driver->owner))
 		return -ENODEV;
 
-	component->module = 1;
+	component->module++;
 
 	return 0;
 }
@@ -316,7 +318,7 @@ void snd_soc_component_module_put(struct snd_soc_component *component,
 	    component->driver->module_get_upon_open == !!upon_open)
 		module_put(component->dev->driver->owner);
 
-	component->module = 0;
+	component->module--;
 }
 
 int snd_soc_component_open(struct snd_soc_component *component,
@@ -324,12 +326,15 @@ int snd_soc_component_open(struct snd_soc_component *component,
 {
 	int ret = 0;
 
-	if (!component->opened &&
-	    component->driver->open)
+	if (unlikely(component->opened == 0xff)) {
+		dev_warn(component->dev, "too many open (%s)\n", component->name);
+		return -EBUSY;
+	}
+
+	if (component->driver->open)
 		ret = component->driver->open(component, substream);
 
-	if (ret == 0)
-		component->opened = 1;
+	component->opened++;
 
 	return ret;
 }
@@ -343,7 +348,7 @@ int snd_soc_component_close(struct snd_soc_component *component,
 	    component->driver->close)
 		ret = component->driver->close(component, substream);
 
-	component->opened = 0;
+	component->opened--;
 
 	return ret;
 }


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

[PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

[Kuninori Morimoto]

ASoC component open/close and snd_soc_component_module_get/put are
called once for each component, but we need it for each substream.
To solve this issue, this patch counts open / get,
and call close / put accordingly.

Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
I tidyuped code.
I hope it can solve the issue.

 include/sound/soc-component.h |  5 +++--
 sound/soc/soc-component.c     | 35 ++++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 1866ecc8e94b..4e78925858c0 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -181,10 +181,11 @@ struct snd_soc_component {
 	const char *debugfs_prefix;
 #endif
 
+	u8 opened;
+	u8 module;
+
 	/* bit field */
 	unsigned int suspended:1; /* is in suspend PM state */
-	unsigned int opened:1;
-	unsigned int module:1;
 };
 
 #define for_each_component_dais(component, dai)\
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index ee00c09df5e7..bdd36be1fb70 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
 int snd_soc_component_module_get(struct snd_soc_component *component,
 				 int upon_open)
 {
-	if (component->module)
-		return 0;
+	if (unlikely(component->module == 0xff)) {
+		dev_warn(component->dev, "too many module get (%s)\n", component->name);
+		return -EBUSY;
+	}
 
 	if (component->driver->module_get_upon_open == !!upon_open &&
 	    !try_module_get(component->dev->driver->owner))
 		return -ENODEV;
 
-	component->module = 1;
+	component->module++;
 
 	return 0;
 }
@@ -312,11 +314,13 @@ int snd_soc_component_module_get(struct snd_soc_component *component,
 void snd_soc_component_module_put(struct snd_soc_component *component,
 				  int upon_open)
 {
-	if (component->module &&
-	    component->driver->module_get_upon_open == !!upon_open)
+	if (!component->module)
+		return;
+
+	if (component->driver->module_get_upon_open == !!upon_open)
 		module_put(component->dev->driver->owner);
 
-	component->module = 0;
+	component->module--;
 }
 
 int snd_soc_component_open(struct snd_soc_component *component,
@@ -324,12 +328,15 @@ int snd_soc_component_open(struct snd_soc_component *component,
 {
 	int ret = 0;
 
-	if (!component->opened &&
-	    component->driver->open)
+	if (unlikely(component->opened == 0xff)) {
+		dev_warn(component->dev, "too many open (%s)\n", component->name);
+		return -EBUSY;
+	}
+
+	if (component->driver->open)
 		ret = component->driver->open(component, substream);
 
-	if (ret == 0)
-		component->opened = 1;
+	component->opened++;
 
 	return ret;
 }
@@ -339,11 +346,13 @@ int snd_soc_component_close(struct snd_soc_component *component,
 {
 	int ret = 0;
 
-	if (component->opened &&
-	    component->driver->close)
+	if (!component->opened)
+		return;
+
+	if (component->driver->close)
 		ret = component->driver->close(component, substream);
 
-	component->opened = 0;
+	component->opened--;
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

[Sridharan, Ranjani]

On Wed, Feb 19, 2020 at 5:01 PM Kuninori Morimoto <
kuninori.morimoto.gx@renesas.com> wrote:

> ASoC component open/close and snd_soc_component_module_get/put are
> called once for each component, but we need it for each substream.
> To solve this issue, this patch counts open / get,
> and call close / put accordingly.
>
> Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
> once")
> Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> I tidyuped code.
> I hope it can solve the issue.
>
>  include/sound/soc-component.h |  5 +++--
>  sound/soc/soc-component.c     | 35 ++++++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
> index 1866ecc8e94b..4e78925858c0 100644
> --- a/include/sound/soc-component.h
> +++ b/include/sound/soc-component.h
> @@ -181,10 +181,11 @@ struct snd_soc_component {
>         const char *debugfs_prefix;
>  #endif
>
> +       u8 opened;
> +       u8 module;
> +
>         /* bit field */
>         unsigned int suspended:1; /* is in suspend PM state */
> -       unsigned int opened:1;
> -       unsigned int module:1;
>  };
>
>  #define for_each_component_dais(component, dai)\
> diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
> index ee00c09df5e7..bdd36be1fb70 100644
> --- a/sound/soc/soc-component.c
> +++ b/sound/soc/soc-component.c
> @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
>  int snd_soc_component_module_get(struct snd_soc_component *component,
>                                  int upon_open)
>  {
> -       if (component->module)
> -               return 0;
> +       if (unlikely(component->module == 0xff)) {
> +               dev_warn(component->dev, "too many module get (%s)\n",
> component->name);
> +               return -EBUSY;
> +       }
>
>         if (component->driver->module_get_upon_open == !!upon_open &&
>             !try_module_get(component->dev->driver->owner))
>                 return -ENODEV;
>
> -       component->module = 1;
> +       component->module++;
>
Thanks, Morimoto-san for the alternate fix. I understand the rationale for
having a count for component->opened, but what is the rationale for the
module count? What it is intended to protect?

Thanks,
Ranjani

Re: [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

[Kuninori Morimoto]

Hi Sridharan

>     ASoC component open/close and snd_soc_component_module_get/put are
>     called once for each component, but we need it for each substream.
>     To solve this issue, this patch counts open / get,
>     and call close / put accordingly.
>
>     Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
>     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>     ---
(snip)
>     @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
>      int snd_soc_component_module_get(struct snd_soc_component *component,
>                                      int upon_open)
>      {
>     -       if (component->module)
>     -               return 0;
>     +       if (unlikely(component->module == 0xff)) {
>     +               dev_warn(component->dev, "too many module get (%s)\n", component->name);
>     +               return -EBUSY;
>     +       }
>
>             if (component->driver->module_get_upon_open == !!upon_open &&
>                 !try_module_get(component->dev->driver->owner))
>                     return -ENODEV;
>
>     -       component->module = 1;
>     +       component->module++;
>
> Thanks, Morimoto-san for the alternate fix. I understand the rationale for having a count for component->opened, but what is the rationale for the module count? What it is
> intended to protect?

I think same as open ?
It protects calling put() from not-get-component.
Because module_put() has WARN_ON(ret < 0).

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

Re: [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

[Sridharan, Ranjani]

On Wed, Feb 19, 2020 at 5:42 PM Kuninori Morimoto <
kuninori.morimoto.gx@renesas.com> wrote:

>
> Hi Sridharan
>
> >     ASoC component open/close and snd_soc_component_module_get/put are
> >     called once for each component, but we need it for each substream.
> >     To solve this issue, this patch counts open / get,
> >     and call close / put accordingly.
> >
> >     Fixes: dd03907bf129 ("ASoC: soc-pcm: call
> snd_soc_component_open/close() once")
> >     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> >     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >     ---
> (snip)
> >     @@ -297,14 +297,16 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_jack);
> >      int snd_soc_component_module_get(struct snd_soc_component
> *component,
> >                                      int upon_open)
> >      {
> >     -       if (component->module)
> >     -               return 0;
> >     +       if (unlikely(component->module == 0xff)) {
> >     +               dev_warn(component->dev, "too many module get
> (%s)\n", component->name);
> >     +               return -EBUSY;
> >     +       }
> >
> >             if (component->driver->module_get_upon_open == !!upon_open &&
> >                 !try_module_get(component->dev->driver->owner))
> >                     return -ENODEV;
> >
> >     -       component->module = 1;
> >     +       component->module++;
> >
> > Thanks, Morimoto-san for the alternate fix. I understand the rationale
> for having a count for component->opened, but what is the rationale for the
> module count? What it is
> > intended to protect?
>
> I think same as open ?
> It protects calling put() from not-get-component.
> Because module_put() has WARN_ON(ret < 0).
>
Can we use the module_refcount instead of adding a new field?

Thanks,
Ranjani

Re: [PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

[Kuninori Morimoto]

Hi Sridharan

>     >     ASoC component open/close and snd_soc_component_module_get/put are
>     >     called once for each component, but we need it for each substream.
>     >     To solve this issue, this patch counts open / get,
>     >     and call close / put accordingly.
>     >
>     >     Fixes: dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close() once")
>     >     Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>     >     Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>     >     ---
(snip)
>     I think same as open ?
>     It protects calling put() from not-get-component.
>     Because module_put() has WARN_ON(ret < 0).
> 
> Can we use the module_refcount instead of adding a new field?

Ahh, maybe yes.
I can try to use it in non-RFC patch.

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hi,

On Thu, 20 Feb 2020, Kuninori Morimoto wrote:

> > ASoC component open/close and snd_soc_component_module_get/put are
> > called independently for each component-substream pair, so the logic
> > in the reverted patch was not sufficient and led to PCM playback and
> > module unload errors.
> 
> But, unfortunately I don't want spaghetti error handling code again.
> I think we can solve it if we can *count* open / module ?
> Can you please test this patch ?

I tested and this version works as well, so:
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

... but, but, I have some doubts about th "opened" tracking as a solution.

A single counter will track the overall number of component-substream 
combinations, but if we have bugs in calling code and e.g. same 
component-substream is passed multiple times to open or close, the 
the single counter will go out of sync.

So if the primary problem is the messy rollback in case one open fails, 
what if we do the rollback directly in soc_pcm_components_open() and do
not add any additional tracking..?

Let me send a proposal patch for that.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for your help

> > But, unfortunately I don't want spaghetti error handling code again.
> > I think we can solve it if we can *count* open / module ?
> > Can you please test this patch ?
> 
> I tested and this version works as well, so:
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> 
> ... but, but, I have some doubts about th "opened" tracking as a solution.
> 
> A single counter will track the overall number of component-substream 
> combinations, but if we have bugs in calling code and e.g. same 
> component-substream is passed multiple times to open or close, the 
> the single counter will go out of sync.
> 
> So if the primary problem is the messy rollback in case one open fails, 
> what if we do the rollback directly in soc_pcm_components_open() and do
> not add any additional tracking..?
> 
> Let me send a proposal patch for that.

Hmm...
It seems the patch was not so good cleanup...

Thank you for your proposal patch. I checked it.
But, if it rollback when error with *last,
my opinion is original code
(= soc_pcm_components_close() needs *last parameter)
(= same as just revert the patch) is better.


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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hello Morimoto-san,

On Fri, 21 Feb 2020, Kuninori Morimoto wrote:

> Kai Vehmanen wrote:
> > So if the primary problem is the messy rollback in case one open fails, 
> > what if we do the rollback directly in soc_pcm_components_open() and do
> > not add any additional tracking..?
> > 
> > Let me send a proposal patch for that.
> It seems the patch was not so good cleanup...
> 
> Thank you for your proposal patch. I checked it.
> But, if it rollback when error with *last,
> my opinion is original code
> (= soc_pcm_components_close() needs *last parameter)
> (= same as just revert the patch) is better.

well, I do think the original code could still need a cleanup, so the 
effort is very much welcome. Having to pass the "last" parameter to 
components_open() just so that we can clean up if errors happen, seems
a bit out of place.

But yeah, easier said than done. We have now three alternatives to solve 
this:

1. do the cleanup locally in soc_pcm_components_open()
	[PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()

2. revert to original implementation with "last" passed to components_open()
	[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

3. implement opened/module counters
	[PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

... I have tested all three and they seem to work.

I still don't really like (1), but I agree (2) adds more code in total.
I worry (3) might backfire with some component-substream combinations.

So maybe we'll go with (1)? We do need to merge one of them, because 
current merged code is broken on many platforms.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for your feedback / test / review

> 1. do the cleanup locally in soc_pcm_components_open()
> 	[PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
> 
> 2. revert to original implementation with "last" passed to components_open()
> 	[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
> 
> 3. implement opened/module counters
> 	[PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()
> 
> ... I have tested all three and they seem to work.
> 
> I still don't really like (1), but I agree (2) adds more code in total.
> I worry (3) might backfire with some component-substream combinations.
> 
> So maybe we'll go with (1)? We do need to merge one of them, because 
> current merged code is broken on many platforms.

It seems Mark accepted (1), and I have no big objection about it.
Thank you for your hard work

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai, again
Cc Mark

Maybe this is too late, but I want to tell
the reason why I wanted to add flag on each component.

> > 1. do the cleanup locally in soc_pcm_components_open()
> > 	[PATCH] ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
> >
> > 2. revert to original implementation with "last" passed to components_open()
> > 	[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
> >
> > 3. implement opened/module counters
> > 	[PATCH][RFC] ASoC: soc-component: count snd_soc_component_open/close()

I can see this kind of implementation at many place.

For example, soc_pcm_open() has error handling.
But, it is same as soc_pcm_close(), but it can't call it directly,
because it needs to care about "successed until where" for now.

If each component / rtd / dai have "done" flag or count,
soc_pcm_open() can call soc_pcm_close() directly
without thinking about "until", because each flag can handle/indicate it.

The good point is we can reduce duplicate implementation.
And it can avoid future bug. Because today, we need to care both
soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.

	static int soc_pcm_open(struct snd_pcm_substream *substream)
	{
		...
		return 0;

		/*
		 * From here, "implementation" is same as soc_pcm_close()
		 */

	config_err:
		for_each_rtd_dais(rtd, i, dai)
			snd_soc_dai_shutdown(dai, substream);

		soc_rtd_shutdown(rtd, substream);
	rtd_startup_err:
		soc_pcm_components_close(substream, NULL);
	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);
		}

		for_each_rtd_components(rtd, i, component)
			if (!component->active)
				pinctrl_pm_select_sleep_state(component->dev);

		return ret;
	}

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Mark Brown]

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

On Wed, Feb 26, 2020 at 09:55:47AM +0900, Kuninori Morimoto wrote:

> If each component / rtd / dai have "done" flag or count,
> soc_pcm_open() can call soc_pcm_close() directly
> without thinking about "until", because each flag can handle/indicate it.

> The good point is we can reduce duplicate implementation.
> And it can avoid future bug. Because today, we need to care both
> soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.

That goal definitely makes sense, if we can avoid problems like the ones
here it seems like a useful change.

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Mark

> > If each component / rtd / dai have "done" flag or count,
> > soc_pcm_open() can call soc_pcm_close() directly
> > without thinking about "until", because each flag can handle/indicate it.
> 
> > The good point is we can reduce duplicate implementation.
> > And it can avoid future bug. Because today, we need to care both
> > soc_pcm_close() and error handling in soc_pcm_open(), it is not good for me.
> 
> That goal definitely makes sense, if we can avoid problems like the ones
> here it seems like a useful change.

Thank you for your feedback.
The one big headache is that, as Kai mentioned, how to know
the "handled substream". We need list or array, but...
I will postpone this kind of patches now, and post other patches.
And I will re-think about it again after that.

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hello Morimoto-san,

On Wed, 26 Feb 2020, Kuninori Morimoto wrote:

> Maybe this is too late, but I want to tell
> the reason why I wanted to add flag on each component.
[...]
> If each component / rtd / dai have "done" flag or count,
> soc_pcm_open() can call soc_pcm_close() directly
> without thinking about "until", because each flag can handle/indicate it.

thanks for the longer explanation. It's true we have a lot of code with 
for_each_xxx() loops, and loop logic where errors can occur. It seems the 
most common approach is to store the index and use for_each_xxx_rollback() 
macros to clean up in case error happens. This does lead to the problem of 
essentially duplicated logic e.g. for soc_pcm_close() and error handling 
of snd_pcm_open(). And yeah, it's also a bit error prone as the logic is 
not exactly the same. E.g. I'm not convinced this is quite right in 
soc_pcm_open():

»       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 config_err;                                                                                                                                                                     
»       »       }                                             
...
config_err:                                                                                                                                                                                                  
»       for_each_rtd_codec_dai(rtd, i, codec_dai)                                                                                                                                                            
»       »       snd_soc_dai_shutdown(codec_dai, substream);                                                                                                                                                  
»       i = rtd->num_cpus;
 
... i.e. we should use _rollback() macro here, but we don't so shutdown 
is called on all codec dais if I read this right.

Having a single cleanup path would be easier, but I think in the end it 
comes down to how cleanly you can track the opened state. It seems biggest 
issue is how to cleanly track the component-substream pairs. Ideally we'd 
have a dedicated state object to represent an opened component-substream 
pair, but that's not how the APIs are defined now. But something to that
effect is needed.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for feedback

> thanks for the longer explanation. It's true we have a lot of code with 
> for_each_xxx() loops, and loop logic where errors can occur. It seems the 
> most common approach is to store the index and use for_each_xxx_rollback() 
> macros to clean up in case error happens. This does lead to the problem of 
> essentially duplicated logic e.g. for soc_pcm_close() and error handling 
> of snd_pcm_open().

Thank you for understanding my headache.
My opinion is that complex duplicated code never bring luck to us.

> And yeah, it's also a bit error prone as the logic is 
> not exactly the same. E.g. I'm not convinced this is quite right in 
> soc_pcm_open():
> 
> »       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 config_err;                                                                                                                                                                     
> »       »       }                                             
> ...
> config_err:                                                                                                                                                                                                  
> »       for_each_rtd_codec_dai(rtd, i, codec_dai)                                                                                                                                                            
> »       »       snd_soc_dai_shutdown(codec_dai, substream);                                                                                                                                                  
> »       i = rtd->num_cpus;

Yeah indeed.
I think this is just one of the BUG.
I guess we can find similar issue everywhere.

> Having a single cleanup path would be easier, but I think in the end it 
> comes down to how cleanly you can track the opened state. It seems biggest 
> issue is how to cleanly track the component-substream pairs. Ideally we'd 
> have a dedicated state object to represent an opened component-substream 
> pair, but that's not how the APIs are defined now. But something to that
> effect is needed.

Yeah, I can understand your concern, but not 100% yet.
In my understanding, counting start vs stop is not enough but not so bad.
If my understanding was correct, your concern is
counting only is not enough, because wrong component-substream pair
can be used, like this ?

	start(substream-A); <=
	start(substream-B);
	start(substream-C);

	stop(substream-Z);  <=
	stop(substream-B);
	stop(substream-C);

But I wonder is it really happen ?
If there is clear such case, yes, we need to consider
component-substream pair list, somehow.

If you are worry about some kind of BUG, I guess we need to solve
is such bug, not error handling method.
But how about step-by-step approach (I like it :) ?
At first, add counting method as 1st step. Indeed it is not enough,
but we can cleanup error handling.
If we found/noticed above not-enough-case,
start to consider about component-substream pair list ?

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai, again

> > Having a single cleanup path would be easier, but I think in the end it 
> > comes down to how cleanly you can track the opened state. It seems biggest 
> > issue is how to cleanly track the component-substream pairs. Ideally we'd 
> > have a dedicated state object to represent an opened component-substream 
> > pair, but that's not how the APIs are defined now. But something to that
> > effect is needed.
> 
> Yeah, I can understand your concern, but not 100% yet.
> In my understanding, counting start vs stop is not enough but not so bad.
> If my understanding was correct, your concern is
> counting only is not enough, because wrong component-substream pair
> can be used, like this ?
> 
> 	start(substream-A); <=
> 	start(substream-B);
> 	start(substream-C);
> 
> 	stop(substream-Z);  <=
> 	stop(substream-B);
> 	stop(substream-C);
> 
> But I wonder is it really happen ?

Ohh, yes indeed !! I was confused.
But Hmm... I don't want to have substream list on each component...
Hmm... I will re-consider it again.

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai, again, and, again

> > 	start(substream-A); <=
> > 	start(substream-B);
> > 	start(substream-C);
> >
> > 	stop(substream-Z);  <=
> > 	stop(substream-B);
> > 	stop(substream-C);
(snip)
> Ohh, yes indeed !! I was confused.
> But Hmm... I don't want to have substream list on each component...
> Hmm... I will re-consider it again.

I don't want to have substream list on each components,
and keep simple code as much as possible.

My current idea is using ID. What do you think ?
It is not super simple though...

	int soc_pcm_components_open(struct snd_pcm_substream *substream, u8 id)
	{
		int ret = 0;

		/*
		 * Add ID to each component to know "which open".
		 */
		for_each_rtd_components(rtd, i, component) {
			if (component->driver->open) {
				ret = component->driver->open(component, substream);
				if (ret < 0)
					return ret;

				component->open_id = id; /* add ID */
			}
		}

		return 0;
	}

	int soc_pcm_components_close(struct snd_pcm_substream *substream, u8 id)
	{
		/*
		 * if ID > 0,  it is only target.
		 * if ID == 0, all components are the target
		 */
		for_each_rtd_components(rtd, i, component) {
			if ((id == 0 ||
			     id == component->open_id) &&
			    component->driver->close)
				component->driver->close(component, substream);
		}
		...
	}


=>	int soc_pcm_clear(..., u8 id)
	{
		...
		/*
		 * if ID > 0,  it is only target.
		 * if ID == 0, all components are the target
		 */
		soc_pcm_components_close(substream, id);
		...
	}

	int soc_pcm_close(...)
	{
		/*
		 * ID = 0
		 * All components are target of close
		 */
=>		soc_pcm_clear(xxx, 0);
	}

	int soc_pcm_open(...)
	{
		static u8 id;

		/* update ID */
		id++;
		if (id == 0)
			id++;

		...
		ret = soc_pcm_components_open(substream, id);
		if (ret < 0)
			goto open_err;
		...

		return 0; /* success */

	open_err:
		/*
		 * ID = id
		 * Only this IDs are the target
		 */
=>		soc_pcm_clear(xxx, id)

		return ret;
	}


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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hey,

catching up with the thread :)

On Fri, 28 Feb 2020, Kuninori Morimoto wrote:

> > > 	start(substream-A); <=
> > > 	start(substream-B);
> > > 	start(substream-C);
> > >
> > > 	stop(substream-Z);  <=
> > > 	stop(substream-B);
> > > 	stop(substream-C);
[snip]
> I don't want to have substream list on each components,
> and keep simple code as much as possible.
[snip]
> My current idea is using ID. What do you think ?
> It is not super simple though...

Hmm, I think then we end up with new problems managing the IDs.
Specifically:

> 	int soc_pcm_open(...)
> 	{
> 		static u8 id;
> 
> 		/* update ID */
> 		id++;
> 		if (id == 0)
> 			id++;

... this really isn't solid. If you have a complex scenario and something 
goes wrong, debugging the ids is going to be painful if they are assigned 
this way.

I think in the end we should go back to this:

int snd_soc_component_open(struct snd_soc_component *component,
»       »       »          struct snd_pcm_substream *substream)

... this essentially creates new state by assigning a new substream to the 
component, and we should explicitly track it. I know you wanted to avoid 
this, but I think in the end it's the cleanest solution and aligned to 
rest of ALSA. Aside cleaning up implementation of close(), this will help 
also in other methods, like e.g.:

int snd_soc_component_prepare(struct snd_soc_component *component,
»       »       »             struct snd_pcm_substream *substream)
{
»       if (component->driver->prepare)
»       »       return component->driver->prepare(component, substream);
»       return 0;
}

.. if prepare() is called with a substream that is not opened for this 
component, we could catch it here if we were tracking substreams.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for your feedback

> int snd_soc_component_open(struct snd_soc_component *component,
> »       »       »          struct snd_pcm_substream *substream)
(snip)
> int snd_soc_component_prepare(struct snd_soc_component *component,
> »       »       »             struct snd_pcm_substream *substream)
> {
> »       if (component->driver->prepare)
> »       »       return component->driver->prepare(component, substream);
> »       return 0;
> }

I guess you are thinking more big scale tracking/solution (?).
Indeed it is needed, but my indicated one is not for it.
It is just for "we want to use soc_pcm_close() as soc_pcm_open() error handling".

> > 	int soc_pcm_open(...)
> > 	{
> > 		static u8 id;
> > 
> > 		/* update ID */
> > 		id++;
> > 		if (id == 0)
> > 			id++;
> 
> ... this really isn't solid. If you have a complex scenario and something 
> goes wrong, debugging the ids is going to be painful if they are assigned 
> this way.

Maybe the naming of "ID" makes you confused ?
It is just "mark" for this "soc_pcm_open()".
If error happen during open, "error handling soc_pcm_close()" cares only this mark.
It is just for avoiding mismatch close.

Your big scale tracking (open/prepare/...) is maybe next step / more advanced problem.
My got feeling is that it is similar to SND_SOC_DPCM_STATE_xxx ?

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hi,

On Mon, 2 Mar 2020, Kuninori Morimoto wrote:

> I guess you are thinking more big scale tracking/solution (?).
> Indeed it is needed, but my indicated one is not for it.
> It is just for "we want to use soc_pcm_close() as soc_pcm_open() error handling".

yes, I'm thinking it would be better to do proper tracking and not do an 
intermediate solution with IDs.

> > > 	int soc_pcm_open(...)
> > > 	{
> > > 		static u8 id;
> > > 
> > > 		/* update ID */
> > > 		id++;
> > > 		if (id == 0)
> > > 			id++;
> 
> Maybe the naming of "ID" makes you confused ?
> It is just "mark" for this "soc_pcm_open()".
> If error happen during open, "error handling soc_pcm_close()" cares only this mark.
> It is just for avoiding mismatch close.

Yes, the main issues I see:
  - the name (maybe "open_tag" would be better than "open_id"),
  - declaration of the id with "static u8 id" -- if multiple unrelated 
    streams are opened concurrently, the id management needs to be handled
    in a thread safe way,
  - the "component->open_id" field only refers to the last substream 
    open -- i.e. field is only relevant in contact of error rollbacks 

The "new id" really just refers to the substream being opened, so 
you could create it from the substream pointer as well. For error 
rollback, you want to close all components of the substream being
opened. But this is still a bit unelegant as you'd end up storing
the last substream opened to component struct (3rd bullet above).

I was thinking more in the lines of:

/* add list of opened substreams to snd_soc_component */
struct snd_soc_component {
...
	struct list_head substream_list;

int snd_soc_component_open(struct snd_soc_component *component,
»       »       »          struct snd_pcm_substream *substream)
{
	int res = 0;
	if (component->driver->open)
		res = component->driver->open(component, substream);

        /* on success, add proxy of substream to component->substream_list  */
...

int snd_soc_component_close(struct snd_soc_component *component,
»       »       »           struct snd_pcm_substream *substream)
{
	/* 
	 * lookup substream from "component->substream_List", 
	 * only call driver->close() if found
	 */
...


... this is arguably more code, but makes the state created in 
snd_soc_component_open() explicit. Upon error in middle of 
components_open(), one can just call soc_pcm_components_close() which will 
close all components for that substream (that had been succesfully 
opened).

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for your feedback

> /* add list of opened substreams to snd_soc_component */
> struct snd_soc_component {
> ...
> 	struct list_head substream_list;
> 
> int snd_soc_component_open(struct snd_soc_component *component,
> »       »       »          struct snd_pcm_substream *substream)
> {
> 	int res = 0;
> 	if (component->driver->open)
> 		res = component->driver->open(component, substream);
> 
>         /* on success, add proxy of substream to component->substream_list  */
> ...
> 
> int snd_soc_component_close(struct snd_soc_component *component,
> »       »       »           struct snd_pcm_substream *substream)
> {
> 	/* 
> 	 * lookup substream from "component->substream_List", 
> 	 * only call driver->close() if found
> 	 */
> ...
> 
> 
> ... this is arguably more code, but makes the state created in 
> snd_soc_component_open() explicit. Upon error in middle of 
> components_open(), one can just call soc_pcm_components_close() which will 
> close all components for that substream (that had been succesfully 
> opened).
k
Yes, totally agree to your code.
But, 1 point I still not yet understand.

close() will be called 1) when open failed, or, 2) normal close.
If my understanding was correct, your code is caring that
2) normal close() might be called without open().

I don't think it happen, but are you worrying that it might be
happen by some BUG, or for some accident ?
If so, I can 100% understand your idea.

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

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hey,

On Tue, 3 Mar 2020, Kuninori Morimoto wrote:

> Kai Vehmanen wrote:
>> int snd_soc_component_close(struct snd_soc_component *component,
>> »       »       »           struct snd_pcm_substream *substream)
>> {
>> 	/* 
>> 	 * lookup substream from "component->substream_List", 
>> 	 * only call driver->close() if found
>> 	 */
>> ...
>> 
>> ... this is arguably more code, but makes the state created in 
>> snd_soc_component_open() explicit. Upon error in middle of 
>
> But, 1 point I still not yet understand.
> 
> close() will be called 1) when open failed, or, 2) normal close.
> If my understanding was correct, your code is caring that
> 2) normal close() might be called without open().

it also covers case (1) when open fails. So instead of the currently 
merged error case rollback code in soc_pcm_components_open(), we'd just 
call snd_pcm_components_close(substream) directly in case 
of error. With tracking of opened substreams in soc-component.c, close() 
is safe to call also in error case.

But you are right, I don't really see how (2) could be hit, so we are 
essentially talking about how to avoid the <10 lines of rollback code in 
soc_pcm_components_open(). :)

That considered, I'm fine if you can come up with a cleaner version to 
handle just case (1), without tracking substreams. Maybe worth a try and 
if it doesn't work (e.g. with ID/tag), we can look at substream tracking.

Br, Kai

Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kuninori Morimoto]

Hi Kai

Thank you for your feedback

> > close() will be called 1) when open failed, or, 2) normal close.
> > If my understanding was correct, your code is caring that
> > 2) normal close() might be called without open().
> 
> it also covers case (1) when open fails. So instead of the currently 
> merged error case rollback code in soc_pcm_components_open(), we'd just 
> call snd_pcm_components_close(substream) directly in case 
> of error. With tracking of opened substreams in soc-component.c, close() 
> is safe to call also in error case.

Oh, yes, of course.
I'm sorry to you for my lack of words.
My question was
"My idea cares open-close mismatch only for 1).
and your's cared both 1) and 2) ?", I mean.

> That considered, I'm fine if you can come up with a cleaner version to 
> handle just case (1), without tracking substreams. Maybe worth a try and 
> if it doesn't work (e.g. with ID/tag), we can look at substream tracking.

I see.
Let's use step-by-step approach.
I will try to care 1) as 1st step.
If it was not enough, or, if we want to have tracker,
let's update it by your idea as 2nd step.

Thank you for your help !!

Best regards
---
Kuninori Morimoto