[PATCH 0/4 v3] ASoC: rsnd: tidyup .remove method

[PATCH 0/4 v3] ASoC: rsnd: tidyup .remove method

[Kuninori Morimoto]

Hi Mark

These are v3 of unbind/bind issue fixup patches.

v2 patch-set was

Kuninori Morimoto (6):
      1) ASoC: rsnd: set device data before snd_soc_register_platform/component
      2) ASoC: soc-core: indicate unregister debug message once
      3) ASoC: soc-core: add snd_soc_remove_card()
      4) ASoC: soc-core: deactivate pins in snd_soc_instantiate_card()
      5) ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()
      6) ASoC: soc-core: call snd_soc_remove_card() when component del

Now, 1) / 2) are applied already.
So, v3 patches is same as 3) - 6)

[PATCH 1/4] ASoC: soc-core: add snd_soc_remove_card()

[Kuninori Morimoto]

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

Added snd_soc_remove_card() is termination method of
snd_soc_instantiate_card()

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

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4bfabc7..4837b5d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1725,6 +1725,14 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 
 }
 
+static void snd_soc_remove_card(struct snd_soc_card *card)
+{
+	card->instantiated = false;
+	snd_soc_dapm_shutdown(card);
+	soc_cleanup_card_resources(card);
+}
+
+
 /* removes a socdev */
 static int soc_remove(struct platform_device *pdev)
 {
@@ -2393,9 +2401,7 @@ EXPORT_SYMBOL_GPL(snd_soc_register_card);
 int snd_soc_unregister_card(struct snd_soc_card *card)
 {
 	if (card->instantiated) {
-		card->instantiated = false;
-		snd_soc_dapm_shutdown(card);
-		soc_cleanup_card_resources(card);
+		snd_soc_remove_card(card);
 		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
 	}
 
-- 
1.7.9.5

[PATCH 2/4] ASoC: soc-core: deactivate pins in snd_soc_instantiate_card()

[Kuninori Morimoto]

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

snd_soc_instantiate_card() is the final method of snd_soc_register_card().
Deactivate pins to sleep state is related to snd_soc_instantiate_card(),
not snd_soc_register_card()

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

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4837b5d..794d8ec 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1652,6 +1652,23 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 	snd_soc_dapm_sync(&card->dapm);
 	mutex_unlock(&card->mutex);
 
+	/* deactivate pins to sleep state */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		int j;
+
+		for (j = 0; j < rtd->num_codecs; j++) {
+			struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+
+			if (!codec_dai->active)
+				pinctrl_pm_select_sleep_state(codec_dai->dev);
+		}
+
+		if (!cpu_dai->active)
+			pinctrl_pm_select_sleep_state(cpu_dai->dev);
+	}
+
 	return 0;
 
 probe_aux_dev_err:
@@ -2372,22 +2389,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
 	if (ret != 0)
 		soc_cleanup_card_debugfs(card);
 
-	/* deactivate pins to sleep state */
-	for (i = 0; i < card->num_rtd; i++) {
-		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
-		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-		int j;
-
-		for (j = 0; j < rtd->num_codecs; j++) {
-			struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
-			if (!codec_dai->active)
-				pinctrl_pm_select_sleep_state(codec_dai->dev);
-		}
-
-		if (!cpu_dai->active)
-			pinctrl_pm_select_sleep_state(cpu_dai->dev);
-	}
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_card);
-- 
1.7.9.5

[PATCH 3/4] ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()

[Kuninori Morimoto]

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

soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(),
but, card debugfs is called from snd_soc_register_card().
cleanup function should be called paired function.

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

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 794d8ec..b7ab676 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1729,8 +1729,6 @@ static int soc_cleanup_card_resources(struct snd_soc_card *card)
 	/* remove and free each DAI */
 	soc_remove_dai_links(card);
 
-	soc_cleanup_card_debugfs(card);
-
 	/* remove the card */
 	if (card->remove)
 		card->remove(card);
