LinuxLists.cc - [PATCH] mtd: orion_nand: fix error code path in probe

2014-10-14 14:16:47

Subject: [PATCH] mtd: orion_nand: fix error code path in probe

This replaces kzalloc() and ioremap() calls by
the corresponding devm_ functions in the probe() routine,
which automatically release the corresponding resources
when probe() fails or when the device is removed.

This simplifies the error management code and
fixes a bug reported by "make coccicheck":

if "board = devm_kzalloc()" fails, the probe()
function jumps incorrectly to label "no_res" and
therefore returns without running "iounmap()"

Signed-off-by: Michael Opdenacker <[email protected]>
---
drivers/mtd/nand/orion_nand.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
index 471b4df3a5ac..a9c2bde16c25 100644
--- a/drivers/mtd/nand/orion_nand.c
+++ b/drivers/mtd/nand/orion_nand.c
@@ -19,7 +19,7 @@
#include <linux/mtd/partitions.h>
#include <linux/clk.h>
#include <linux/err.h>
-#include <asm/io.h>
+#include <linux/io.h>
#include <asm/sizes.h>
#include <linux/platform_data/mtd-orion_nand.h>

@@ -85,32 +85,30 @@ static int __init orion_nand_probe(struct platform_device *pdev)
int ret = 0;
u32 val = 0;

- nc = kzalloc(sizeof(struct nand_chip) + sizeof(struct mtd_info), GFP_KERNEL);
+ nc = devm_kzalloc(&pdev->dev,
+ sizeof(struct nand_chip) + sizeof(struct mtd_info),
+ GFP_KERNEL);
if (!nc) {
- ret = -ENOMEM;
- goto no_res;
+ return -ENOMEM;
}
mtd = (struct mtd_info *)(nc + 1);

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
- ret = -ENODEV;
- goto no_res;
+ return -ENODEV;
}

- io_base = ioremap(res->start, resource_size(res));
+ io_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
if (!io_base) {
dev_err(&pdev->dev, "ioremap failed\n");
- ret = -EIO;
- goto no_res;
+ return -EIO;
}

if (pdev->dev.of_node) {
board = devm_kzalloc(&pdev->dev, sizeof(struct orion_nand_data),
GFP_KERNEL);
if (!board) {
- ret = -ENOMEM;
- goto no_res;
+ return -ENOMEM;
}
if (!of_property_read_u32(pdev->dev.of_node, "cle", &val))
board->cle = (u8)val;
@@ -185,9 +183,6 @@ no_dev:
clk_disable_unprepare(clk);
clk_put(clk);
}
- iounmap(io_base);
-no_res:
- kfree(nc);

return ret;
}
@@ -195,15 +190,10 @@ no_res:
static int orion_nand_remove(struct platform_device *pdev)
{
struct mtd_info *mtd = platform_get_drvdata(pdev);
- struct nand_chip *nc = mtd->priv;
struct clk *clk;

nand_release(mtd);

- iounmap(nc->IO_ADDR_W);
-
- kfree(nc);
-
clk = clk_get(&pdev->dev, NULL);
if (!IS_ERR(clk)) {
clk_disable_unprepare(clk);
--
1.9.1

2014-10-14 21:36:08

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH] mtd: orion_nand: fix error code path in probe

On Tue, Oct 14, 2014 at 04:16:38PM +0200, Michael Opdenacker wrote:
> This replaces kzalloc() and ioremap() calls by
> the corresponding devm_ functions in the probe() routine,
> which automatically release the corresponding resources
> when probe() fails or when the device is removed.
>
> This simplifies the error management code and
> fixes a bug reported by "make coccicheck":
>
> if "board = devm_kzalloc()" fails, the probe()
> function jumps incorrectly to label "no_res" and
> therefore returns without running "iounmap()"
>
> Signed-off-by: Michael Opdenacker <[email protected]>
> ---
> drivers/mtd/nand/orion_nand.c | 28 +++++++++-------------------
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
> index 471b4df3a5ac..a9c2bde16c25 100644
> --- a/drivers/mtd/nand/orion_nand.c
> +++ b/drivers/mtd/nand/orion_nand.c
> @@ -19,7 +19,7 @@
> #include <linux/mtd/partitions.h>
> #include <linux/clk.h>
> #include <linux/err.h>
> -#include <asm/io.h>
> +#include <linux/io.h>
> #include <asm/sizes.h>
> #include <linux/platform_data/mtd-orion_nand.h>
>
> @@ -85,32 +85,30 @@ static int __init orion_nand_probe(struct platform_device *pdev)
> int ret = 0;
> u32 val = 0;
>
> - nc = kzalloc(sizeof(struct nand_chip) + sizeof(struct mtd_info), GFP_KERNEL);
> + nc = devm_kzalloc(&pdev->dev,
> + sizeof(struct nand_chip) + sizeof(struct mtd_info),
> + GFP_KERNEL);
> if (!nc) {
> - ret = -ENOMEM;
> - goto no_res;
> + return -ENOMEM;
> }
> mtd = (struct mtd_info *)(nc + 1);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> - ret = -ENODEV;
> - goto no_res;
> + return -ENODEV;
> }
>
> - io_base = ioremap(res->start, resource_size(res));
> + io_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> if (!io_base) {
> dev_err(&pdev->dev, "ioremap failed\n");
> - ret = -EIO;
> - goto no_res;
> + return -EIO;
> }

