diff mbox

ASoC: soc-core: call soc_cleanup_card_debugfs() from snd_soc_unregister_card()

Message ID 87a8z341ao.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto March 24, 2015, 1:44 a.m. UTC 
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(-)

Comments

Takashi Iwai March 24, 2015, 6:54 a.m. UTC | #1 
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);
>  		dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
>  	}
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Kuninori Morimoto March 24, 2015, 8:17 a.m. UTC | #2 
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:
> 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.

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()
Takashi Iwai March 24, 2015, 8:35 a.m. UTC | #3 
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:
> > 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.
> 
> 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
Kuninori Morimoto March 24, 2015, 9 a.m. UTC | #4 
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
diff mbox

Patch

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);
 	}