@@ -2403,6 +2401,8 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
 {
 	if (card->instantiated) {
 		snd_soc_remove_card(card);
+		soc_cleanup_card_debugfs(card);
+
 		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
 	}
 
-- 
1.7.9.5

[PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Kuninori Morimoto]

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

ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
are based on component structure. Now, each CARD device knows
connected component. But CARD doesn't notice if connected component
was removed when user called rmmod or unbind in current implementation.
Thus, CARD which lost some components still exist in system.
And then, ALSA sound card will have some problem if user used this CARD
in such timing. This patch temporarily removes CARD from system
if connected component was removed, and re-add it if some component
was added.

Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |    1 +
 sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index b4fca9a..a90eff4 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1083,6 +1083,7 @@ struct snd_soc_card {
 	struct list_head paths;
 	struct list_head dapm_list;
 	struct list_head dapm_dirty;
+	struct list_head unbinded_list;
 
 	/* Generic DAPM context for the card */
 	struct snd_soc_dapm_context dapm;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b7ab676..f8d5498 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
 static LIST_HEAD(platform_list);
 static LIST_HEAD(codec_list);
 static LIST_HEAD(component_list);
+static LIST_HEAD(unbinded_card_list);
 
 /*
  * This is a timeout to do a DAPM powerdown after a stream is closed().
@@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
 		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
 	}
 
+	mutex_lock(&client_mutex);
+	list_del(&card->unbinded_list);
+	mutex_unlock(&client_mutex);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
@@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
 
 static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
 {
+	struct snd_soc_card *card, *_card;
+	int ret;
+
 	if (!component->write && !component->read) {
 		if (!component->regmap)
 			component->regmap = dev_get_regmap(component->dev, NULL);
@@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
 	}
 
 	list_add(&component->list, &component_list);
+
+	/* re-add temporarily removed card if exist */
+	list_for_each_entry_safe(card, _card, &unbinded_card_list,
+				 unbinded_list) {
+		ret = snd_soc_instantiate_card(card);
+		if (ret < 0)
+			continue;
+
+		list_del(&card->unbinded_list);
+	}
 }
 
 static void snd_soc_component_add(struct snd_soc_component *component)
@@ -2694,7 +2712,15 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
 
 static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
 {
+	struct snd_soc_card *card = component->card;
+
 	list_del(&component->list);
+
+	/* card is removed temporarily */
+	if (card->instantiated) {
+		list_add(&card->unbinded_list, &unbinded_card_list);
+		snd_soc_remove_card(card);
+	}
 }
 
 static void snd_soc_component_del(struct snd_soc_component *component)
-- 
1.7.9.5

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Takashi Iwai]

At Mon, 9 Feb 2015 08:52:49 +0000,
Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
> are based on component structure. Now, each CARD device knows
> connected component. But CARD doesn't notice if connected component
> was removed when user called rmmod or unbind in current implementation.
> Thus, CARD which lost some components still exist in system.
> And then, ALSA sound card will have some problem if user used this CARD
> in such timing. This patch temporarily removes CARD from system
> if connected component was removed, and re-add it if some component
> was added.
> 
> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc.h  |    1 +
>  sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index b4fca9a..a90eff4 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
>  	struct list_head paths;
>  	struct list_head dapm_list;
>  	struct list_head dapm_dirty;
> +	struct list_head unbinded_list;
>  
>  	/* Generic DAPM context for the card */
>  	struct snd_soc_dapm_context dapm;
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index b7ab676..f8d5498 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
>  static LIST_HEAD(platform_list);
>  static LIST_HEAD(codec_list);
>  static LIST_HEAD(component_list);
> +static LIST_HEAD(unbinded_card_list);
>  
>  /*
>   * This is a timeout to do a DAPM powerdown after a stream is closed().
> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
>  		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
>  	}
>  
> +	mutex_lock(&client_mutex);
> +	list_del(&card->unbinded_list);
> +	mutex_unlock(&client_mutex);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
>  
>  static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>  {
> +	struct snd_soc_card *card, *_card;
> +	int ret;
> +
>  	if (!component->write && !component->read) {
>  		if (!component->regmap)
>  			component->regmap = dev_get_regmap(component->dev, NULL);
> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>  	}
>  
>  	list_add(&component->list, &component_list);
> +
> +	/* re-add temporarily removed card if exist */
> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
> +				 unbinded_list) {
> +		ret = snd_soc_instantiate_card(card);
> +		if (ret < 0)
> +			continue;
> +
> +		list_del(&card->unbinded_list);
> +	}

