Re: [PATCH v2] mtd: orion_nand: Improve error handling inorion_nand_probe

[Emil Goode]

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

On Mon, Jun 10, 2013 at 02:34:57PM +0900, Jingoo Han wrote:
> Monday, June 10, 2013 9:01 AM, Emil Goode wrote:
> > 
> > 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 <emilgoode@xxxxxxxxx>
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
> > index 8fbd002..76b9fba 100644
> > --- a/drivers/mtd/nand/orion_nand.c
> > +++ b/drivers/mtd/nand/orion_nand.c
> > @@ -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");
> 
> 
> > -		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");
> 
> 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.
> 
> 
> > -		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. :)
> 
> 
> > +	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
> 
> 
> > -			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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/