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 that it has organized from which [component]. But current [component] doesn't inform to [CARD] that it was removed when user called rmmod or unbind it. 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 unregister [CARD] when [component] was removed. If you want to use this [CARD] again, you need to re-insmod or re-bind removed [component] and [CARD] again.

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 --- sound/soc/soc-core.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index ededb97..cd5db21 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2695,6 +2695,7 @@ static void snd_soc_component_add(struct snd_soc_component *component) static void snd_soc_component_cleanup(struct snd_soc_component *component) { snd_soc_unregister_dais(component); + snd_soc_unregister_card(component->card); kfree(component->name); }

Hi Mark

Thank you for your feedback

...

ASoC devices are organized as [CPU]-[CARD]-[CODEC]. Then, [CPU]/[CODEC] are based on [component] structure. Now, each [CARD] device knows that it has organized from which [component]. But current [component] doesn't inform to [CARD] that it was removed when user called rmmod or unbind it. 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 unregister [CARD] when [component] was removed. If you want to use this [CARD] again, you need to re-insmod or re-bind removed [component] and [CARD] again.

So, there's a problem here but...

...

@@ -2695,6 +2695,7 @@ static void snd_soc_component_add(struct snd_soc_component *component) static void snd_soc_component_cleanup(struct snd_soc_component *component) { snd_soc_unregister_dais(component);

• snd_soc_unregister_card(component->card); kfree(component->name);

}

...this doesn't look like the right fix. If we just plain unregister the card then if the component that was removed is loaded again then the card won't be reinstantiated since the core has forgotten about the card. Further, if the card driver is removed then we'll get a duplicate attempt to unregister it which doesn't seem clever.

What we need to do here is undo all the work that was done to instantiate the card but not actually unregister it, returning everything to the state it was in before the card was instantiated. That's obviously a much more substantial change than this but it's what's needed (and is the main reason this is broken at the minute).

Hmm..

Main big problem is that the cpu/codec information in card is written in card->rtd[num], and it has deeply relationship to many functions/datas. Indeed it needs more substantial change if we solve this issue. But, I thought register/unregister card is very easy hack, and it is easy to check.

I think this duplicate unregister card is no problem, because snd_soc_unregister_card() is checking card->instantiated. 2nd unregister do nothing.

My hack (= on snd_soc_component_cleanup) is caring both cpu/codec, bacause soc_unregister_codec() doesn't have relationship to snd_soc_unregister_component()

It needs more and more change if we solve this issue without above hack, then, it is very difficult to check all consistency for all features, all situation. I'm afraid that it can be next bug. So, how about this ? if un-loaded component unregister card, and if re-loaded component re-register un-loaded card automatically, it is easy, and safety ?

Best regards --- Kuninori Morimoto

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

Current snd_soc_unregister_card() indicates unregistered debug message when it was called. But, it should be called only when it was really unregistered.

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

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index cd5db21..148c959 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2409,8 +2409,8 @@ int snd_soc_unregister_card(struct snd_soc_card *card) card->instantiated = false; snd_soc_dapm_shutdown(card); soc_cleanup_card_resources(card); + dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name); } - dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);

return 0; }

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

Set device data before snd_soc_register_platform/component. Otherwise, it will use NULL pointer if user calls unbind -> bind or rmmod -> insmod

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

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 1b53605..110577c5 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1252,6 +1252,8 @@ static int rsnd_probe(struct platform_device *pdev) goto exit_snd_probe; }

+ dev_set_drvdata(dev, priv); + /* * asoc register */ @@ -1268,8 +1270,6 @@ static int rsnd_probe(struct platform_device *pdev) goto exit_snd_soc; }

- dev_set_drvdata(dev, priv); - pm_runtime_enable(dev);

dev_info(dev, "probed\n");

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

Current snd_soc_unregister_card() indicates unregistered debug message when it was called. But, it should be called only when it was really unregistered.

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

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 985052b..11c28fd 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2386,8 +2386,9 @@ int snd_soc_unregister_card(struct snd_soc_card *card) card->instantiated = false; snd_soc_dapm_shutdown(card); soc_cleanup_card_resources(card); + dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", + card->name); } - dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);

return 0; }

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 39a8cab..db7b070 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1642,6 +1642,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: @@ -2362,22 +2379,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);

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 again 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 eb16b44..8d53dae 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(). @@ -2397,6 +2398,10 @@ int snd_soc_unregister_card(struct snd_soc_card *card) card->name); }

+ mutex_lock(&client_mutex); + list_del(&card->unbinded_list); + mutex_unlock(&client_mutex); + return 0; } EXPORT_SYMBOL_GPL(snd_soc_unregister_card); @@ -2660,6 +2665,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); @@ -2668,6 +2676,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) @@ -2685,7 +2703,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)