[Cc'ed Lars-Peter]

This would instantiate a card even if it's irrelevant with the given
component?  If so, it looks fragile, and possibly racy, when there are
multiple cards.


thanks,

Takashi

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Lars-Peter Clausen]

On 02/09/2015 11:48 AM, Takashi Iwai wrote:
> At Mon, 9 Feb 2015 08:52:49 +0000,
> Kuninori Morimoto wrote:
>>
>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
>> are based on component structure. Now, each CARD device knows
>> connected component. But CARD doesn't notice if connected component
>> was removed when user called rmmod or unbind in current implementation.
>> Thus, CARD which lost some components still exist in system.
>> And then, ALSA sound card will have some problem if user used this CARD
>> in such timing. This patch temporarily removes CARD from system
>> if connected component was removed, and re-add it if some component
>> was added.
>>
>> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
>> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> ---
>>   include/sound/soc.h  |    1 +
>>   sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index b4fca9a..a90eff4 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
>>   	struct list_head paths;
>>   	struct list_head dapm_list;
>>   	struct list_head dapm_dirty;
>> +	struct list_head unbinded_list;
>>
>>   	/* Generic DAPM context for the card */
>>   	struct snd_soc_dapm_context dapm;
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index b7ab676..f8d5498 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
>>   static LIST_HEAD(platform_list);
>>   static LIST_HEAD(codec_list);
>>   static LIST_HEAD(component_list);
>> +static LIST_HEAD(unbinded_card_list);
>>
>>   /*
>>    * This is a timeout to do a DAPM powerdown after a stream is closed().
>> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
>>   		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
>>   	}
>>
>> +	mutex_lock(&client_mutex);
>> +	list_del(&card->unbinded_list);
>> +	mutex_unlock(&client_mutex);
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
>> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
>>
>>   static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>>   {
>> +	struct snd_soc_card *card, *_card;
>> +	int ret;
>> +
>>   	if (!component->write && !component->read) {
>>   		if (!component->regmap)
>>   			component->regmap = dev_get_regmap(component->dev, NULL);
>> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>>   	}
>>
>>   	list_add(&component->list, &component_list);
>> +
>> +	/* re-add temporarily removed card if exist */
>> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
>> +				 unbinded_list) {
>> +		ret = snd_soc_instantiate_card(card);
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		list_del(&card->unbinded_list);
>> +	}
>
> [Cc'ed Lars-Peter]
>
> This would instantiate a card even if it's irrelevant with the given
> component?  If so, it looks fragile, and possibly racy, when there are
> multiple cards.

That shouldn't be a problem. snd_soc_instantiate_card() does the proper 
locking and will return an error if not all components are ready yet.

I think the main issue with this path is that snd_soc_unregister_card() will 
crash if the card is not on the unbinded_card_list, which in most cases it 
won't be.

The other issue is that it introduces subtly issues with the suspend and 
resume order. Suspend and resume are called in the order in which the probe 
functions of devices are called (and succeed). By returning -EPROBE_DEFER in 
the card driver we make sure that the card's probe function is always called 
after the probe functions of all the components of the card have run. This 
again causes the card's suspend function to be called before any suspend 
function of any of it's components. Now with this patch it is possible again 
for a component's probe function to be called after the card's probe 
function which changes the suspend and resume order and might break things.

This patch essentially is a partial revert of commit b19e6e7b763 ("ASoC: 
core: Use driver core probe deferral") where the card list was replaced with 
the -EPROBE_DEFER mechanism.

So while this patch fixes the nasty crash it introduces some other subtle 
issue. Maybe we should also add a big WARN() when a component of a card is 
removed while still in use until the other issues are also fixed.

Ideally the correct fix would somehow make sure that the card itself is 
unbound and put back onto the probe_defer list.

- Lars

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Takashi Iwai]

At Mon, 09 Feb 2015 12:26:02 +0100,
Lars-Peter Clausen wrote:
> 
> On 02/09/2015 11:48 AM, Takashi Iwai wrote:
> > At Mon, 9 Feb 2015 08:52:49 +0000,
> > Kuninori Morimoto wrote:
> >>
> >> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>
> >> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
> >> are based on component structure. Now, each CARD device knows
> >> connected component. But CARD doesn't notice if connected component
> >> was removed when user called rmmod or unbind in current implementation.
> >> Thus, CARD which lost some components still exist in system.
> >> And then, ALSA sound card will have some problem if user used this CARD
> >> in such timing. This patch temporarily removes CARD from system
> >> if connected component was removed, and re-add it if some component
> >> was added.
> >>
> >> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
> >> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
> >> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >> ---
> >>   include/sound/soc.h  |    1 +
> >>   sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
> >>   2 files changed, 27 insertions(+)
> >>
> >> diff --git a/include/sound/soc.h b/include/sound/soc.h
> >> index b4fca9a..a90eff4 100644
> >> --- a/include/sound/soc.h
> >> +++ b/include/sound/soc.h
> >> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
> >>   	struct list_head paths;
> >>   	struct list_head dapm_list;
> >>   	struct list_head dapm_dirty;
> >> +	struct list_head unbinded_list;
> >>
> >>   	/* Generic DAPM context for the card */
> >>   	struct snd_soc_dapm_context dapm;
> >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> >> index b7ab676..f8d5498 100644
> >> --- a/sound/soc/soc-core.c
> >> +++ b/sound/soc/soc-core.c
> >> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
> >>   static LIST_HEAD(platform_list);
> >>   static LIST_HEAD(codec_list);
> >>   static LIST_HEAD(component_list);
> >> +static LIST_HEAD(unbinded_card_list);
> >>
> >>   /*
> >>    * This is a timeout to do a DAPM powerdown after a stream is closed().
> >> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
> >>   		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
> >>   	}
> >>
> >> +	mutex_lock(&client_mutex);
> >> +	list_del(&card->unbinded_list);
> >> +	mutex_unlock(&client_mutex);
> >> +
> >>   	return 0;
> >>   }
> >>   EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
> >> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
> >>
> >>   static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>   {
> >> +	struct snd_soc_card *card, *_card;
> >> +	int ret;
> >> +
> >>   	if (!component->write && !component->read) {
> >>   		if (!component->regmap)
> >>   			component->regmap = dev_get_regmap(component->dev, NULL);
> >> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>   	}
> >>
> >>   	list_add(&component->list, &component_list);
> >> +
> >> +	/* re-add temporarily removed card if exist */
> >> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
> >> +				 unbinded_list) {
> >> +		ret = snd_soc_instantiate_card(card);
> >> +		if (ret < 0)
> >> +			continue;
> >> +
> >> +		list_del(&card->unbinded_list);
> >> +	}
> >
> > [Cc'ed Lars-Peter]
> >
> > This would instantiate a card even if it's irrelevant with the given
> > component?  If so, it looks fragile, and possibly racy, when there are
> > multiple cards.
> 
> That shouldn't be a problem. snd_soc_instantiate_card() does the proper 
> locking and will return an error if not all components are ready yet.

But there is no check of card->instantiated there, so the whole path
can be called again even if the card was already instantiated?


Takashi

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Lars-Peter Clausen]

