[alsa-devel] [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap
From: Julia Lawall Julia.Lawall@lip6.fr
Add missing iounmap in error handling code, in a case where the function already preforms iounmap on some other execution path.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @@ expression e; statement S,S1; int ret; @@ e = (ioremap|ioremap_nocache)(...) ... when != iounmap(e) if (<+...e...+>) S ... when any when != iounmap(e) *if (...) { ... when != iounmap(e) return ...; } ... when any iounmap(e); // </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/soc/mxs/mxs-saif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index 049e543..5ee0adb 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start;
@@ -739,6 +739,7 @@ failed_register: failed_get_irq2: free_irq(saif->irq, saif); failed_get_irq1: +failed_get_resource: iounmap(saif->base); failed_ioremap: release_mem_region(iores->start, resource_size(iores));
Hi,
On Thu, Jan 12, 2012 at 10:55:03AM +0100, Julia Lawall wrote:
From: Julia Lawall Julia.Lawall@lip6.fr
Add missing iounmap in error handling code, in a case where the function already preforms iounmap on some other execution path.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @@ expression e; statement S,S1; int ret; @@ e = (ioremap|ioremap_nocache)(...) ... when != iounmap(e) if (<+...e...+>) S ... when any when != iounmap(e) *if (...) { ... when != iounmap(e) return ...; } ... when any iounmap(e); // </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
This is not related to this patch actually, but when you send a series of patches could you make sure to send them all as a thread ? It makes it easier (at least for me) to see that those are all related to each other (in your case, sematich patches).
In order to do that with git you could:
$ git format-patch -o patches --cover-letter linus/master
[ edit cover letter]
$ git send-email --to lkml patches/
that will do the trick. Thanks
-----Original Message----- From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- owner@vger.kernel.org] On Behalf Of Julia Lawall Sent: Thursday, January 12, 2012 5:55 PM To: Liam Girdwood Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap
From: Julia Lawall Julia.Lawall@lip6.fr
Add missing iounmap in error handling code, in a case where the function already preforms iounmap on some other execution path.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @@ expression e; statement S,S1; int ret; @@ e = (ioremap|ioremap_nocache)(...) ... when != iounmap(e) if (<+...e...+>) S ... when any when != iounmap(e) *if (...) { ... when != iounmap(e) return ...; } ... when any iounmap(e); // </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
sound/soc/mxs/mxs-saif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index 049e543..5ee0adb 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret);
goto failed_ioremap;
goto failed_get_resource;@@ -739,6 +739,7 @@ failed_register: failed_get_irq2: free_irq(saif->irq, saif); failed_get_irq1: +failed_get_resource:
There's already a 'failed_get_resource' there, wouldn't your change break The original code?
BTW, I guess a better way is that you can submit a patch to change the driver To use devm_alloc_* and it's friend routines, then we do not need to fix Such things any more.
Regards Dong Aisheng
iounmap(saif->base); failed_ioremap: release_mem_region(iores->start, resource_size(iores));
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, 13 Jan 2012, Dong Aisheng-B29396 wrote:
-----Original Message----- From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- owner@vger.kernel.org] On Behalf Of Julia Lawall Sent: Thursday, January 12, 2012 5:55 PM To: Liam Girdwood Cc: kernel-janitors@vger.kernel.org; Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org Subject: [PATCH 1/15] sound/soc/mxs/mxs-saif.c: add missing iounmap
From: Julia Lawall Julia.Lawall@lip6.fr
Add missing iounmap in error handling code, in a case where the function already preforms iounmap on some other execution path.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @@ expression e; statement S,S1; int ret; @@ e = (ioremap|ioremap_nocache)(...) ... when != iounmap(e) if (<+...e...+>) S ... when any when != iounmap(e) *if (...) { ... when != iounmap(e) return ...; } ... when any iounmap(e); // </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
sound/soc/mxs/mxs-saif.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index 049e543..5ee0adb 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -680,7 +680,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret);
goto failed_ioremap;
goto failed_get_resource;@@ -739,6 +739,7 @@ failed_register: failed_get_irq2: free_irq(saif->irq, saif); failed_get_irq1: +failed_get_resource:
There's already a 'failed_get_resource' there, wouldn't your change break The original code?
BTW, I guess a better way is that you can submit a patch to change the driver To use devm_alloc_* and it's friend routines, then we do not need to fix Such things any more.
Thanks for the suggestions. I will fix it up with the devm_ solution.
julia
From: Julia Lawall Julia.Lawall@lip6.fr
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
By removing the need for iounmap, this also eliminates a missing iounmap problem, in the the original error handling code following a successful call to ioremap.
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/soc/mxs/mxs-saif.c | 44 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 30 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index f204dba..6a72ad9 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -630,7 +630,7 @@ static int mxs_saif_probe(struct platform_device *pdev) if (pdev->id >= ARRAY_SIZE(mxs_saif)) return -EINVAL;
- saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM;
@@ -652,10 +652,9 @@ static int mxs_saif_probe(struct platform_device *pdev)
saif->clk = clk_get(&pdev->dev, NULL); if (IS_ERR(saif->clk)) { - ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return PTR_ERR(saif->clk); }
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -666,18 +665,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { + if (!devm_request_mem_region(&pdev->dev, iores->start, + resource_size(iores), "mxs-saif")) { dev_err(&pdev->dev, "request_mem_region failed\n"); ret = -EBUSY; goto failed_get_resource; }
- saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_ioremap(&pdev->dev, iores->start, + resource_size(iores)); if (!saif->base) { dev_err(&pdev->dev, "ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; }
dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -685,7 +685,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start;
@@ -694,14 +694,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -709,7 +710,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; }
platform_set_drvdata(pdev, saif); @@ -717,7 +718,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; }
saif->soc_platform_pdev = platform_device_alloc( @@ -740,36 +741,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif);
return ret; }
static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev);
platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif);
return 0; }
On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote:
if (IS_ERR(saif->clk)) {
ret = PTR_ERR(saif->clk);goto failed_clk;
return PTR_ERR(saif->clk);
Looks like the transformation is overly enthusiastic here - the assignment to ret is removed but ret is used in the log message.
On Tue, 24 Jan 2012, Mark Brown wrote:
On Tue, Jan 24, 2012 at 06:29:13PM +0100, Julia Lawall wrote:
if (IS_ERR(saif->clk)) {
ret = PTR_ERR(saif->clk);goto failed_clk;
return PTR_ERR(saif->clk);Looks like the transformation is overly enthusiastic here - the assignment to ret is removed but ret is used in the log message.
Uh, no, I did that stupid thing by hand. I'll fix it. Thanks.
julia
On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote:
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
This is a really nice cleanup - I tried to apply it but unfortunately it needs a rebase due to the change to use clk_prepare()/clk_unprepare() which went in recently. Could you rebase against current -next please?
On Thu, 26 Jan 2012, Mark Brown wrote:
On Tue, Jan 24, 2012 at 09:45:26PM +0100, Julia Lawall wrote:
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
This is a really nice cleanup - I tried to apply it but unfortunately it needs a rebase due to the change to use clk_prepare()/clk_unprepare() which went in recently. Could you rebase against current -next please?
Could you tell me in more detail what the problem is? I pulled linux-next and regenerated the patch, but got exactly the same thing as before. I do see some calls to clk_prepare_enable in the code.
thanks, julia
On Thu, Jan 26, 2012 at 12:24:21PM +0100, Julia Lawall wrote:
On Thu, 26 Jan 2012, Mark Brown wrote:
This is a really nice cleanup - I tried to apply it but unfortunately it needs a rebase due to the change to use clk_prepare()/clk_unprepare() which went in recently. Could you rebase against current -next please?
Could you tell me in more detail what the problem is? I pulled linux-next and regenerated the patch, but got exactly the same thing as before. I do see some calls to clk_prepare_enable in the code.
I tried to apply it with git am but even with -3 it wouldn't apply (-3 said it didn't know about the original blobs which was worrying), looking at the logs the clk_prepare stuff seemed most likely to have overlapped. Perhaps your regeneration will be enough.
@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {
- if (!devm_request_mem_region(&pdev->dev, iores->start,
resource_size(iores), "mxs-saif")) {
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_ioremap(&pdev->dev, iores->start,
resource_size(iores));
devm_request_and_ioremap() to save even more code?
On Thu, 26 Jan 2012, Wolfram Sang wrote:
@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {
- if (!devm_request_mem_region(&pdev->dev, iores->start,
resource_size(iores), "mxs-saif")) {
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_ioremap(&pdev->dev, iores->start,
resource_size(iores));devm_request_and_ioremap() to save even more code?
I didn't do that because apparently it is not yet in a release? I have another for introducing those...
julia
On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
On Thu, 26 Jan 2012, Wolfram Sang wrote:
devm_request_and_ioremap() to save even more code?
I didn't do that because apparently it is not yet in a release? I have another for introducing those...
It went in during the merge window so patches using it should be good to go now.
From: Julia Lawall Julia.Lawall@lip6.fr
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
By removing the need for kfree and iounmap, this also eliminates missing resource-release problems.
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- This is a patch against the version in git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
sound/soc/mxs/mxs-saif.c | 47 ++++++++++++----------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev) return ret; }
- saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM;
@@ -652,7 +652,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return ret; }
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - ret = -EBUSY; - goto failed_get_resource; - } - - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_request_and_ioremap(&pdev->dev, iores); if (!saif->base) { - dev_err(&pdev->dev, "ioremap failed\n"); + dev_err(&pdev->dev, "devm_request_and_ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; }
dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -682,7 +675,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start;
@@ -691,14 +684,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -706,7 +700,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; }
platform_set_drvdata(pdev, saif); @@ -714,7 +708,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; }
saif->soc_platform_pdev = platform_device_alloc( @@ -737,36 +731,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif);
return ret; }
static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev);
platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif);
return 0; }
Julia,
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {dev_err(&pdev->dev, "request_mem_region failed\n");ret = -EBUSY;goto failed_get_resource;- }
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_request_and_ioremap(&pdev->dev, iores);
You can skip checking 'iores', too. I also did that in the example, but a lot of people seem to miss it. Where did you get the information how to use devm_request_and_ioremap? I probably need to spread the word even more...
Thanks,
Wolfram
On Thu, 26 Jan 2012, Wolfram Sang wrote:
Julia,
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -663,18 +663,11 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {dev_err(&pdev->dev, "request_mem_region failed\n");ret = -EBUSY;goto failed_get_resource;- }
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_request_and_ioremap(&pdev->dev, iores);
You can skip checking 'iores', too. I also did that in the example, but a lot of people seem to miss it.
I can try to do that, but it seems a little bit unintuitive. Perhaps it would be easier for people to remember to put in error handling code when they need it if they always have to do it? If I remove it, there will be one call that has no test and then another call a few lines later that does.
Where did you get the information how to use devm_request_and_ioremap? I probably need to spread the word even more...
I saw it in Documentation/driver-model/devres.txt and then looked around for some examples.
julia
You can skip checking 'iores', too. I also did that in the example, but a lot of people seem to miss it.
I can try to do that, but it seems a little bit unintuitive. Perhaps it would be easier for people to remember to put in error handling code when they need it if they always have to do it? If I remove it, there will be one call that has no test and then another call a few lines later that does.
I see your point. I would still like to get rid of the duplicated code (then it can't be forgotten as well). Maybe I should have named the function something alike devm_check_and_request_and_ioremap()? Then I could have also introduced a similar function for requesting irq. Will think about this a bit more. Thanks for updating your patch!
On Sat, 28 Jan 2012, Wolfram Sang wrote:
You can skip checking 'iores', too. I also did that in the example, but a lot of people seem to miss it.
I can try to do that, but it seems a little bit unintuitive. Perhaps it would be easier for people to remember to put in error handling code when they need it if they always have to do it? If I remove it, there will be one call that has no test and then another call a few lines later that does.
I see your point. I would still like to get rid of the duplicated code (then it can't be forgotten as well). Maybe I should have named the function something alike devm_check_and_request_and_ioremap()? Then I could have also introduced a similar function for requesting irq. Will think about this a bit more. Thanks for updating your patch!
Despite believing in my point, error handling code is where people make a lot of mistakes. Getting rid of it seems always to be a good idea. Maybe such checks could be moved into more uses of the result of platform_get_resource.
julia
How important is the name argument to request_mem_region?
devm_request_and_ioremap uses:
name = res->name ?: dev_name(dev);
In trying to introduce uses of devm_request_and_ioremap I thus look for res->name and dev_name(dev) as well as the name in the driver structure in the platform_driver structure. But sometimes the name is pdev->name, ie the name field of the argument to the probe function. And sometimes it is a string that is slightly different than what is in the driver structure.
thanks, julia
And how about a function devm_clk_get? It looks like there would be around 150 opportunities for its use.
julia
On Sat, Feb 04, 2012 at 11:08:24PM +0100, Julia Lawall wrote:
And how about a function devm_clk_get? It looks like there would be around 150 opportunities for its use.
Note that currently there's no standard clk API so any work there is a bit more painful than it perhaps should be.
From: Julia Lawall Julia.Lawall@lip6.fr
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
By removing the need for kfree and iounmap, this also eliminates missing resource-release problems.
This also removes a now unneeded test on iores.
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
---
patch against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
sound/soc/mxs/mxs-saif.c | 53 ++++++++++------------------------------------- 1 file changed, 12 insertions(+), 41 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -632,7 +632,7 @@ static int mxs_saif_probe(struct platform_device *pdev) return ret; }
- saif = kzalloc(sizeof(*saif), GFP_KERNEL); + saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL); if (!saif) return -ENOMEM;
@@ -652,29 +652,16 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = PTR_ERR(saif->clk); dev_err(&pdev->dev, "Cannot get the clock: %d\n", ret); - goto failed_clk; + return ret; }
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!iores) { - ret = -ENODEV; - dev_err(&pdev->dev, "failed to get io resource: %d\n", - ret); - goto failed_get_resource; - }
- if (!request_mem_region(iores->start, resource_size(iores), - "mxs-saif")) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - ret = -EBUSY; - goto failed_get_resource; - } - - saif->base = ioremap(iores->start, resource_size(iores)); + saif->base = devm_request_and_ioremap(&pdev->dev, iores); if (!saif->base) { - dev_err(&pdev->dev, "ioremap failed\n"); + dev_err(&pdev->dev, "devm_request_and_ioremap failed\n"); ret = -ENODEV; - goto failed_ioremap; + goto failed_get_resource; }
dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -682,7 +669,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = -ENODEV; dev_err(&pdev->dev, "failed to get dma resource: %d\n", ret); - goto failed_ioremap; + goto failed_get_resource; } saif->dma_param.chan_num = dmares->start;
@@ -691,14 +678,15 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->irq; dev_err(&pdev->dev, "failed to get irq resource: %d\n", ret); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dev = &pdev->dev; - ret = request_irq(saif->irq, mxs_saif_irq, 0, "mxs-saif", saif); + ret = devm_request_irq(&pdev->dev, saif->irq, mxs_saif_irq, 0, + "mxs-saif", saif); if (ret) { dev_err(&pdev->dev, "failed to request irq\n"); - goto failed_get_irq1; + goto failed_get_resource; }
saif->dma_param.chan_irq = platform_get_irq(pdev, 1); @@ -706,7 +694,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = saif->dma_param.chan_irq; dev_err(&pdev->dev, "failed to get dma irq resource: %d\n", ret); - goto failed_get_irq2; + goto failed_get_resource; }
platform_set_drvdata(pdev, saif); @@ -714,7 +702,7 @@ static int mxs_saif_probe(struct platform_device *pdev) ret = snd_soc_register_dai(&pdev->dev, &mxs_saif_dai); if (ret) { dev_err(&pdev->dev, "register DAI failed\n"); - goto failed_register; + goto failed_get_resource; }
saif->soc_platform_pdev = platform_device_alloc( @@ -737,36 +725,19 @@ failed_pdev_add: platform_device_put(saif->soc_platform_pdev); failed_pdev_alloc: snd_soc_unregister_dai(&pdev->dev); -failed_register: -failed_get_irq2: - free_irq(saif->irq, saif); -failed_get_irq1: - iounmap(saif->base); -failed_ioremap: - release_mem_region(iores->start, resource_size(iores)); failed_get_resource: clk_put(saif->clk); -failed_clk: - kfree(saif);
return ret; }
static int __devexit mxs_saif_remove(struct platform_device *pdev) { - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct mxs_saif *saif = platform_get_drvdata(pdev);
platform_device_unregister(saif->soc_platform_pdev); - snd_soc_unregister_dai(&pdev->dev); - - iounmap(saif->base); - release_mem_region(res->start, resource_size(res)); - free_irq(saif->irq, saif); - clk_put(saif->clk); - kfree(saif);
return 0; }
On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
From: Julia Lawall Julia.Lawall@lip6.fr
The various devm_ functions allocate memory that is released when a driver detaches. This patch uses these functions for data that is allocated in the probe function of a platform device and is only freed in the remove function.
By removing the need for kfree and iounmap, this also eliminates missing resource-release problems.
This also removes a now unneeded test on iores.
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
Looks good, will test it later today.
On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
patch against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
Hrm, I know there had been some debate about what to generate against but this now once again fails to apply against for-3.4... No idea what's going on there and I didn't investigate the conflicts, sorry.
I had been waiting in the vauge hope that some of the mxs maintainers would review.
On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
patch against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
Hrm, I know there had been some debate about what to generate against but this now once again fails to apply against for-3.4... No idea what's going on there and I didn't investigate the conflicts, sorry.
I had been waiting in the vauge hope that some of the mxs maintainers would review.
I also failed to apply it against for-3.4. It seemed the conflicts are a lot.
Hi Julia,
Can you regenerate the patch based on Mark's for-3.4 branch?
BTW, i wonder if this patch's title should be changed a bit since what this patch does is not adding missing iounmap anymore. Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends.
Regards Dong Aisheng
On Wed, 8 Feb 2012, Dong Aisheng wrote:
On Tue, Feb 7, 2012 at 12:20 AM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Sat, Jan 28, 2012 at 08:03:34AM +0100, Julia Lawall wrote:
patch against git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
Hrm, I know there had been some debate about what to generate against but this now once again fails to apply against for-3.4... No idea what's going on there and I didn't investigate the conflicts, sorry.
I had been waiting in the vauge hope that some of the mxs maintainers would review.
I also failed to apply it against for-3.4. It seemed the conflicts are a lot.
Hi Julia,
Can you regenerate the patch based on Mark's for-3.4 branch?
BTW, i wonder if this patch's title should be changed a bit since what this patch does is not adding missing iounmap anymore. Maybe like: ASoC: mxs-saif: convert to use devm_kmalloc and its friends.
Yes, thanks for the suggestions. I will do both shortly (today or tomorrow).
julia
On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
On Thu, 26 Jan 2012, Wolfram Sang wrote:
@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {
- if (!devm_request_mem_region(&pdev->dev, iores->start,
resource_size(iores), "mxs-saif")) {
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_ioremap(&pdev->dev, iores->start,
resource_size(iores));devm_request_and_ioremap() to save even more code?
I didn't do that because apparently it is not yet in a release? I have another for introducing those...
It got included in the last merge window:
commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0 Author: Wolfram Sang w.sang@pengutronix.de Date: Tue Oct 25 15:16:47 2011 +0200
lib: devres: add convenience function to remap a resource ...
On Thu, 26 Jan 2012, Wolfram Sang wrote:
On Thu, Jan 26, 2012 at 12:21:35PM +0100, Julia Lawall wrote:
On Thu, 26 Jan 2012, Wolfram Sang wrote:
@@ -666,18 +666,19 @@ static int mxs_saif_probe(struct platform_device *pdev) goto failed_get_resource; }
- if (!request_mem_region(iores->start, resource_size(iores),
"mxs-saif")) {
- if (!devm_request_mem_region(&pdev->dev, iores->start,
resource_size(iores), "mxs-saif")) {
- saif->base = ioremap(iores->start, resource_size(iores));
- saif->base = devm_ioremap(&pdev->dev, iores->start,
resource_size(iores));devm_request_and_ioremap() to save even more code?
I didn't do that because apparently it is not yet in a release? I have another for introducing those...
It got included in the last merge window:
commit 72f8c0bfa0de64c68ee59f40eb9b2683bffffbb0 Author: Wolfram Sang w.sang@pengutronix.de Date: Tue Oct 25 15:16:47 2011 +0200
lib: devres: add convenience function to remap a resource ...
OK, thanks. I'll take that into account.
julia
participants (6)
-
Dong Aisheng -
Dong Aisheng-B29396 -
Felipe Balbi -
Julia Lawall -
Mark Brown -
Wolfram Sang