mtd: orion_nand: Improve error handling in orion_nand_probe

PATCH In review Emil Goode 2 participants 3 messages Jun 9, 2013 – Jun 10, 2013

Subsystems (3)

Kernel janitors Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Emil Goode

author

2013-06-09 23:58 UTC #0 lore raw

PATCH v2 1/1 mtd: orion_nand: Improve error handling in orion_nand_probe

This patch fixes some issues in the error handling and simplifies the code by converting to devm* functions.

If the kzalloc call fails it is unnecessary to use the label no_res and pass a NULL pointer to kfree. If the devm_kzalloc call fails on line 110 we forget to call iounmap for the previous ioremap call.

The following changes are introduced:
- Convert to devm_kzalloc and remove calls to kfree.
- Convert to devm_ioremap_resource that adds a missing call to *request_mem_region and remove calls to iounmap.
- The devm_ioremap_resource function checks the passed resource so we can remove the NULL check after the platform_get_resource call.

Signed-off-by Emil Goode <[email protected]>

---
This patch is only build tested
v2: Fix change log typo and remove error messages for kzalloc calls

 drivers/mtd/nand/orion_nand.c |   49 +++++++++++++----------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

drivers/mtd/nand/orion_nand.c +15 ~~-34~~

index 8fbd002..76b9fba 100644
@@ -85,35 +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) {
-		printk(KERN_ERR "orion_nand: failed to allocate device structure.\n");
-		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 = ioremap(res->start, resource_size(res));
-	if (!io_base) {
-		printk(KERN_ERR "orion_nand: ioremap failed\n");
-		ret = -EIO;
-		goto no_res;
-	}
+	io_base = devm_ioremap_resource(&pdev->dev, 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) {
-			printk(KERN_ERR "orion_nand: failed to allocate board structure.\n");
-			ret = -ENOMEM;
-			goto no_res;
-		}
+				     GFP_KERNEL);
+		if (!board)
+			return -ENOMEM;
+
 		if (!of_property_read_u32(pdev->dev.of_node, "cle", &val))
 			board->cle = (u8)val;
 		else

@@ -167,7 +156,7 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 
 	if (nand_scan(mtd, 1)) {
 		ret = -ENXIO;
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	mtd->name = "orion_nand";

@@ -176,20 +165,17 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 			board->parts, board->nr_parts);
 	if (ret) {
 		nand_release(mtd);
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	return 0;
 
-no_dev:
+disable_clk:
 	if (!IS_ERR(clk)) {
 		clk_disable_unprepare(clk);
 		clk_put(clk);
 	}
 	platform_set_drvdata(pdev, NULL);
-	iounmap(io_base);
-no_res:
-	kfree(nc);
 
 	return ret;
 }

@@ -197,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);

Signature · 1 line

1.7.10.4

Jingoo Han

2013-06-10 05:34 UTC #1 lore raw

Quoted 28 lines · Emil Goode

This patch fixes some issues in the error handling and simplifies the code by converting to devm* functions.
If the kzalloc call fails it is unnecessary to use the label no_res and pass a NULL pointer to kfree. If the devm_kzalloc call fails on line 110 we forget to call iounmap for the previous ioremap call.
The following changes are introduced:
- Convert to devm_kzalloc and remove calls to kfree.
- Convert to devm_ioremap_resource that adds a missing call to *request_mem_region and remove calls to iounmap.
- The devm_ioremap_resource function checks the passed resource so we can remove the NULL check after the platform_get_resource call.
Signed-off-by Emil Goode <[email protected]>
---
This patch is only build tested
v2: Fix change log typo and remove error messages for kzalloc calls
 drivers/mtd/nand/orion_nand.c |   49 +++++++++++++----------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)
drivers/mtd/nand/orion_nand.c +0 -0
index 8fbd002..76b9fba 100644

drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 90

Quoted 6 lines

drivers/mtd/nand/orion_nand.c +0 -3