On 02/09/2015 02:07 PM, Takashi Iwai wrote:
> At Mon, 09 Feb 2015 12:26:02 +0100,
> Lars-Peter Clausen wrote:
>>
>> On 02/09/2015 11:48 AM, Takashi Iwai wrote:
>>> At Mon, 9 Feb 2015 08:52:49 +0000,
>>> Kuninori Morimoto wrote:
>>>>
>>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>
>>>> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
>>>> are based on component structure. Now, each CARD device knows
>>>> connected component. But CARD doesn't notice if connected component
>>>> was removed when user called rmmod or unbind in current implementation.
>>>> Thus, CARD which lost some components still exist in system.
>>>> And then, ALSA sound card will have some problem if user used this CARD
>>>> in such timing. This patch temporarily removes CARD from system
>>>> if connected component was removed, and re-add it if some component
>>>> was added.
>>>>
>>>> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
>>>> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
>>>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>> ---
>>>>    include/sound/soc.h  |    1 +
>>>>    sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
>>>>    2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>>>> index b4fca9a..a90eff4 100644
>>>> --- a/include/sound/soc.h
>>>> +++ b/include/sound/soc.h
>>>> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
>>>>    	struct list_head paths;
>>>>    	struct list_head dapm_list;
>>>>    	struct list_head dapm_dirty;
>>>> +	struct list_head unbinded_list;
>>>>
>>>>    	/* Generic DAPM context for the card */
>>>>    	struct snd_soc_dapm_context dapm;
>>>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>>>> index b7ab676..f8d5498 100644
>>>> --- a/sound/soc/soc-core.c
>>>> +++ b/sound/soc/soc-core.c
>>>> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
>>>>    static LIST_HEAD(platform_list);
>>>>    static LIST_HEAD(codec_list);
>>>>    static LIST_HEAD(component_list);
>>>> +static LIST_HEAD(unbinded_card_list);
>>>>
>>>>    /*
>>>>     * This is a timeout to do a DAPM powerdown after a stream is closed().
>>>> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
>>>>    		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
>>>>    	}
>>>>
>>>> +	mutex_lock(&client_mutex);
>>>> +	list_del(&card->unbinded_list);
>>>> +	mutex_unlock(&client_mutex);
>>>> +
>>>>    	return 0;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
>>>> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
>>>>
>>>>    static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>>>>    {
>>>> +	struct snd_soc_card *card, *_card;
>>>> +	int ret;
>>>> +
>>>>    	if (!component->write && !component->read) {
>>>>    		if (!component->regmap)
>>>>    			component->regmap = dev_get_regmap(component->dev, NULL);
>>>> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
>>>>    	}
>>>>
>>>>    	list_add(&component->list, &component_list);
>>>> +
>>>> +	/* re-add temporarily removed card if exist */
>>>> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
>>>> +				 unbinded_list) {
>>>> +		ret = snd_soc_instantiate_card(card);
>>>> +		if (ret < 0)
>>>> +			continue;
>>>> +
>>>> +		list_del(&card->unbinded_list);
>>>> +	}
>>>
>>> [Cc'ed Lars-Peter]
>>>
>>> This would instantiate a card even if it's irrelevant with the given
>>> component?  If so, it looks fragile, and possibly racy, when there are
>>> multiple cards.
>>
>> That shouldn't be a problem. snd_soc_instantiate_card() does the proper
>> locking and will return an error if not all components are ready yet.
>
> But there is no check of card->instantiated there, so the whole path
> can be called again even if the card was already instantiated?

The card won't be on the list of unbound cards if it is not instantiated. 
And this whole section is protected by the client_mutex.

- Lars

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Takashi Iwai]