Hi Michael

It is quite a common pattern to use:

res = platform_get_resource(dev, IORESOURCE_MEM, 0);
c->membase = devm_ioremap_resource(&dev->dev, res);
if (IS_ERR(c->membase))
return PTR_ERR(c->membase)

which is more compact.

>
> if (pdev->dev.of_node) {
> board = devm_kzalloc(&pdev->dev, sizeof(struct orion_nand_data),
> GFP_KERNEL);
> if (!board) {
> - ret = -ENOMEM;
> - goto no_res;
> + return -ENOMEM;
> }

Doesn't this now break the coding style? No need to have the {} since
it is a single statement.

Andrew

2014-10-15 21:41:23

by Ezequiel Garcia

[permalink] [raw]

Subject: Re: [PATCH] mtd: orion_nand: fix error code path in probe

On 14 Oct 11:35 PM, Andrew Lunn wrote:
> On Tue, Oct 14, 2014 at 04:16:38PM +0200, Michael Opdenacker wrote:
> > This replaces kzalloc() and ioremap() calls by
> > the corresponding devm_ functions in the probe() routine,
> > which automatically release the corresponding resources
> > when probe() fails or when the device is removed.
> >
> > This simplifies the error management code and
> > fixes a bug reported by "make coccicheck":
> >
> > if "board = devm_kzalloc()" fails, the probe()
> > function jumps incorrectly to label "no_res" and
> > therefore returns without running "iounmap()"
> >
> > Signed-off-by: Michael Opdenacker <[email protected]>
> > ---
> > drivers/mtd/nand/orion_nand.c | 28 +++++++++-------------------
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
> > index 471b4df3a5ac..a9c2bde16c25 100644
> > --- a/drivers/mtd/nand/orion_nand.c
> > +++ b/drivers/mtd/nand/orion_nand.c
> > @@ -19,7 +19,7 @@
> > #include <linux/mtd/partitions.h>
> > #include <linux/clk.h>
> > #include <linux/err.h>
> > -#include <asm/io.h>
> > +#include <linux/io.h>
> > #include <asm/sizes.h>
> > #include <linux/platform_data/mtd-orion_nand.h>
> >
> > @@ -85,32 +85,30 @@ static int __init orion_nand_probe(struct platform_device *pdev)
> > int ret = 0;
> > u32 val = 0;
> >
> > - nc = kzalloc(sizeof(struct nand_chip) + sizeof(struct mtd_info), GFP_KERNEL);
> > + nc = devm_kzalloc(&pdev->dev,
> > + sizeof(struct nand_chip) + sizeof(struct mtd_info),
> > + GFP_KERNEL);
> > if (!nc) {
> > - ret = -ENOMEM;
> > - goto no_res;
> > + return -ENOMEM;
> > }
> > mtd = (struct mtd_info *)(nc + 1);
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > - ret = -ENODEV;
> > - goto no_res;
> > + return -ENODEV;
> > }
> >
> > - io_base = ioremap(res->start, resource_size(res));
> > + io_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > if (!io_base) {
> > dev_err(&pdev->dev, "ioremap failed\n");
> > - ret = -EIO;
> > - goto no_res;
> > + return -EIO;
> > }
>
> Hi Michael
>
> It is quite a common pattern to use:
>
> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> c->membase = devm_ioremap_resource(&dev->dev, res);
> if (IS_ERR(c->membase))
> return PTR_ERR(c->membase)
>
> which is more compact.
>

Be careful with this. devm_ioremap and devm_ioremap_resource are not
the same thing, as the former requests the region as well.