@@ -85,35 +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) {
-		printk(KERN_ERR "orion_nand: failed to allocate device structure.\n");

CC'ed Dan Carpenter, Andy Shevchenko

You don't need to remove this error message.
You would replace 'printk(KERN_ERR "orion_nand:...)' with 'dev_err(&pdev->dev,)'.

	dev_err("failed to allocate device structure.\n");

Quoted 9 lines

drivers/mtd/nand/orion_nand.c +5 -3

-		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);

drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 104

Quoted 10 lines

drivers/mtd/nand/orion_nand.c +0 -8

 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		ret = -ENODEV;
-		goto no_res;
-	}
-
-	io_base = ioremap(res->start, resource_size(res));
-	if (!io_base) {
-		printk(KERN_ERR "orion_nand: ioremap failed\n");

Yes, this error message is not necessary. devm_ioremap_resource() provides its own error messages; so all explicit error messages can be removed from the failure code paths.

drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 96

Quoted 4 lines

drivers/mtd/nand/orion_nand.c +1 -3

-		ret = -EIO;
-		goto no_res;
-	}
+	io_base = devm_ioremap_resource(&pdev->dev, res);

How about using devm_ioremap() instead of devm_ioremap_resource()?

Only ioremap() was used previously; however, devm_ioremap_resource() internally calls both devm_request_mem_region() and devm_ioremap().

If it is wrong, please let me know kindly. :)

drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 113

Quoted 8 lines

drivers/mtd/nand/orion_nand.c +2 -3

+	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) {
-			printk(KERN_ERR "orion_nand: failed to allocate board structure.\n");

As above mentioned, you don't need to remove this error message. You would replace 'printk(KERN_ERR "orion_nand:...)' with 'dev_err(&pdev->dev,)'.

	dev_err("failed to allocate board structure.\n");

Best regards,
Jingoo Han

Quoted 60 lines

drivers/mtd/nand/orion_nand.c +7 ~~-15~~

-			ret = -ENOMEM;
-			goto no_res;
-		}
+				     GFP_KERNEL);
+		if (!board)
+			return -ENOMEM;
+
 		if (!of_property_read_u32(pdev->dev.of_node, "cle", &val))
 			board->cle = (u8)val;
 		else

@@ -167,7 +156,7 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 
 	if (nand_scan(mtd, 1)) {
 		ret = -ENXIO;
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	mtd->name = "orion_nand";

@@ -176,20 +165,17 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 			board->parts, board->nr_parts);
 	if (ret) {
 		nand_release(mtd);
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	return 0;
 
-no_dev:
+disable_clk:
 	if (!IS_ERR(clk)) {
 		clk_disable_unprepare(clk);
 		clk_put(clk);
 	}
 	platform_set_drvdata(pdev, NULL);
-	iounmap(io_base);
-no_res:
-	kfree(nc);
 
 	return ret;
 }

@@ -197,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.7.10.4

Emil Goode

author

2013-06-10 10:35 UTC #2 lore raw

Hello Jingoo,

Thank you for the review. There was another discussion and the following patch was sent that converts printk to dev_err.

http://lists.infradead.org/pipermail/linux-mtd/2013-June/047198.html

My conclusion of the discussion was that error messages for kzalloc calls are probably made out of habit and that it's better to rely on the internal error messages of kzalloc.

http://lists.infradead.org/pipermail/linux-mtd/2013-June/047203.html

So instead of sending a second version that applies on top of the "convert printk to dev_*" patch, I decided to remove the error messages for the kzalloc calls.

About the introduced call to devm_request_mem_region that is made inside of devm_ioremap_resource. My understanding is that request_mem_region should be used for all memory mappen I/O to inform the kernel of the I/O adress range that will be used by the driver.

There are other examples in drivers/mtd/nand of devm_ioremap_resource beeing used in probe functions.

Here are some of them:
drivers/mtd/nand/lpc32xx_slc.c
drivers/mtd/nand/fsmc_nand.c
drivers/mtd/nand/davinci_nand.c

Best regards,

Emil

Quoted 164 lines · Jingoo Han