At Mon, 09 Feb 2015 14:09:50 +0100,
Lars-Peter Clausen wrote:
> 
> On 02/09/2015 02:07 PM, Takashi Iwai wrote:
> > At Mon, 09 Feb 2015 12:26:02 +0100,
> > Lars-Peter Clausen wrote:
> >>
> >> On 02/09/2015 11:48 AM, Takashi Iwai wrote:
> >>> At Mon, 9 Feb 2015 08:52:49 +0000,
> >>> Kuninori Morimoto wrote:
> >>>>
> >>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>>
> >>>> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
> >>>> are based on component structure. Now, each CARD device knows
> >>>> connected component. But CARD doesn't notice if connected component
> >>>> was removed when user called rmmod or unbind in current implementation.
> >>>> Thus, CARD which lost some components still exist in system.
> >>>> And then, ALSA sound card will have some problem if user used this CARD
> >>>> in such timing. This patch temporarily removes CARD from system
> >>>> if connected component was removed, and re-add it if some component
> >>>> was added.
> >>>>
> >>>> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
> >>>> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
> >>>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> >>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>> ---
> >>>>    include/sound/soc.h  |    1 +
> >>>>    sound/soc/soc-core.c |   26 ++++++++++++++++++++++++++
> >>>>    2 files changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/include/sound/soc.h b/include/sound/soc.h
> >>>> index b4fca9a..a90eff4 100644
> >>>> --- a/include/sound/soc.h
> >>>> +++ b/include/sound/soc.h
> >>>> @@ -1083,6 +1083,7 @@ struct snd_soc_card {
> >>>>    	struct list_head paths;
> >>>>    	struct list_head dapm_list;
> >>>>    	struct list_head dapm_dirty;
> >>>> +	struct list_head unbinded_list;
> >>>>
> >>>>    	/* Generic DAPM context for the card */
> >>>>    	struct snd_soc_dapm_context dapm;
> >>>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> >>>> index b7ab676..f8d5498 100644
> >>>> --- a/sound/soc/soc-core.c
> >>>> +++ b/sound/soc/soc-core.c
> >>>> @@ -56,6 +56,7 @@ static DEFINE_MUTEX(client_mutex);
> >>>>    static LIST_HEAD(platform_list);
> >>>>    static LIST_HEAD(codec_list);
> >>>>    static LIST_HEAD(component_list);
> >>>> +static LIST_HEAD(unbinded_card_list);
> >>>>
> >>>>    /*
> >>>>     * This is a timeout to do a DAPM powerdown after a stream is closed().
> >>>> @@ -2406,6 +2407,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card)
> >>>>    		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
> >>>>    	}
> >>>>
> >>>> +	mutex_lock(&client_mutex);
> >>>> +	list_del(&card->unbinded_list);
> >>>> +	mutex_unlock(&client_mutex);
> >>>> +
> >>>>    	return 0;
> >>>>    }
> >>>>    EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
> >>>> @@ -2669,6 +2674,9 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
> >>>>
> >>>>    static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>>>    {
> >>>> +	struct snd_soc_card *card, *_card;
> >>>> +	int ret;
> >>>> +
> >>>>    	if (!component->write && !component->read) {
> >>>>    		if (!component->regmap)
> >>>>    			component->regmap = dev_get_regmap(component->dev, NULL);
> >>>> @@ -2677,6 +2685,16 @@ static void snd_soc_component_add_unlocked(struct snd_soc_component *component)
> >>>>    	}
> >>>>
> >>>>    	list_add(&component->list, &component_list);
> >>>> +
> >>>> +	/* re-add temporarily removed card if exist */
> >>>> +	list_for_each_entry_safe(card, _card, &unbinded_card_list,
> >>>> +				 unbinded_list) {
> >>>> +		ret = snd_soc_instantiate_card(card);
> >>>> +		if (ret < 0)
> >>>> +			continue;
> >>>> +
> >>>> +		list_del(&card->unbinded_list);
> >>>> +	}
> >>>
> >>> [Cc'ed Lars-Peter]
> >>>
> >>> This would instantiate a card even if it's irrelevant with the given
> >>> component?  If so, it looks fragile, and possibly racy, when there are
> >>> multiple cards.
> >>
> >> That shouldn't be a problem. snd_soc_instantiate_card() does the proper
> >> locking and will return an error if not all components are ready yet.
> >
> > But there is no check of card->instantiated there, so the whole path
> > can be called again even if the card was already instantiated?
> 
> The card won't be on the list of unbound cards if it is not instantiated. 
> And this whole section is protected by the client_mutex.

OK, that explains.  Thanks.

(BTW, client_mutex doesn't cover the whole places accessing the
 component_list in soc-core.c.)


Takashi

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Lars-Peter Clausen]

On 02/09/2015 02:35 PM, Takashi Iwai wrote:
[...]
> (BTW, client_mutex doesn't cover the whole places accessing the
>   component_list in soc-core.c.)

Yea, that looks pretty broken, I'll have a look.

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Kuninori Morimoto]

