Re: [PATCH v2] mtd: orion_nand: Improve error handling in orion_nand_probe

[Jingoo Han <jg1.han@samsung.com>]

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@gmail.com>
> ---
> 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