Quoted 28 lines · Emil Goode
This patch fixes some issues in the error handling and simplifies the code by converting to devm* functions.
If the kzalloc call fails it is unnecessary to use the label no_res and pass a NULL pointer to kfree. If the devm_kzalloc call fails on line 110 we forget to call iounmap for the previous ioremap call.
The following changes are introduced:
- Convert to devm_kzalloc and remove calls to kfree.
- Convert to devm_ioremap_resource that adds a missing call to *request_mem_region and remove calls to iounmap.
- The devm_ioremap_resource function checks the passed resource so we can remove the NULL check after the platform_get_resource call.
Signed-off-by Emil Goode <[email protected]>
---
This patch is only build tested
v2: Fix change log typo and remove error messages for kzalloc calls
 drivers/mtd/nand/orion_nand.c |   49 +++++++++++++----------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)
drivers/mtd/nand/orion_nand.c +0 -0
index 8fbd002..76b9fba 100644
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 90
Quoted 6 lines
drivers/mtd/nand/orion_nand.c +0 -3
@@ -85,35 +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) {
-		printk(KERN_ERR "orion_nand: failed to allocate device structure.\n");
CC'ed Dan Carpenter, Andy Shevchenko
You don't need to remove this error message.
You would replace 'printk(KERN_ERR "orion_nand:...)' with 'dev_err(&pdev->dev,)'.
	dev_err("failed to allocate device structure.\n");
Quoted 9 lines
drivers/mtd/nand/orion_nand.c +5 -3
-		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);
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 104
Quoted 10 lines
drivers/mtd/nand/orion_nand.c +0 -8
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		ret = -ENODEV;
-		goto no_res;
-	}
-
-	io_base = ioremap(res->start, resource_size(res));
-	if (!io_base) {
-		printk(KERN_ERR "orion_nand: ioremap failed\n");
Yes, this error message is not necessary. devm_ioremap_resource() provides its own error messages; so all explicit error messages can be removed from the failure code paths.
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 96
Quoted 4 lines
drivers/mtd/nand/orion_nand.c +1 -3
-		ret = -EIO;
-		goto no_res;
-	}
+	io_base = devm_ioremap_resource(&pdev->dev, res);
How about using devm_ioremap() instead of devm_ioremap_resource()?
Only ioremap() was used previously; however, devm_ioremap_resource() internally calls both devm_request_mem_region() and devm_ioremap().
If it is wrong, please let me know kindly. :)
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 113
Quoted 8 lines
drivers/mtd/nand/orion_nand.c +2 -3
+	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) {
-			printk(KERN_ERR "orion_nand: failed to allocate board structure.\n");
As above mentioned, you don't need to remove this error message. You would replace 'printk(KERN_ERR "orion_nand:...)' with 'dev_err(&pdev->dev,)'.
	dev_err("failed to allocate board structure.\n");
Best regards,
Jingoo Han
Quoted 60 lines
drivers/mtd/nand/orion_nand.c +7 ~~-15~~
-			ret = -ENOMEM;
-			goto no_res;
-		}
+				     GFP_KERNEL);
+		if (!board)
+			return -ENOMEM;
+
 		if (!of_property_read_u32(pdev->dev.of_node, "cle", &val))
 			board->cle = (u8)val;
 		else
@@ -167,7 +156,7 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 
 	if (nand_scan(mtd, 1)) {
 		ret = -ENXIO;
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	mtd->name = "orion_nand";
@@ -176,20 +165,17 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 			board->parts, board->nr_parts);
 	if (ret) {
 		nand_release(mtd);
-		goto no_dev;
+		goto disable_clk;
 	}
 
 	return 0;
 
-no_dev:
+disable_clk:
 	if (!IS_ERR(clk)) {
 		clk_disable_unprepare(clk);
 		clk_put(clk);
 	}
 	platform_set_drvdata(pdev, NULL);
-	iounmap(io_base);
-no_res:
-	kfree(nc);
 
 	return ret;
 }
@@ -197,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.7.10.4