Hi Lars

> >> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
> >> are based on component structure. Now, each CARD device knows
> >> connected component. But CARD doesn't notice if connected component
> >> was removed when user called rmmod or unbind in current implementation.
> >> Thus, CARD which lost some components still exist in system.
> >> And then, ALSA sound card will have some problem if user used this CARD
> >> in such timing. This patch temporarily removes CARD from system
> >> if connected component was removed, and re-add it if some component
> >> was added.
> >>
> >> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
> >> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
> >> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >> ---
(snip)
> The other issue is that it introduces subtly issues with the suspend and 
> resume order. Suspend and resume are called in the order in which the probe 
> functions of devices are called (and succeed). By returning -EPROBE_DEFER in 
> the card driver we make sure that the card's probe function is always called 
> after the probe functions of all the components of the card have run. This 
> again causes the card's suspend function to be called before any suspend 
> function of any of it's components. Now with this patch it is possible again 
> for a component's probe function to be called after the card's probe 
> function which changes the suspend and resume order and might break things.
> 
> This patch essentially is a partial revert of commit b19e6e7b763 ("ASoC: 
> core: Use driver core probe deferral") where the card list was replaced with 
> the -EPROBE_DEFER mechanism.
> 
> So while this patch fixes the nasty crash it introduces some other subtle 
> issue. Maybe we should also add a big WARN() when a component of a card is 
> removed while still in use until the other issues are also fixed.

OK. I can send such patch.
Oops ? does this mean my patch-set + WARN() ? or just WARN() ?


Best regards
---
Kuninori Morimoto

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Lars-Peter Clausen]

