[PATCH 0/5 v4] ASoC: rsnd: tidyup .remove method

[PATCH 0/5 v4] ASoC: rsnd: tidyup .remove method

[Kuninori Morimoto]

Hi Mark

These are v4 of unbind/bind issue fixup patches.
It adds new snd_soc_remove_card() and temporally remove card from system.
and rebind it again if next component was binded.
Then, it indicates warning

Kuninori Morimoto (5):
      ASoC: soc-core: add snd_soc_remove_card()
      ASoC: soc-core: deactivate pins in snd_soc_instantiate_card()
      ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()
      ASoC: soc-core: call snd_soc_remove_card() when component del
      ASoC: soc-core: indicate warning for unbind/bind issue

 include/sound/soc.h  |    1 +
 sound/soc/soc-core.c |   81 +++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 61 insertions(+), 21 deletions(-)

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

[PATCH 1/5 v4] 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>
---
v3 -> v4

 - no change

 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 710993b..a760f6e 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/5 v4] 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>
---
v3 -> v4

 - no change

 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 a760f6e..7f123bf 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/5 v4] 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>
---
v3 -> v4

 - no change

 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 7f123bf..5d85b5b 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/5 v4] 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 components. But CARD doesn't notice if connected component
was removed when user used 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>
---
v3 -> v4

 - initialize unbinded_list
 - used mutex_lock in snd_soc_unregister_card()
 - check component->probed in snd_soc_component_del_unlocked()

 include/sound/soc.h  |    1 +
 sound/soc/soc-core.c |   27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index ac8b333..a9272a4 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1084,6 +1084,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 5d85b5b..f04e97d 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().
@@ -2379,6 +2380,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
 		card->rtd_aux[i].card = card;
 
 	INIT_LIST_HEAD(&card->dapm_dirty);
+	INIT_LIST_HEAD(&card->unbinded_list);
 	card->instantiated = 0;
 	mutex_init(&card->mutex);
 	mutex_init(&card->dapm_mutex);
@@ -2406,6 +2408,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_init(&card->unbinded_list);
+	mutex_unlock(&client_mutex);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
@@ -2669,6 +2675,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 +2686,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_init(&card->unbinded_list);
+	}
 }
 
 static void snd_soc_component_add(struct snd_soc_component *component)
@@ -2694,7 +2713,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 (component->probed && 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

[PATCH 5/5 v4] ASoC: soc-core: indicate warning for unbind/bind issue

[Kuninori Morimoto]

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

We have unbinded_card_list for unbind / bind issue of component.
But suspend / 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 unbind / bind
fixup 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.
Ideally the correct fix would somehow make sure that the card itself is
unbound and put back onto the probe_defer list in the future.
We indicate warning to this issue for now.

Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v3 -> v4

 - new patch

 sound/soc/soc-core.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index f04e97d..c286a76 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2719,6 +2719,11 @@ static void snd_soc_component_del_unlocked(struct snd_soc_component *component)
 
 	/* card is removed temporarily */
 	if (component->probed && card->instantiated) {
+		dev_warn(component->dev,
+			 "%s is unbinded. but we recommend you to "
+			 "unbind/bind or rmmod/insmod it by yourself\n",
+			 card->name);
+
 		list_add(&card->unbinded_list, &unbinded_card_list);
 		snd_soc_remove_card(card);
 	}
-- 
1.7.9.5

Re: [PATCH 0/5 v4] ASoC: rsnd: tidyup .remove method

[Kuninori Morimoto]

Hi Mark

> These are v4 of unbind/bind issue fixup patches.
> It adds new snd_soc_remove_card() and temporally remove card from system.
> and rebind it again if next component was binded.
> Then, it indicates warning
> 
> Kuninori Morimoto (5):
>       ASoC: soc-core: add snd_soc_remove_card()
>       ASoC: soc-core: deactivate pins in snd_soc_instantiate_card()
>       ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()
>       ASoC: soc-core: call snd_soc_remove_card() when component del
>       ASoC: soc-core: indicate warning for unbind/bind issue

Please let me know if I need to resend these patches.

Re: [PATCH 0/5 v4] ASoC: rsnd: tidyup .remove method

[Kuninori Morimoto]

Hi Mark

> These are v4 of unbind/bind issue fixup patches.
> It adds new snd_soc_remove_card() and temporally remove card from system.
> and rebind it again if next component was binded.
> Then, it indicates warning
> 
> Kuninori Morimoto (5):
>       ASoC: soc-core: add snd_soc_remove_card()
>       ASoC: soc-core: deactivate pins in snd_soc_instantiate_card()
>       ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()
>       ASoC: soc-core: call snd_soc_remove_card() when component del
>       ASoC: soc-core: indicate warning for unbind/bind issue

Sorry for my reminder, but what is current status of these patches ?

Best regards
---
Kuninori Morimoto

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 738 bytes --]