It can break things if the region is shared across several drivers.
I don't think this is the case, so in fact adding the request is correct,
but it's a more intrusive change than just "code cleanup".

--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

2014-10-16 04:43:32

by Michael Opdenacker

[permalink] [raw]

Subject: Re: [PATCH] mtd: orion_nand: fix error code path in probe

Andrew, Ezequiel,

Many thanks for your review!

On 10/15/2014 11:39 PM, Ezequiel Garcia wrote:
> On 14 Oct 11:35 PM, Andrew Lunn wrote:
>
>> Hi Michael
>>
>> It is quite a common pattern to use:
>>
>> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> c->membase = devm_ioremap_resource(&dev->dev, res);
>> if (IS_ERR(c->membase))
>> return PTR_ERR(c->membase)
>>
>> which is more compact.

I like it, thanks for the suggestion!

>>
> Be careful with this. devm_ioremap and devm_ioremap_resource are not
> the same thing, as the former requests the region as well.
>
> It can break things if the region is shared across several drivers.
> I don't think this is the case, so in fact adding the request is correct,
> but it's a more intrusive change than just "code cleanup".

Right. If I understand correctly, requesting the region should always be
done anyway, so this should be a welcome change.

What Andrew suggests also changes the return value: -ENOMEM instead of
-EIO, though it should be more standard. This could have side effects too!

I'll post a V2 right away.

Thanks again!

Cheers,

Michael.

--
Michael Opdenacker, CEO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
+33 484 258 098

2014-10-16 04:58:45

by Michael Opdenacker

[permalink] [raw]

Subject: [PATCH v2] mtd: orion_nand: fix error code path in probe

This replaces kzalloc() and ioremap() calls by devm_ functions
in the probe() routine, which automatically release the corresponding
resources when probe() fails or when the device is removed.

This simplifies simplifies the error management code, and brings
the below improvements or changes:

A. Fixing a bug reported by "make coccicheck":

If "board = devm_kzalloc()" fails, the probe() function jumps
incorrectly to label "no_res" and therefore returns without
running iounmap().

B. Requesting the memory region

Using devm_ioremap_resource() makes the probe() function request
the corresponding memory region before running ioremap(), as
it is supposed to do.

C. Standardizing the error codes:

The use of devm_ioremap_resource() changes the return value:
* -ENOMEM instead of -EIO in case of ioremap() failure,
* -EINVAL instead of -ENODEV in case of platform_get_resource()
failure.

Signed-off-by: Michael Opdenacker <[email protected]>
---
drivers/mtd/nand/orion_nand.c | 39 +++++++++++----------------------------
1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
index 471b4df3a5ac..0b49d5d34c50 100644
--- a/drivers/mtd/nand/orion_nand.c
+++ b/drivers/mtd/nand/orion_nand.c
@@ -19,7 +19,7 @@
#include <linux/mtd/partitions.h>
#include <linux/clk.h>
#include <linux/err.h>
-#include <asm/io.h>
+#include <linux/io.h>
#include <asm/sizes.h>
#include <linux/platform_data/mtd-orion_nand.h>

@@ -85,33 +85,24 @@ static int __init orion_nand_probe(struct platform_device *pdev)
int ret = 0;
u32 val = 0;

- nc = kzalloc(sizeof(struct nand_chip) + sizeof(struct mtd_info), GFP_KERNEL);
- if (!nc) {
- ret = -ENOMEM;
- goto no_res;
- }
+ nc = devm_kzalloc(&pdev->dev,
+ sizeof(struct nand_chip) + sizeof(struct mtd_info),
+ GFP_KERNEL);
+ if (!nc)
+ return -ENOMEM;
mtd = (struct mtd_info *)(nc + 1);

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- ret = -ENODEV;
- goto no_res;
- }
+ io_base = devm_ioremap_resource(&pdev->dev, res);

- io_base = ioremap(res->start, resource_size(res));
- if (!io_base) {
- dev_err(&pdev->dev, "ioremap failed\n");
- ret = -EIO;
- goto no_res;
- }
+ if (IS_ERR(io_base))
+ return PTR_ERR(io_base);

if (pdev->dev.of_node) {
board = devm_kzalloc(&pdev->dev, sizeof(struct orion_nand_data),
GFP_KERNEL);
- if (!board) {
- ret = -ENOMEM;
- goto no_res;
- }
+ if (!board)
+ return -ENOMEM;
if (!of_property_read_u32(pdev->dev.of_node, "cle", &val))
board->cle = (u8)val;
else
@@ -185,9 +176,6 @@ no_dev:
clk_disable_unprepare(clk);
clk_put(clk);
}
- iounmap(io_base);
-no_res:
- kfree(nc);