On 02/10/2015 01:44 AM, Kuninori Morimoto wrote:
>
> Hi Lars
>
>>>> ASoC devices are organized as CPU-CARD-CODEC. Then, CPU/CODEC
>>>> are based on component structure. Now, each CARD device knows
>>>> connected component. But CARD doesn't notice if connected component
>>>> was removed when user called rmmod or unbind in current implementation.
>>>> Thus, CARD which lost some components still exist in system.
>>>> And then, ALSA sound card will have some problem if user used this CARD
>>>> in such timing. This patch temporarily removes CARD from system
>>>> if connected component was removed, and re-add it if some component
>>>> was added.
>>>>
>>>> Reported-by: Nguyen Viet Dung <nv-dung@jinso.co.jp>
>>>> Reported-by: Bui Duc Phuc <bd-phuc@jinso.co.jp>
>>>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>> ---
> (snip)
>> The other issue is that it introduces subtly issues with the suspend and
>> resume order. Suspend and resume are called in the order in which the probe
>> functions of devices are called (and succeed). By returning -EPROBE_DEFER in
>> the card driver we make sure that the card's probe function is always called
>> after the probe functions of all the components of the card have run. This
>> again causes the card's suspend function to be called before any suspend
>> function of any of it's components. Now with this patch it is possible again
>> for a component's probe function to be called after the card's probe
>> function which changes the suspend and resume order and might break things.
>>
>> This patch essentially is a partial revert of commit b19e6e7b763 ("ASoC:
>> core: Use driver core probe deferral") where the card list was replaced with
>> the -EPROBE_DEFER mechanism.
>>
>> So while this patch fixes the nasty crash it introduces some other subtle
>> issue. Maybe we should also add a big WARN() when a component of a card is
>> removed while still in use until the other issues are also fixed.
>
> OK. I can send such patch.
> Oops ? does this mean my patch-set + WARN() ? or just WARN() ?

Both. Or an alternative would be not to allow re-binding the card and force 
the user to unbind/bind the card before it starts working again.

- Lars

Re: [PATCH 4/4] ASoC: soc-core: call snd_soc_remove_card() when component del

[Kuninori Morimoto]

Hi Lars

> > OK. I can send such patch.
> > Oops ? does this mean my patch-set + WARN() ? or just WARN() ?
> 
> Both. Or an alternative would be not to allow re-binding the card and force 
> the user to unbind/bind the card before it starts working again.

Thanks.
My v1 patch had this idea, but Mark wanted automatically re-binding.