On Fri, Feb 13, 2015 at 04:43:27AM +0000, Kuninori Morimoto wrote:

> 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()

I'm not quite sure I understand why this is being done.  There doesn't
seem to be any reason to defer initialization of resources owned by a
device until the card is finally instantiated, doing this as early as
possible means that we're in the state we want to be even if we're still
waiting for some other component of the card to appear and register
itself (which may never happen of course) so the hardware is in the
state we want for longer.  I may be missing something here though.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 425 bytes --]

On Fri, Feb 13, 2015 at 04:43:40AM +0000, Kuninori Morimoto wrote:
> 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.

This makes sense but there seems to be a dependency on one of the
earlier patches in the series.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]

On Fri, Feb 13, 2015 at 04:43:53AM +0000, Kuninori Morimoto wrote:

> +	struct list_head unbinded_list;

This should be "unbound".  However the whole idea of having this list
seems to be a bit problematic - it's moving us back to the old mechanism
we used to have where we open coded deferred probe.  What would be
better would be if we could force the driver to unbind so we go back to
the state we were in prior to the card being instantiated.  I had a
brief look and it looks like device_release_driver() might be what's
needed but it's unclear to me if this will do exactly what we want and
cause the device to try to rebind.  We probably need to ask Greg unless
there's an obvious answer I'm missing right now.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 5/5 v4] ASoC: soc-core: indicate warning for unbind/bind issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 714 bytes --]

On Fri, Feb 13, 2015 at 04:44:05AM +0000, Kuninori Morimoto wrote:

> Ideally the correct fix would somehow make sure that the card itself is
> unbound and put back onto the probe_defer list in the future.
> We indicate warning to this issue for now.

I don't think we should be trying to do this at the ASoC level at all,
we should do it at the driver core level.  A similar issue applies to
other inter device dependencies - if someone manages to remove a GPIO
controller, a clock or a regulator for example.  The components of a
card are really no different here.  Like I said in my mail just now
let's have a look at how this is supposed to work at the driver core
level and ask Greg if there's nothing clear.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Kuninori Morimoto]

Hi Mark

> > 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.
> 
> This makes sense but there seems to be a dependency on one of the
> earlier patches in the series.

Thank you.
I will re-send this as solo patch.

Re: [PATCH 5/5 v4] ASoC: soc-core: indicate warning for unbind/bind issue

[Kuninori Morimoto]

Hi Mark

> > Ideally the correct fix would somehow make sure that the card itself is
> > unbound and put back onto the probe_defer list in the future.
> > We indicate warning to this issue for now.
> 
> I don't think we should be trying to do this at the ASoC level at all,
> we should do it at the driver core level.  A similar issue applies to
> other inter device dependencies - if someone manages to remove a GPIO
> controller, a clock or a regulator for example.  The components of a
> card are really no different here.  Like I said in my mail just now
> let's have a look at how this is supposed to work at the driver core
> level and ask Greg if there's nothing clear.

Thank you.
Yes, we need more generic approach for this.