[alsa-devel] [PATCH] ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Card debugfs is created in snd_soc_register_card(), but soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(). Cleanup function should be called from paired unregister function.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4c26074..211783f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1751,8 +1751,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); @@ -2447,6 +2445,7 @@ int snd_soc_unregister_card(struct snd_soc_card *card) card->instantiated = false; snd_soc_dapm_shutdown(card); soc_cleanup_card_resources(card); + soc_cleanup_card_debugfs(card); dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name); }
At Tue, 24 Mar 2015 01:44:49 +0000, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Card debugfs is created in snd_soc_register_card(), but soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(). Cleanup function should be called from paired unregister function.
Why only soc_cleanup_card_debugfs() needs to be handled specially? Other stuff in soc_cleanup_card_resources() are also initialized in soc_register_card().
If the reason is different (e.g. for further fix of hot unplug bug), it'd be understandable, though. In general, the ALSA device free consists of three phases: 1. device disconnection 2. wait until all devices are closed 3. actual free all resources
And soc_cleanup_card_debugfs() basically belongs to 1, not 3.
The above differences don't matter for now because currently ASoC calls snd_card_free() and this function does all these by itself. But if anyone wants to implement a proper hotplug/unplug, this difference would become clearer; i.e. you need to split the stuff in soc_cleanup_card_resources() into two, one for 1 and another for 3.
BTW, the call of soc_cleanup_card_debugfs() was forgotten in a few error paths of snd_soc_register_card(). Any taker?
thanks,
Takashi
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4c26074..211783f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1751,8 +1751,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);
@@ -2447,6 +2445,7 @@ int snd_soc_unregister_card(struct snd_soc_card *card) card->instantiated = false; snd_soc_dapm_shutdown(card); soc_cleanup_card_resources(card);
soc_cleanup_card_debugfs(card);-- 1.9.1
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Takashi
Thank you for your feedback
If the reason is different (e.g. for further fix of hot unplug bug), it'd be understandable, though. In general, the ALSA device free consists of three phases:
- device disconnection
- wait until all devices are closed
- actual free all resources
And soc_cleanup_card_debugfs() basically belongs to 1, not 3.
Hmm... I'm not 100% understand detail of ASoC functions, but if so, debugfs should be created when "device connect", but current code is creating it in "device register". Can I assume that snd_soc_instantiate_card() <-> soc_cleanup_card_resources() are paired function ? I'm not sure... Anyway, it is easy to understand for me if paired functions are called from paired parent functions if possible. Currently debugfs is like this.
snd_soc_register_card() <-> snd_soc_unregister_card() - soc_init_card_debugfs() ???? - snd_soc_instantiate_card() <-> - soc_cleanup_card_resources() - soc_bind_dai_link() - soc_remove_dai_links() - soc_probe_aux_dev() - soc_remove_aux_dev() - card->probe() - card->remove() - ???? - soc_cleanup_card_debugfs()
At Tue, 24 Mar 2015 08:17:13 +0000, Kuninori Morimoto wrote:
Hi Takashi
Thank you for your feedback
If the reason is different (e.g. for further fix of hot unplug bug), it'd be understandable, though. In general, the ALSA device free consists of three phases:
- device disconnection
- wait until all devices are closed
- actual free all resources
And soc_cleanup_card_debugfs() basically belongs to 1, not 3.
Hmm... I'm not 100% understand detail of ASoC functions, but if so, debugfs should be created when "device connect", but current code is creating it in "device register".
Note that there is no "device connect" phase in ALSA. It's called before snd_card_register(), so it's an "init" phase. The "device connect" phase is invoked at snd_card_register().
Can I assume that snd_soc_instantiate_card() <-> soc_cleanup_card_resources() are paired function ? I'm not sure... Anyway, it is easy to understand for me if paired functions are called from paired parent functions if possible. Currently debugfs is like this.
snd_soc_register_card() <-> snd_soc_unregister_card()
- soc_init_card_debugfs() ????
- snd_soc_instantiate_card() <-> - soc_cleanup_card_resources()
- soc_bind_dai_link() - soc_remove_dai_links()
- soc_probe_aux_dev() - soc_remove_aux_dev()
- card->probe() - card->remove()
- ???? - soc_cleanup_card_debugfs()
IMO, a better change would be to call soc_init_card_debugfs() in snd_soc_instantiate_card(), supposing that the debugfs is available only for instantiated objects. This will fix the messy leakage at error paths, too.
Takashi
Hi Takashi
snd_soc_register_card() <-> snd_soc_unregister_card()
- soc_init_card_debugfs() ????
- snd_soc_instantiate_card() <-> - soc_cleanup_card_resources()
- soc_bind_dai_link() - soc_remove_dai_links()
- soc_probe_aux_dev() - soc_remove_aux_dev()
- card->probe() - card->remove()
- ???? - soc_cleanup_card_debugfs()
IMO, a better change would be to call soc_init_card_debugfs() in snd_soc_instantiate_card(), supposing that the debugfs is available only for instantiated objects. This will fix the messy leakage at error paths, too.
OK, here we go.
My concern is that my next patch will "return ret" if snd_soc_instantiate_card() was failed, but is it OK ?
Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_init_card_debugfs() is called from snd_soc_register_card() but, soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(), not from paired function.
This differences don't matter for now. But if anyone wants to implement a proper hotplug/unplug, this difference would become clearer.
Now, we can assume that snd_soc_instantiate_card() and soc_cleanup_card_resources() are paired function. soc_init_card_debugfs() / soc_cleanup_card_debugfs() paired function should be called from these.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4c26074..76bfff2 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1690,6 +1690,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) mutex_unlock(&card->mutex); mutex_unlock(&client_mutex);
+ soc_init_card_debugfs(card); + return 0;
probe_aux_dev_err: @@ -2381,8 +2383,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
snd_soc_initialize_card_lists(card);
- soc_init_card_debugfs(card); - card->rtd = devm_kzalloc(card->dev, sizeof(struct snd_soc_pcm_runtime) * (card->num_links + card->num_aux_devs), @@ -2413,7 +2413,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
ret = snd_soc_instantiate_card(card); if (ret != 0) - soc_cleanup_card_debugfs(card); + return ret;
/* deactivate pins to sleep state */ for (i = 0; i < card->num_rtd; i++) {
On Tue, Mar 24, 2015 at 09:01:00AM +0000, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_init_card_debugfs() is called from snd_soc_register_card() but, soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(), not from paired function.
Applied, thanks.
On Tue, Mar 24, 2015 at 09:01:00AM +0000, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current soc_init_card_debugfs() is called from snd_soc_register_card() but, soc_cleanup_card_debugfs() is called from soc_cleanup_card_resources(), not from paired function.
Oh, and as I'm fairly sure I've said before please don't send new patches in reply to existing threads - it makes things harder to follow.
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Takashi Iwai