[1/6] ASoC: soc-core: use devm_kzalloc() for rtd

Commit Message

Kuninori Morimoto Oct. 2, 2019, 5:22 a.m. UTC

From: Kuninori Morimoto <[e-mail]>

Current rtd, rtd->dev, rtd->codec_dais are created by normal kzalloc(),
but we want to use devm_kzalloc() as much as possible.

Created rtd->dev is registered by device_register() at
soc_new_pcm_runtime(), and it will be freed at
soc_free_pcm_runtime() by device_unregister().

This means, if we can use devm_kzalloc(rtd->dev, xxx) for
rtd / rtd->codec_dais, all these are automatically freed
via soc_free_pcm_runtime().
This patch uses devm_kzalloc(rtd->dev, xxx) for rtd / rtd->codec_dais.

Signed-off-by: Kuninori Morimoto <[e-mail]>
---
 sound/soc/soc-core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Mark Brown Oct. 2, 2019, 6:55 p.m. UTC | #1

On Wed, Oct 02, 2019 at 02:22:32PM +0900, Kuninori Morimoto wrote:

> From: Kuninori Morimoto <[e-mail]>

> Current rtd, rtd->dev, rtd->codec_dais are created by normal kzalloc(),
> but we want to use devm_kzalloc() as much as possible.
> 
> Created rtd->dev is registered by device_register() at
> soc_new_pcm_runtime(), and it will be freed at
> soc_free_pcm_runtime() by device_unregister().

These aren't using devm_ because they are done at card init time and so
might happen multiple times when other card components get removed and
added.  This shouldn't happen too much but if it does then it could end
up consuming a noticeable amount of memory.

Kuninori Morimoto Oct. 3, 2019, 12:31 a.m. UTC | #2

Hi Mark

> > From: Kuninori Morimoto <[e-mail]>
> 
> > Current rtd, rtd->dev, rtd->codec_dais are created by normal kzalloc(),
> > but we want to use devm_kzalloc() as much as possible.
> > 
> > Created rtd->dev is registered by device_register() at
> > soc_new_pcm_runtime(), and it will be freed at
> > soc_free_pcm_runtime() by device_unregister().
> 
> These aren't using devm_ because they are done at card init time and so
> might happen multiple times when other card components get removed and
> added.  This shouldn't happen too much but if it does then it could end
> up consuming a noticeable amount of memory.

I see.
Actually my local patch which is not yet posted can solve this
multiple times issue.
Mergeing these can be good solution.
Please drop it so far.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Kuninori Morimoto Oct. 3, 2019, 12:56 a.m. UTC | #3

Hi Mark, again

> > > From: Kuninori Morimoto <[e-mail]>
> > 
> > > Current rtd, rtd->dev, rtd->codec_dais are created by normal kzalloc(),
> > > but we want to use devm_kzalloc() as much as possible.
> > > 
> > > Created rtd->dev is registered by device_register() at
> > > soc_new_pcm_runtime(), and it will be freed at
> > > soc_free_pcm_runtime() by device_unregister().
> > 
> > These aren't using devm_ because they are done at card init time and so
> > might happen multiple times when other card components get removed and
> > added.  This shouldn't happen too much but if it does then it could end
> > up consuming a noticeable amount of memory.
> 
> I see.
> Actually my local patch which is not yet posted can solve this
> multiple times issue.
> Mergeing these can be good solution.
> Please drop it so far.

Oops, it was alreay posted and accepted :)

	d918a37610b1bf71faa86f589bd7604f71c1e05f
	("ASoC: soc-core: tidyup soc_new_pcm_runtime() alloc order")

above rtd, rtd->codec_dais are devm_kzalloc() by original dev
(which will be rtd->dev) instead of card/component dev.

It is registered by device_register() at soc_new_pcm_runtime(),
and will be unregistered by device_unregister() at soc_free_pcm_runtime()
when card was removed.

It is a little bit tricky, but 
these are chain-freed when card was removed.
If my understanding was correct, we don't have memory leak
by multiple times card/components remove.
But, please double check.

	user removed card
	 -> snd_soc_unbind_card()
	  -> soc_cleanup_card_resources()
	   -> soc_remove_pcm_runtimes()
	    -> soc_free_pcm_runtime()
(*)	     -> device_unregister(rtd->dev);

rtd->dev will be kfree() by soc_release_rtd_dev at (*),
then, rtd, rtd->codec_dais will be kfree() via devm_

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Patch

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4a47ba9..b16a9422 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -370,7 +370,6 @@  static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
 	if (!rtd)
 		return;
 
-	kfree(rtd->codec_dais);
 	list_del(&rtd->list);
 
 	/*
@@ -384,7 +383,6 @@  static void soc_free_pcm_runtime(struct snd_soc_pcm_runtime *rtd)
 	 *	soc_new_pcm_runtime()
 	 */
 	device_unregister(rtd->dev);
-	kfree(rtd);
 }
 
 static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
@@ -416,7 +414,7 @@  static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
 	/*
 	 * for rtd
 	 */
-	rtd = kzalloc(sizeof(struct snd_soc_pcm_runtime), GFP_KERNEL);
+	rtd = devm_kzalloc(dev, sizeof(*rtd), GFP_KERNEL);
 	if (!rtd)
 		goto free_rtd;
 
@@ -426,7 +424,7 @@  static struct snd_soc_pcm_runtime *soc_new_pcm_runtime(
 	/*
 	 * for rtd->codec_dais
 	 */
-	rtd->codec_dais = kcalloc(dai_link->num_codecs,
+	rtd->codec_dais = devm_kcalloc(dev, dai_link->num_codecs,
 					sizeof(struct snd_soc_dai *),
 					GFP_KERNEL);
 	if (!rtd->codec_dais)