From: Julia Lawall <[email protected]>
Use devm_kzalloc for all calls to kzalloc and not just the first. Use devm
functions for other allocations as well.
Move the call to platform_get_resource(pdev, IORESOURCE_MEM, 0) closer to
where its result is passed to devm_request_and_ioremap to make the lack of
need for a NULL test more evident.
The semantic match that finds the inconsistency is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
@@
*devm_kzalloc(...)
...
*kzalloc(...)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/mtd/devices/spear_smi.c | 72 +++++++++-------------------------------
1 file changed, 17 insertions(+), 55 deletions(-)
diff --git a/drivers/mtd/devices/spear_smi.c b/drivers/mtd/devices/spear_smi.c
index b85f183..f889fad 100644
--- a/drivers/mtd/devices/spear_smi.c
+++ b/drivers/mtd/devices/spear_smi.c
@@ -827,7 +827,7 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
if (!flash_info)
return -ENODEV;
- flash = kzalloc(sizeof(*flash), GFP_ATOMIC);
+ flash = devm_kzalloc(&pdev->dev, sizeof(*flash), GFP_ATOMIC);
if (!flash)
return -ENOMEM;
flash->bank = bank;
@@ -838,15 +838,13 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
flash_index = spear_smi_probe_flash(dev, bank);
if (flash_index < 0) {
dev_info(&dev->pdev->dev, "smi-nor%d not found\n", bank);
- ret = flash_index;
- goto err_probe;
+ return flash_index;
}
/* map the memory for nor flash chip */
- flash->base_addr = ioremap(flash_info->mem_base, flash_info->size);
- if (!flash->base_addr) {
- ret = -EIO;
- goto err_probe;
- }
+ flash->base_addr = devm_ioremap(&pdev->dev, flash_info->mem_base,
+ flash_info->size);
+ if (!flash->base_addr)
+ return -EIO;
dev->flash[bank] = flash;
flash->mtd.priv = dev;
@@ -888,17 +886,10 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
count);
if (ret) {
dev_err(&dev->pdev->dev, "Err MTD partition=%d\n", ret);
- goto err_map;
+ return ret;
}
return 0;
-
-err_map:
- iounmap(flash->base_addr);
-
-err_probe:
- kfree(flash);
- return ret;
}
/**
@@ -942,13 +933,6 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
}
}
- smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!smi_base) {
- ret = -ENODEV;
- dev_err(&pdev->dev, "invalid smi base address\n");
- goto err;
- }
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = -ENODEV;
@@ -956,26 +940,20 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
goto err;
}
- dev = kzalloc(sizeof(*dev), GFP_ATOMIC);
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_ATOMIC);
if (!dev) {
ret = -ENOMEM;
dev_err(&pdev->dev, "mem alloc fail\n");
goto err;
}
- smi_base = request_mem_region(smi_base->start, resource_size(smi_base),
- pdev->name);
- if (!smi_base) {
- ret = -EBUSY;
- dev_err(&pdev->dev, "request mem region fail\n");
- goto err_mem;
- }
+ smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dev->io_base = ioremap(smi_base->start, resource_size(smi_base));
+ dev->io_base = devm_request_and_ioremap(&pdev->dev, smi_base);
if (!dev->io_base) {
ret = -EIO;
- dev_err(&pdev->dev, "ioremap fail\n");
- goto err_ioremap;
+ dev_err(&pdev->dev, "devm_request_and_ioremap fail\n");
+ goto err;
}
dev->pdev = pdev;
@@ -991,17 +969,18 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
dev->num_flashes = MAX_NUM_FLASH_CHIP;
}
- dev->clk = clk_get(&pdev->dev, NULL);
+ dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk)) {
ret = PTR_ERR(dev->clk);
- goto err_clk;
+ goto err;
}
ret = clk_prepare_enable(dev->clk);
if (ret)
- goto err_clk_prepare_enable;
+ goto err;
- ret = request_irq(irq, spear_smi_int_handler, 0, pdev->name, dev);
+ ret = devm_request_irq(&pdev->dev, irq, spear_smi_int_handler, 0,
+ pdev->name, dev);
if (ret) {
dev_err(&dev->pdev->dev, "SMI IRQ allocation failed\n");
goto err_irq;
@@ -1024,18 +1003,9 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
return 0;
err_bank_setup:
- free_irq(irq, dev);
platform_set_drvdata(pdev, NULL);
err_irq:
clk_disable_unprepare(dev->clk);
-err_clk_prepare_enable:
- clk_put(dev->clk);
-err_clk:
- iounmap(dev->io_base);
-err_ioremap:
- release_mem_region(smi_base->start, resource_size(smi_base));
-err_mem:
- kfree(dev);
err:
return ret;
}
@@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
ret = mtd_device_unregister(&flash->mtd);
if (ret)
dev_err(&pdev->dev, "error removing mtd\n");
-
- iounmap(flash->base_addr);
- kfree(flash);
}
irq = platform_get_irq(pdev, 0);
- free_irq(irq, dev);
clk_disable_unprepare(dev->clk);
- clk_put(dev->clk);
- iounmap(dev->io_base);
- kfree(dev);
smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- release_mem_region(smi_base->start, resource_size(smi_base));
platform_set_drvdata(pdev, NULL);
return 0;
[See here for the original patch:
http://lists.infradead.org/pipermail/linux-mtd/2012-August/043112.html]
Julia, sorry for long delay,
aiaiai gives says that this patch introduced 3 warnings:
--------------------------------------------------------------------------------
Successfully built configuration "arm-spear6xx_defconfig,arm,arm-unknown-linux-gnueabi-", results:
--- before_patching.log
+++ after_patching.log
@@ @@
-drivers/mtd/devices/spear_smi.c: In function 'spear_smi_remove':
-drivers/mtd/devices/spear_smi.c:1022:30: warning: variable 'pdata' set but not used [-Wunused-but-set-variable]
+drivers/mtd/devices/spear_smi.c: In function 'spear_smi_remove':
+drivers/mtd/devices/spear_smi.c:1026:9: warning: variable 'irq' set but not used [-Wunused-but-set-variable]
+drivers/mtd/devices/spear_smi.c:1022:30: warning: variable 'pdata' set but not used [-Wunused-but-set-variable]
+drivers/mtd/devices/spear_smi.c:1024:19: warning: variable 'smi_base' set but not used [-Wunused-but-set-variable]
--------------------------------------------------------------------------------
On Sat, 2012-08-04 at 22:36 +0200, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Use devm_kzalloc for all calls to kzalloc and not just the first. Use devm
> functions for other allocations as well.
>
> Move the call to platform_get_resource(pdev, IORESOURCE_MEM, 0) closer to
> where its result is passed to devm_request_and_ioremap to make the lack of
> need for a NULL test more evident.
>
> The semantic match that finds the inconsistency is as follows:
> (http://coccinelle.lip6.fr/)
...
> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
> ret = mtd_device_unregister(&flash->mtd);
> if (ret)
> dev_err(&pdev->dev, "error removing mtd\n");
> -
> - iounmap(flash->base_addr);
> - kfree(flash);
> }
>
> irq = platform_get_irq(pdev, 0);
> - free_irq(irq, dev);
I guess 'platform_get_irq()' should be killed as well? Stefan, this is
strange code - we get irq, without checking for error, and then free it?
What is the rationale?
> clk_disable_unprepare(dev->clk);
> - clk_put(dev->clk);
> - iounmap(dev->io_base);
> - kfree(dev);
>
> smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - release_mem_region(smi_base->start, resource_size(smi_base));
> platform_set_drvdata(pdev, NULL);
Why do we set platform data to NULL, is this needed?
--
Best Regards,
Artem Bityutskiy
On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
>> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
>> ret = mtd_device_unregister(&flash->mtd);
>> if (ret)
>> dev_err(&pdev->dev, "error removing mtd\n");
>> -
>> - iounmap(flash->base_addr);
>> - kfree(flash);
>> }
>>
>> irq = platform_get_irq(pdev, 0);
>> - free_irq(irq, dev);
>
> I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> strange code - we get irq, without checking for error, and then free it?
> What is the rationale?
Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().
>> clk_disable_unprepare(dev->clk);
>> - clk_put(dev->clk);
>> - iounmap(dev->io_base);
>> - kfree(dev);
>>
>> smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - release_mem_region(smi_base->start, resource_size(smi_base));
>> platform_set_drvdata(pdev, NULL);
>
> Why do we set platform data to NULL, is this needed?
It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.
Thanks,
Stefan
On Fri, 2012-08-24 at 15:14 +0200, Stefan Roese wrote:
> On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
> >> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
> >> ret = mtd_device_unregister(&flash->mtd);
> >> if (ret)
> >> dev_err(&pdev->dev, "error removing mtd\n");
> >> -
> >> - iounmap(flash->base_addr);
> >> - kfree(flash);
> >> }
> >>
> >> irq = platform_get_irq(pdev, 0);
> >> - free_irq(irq, dev);
> >
> > I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> > strange code - we get irq, without checking for error, and then free it?
> > What is the rationale?
>
> Yes, this seems bogus. platform_get_irq() definitely should be removed
> from spear_smi_remove().
OK, thanks, I've massaged Julia's patch and pushed the following (also
attached) to l2-mtd.git, compile-tested only.
From d44ab8be1a01b83c744d8077a54064e179b0e2b8 Mon Sep 17 00:00:00 2001
From: Julia Lawall <[email protected]>
Date: Sat, 4 Aug 2012 22:36:38 +0200
Subject: [PATCH] mtd: spear_smi: use devm_ functions consistently
Use devm_kzalloc for all calls to kzalloc and not just the first. Use devm
functions for other allocations as well.
Move the call to platform_get_resource(pdev, IORESOURCE_MEM, 0) closer to
where its result is passed to devm_request_and_ioremap to make the lack of
need for a NULL test more evident.
The semantic match that finds the inconsistency is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
@@
*devm_kzalloc(...)
...
*kzalloc(...)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
Signed-off-by: Artem Bityutskiy <[email protected]>
---
drivers/mtd/devices/spear_smi.c | 83 +++++++++------------------------------
1 file changed, 18 insertions(+), 65 deletions(-)
diff --git a/drivers/mtd/devices/spear_smi.c b/drivers/mtd/devices/spear_smi.c
index 421bc65..dcc3c95 100644
--- a/drivers/mtd/devices/spear_smi.c
+++ b/drivers/mtd/devices/spear_smi.c
@@ -827,7 +827,7 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
if (!flash_info)
return -ENODEV;
- flash = kzalloc(sizeof(*flash), GFP_ATOMIC);
+ flash = devm_kzalloc(&pdev->dev, sizeof(*flash), GFP_ATOMIC);
if (!flash)
return -ENOMEM;
flash->bank = bank;
@@ -838,15 +838,13 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
flash_index = spear_smi_probe_flash(dev, bank);
if (flash_index < 0) {
dev_info(&dev->pdev->dev, "smi-nor%d not found\n", bank);
- ret = flash_index;
- goto err_probe;
+ return flash_index;
}
/* map the memory for nor flash chip */
- flash->base_addr = ioremap(flash_info->mem_base, flash_info->size);
- if (!flash->base_addr) {
- ret = -EIO;
- goto err_probe;
- }
+ flash->base_addr = devm_ioremap(&pdev->dev, flash_info->mem_base,
+ flash_info->size);
+ if (!flash->base_addr)
+ return -EIO;
dev->flash[bank] = flash;
flash->mtd.priv = dev;
@@ -888,17 +886,10 @@ static int spear_smi_setup_banks(struct platform_device *pdev,
count);
if (ret) {
dev_err(&dev->pdev->dev, "Err MTD partition=%d\n", ret);
- goto err_map;
+ return ret;
}
return 0;
-
-err_map:
- iounmap(flash->base_addr);
-
-err_probe:
- kfree(flash);
- return ret;
}
/**
@@ -942,13 +933,6 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
}
}
- smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!smi_base) {
- ret = -ENODEV;
- dev_err(&pdev->dev, "invalid smi base address\n");
- goto err;
- }
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = -ENODEV;
@@ -956,26 +940,20 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
goto err;
}
- dev = kzalloc(sizeof(*dev), GFP_ATOMIC);
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_ATOMIC);
if (!dev) {
ret = -ENOMEM;
dev_err(&pdev->dev, "mem alloc fail\n");
goto err;
}
- smi_base = request_mem_region(smi_base->start, resource_size(smi_base),
- pdev->name);
- if (!smi_base) {
- ret = -EBUSY;
- dev_err(&pdev->dev, "request mem region fail\n");
- goto err_mem;
- }
+ smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dev->io_base = ioremap(smi_base->start, resource_size(smi_base));
+ dev->io_base = devm_request_and_ioremap(&pdev->dev, smi_base);
if (!dev->io_base) {
ret = -EIO;
- dev_err(&pdev->dev, "ioremap fail\n");
- goto err_ioremap;
+ dev_err(&pdev->dev, "devm_request_and_ioremap fail\n");
+ goto err;
}
dev->pdev = pdev;
@@ -991,17 +969,18 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
dev->num_flashes = MAX_NUM_FLASH_CHIP;
}
- dev->clk = clk_get(&pdev->dev, NULL);
+ dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk)) {
ret = PTR_ERR(dev->clk);
- goto err_clk;
+ goto err;
}
ret = clk_prepare_enable(dev->clk);
if (ret)
- goto err_clk_prepare_enable;
+ goto err;
- ret = request_irq(irq, spear_smi_int_handler, 0, pdev->name, dev);
+ ret = devm_request_irq(&pdev->dev, irq, spear_smi_int_handler, 0,
+ pdev->name, dev);
if (ret) {
dev_err(&dev->pdev->dev, "SMI IRQ allocation failed\n");
goto err_irq;
@@ -1024,18 +1003,9 @@ static int __devinit spear_smi_probe(struct platform_device *pdev)
return 0;
err_bank_setup:
- free_irq(irq, dev);
platform_set_drvdata(pdev, NULL);
err_irq:
clk_disable_unprepare(dev->clk);
-err_clk_prepare_enable:
- clk_put(dev->clk);
-err_clk:
- iounmap(dev->io_base);
-err_ioremap:
- release_mem_region(smi_base->start, resource_size(smi_base));
-err_mem:
- kfree(dev);
err:
return ret;
}
@@ -1049,11 +1019,8 @@ err:
static int __devexit spear_smi_remove(struct platform_device *pdev)
{
struct spear_smi *dev;
- struct spear_smi_plat_data *pdata;
struct spear_snor_flash *flash;
- struct resource *smi_base;
- int ret;
- int i, irq;
+ int ret, i;
dev = platform_get_drvdata(pdev);
if (!dev) {
@@ -1061,8 +1028,6 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
return -ENODEV;
}
- pdata = dev_get_platdata(&pdev->dev);
-
/* clean up for all nor flash */
for (i = 0; i < dev->num_flashes; i++) {
flash = dev->flash[i];
@@ -1073,21 +1038,9 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
ret = mtd_device_unregister(&flash->mtd);
if (ret)
dev_err(&pdev->dev, "error removing mtd\n");
-
- iounmap(flash->base_addr);
- kfree(flash);
}
- irq = platform_get_irq(pdev, 0);
- free_irq(irq, dev);
-
clk_disable_unprepare(dev->clk);
- clk_put(dev->clk);
- iounmap(dev->io_base);
- kfree(dev);
-
- smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- release_mem_region(smi_base->start, resource_size(smi_base));
platform_set_drvdata(pdev, NULL);
return 0;
--
1.7.10.4
--
Best Regards,
Artem Bityutskiy