diff mbox series

ASoC: meson: aiu-fifo.c: use devm_kzalloc(), and remove .remove function

Message ID 8735d1mjf3.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show
Series ASoC: meson: aiu-fifo.c: use devm_kzalloc(), and remove .remove function | expand

Commit Message

Kuninori Morimoto Sept. 9, 2022, 1:21 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
it by devm_kzalloc(), and remove kfree().
This patch do it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/meson/aiu-fifo.c | 10 +---------
 sound/soc/meson/aiu-fifo.h |  1 -
 sound/soc/meson/aiu.c      |  2 --
 3 files changed, 1 insertion(+), 12 deletions(-)

Comments

Jerome Brunet Sept. 9, 2022, 8:27 a.m. UTC | #1 
On Fri 09 Sep 2022 at 01:21, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
> it by devm_kzalloc(), and remove kfree().
> This patch do it.

I'm not sure about this change Kuninori.

This is the dai probe, not the device driver probe.
If I'm not mistaken it gets called when binding the card.

The components and card drivers are different here.

If the card probes several times for any reason, EPROBE_DEFER for
example, wouldn't this allocate the memory several times without
releasing it ?

>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/meson/aiu-fifo.c | 10 +---------
>  sound/soc/meson/aiu-fifo.h |  1 -
>  sound/soc/meson/aiu.c      |  2 --
>  3 files changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/sound/soc/meson/aiu-fifo.c b/sound/soc/meson/aiu-fifo.c
> index d67ff4cdabd5..843e5067e07d 100644
> --- a/sound/soc/meson/aiu-fifo.c
> +++ b/sound/soc/meson/aiu-fifo.c
> @@ -196,7 +196,7 @@ int aiu_fifo_dai_probe(struct snd_soc_dai *dai)
>  {
>  	struct aiu_fifo *fifo;
>  
> -	fifo = kzalloc(sizeof(*fifo), GFP_KERNEL);
> +	fifo = devm_kzalloc(dai->dev, sizeof(*fifo), GFP_KERNEL);
>  	if (!fifo)
>  		return -ENOMEM;
>  
> @@ -204,11 +204,3 @@ int aiu_fifo_dai_probe(struct snd_soc_dai *dai)
>  
>  	return 0;
>  }
> -
> -int aiu_fifo_dai_remove(struct snd_soc_dai *dai)
> -{
> -	kfree(dai->playback_dma_data);
> -
> -	return 0;
> -}
> -
> diff --git a/sound/soc/meson/aiu-fifo.h b/sound/soc/meson/aiu-fifo.h
> index 42ce266677cc..fb323a4385f7 100644
> --- a/sound/soc/meson/aiu-fifo.h
> +++ b/sound/soc/meson/aiu-fifo.h
> @@ -26,7 +26,6 @@ struct aiu_fifo {
>  };
>  
>  int aiu_fifo_dai_probe(struct snd_soc_dai *dai);
> -int aiu_fifo_dai_remove(struct snd_soc_dai *dai);
>  
>  snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
>  				   struct snd_pcm_substream *substream);
> diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
> index 88e611e64d14..7e632aa31368 100644
> --- a/sound/soc/meson/aiu.c
> +++ b/sound/soc/meson/aiu.c
> @@ -123,7 +123,6 @@ static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = {
>  		.ops		= &aiu_fifo_i2s_dai_ops,
>  		.pcm_new	= aiu_fifo_pcm_new,
>  		.probe		= aiu_fifo_i2s_dai_probe,
> -		.remove		= aiu_fifo_dai_remove,
>  	},
>  	[CPU_SPDIF_FIFO] = {
>  		.name = "SPDIF FIFO",
> @@ -139,7 +138,6 @@ static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = {
>  		.ops		= &aiu_fifo_spdif_dai_ops,
>  		.pcm_new	= aiu_fifo_pcm_new,
>  		.probe		= aiu_fifo_spdif_dai_probe,
> -		.remove		= aiu_fifo_dai_remove,
>  	},
>  	[CPU_I2S_ENCODER] = {
>  		.name = "I2S Encoder",
Mark Brown Sept. 9, 2022, 4:28 p.m. UTC | #2 
On Fri, Sep 09, 2022 at 10:27:22AM +0200, Jerome Brunet wrote:
> On Fri 09 Sep 2022 at 01:21, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> > Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
> > it by devm_kzalloc(), and remove kfree().
> > This patch do it.

> I'm not sure about this change Kuninori.

> This is the dai probe, not the device driver probe.
> If I'm not mistaken it gets called when binding the card.

> The components and card drivers are different here.

> If the card probes several times for any reason, EPROBE_DEFER for
> example, wouldn't this allocate the memory several times without
> releasing it ?

Yes, indeed.  You'd need to move the allocation to the device level
probe to convert to devm (which *would* be a good thing to do if
possible).
Kuninori Morimoto Sept. 12, 2022, 11:17 p.m. UTC | #3 
Hi Jerome, Mark

> > > Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
> > > it by devm_kzalloc(), and remove kfree().
> > > This patch do it.
> 
> > I'm not sure about this change Kuninori.
> 
> > This is the dai probe, not the device driver probe.
> > If I'm not mistaken it gets called when binding the card.
> 
> > The components and card drivers are different here.
> 
> > If the card probes several times for any reason, EPROBE_DEFER for
> > example, wouldn't this allocate the memory several times without
> > releasing it ?
> 
> Yes, indeed.  You'd need to move the allocation to the device level
> probe to convert to devm (which *would* be a good thing to do if
> possible).

Oh, yes, indeed.
I will fixup it in v2.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Sept. 13, 2022, 1:56 a.m. UTC | #4 
Hi Jerome, Mark, again

> > > > Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
> > > > it by devm_kzalloc(), and remove kfree().
> > > > This patch do it.
> > 
> > > I'm not sure about this change Kuninori.
> > 
> > > This is the dai probe, not the device driver probe.
> > > If I'm not mistaken it gets called when binding the card.
> > 
> > > The components and card drivers are different here.
> > 
> > > If the card probes several times for any reason, EPROBE_DEFER for
> > > example, wouldn't this allocate the memory several times without
> > > releasing it ?
> > 
> > Yes, indeed.  You'd need to move the allocation to the device level
> > probe to convert to devm (which *would* be a good thing to do if
> > possible).
> 
> Oh, yes, indeed.
> I will fixup it in v2.

If there was EPROBE_DEFER issue, snd_soc_bind_card() will return at (A)
*before* calling probe callback on each DAIs at (B).
So, I think calling devm_kzalloc() at .probe is maybe no problem.

	static int snd_soc_bind_card(...)
	{
		...

		for_each_card_prelinks(card, i, dai_link) {
(A)			ret = snd_soc_add_pcm_runtime(card, dai_link);
			if (ret < 0)
				goto probe_end;
		}

		...

		/* probe all DAI links on this card */
(B)		ret = soc_probe_link_dais(card);
		...
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Jerome Brunet Sept. 13, 2022, 8:06 a.m. UTC | #5 
On Tue 13 Sep 2022 at 01:56, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Jerome, Mark, again
>
>> > > > Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
>> > > > it by devm_kzalloc(), and remove kfree().
>> > > > This patch do it.
>> > 
>> > > I'm not sure about this change Kuninori.
>> > 
>> > > This is the dai probe, not the device driver probe.
>> > > If I'm not mistaken it gets called when binding the card.
>> > 
>> > > The components and card drivers are different here.
>> > 
>> > > If the card probes several times for any reason, EPROBE_DEFER for
>> > > example, wouldn't this allocate the memory several times without
>> > > releasing it ?
>> > 
>> > Yes, indeed.  You'd need to move the allocation to the device level
>> > probe to convert to devm (which *would* be a good thing to do if
>> > possible).

The AIU is a device that provides multiple ASoC components. I know it is
border line to have this and I'm sorry I was not able to contribute on
this lately.

The device is a bit complex as it is. The allocation in question really
belongs to DAI. Moving it to the device level might be possible but it
would not help make the code easy to follow and maintain.

I usually pick devm_ function whenever possible but here I chose the old
school way on purpose, to make sure the memory is managed correctly.

Beside the general recommendation to use devm when possible, is there an
issue I am missing ?

>> 
>> Oh, yes, indeed.
>> I will fixup it in v2.
>
> If there was EPROBE_DEFER issue, snd_soc_bind_card() will return at (A)
> *before* calling probe callback on each DAIs at (B).
> So, I think calling devm_kzalloc() at .probe is maybe no problem.
>
> 	static int snd_soc_bind_card(...)
> 	{
> 		...
>
> 		for_each_card_prelinks(card, i, dai_link) {
> (A)			ret = snd_soc_add_pcm_runtime(card, dai_link);
> 			if (ret < 0)
> 				goto probe_end;
> 		}
>
> 		...
>
> 		/* probe all DAI links on this card */
> (B)		ret = soc_probe_link_dais(card);
> 		...
> 	}

It would still allocate the memory multiple times if the card driver
gets unloaded and reloaded, manually for example.

It would be strange to do so, but it remains incorrect from the driver
to allocate the memory like this.

>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto Sept. 14, 2022, 12:04 a.m. UTC | #6 
Hi Jerome

> The AIU is a device that provides multiple ASoC components. I know it is
> border line to have this and I'm sorry I was not able to contribute on
> this lately.
> 
> The device is a bit complex as it is. The allocation in question really
> belongs to DAI. Moving it to the device level might be possible but it
> would not help make the code easy to follow and maintain.
> 
> I usually pick devm_ function whenever possible but here I chose the old
> school way on purpose, to make sure the memory is managed correctly.
(snip)
> It would still allocate the memory multiple times if the card driver
> gets unloaded and reloaded, manually for example.
> 
> It would be strange to do so, but it remains incorrect from the driver
> to allocate the memory like this.

OK, let's skip this patch.
It is not a big deal

Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/sound/soc/meson/aiu-fifo.c b/sound/soc/meson/aiu-fifo.c
index d67ff4cdabd5..843e5067e07d 100644
--- a/sound/soc/meson/aiu-fifo.c
+++ b/sound/soc/meson/aiu-fifo.c
@@ -196,7 +196,7 @@  int aiu_fifo_dai_probe(struct snd_soc_dai *dai)
 {
 	struct aiu_fifo *fifo;
 
-	fifo = kzalloc(sizeof(*fifo), GFP_KERNEL);
+	fifo = devm_kzalloc(dai->dev, sizeof(*fifo), GFP_KERNEL);
 	if (!fifo)
 		return -ENOMEM;
 
@@ -204,11 +204,3 @@  int aiu_fifo_dai_probe(struct snd_soc_dai *dai)
 
 	return 0;
 }
-
-int aiu_fifo_dai_remove(struct snd_soc_dai *dai)
-{
-	kfree(dai->playback_dma_data);
-
-	return 0;
-}
-
diff --git a/sound/soc/meson/aiu-fifo.h b/sound/soc/meson/aiu-fifo.h
index 42ce266677cc..fb323a4385f7 100644
--- a/sound/soc/meson/aiu-fifo.h
+++ b/sound/soc/meson/aiu-fifo.h
@@ -26,7 +26,6 @@  struct aiu_fifo {
 };
 
 int aiu_fifo_dai_probe(struct snd_soc_dai *dai);
-int aiu_fifo_dai_remove(struct snd_soc_dai *dai);
 
 snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
 				   struct snd_pcm_substream *substream);
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
index 88e611e64d14..7e632aa31368 100644
--- a/sound/soc/meson/aiu.c
+++ b/sound/soc/meson/aiu.c
@@ -123,7 +123,6 @@  static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = {
 		.ops		= &aiu_fifo_i2s_dai_ops,
 		.pcm_new	= aiu_fifo_pcm_new,
 		.probe		= aiu_fifo_i2s_dai_probe,
-		.remove		= aiu_fifo_dai_remove,
 	},
 	[CPU_SPDIF_FIFO] = {
 		.name = "SPDIF FIFO",
@@ -139,7 +138,6 @@  static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = {
 		.ops		= &aiu_fifo_spdif_dai_ops,
 		.pcm_new	= aiu_fifo_pcm_new,
 		.probe		= aiu_fifo_spdif_dai_probe,
-		.remove		= aiu_fifo_dai_remove,
 	},
 	[CPU_I2S_ENCODER] = {
 		.name = "I2S Encoder",