return ret;
}
@@ -195,15 +183,10 @@ no_res:
static int orion_nand_remove(struct platform_device *pdev)
{
struct mtd_info *mtd = platform_get_drvdata(pdev);
- struct nand_chip *nc = mtd->priv;
struct clk *clk;

nand_release(mtd);

- iounmap(nc->IO_ADDR_W);
-
- kfree(nc);
-
clk = clk_get(&pdev->dev, NULL);
if (!IS_ERR(clk)) {
clk_disable_unprepare(clk);
--
1.9.1

2014-10-16 05:06:16

by Michael Opdenacker

[permalink] [raw]

Subject: Re: [PATCH] mtd: orion_nand: fix error code path in probe

On 10/14/2014 11:35 PM, Andrew Lunn wrote:
>
>
> if (pdev->dev.of_node) {
> board = devm_kzalloc(&pdev->dev, sizeof(struct orion_nand_data),
> GFP_KERNEL);
> if (!board) {
> - ret = -ENOMEM;
> - goto no_res;
> + return -ENOMEM;
> }
> Doesn't this now break the coding style? No need to have the {} since
> it is a single statement.

Right, I've checked Documentation/CodingStyle, and this is just
recommended, not absolutely required. That could be the reason why
check_patch.pl didn't complain.

My v2 changes this.

Thanks!

Michael.

--
Michael Opdenacker, CEO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
+33 484 258 098

2014-10-16 07:39:34

by Jingoo Han

[permalink] [raw]

Subject: Re: [PATCH v2] mtd: orion_nand: fix error code path in probe

On Thursday, October 16, 2014 1:59 PM, Michael Opdenacker wrote:
>
> This replaces kzalloc() and ioremap() calls by devm_ functions
> in the probe() routine, which automatically release the corresponding
> resources when probe() fails or when the device is removed.
>
> This simplifies simplifies the error management code, and brings
> the below improvements or changes:
>
> A. Fixing a bug reported by "make coccicheck":
>
> If "board = devm_kzalloc()" fails, the probe() function jumps
> incorrectly to label "no_res" and therefore returns without
> running iounmap().
>
> B. Requesting the memory region
>
> Using devm_ioremap_resource() makes the probe() function request
> the corresponding memory region before running ioremap(), as
> it is supposed to do.
>
> C. Standardizing the error codes:
>
> The use of devm_ioremap_resource() changes the return value:
> * -ENOMEM instead of -EIO in case of ioremap() failure,
> * -EINVAL instead of -ENODEV in case of platform_get_resource()
> failure.
>
> Signed-off-by: Michael Opdenacker <[email protected]>

Reviewed-by: Jingoo Han <[email protected]>

Best regards,
Jingoo Han

> ---
> drivers/mtd/nand/orion_nand.c | 39 +++++++++++----------------------------
> 1 file changed, 11 insertions(+), 28 deletions(-)

2014-10-18 19:36:56

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH v2] mtd: orion_nand: fix error code path in probe

On Thu, Oct 16, 2014 at 06:58:35AM +0200, Michael Opdenacker wrote:
> This replaces kzalloc() and ioremap() calls by devm_ functions
> in the probe() routine, which automatically release the corresponding
> resources when probe() fails or when the device is removed.
>
> This simplifies simplifies the error management code, and brings
> the below improvements or changes:
>
> A. Fixing a bug reported by "make coccicheck":
>
> If "board = devm_kzalloc()" fails, the probe() function jumps
> incorrectly to label "no_res" and therefore returns without
> running iounmap().
>
> B. Requesting the memory region
>
> Using devm_ioremap_resource() makes the probe() function request
> the corresponding memory region before running ioremap(), as
> it is supposed to do.
>
> C. Standardizing the error codes:
>
> The use of devm_ioremap_resource() changes the return value:
> * -ENOMEM instead of -EIO in case of ioremap() failure,
> * -EINVAL instead of -ENODEV in case of platform_get_resource()
> failure.
>
> Signed-off-by: Michael Opdenacker <[email protected]>

Acked-by: Andrew Lunn <[email protected]>

I wanted to test it, but i don't have easy access to a device using
nand. All mine are SPI based :-(

Andrew

2014-10-22 08:49:52

by Brian Norris

[permalink] [raw]