[alsa-devel] [PATCH 0/5 v4] ASoC: rsnd: tidyup .remove method
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
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); }
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);
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.
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); }
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.
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.
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)
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.
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); }
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.
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.
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.
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
participants (2)
-
Kuninori Morimoto -
Mark Brown