ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
diff mbox series

Message ID 20200220094955.16968-1-kai.vehmanen@linux.intel.com
State Accepted
Commit d2aaa8d8bfba93237ac944ee058fb98e2c2ef983
Headers show
Series
  • ASoC: soc-pcm: fix state tracking error in snd_soc_component_open/close()
Related show

Commit Message

Kai Vehmanen Feb. 20, 2020, 9:49 a.m. UTC
ASoC component open/close and snd_soc_component_module_get/put are called
independently for each component-substream pair, so the logic added in
commit dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
once") was not sufficient and led to PCM playback and module unload errors.

Implement handling of failures directly in soc_pcm_components_open(),
so that any successfully opened components are closed upon error with
other components. This allows to clean up error handling in
soc_pcm_open() without adding more state tracking.

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           | 27 +++++++++++++++++++++------
 3 files changed, 30 insertions(+), 39 deletions(-)

Comments

Mark Brown Feb. 24, 2020, 12:01 p.m. UTC | #1
On Thu, Feb 20, 2020 at 11:49:55AM +0200, 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 added in
> commit dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
> once") was not sufficient and led to PCM playback and module unload errors.
> 
> Implement handling of failures directly in soc_pcm_components_open(),
> so that any successfully opened components are closed upon error with
> other components. This allows to clean up error handling in
> soc_pcm_open() without adding more state tracking.

Do people have thoughts on this?  I do like this approach but can't
really test effectively myself.
Dmitry Osipenko Feb. 24, 2020, 3:47 p.m. UTC | #2
24.02.2020 15:01, Mark Brown пишет:
> On Thu, Feb 20, 2020 at 11:49:55AM +0200, 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 added in
>> commit dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
>> once") was not sufficient and led to PCM playback and module unload errors.
>>
>> Implement handling of failures directly in soc_pcm_components_open(),
>> so that any successfully opened components are closed upon error with
>> other components. This allows to clean up error handling in
>> soc_pcm_open() without adding more state tracking.
> 
> Do people have thoughts on this?  I do like this approach but can't
> really test effectively myself.
> 

I haven't tried to review this patch, but it works fine on NVIDIA Tegra:

Tested-by: Dmitry Osipenko <digetx@gmail.com>
Pierre-Louis Bossart Feb. 24, 2020, 5:05 p.m. UTC | #3
On 2/24/20 9:47 AM, Dmitry Osipenko wrote:
> 24.02.2020 15:01, Mark Brown пишет:
>> On Thu, Feb 20, 2020 at 11:49:55AM +0200, 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 added in
>>> commit dd03907bf129 ("ASoC: soc-pcm: call snd_soc_component_open/close()
>>> once") was not sufficient and led to PCM playback and module unload errors.
>>>
>>> Implement handling of failures directly in soc_pcm_components_open(),
>>> so that any successfully opened components are closed upon error with
>>> other components. This allows to clean up error handling in
>>> soc_pcm_open() without adding more state tracking.
>>
>> Do people have thoughts on this?  I do like this approach but can't
>> really test effectively myself.
>>
> 
> I haven't tried to review this patch, but it works fine on NVIDIA Tegra:
> 
> Tested-by: Dmitry Osipenko <digetx@gmail.com>

LGTM - I believe Kai tested for Intel platforms.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Note that the code could be simplified further with a 
for_each_rtd_components_rollback() macro, and we could also simplify the 
hw_free code below - the use of the 'last' pointer is not really 
necessary since we already have an index. That's for another cleanup though.

static int soc_pcm_components_hw_free(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;

Patch
diff mbox series

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..9e761f932887 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -469,28 +469,43 @@  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 = substream->private_data;
+	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);
 		if (ret < 0) {
 			dev_err(component->dev,
 				"ASoC: can't get module %s\n",
 				component->name);
-			return ret;
+			break;
 		}
 
 		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);
-			return ret;
+			break;
 		}
 	}
 
-	return 0;
+	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)
@@ -585,7 +600,7 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 	if (ret < 0) {
 		pr_err("ASoC: %s startup failed: %d\n",
 		       rtd->dai_link->name, ret);
-		goto component_err;
+		goto rtd_startup_err;
 	}
 
 	/* startup the audio subsystem */
@@ -681,9 +696,9 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 	snd_soc_dai_shutdown(cpu_dai, substream);
 
 	soc_rtd_shutdown(rtd, substream);
-component_err:
+rtd_startup_err:
 	soc_pcm_components_close(substream);
-
+component_err:
 	mutex_unlock(&rtd->card->pcm_mutex);
 
 	for_each_rtd_components(rtd, i, component) {