[PATCH] ASoC: meson: aiu-fifo.c: use devm_kzalloc(), and remove .remove function
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(-)
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",
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,@@ -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,
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).
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
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
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
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
participants (3)
-
Jerome Brunet -
Kuninori Morimoto -
Mark Brown