| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 10 Jun 2013 14:34:57 +0900
From: Jingoo Han <jg1.han@...sung.com>
To: 'Emil Goode' <emilgoode@...il.com>
Cc: David Woodhouse <dwmw2@...radead.org>,
Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
Andrew Lunn <andrew@...n.ch>,
Bill Pemberton <wfp5p@...ginia.edu>,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
kernel-janitors@...r.kernel.org, Jingoo Han <jg1.han@...sung.com>,
'Dan Carpenter' <dan.carpenter@...cle.com>,
'Andy Shevchenko' <andy.shevchenko@...il.com>
Subject: Re: [PATCH v2] mtd: orion_nand: Improve error handling in
orion_nand_probe
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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists