sbus: Introduce the use of the managed version of kzalloc
PATCH 1/1 sbus: Introduce the use of the managed version of kzalloc
This patch moves data allocated using kzalloc to managed data allocated using devm_kzalloc and cleans now unnecessary kfrees in probe and remove functions. The header file is added to make the devm function explicitly available. Also, unnecessary assignment statements and labels are removed.
The following Coccinelle semantic patch was used for making a part of the change:
@platform@
identifier p, probefn, removefn;
@@
struct platform_driver p = {
.probe = probefn,
.remove = removefn,
};
@prb@
identifier platform.probefn, pdev;
expression e, e1, e2;
@@probefn(struct platform_device *pdev, ...) {
<+...
- e = kzalloc(e1, e2)
+ e = devm_kzalloc(&pdev->dev, e1, e2)
...
?-kfree(e);
...+>
}
@rem depends on prb@
identifier platform.removefn;
expression e;
@@
removefn(...) {
<...
- kfree(e);
...>
}
---
Not compile tested due to incompatible architecture.
To send to: "David S. Miller" <[email protected]>,[email protected],[email protected]
drivers/sbus/char/display7seg.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
drivers/sbus/char/display7seg.c +8 -15
@@ -20,6 +20,7 @@#include <asm/io.h> #include <asm/display7seg.h> +#include <linux/device.h> #define D7S_MINOR 193#define DRIVER_NAME "d7s"
@@ -173,22 +174,21 @@ static struct miscdevice d7s_miscdev = {static int d7s_probe (struct platform_device *op) {struct device_node *opts; -int err = -EINVAL ; +int err;struct d7s *p;u8 regs;if (d7s_device) - goto out; +return -EINVAL ; - p =kzalloc (sizeof (*p),GFP_KERNEL ); - err = -ENOMEM ; + p =devm_kzalloc (&op->dev,sizeof (*p),GFP_KERNEL );if (!p) - goto out; +return -ENOMEM ; p->regs =of_ioremap (&op->resource[0 ],0 ,sizeof (u8),"d7s" );if (!p->regs) { printk(KERN_ERR PFX "Cannot map chip registers\n"); - goto out_free; +return -ENOMEM ; } err =misc_register (&d7s_miscdev);
@@ -222,17 +222,11 @@ static int d7s_probe(struct platform_device *op)dev_set_drvdata (&op->dev, p); d7s_device = p; - err =0 ; - -out: -return err; +return 0 ; out_iounmap:of_iounmap (&op->resource[0 ], p->regs,sizeof (u8)); - -out_free: -kfree (p); - goto out; +return err; }static int d7s_remove (struct platform_device *op)
@@ -251,7 +245,6 @@ static int d7s_remove(struct platform_device *op)misc_deregister (&d7s_miscdev);of_iounmap (&op->resource[0 ], p->regs,sizeof (u8)); -kfree (p);return 0 ; }
Signature · 1 line
1.9.1
Hi Himangi.
Quoted 8 lines · Himangi Saraogi
This patch moves data allocated using kzalloc to managed data allocated using
devm_kzallocand cleans now unnecessary kfrees in probe and remove functions. The header file is added to make the devm function explicitly available. Also, unnecessary assignment statements and labels are removed.---
Not compile tested due to incompatible architecture.
I can confirm that i builds.
Quoted 33 lines
To send to: "David S. Miller" <[email protected]>,[email protected],[email protected]
drivers/sbus/char/display7seg.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)drivers/sbus/char/display7seg.c +3
-4@@ -20,6 +20,7 @@#include <asm/io.h> #include <asm/display7seg.h> +#include <linux/device.h> #define D7S_MINOR 193#define DRIVER_NAME "d7s"@@ -173,22 +174,21 @@ static struct miscdevice d7s_miscdev = {static int d7s_probe (struct platform_device *op) {struct device_node *opts; -int err = -EINVAL ; +int err;struct d7s *p;u8 regs;if (d7s_device) - goto out; +return -EINVAL ; - p =kzalloc (sizeof (*p),GFP_KERNEL ); - err = -ENOMEM ;
drivers/sbus/char/display7seg.c static struct miscdevice d7s_miscdev = { · line 191
Quoted 10 lines
drivers/sbus/char/display7seg.c +3
-2+ p =devm_kzalloc (&op->dev,sizeof (*p),GFP_KERNEL );if (!p) - goto out; +return -ENOMEM ; p->regs =of_ioremap (&op->resource[0 ],0 ,sizeof (u8),"d7s" );if (!p->regs) { printk(KERN_ERR PFX "Cannot map chip registers\n"); - goto out_free; +return -ENOMEM ;
Can you please explain how it can be correct to return "-ENOMEM", when we fail to map the chip registers.
Before your patch this resulted in "-EINVAL" - so this is a change in behaviour.
Quoted 11 lines
drivers/sbus/char/display7seg.c +0
-4} err =misc_register (&d7s_miscdev);@@ -222,17 +222,11 @@ static int d7s_probe(struct platform_device *op)dev_set_drvdata (&op->dev, p); d7s_device = p; - err =0 ; - -out: -return err;
drivers/sbus/char/display7seg.c static int d7s_probe(struct platform_device *op) · line 230
Quoted 10 lines
drivers/sbus/char/display7seg.c +2
-4+return 0 ; out_iounmap:of_iounmap (&op->resource[0 ], p->regs,sizeof (u8)); - -out_free: -kfree (p); - goto out; +return err; }
It is not am improvement IMO that you replace the nice goto based error handling with random return points in the code.
Sam
Quoted 8 lines · Sam Ravnborg
Hi Himangi.
Quoted 5 lines · Himangi Saraogi
This patch moves data allocated using kzalloc to managed data allocated using
devm_kzallocand cleans now unnecessary kfrees in probe and remove functions. The header file is added to make the devm function explicitly available. Also, unnecessary assignment statements and labels are removed.
Another note.
This patch touches only the display7seg dirver, so please prefix your patch with "display7seg: bla bla"
Sam
Quoted 52 lines · Sam Ravnborg
Hi Himangi.
Quoted 8 lines · Himangi Saraogi
This patch moves data allocated using kzalloc to managed data allocated using
devm_kzallocand cleans now unnecessary kfrees in probe and remove functions. The header file is added to make the devm function explicitly available. Also, unnecessary assignment statements and labels are removed.---
Not compile tested due to incompatible architecture.I can confirm that i builds.
Quoted 38 lines
To send to: "David S. Miller" <[email protected]>,[email protected],[email protected]
drivers/sbus/char/display7seg.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)drivers/sbus/char/display7seg.c +5
-5@@ -20,6 +20,7 @@#include <asm/io.h> #include <asm/display7seg.h> +#include <linux/device.h> #define D7S_MINOR 193#define DRIVER_NAME "d7s"@@ -173,22 +174,21 @@ static struct miscdevice d7s_miscdev = {static int d7s_probe (struct platform_device *op) {struct device_node *opts; -int err = -EINVAL ; +int err;struct d7s *p;u8 regs;if (d7s_device) - goto out; +return -EINVAL ; - p =kzalloc (sizeof (*p),GFP_KERNEL ); - err = -ENOMEM ; + p =devm_kzalloc (&op->dev,sizeof (*p),GFP_KERNEL );if (!p) - goto out; +return -ENOMEM ;
Quoted 9 lines
drivers/sbus/char/display7seg.c static struct miscdevice d7s_miscdev = { · line 191
Quoted 5 lines
drivers/sbus/char/display7seg.c +1
-1p->regs =of_ioremap (&op->resource[0 ],0 ,sizeof (u8),"d7s" );if (!p->regs) { printk(KERN_ERR PFX "Cannot map chip registers\n"); - goto out_free; +return -ENOMEM ;Can you please explain how it can be correct to return "
-ENOMEM", when we fail to map the chip registers.
Before your patch this resulted in "-EINVAL" - so this is a change in behaviour.
I don't see it. It looks to me that it returned ENOMEM, due to the assignment after the kzalloc.
julia
Quoted 26 lines
Quoted 11 lines
drivers/sbus/char/display7seg.c +0
-4} err =misc_register (&d7s_miscdev);@@ -222,17 +222,11 @@ static int d7s_probe(struct platform_device *op)dev_set_drvdata (&op->dev, p); d7s_device = p; - err =0 ; - -out: -return err;drivers/sbus/char/display7seg.c static int d7s_probe(struct platform_device *op) · line 230
Quoted 10 lines
drivers/sbus/char/display7seg.c +2
-4+return 0 ; out_iounmap:of_iounmap (&op->resource[0 ], p->regs,sizeof (u8)); - -out_free: -kfree (p); - goto out; +return err; }It is not am improvement IMO that you replace the nice goto based error handling with random return points in the code.
Sam