[PATCH v3 00/11] Fix NAND controllers probe functions error path

[PATCH v3 00/11] Fix NAND controllers probe functions error path

[Miquel Raynal]

Hello all,

In order to remove the limitation that prevents dynamic allocations
during the identification phase (nand_scan_ident()), we need to get rid
of the nand_scan_ident()/nand_scan_tail() calls from drivers and only
export nand_scan().

This series prepares the migration to nand_scan() by first fixing (and
enhancing some times) the probe function error path of the drivers that
do not use nand_scan() yet.

The three main problems addressed in the series are:
0/ unproper error path (fuzzy, styling issues, undescriptive goto's).
1/ a wrong error path that does not free/disable the resources
   correctly.
2/ nand_cleanup() is not called upon error after a successful
   nand_scan_tail().
3/ nand_release() is called instead of nand_cleanup(). nand_release()
   does call nand_cleanup() and also mtd_device_unregister(), which
   should not be called while mtd_device_register() as succeeded.

Thanks,
Miquèl

Changes since v2:
=================
 * Reworked the few remaining patches that needed small changes.
 * docg4: changed the commit message as proposed.
 * fsl_elbc/fsl_ifc: moved the wrong nand_release() out of the
   'remove' function to avoid double free possible issues.
 * fsmc/hisi504/lpc32xx_mlc/lpc32xx_slc: split the changes in two
   commits, one to fix the goto labels/style, one that actually fixes
   the function.

Changes since v1:
=================
 * Extracted only the patches fixing the error path from the first huge
   series (50+ patches) that also converted the drivers to nand_scan().
 * Changed wrong nand_release() calls to nand_cleanup().


Miquel Raynal (11):
  mtd: rawnand: docg4: fix the probe function error path
  mtd: rawnand: fsl_elbc: fix probe function error path
  mtd: rawnand: fsl_ifc: fix probe function error path
  mtd: rawnand: fsmc: clean the probe function style
  mtd: rawnand: fsmc: fix the probe function error path
  mtd: rawnand: hisi504: clean the probe function error path
  mtd: rawnand: hisi504: fix the probe function error path
  mtd: rawnand: lpc32xx_mlc: clean the probe function
  mtd: rawnand: lpc32xx_mlc: fix the probe function error path
  mtd: rawnand: lpc32xx_slc: clean the probe function
  mtd: rawnand: lpc32xx_slc: fix the probe function error path

 drivers/mtd/nand/raw/docg4.c         | 22 +++++++++++----------
 drivers/mtd/nand/raw/fsl_elbc_nand.c | 13 ++++++++----
 drivers/mtd/nand/raw/fsl_ifc_nand.c  | 12 +++++++++---
 drivers/mtd/nand/raw/fsmc_nand.c     | 27 +++++++++++++------------
 drivers/mtd/nand/raw/hisi504_nand.c  | 35 ++++++++++++---------------------
 drivers/mtd/nand/raw/lpc32xx_mlc.c   | 38 +++++++++++++++++++-----------------
 drivers/mtd/nand/raw/lpc32xx_slc.c   | 26 ++++++++++++------------
 7 files changed, 91 insertions(+), 82 deletions(-)

-- 
2.14.1

[PATCH v3 01/11] mtd: rawnand: docg4: fix the probe function error path

[Miquel Raynal]

nand_release() should not be called on an MTD device that has not been
registered. While it should work thanks to the checks done in
mtd_device_unregister() it's a bad practice to cleanup/release
something that has not previously been initialized/allocated.

Rework the error path to follow this rule.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/docg4.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/docg4.c b/drivers/mtd/nand/raw/docg4.c
index 1314aa99b9ab..bb96cb33cd6b 100644
--- a/drivers/mtd/nand/raw/docg4.c
+++ b/drivers/mtd/nand/raw/docg4.c
@@ -1341,7 +1341,7 @@ static int __init probe_docg4(struct platform_device *pdev)
 	nand = kzalloc(len, GFP_KERNEL);
 	if (nand == NULL) {
 		retval = -ENOMEM;
-		goto fail_unmap;
+		goto unmap;
 	}
 
 	mtd = nand_to_mtd(nand);
@@ -1357,7 +1357,7 @@ static int __init probe_docg4(struct platform_device *pdev)
 	doc->bch = init_bch(DOCG4_M, DOCG4_T, DOCG4_PRIMITIVE_POLY);
 	if (doc->bch == NULL) {
 		retval = -EINVAL;
-		goto fail;
+		goto free_nand;
 	}
 
 	platform_set_drvdata(pdev, doc);
@@ -1366,30 +1366,32 @@ static int __init probe_docg4(struct platform_device *pdev)
 	retval = read_id_reg(mtd);
 	if (retval == -ENODEV) {
 		dev_warn(dev, "No diskonchip G4 device found.\n");
-		goto fail;
+		goto free_bch;
 	}
 
 	retval = nand_scan_tail(mtd);
 	if (retval)
-		goto fail;
+		goto free_bch;
 
 	retval = read_factory_bbt(mtd);
 	if (retval)
-		goto fail;
+		goto cleanup_nand;
 
 	retval = mtd_device_parse_register(mtd, part_probes, NULL, NULL, 0);
 	if (retval)
-		goto fail;
+		goto cleanup_nand;
 
 	doc->mtd = mtd;
+
 	return 0;
 
-fail:
-	nand_release(mtd); /* deletes partitions and mtd devices */
+cleanup_nand:
+	nand_cleanup(nand);
+free_bch:
 	free_bch(doc->bch);
+free_nand:
 	kfree(nand);
-
-fail_unmap:
+unmap:
 	iounmap(virtadr);
 
 	return retval;
-- 
2.14.1

Re: [PATCH v3 01/11] mtd: rawnand: docg4: fix the probe function error path

[Miquel Raynal]

Hello,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Sat, 21 Apr 2018
20:00:33 +0200:

> nand_release() should not be called on an MTD device that has not been
> registered. While it should work thanks to the checks done in
> mtd_device_unregister() it's a bad practice to cleanup/release
> something that has not previously been initialized/allocated.
> 
> Rework the error path to follow this rule.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

This patch is marked applied in patchwork while it is not present
anywhere in the official trees and has probably been forgotten. Applied
on nand/next.

Regards,
Miquèl

[PATCH v3 02/11] mtd: rawnand: fsl_elbc: fix probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup().
The helper mtd_device_parse_register() returns an error code that should
be checked and nand_cleanup() called accordingly.

However, in this driver, fsl_elbc_chip_remove() which is called upon
error already triggers a nand_release() which is wrong, because a
nand_release() should be triggered only if an mtd_register() succeeded.

Move the nand_release() call out of the fsl_elbc_chip_remove() and put
it back in the *_remove() hook.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/fsl_elbc_nand.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_elbc_nand.c b/drivers/mtd/nand/raw/fsl_elbc_nand.c
index d28df991c73c..51f0b340bc0d 100644
--- a/drivers/mtd/nand/raw/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_elbc_nand.c
@@ -813,8 +813,6 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
 	struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = priv->ctrl->nand;
 	struct mtd_info *mtd = nand_to_mtd(&priv->chip);
 
-	nand_release(mtd);
-
 	kfree(mtd->name);
 
 	if (priv->vbase)
@@ -926,15 +924,20 @@ static int fsl_elbc_nand_probe(struct platform_device *pdev)
 
 	/* First look for RedBoot table or partitions on the command
 	 * line, these take precedence over device tree information */
-	mtd_device_parse_register(mtd, part_probe_types, NULL,
-				  NULL, 0);
+	ret = mtd_device_parse_register(mtd, part_probe_types, NULL, NULL, 0);
+	if (ret)
+		goto cleanup_nand;
 
 	pr_info("eLBC NAND device at 0x%llx, bank %d\n",
 		(unsigned long long)res.start, priv->bank);
+
 	return 0;
 
+cleanup_nand:
+	nand_cleanup(&priv->chip);
 err:
 	fsl_elbc_chip_remove(priv);
+
 	return ret;
 }
 
@@ -942,7 +945,9 @@ static int fsl_elbc_nand_remove(struct platform_device *pdev)
 {
 	struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
 	struct fsl_elbc_mtd *priv = dev_get_drvdata(&pdev->dev);
+	struct mtd_info *mtd = nand_to_mtd(&priv->chip);
 
+	nand_release(mtd);
 	fsl_elbc_chip_remove(priv);
 
 	mutex_lock(&fsl_elbc_nand_mutex);
-- 
2.14.1

[PATCH v3 03/11] mtd: rawnand: fsl_ifc: fix probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup().
The helper mtd_device_parse_register() returns an error code that should
be checked and nand_cleanup() called accordingly.

However, in this driver, fsl_ifc_chip_remove() which is called upon
error already triggers a nand_release() which is wrong, because a
nand_release() should be triggered only if an mtd_register() succeeded.

Move the nand_release() call out of the fsl_ifc_chip_remove() and put it
back in the *_remove() hook.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 61aae0224078..00a609d4473e 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -924,8 +924,6 @@ static int fsl_ifc_chip_remove(struct fsl_ifc_mtd *priv)
 {
 	struct mtd_info *mtd = nand_to_mtd(&priv->chip);
 
-	nand_release(mtd);
-
 	kfree(mtd->name);
 
 	if (priv->vbase)
@@ -1059,21 +1057,29 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 
 	/* First look for RedBoot table or partitions on the command
 	 * line, these take precedence over device tree information */
-	mtd_device_parse_register(mtd, part_probe_types, NULL, NULL, 0);
+	ret = mtd_device_parse_register(mtd, part_probe_types, NULL, NULL, 0);
+	if (ret)
+		goto cleanup_nand;
 
 	dev_info(priv->dev, "IFC NAND device at 0x%llx, bank %d\n",
 		 (unsigned long long)res.start, priv->bank);
+
 	return 0;
 
+cleanup_nand:
+	nand_cleanup(&priv->chip);
 err:
 	fsl_ifc_chip_remove(priv);
+
 	return ret;
 }
 
 static int fsl_ifc_nand_remove(struct platform_device *dev)
 {
 	struct fsl_ifc_mtd *priv = dev_get_drvdata(&dev->dev);
+	struct mtd_info *mtd = nand_to_mtd(&priv->chip);
 
+	nand_release(mtd);
 	fsl_ifc_chip_remove(priv);
 
 	mutex_lock(&fsl_ifc_nand_mutex);
-- 
2.14.1

[PATCH v3 04/11] mtd: rawnand: fsmc: clean the probe function style

[Miquel Raynal]

Before fixing the error path of the probe function, fix the style of the
probe function and mostly the goto labels: they should indicate what
the next cleanup is, not the point from which they can be accessed.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/fsmc_nand.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 28c48dcc514e..35af3890ddba 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -1022,12 +1022,12 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 		host->read_dma_chan = dma_request_channel(mask, filter, NULL);
 		if (!host->read_dma_chan) {
 			dev_err(&pdev->dev, "Unable to get read dma channel\n");
-			goto err_req_read_chnl;
+			goto disable_clk;
 		}
 		host->write_dma_chan = dma_request_channel(mask, filter, NULL);
 		if (!host->write_dma_chan) {
 			dev_err(&pdev->dev, "Unable to get write dma channel\n");
-			goto err_req_write_chnl;
+			goto release_dma_read_chan;
 		}
 	}
 
@@ -1050,7 +1050,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	ret = nand_scan_ident(mtd, 1, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "No NAND Device found!\n");
-		goto err_scan_ident;
+		goto release_dma_write_chan;
 	}
 
 	if (AMBA_REV_BITS(host->pid) >= 8) {
@@ -1065,7 +1065,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 			dev_warn(&pdev->dev, "No oob scheme defined for oobsize %d\n",
 				 mtd->oobsize);
 			ret = -EINVAL;
-			goto err_probe;
+			goto release_dma_write_chan;
 		}
 
 		mtd_set_ooblayout(mtd, &fsmc_ecc4_ooblayout_ops);
@@ -1090,7 +1090,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 
 		default:
 			dev_err(&pdev->dev, "Unsupported ECC mode!\n");
-			goto err_probe;
+			goto release_dma_write_chan;
 		}
 
 		/*
@@ -1110,7 +1110,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 					 "No oob scheme defined for oobsize %d\n",
 					 mtd->oobsize);
 				ret = -EINVAL;
-				goto err_probe;
+				goto release_dma_write_chan;
 			}
 		}
 	}
@@ -1118,26 +1118,27 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	/* Second stage of scan to fill MTD data-structures */
 	ret = nand_scan_tail(mtd);
 	if (ret)
-		goto err_probe;
+		goto release_dma_write_chan;
 
 	mtd->name = "nand";
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret)
-		goto err_probe;
+		goto release_dma_write_chan;
 
 	platform_set_drvdata(pdev, host);
 	dev_info(&pdev->dev, "FSMC NAND driver registration successful\n");
+
 	return 0;
 
-err_probe:
-err_scan_ident:
+release_dma_write_chan:
 	if (host->mode == USE_DMA_ACCESS)
 		dma_release_channel(host->write_dma_chan);
-err_req_write_chnl:
+release_dma_read_chan:
 	if (host->mode == USE_DMA_ACCESS)
 		dma_release_channel(host->read_dma_chan);
-err_req_read_chnl:
+disable_clk:
 	clk_disable_unprepare(host->clk);
+
 	return ret;
 }
 
-- 
2.14.1

[PATCH v3 05/11] mtd: rawnand: fsmc: fix the probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup().

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/fsmc_nand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 35af3890ddba..f4a5a317d4ae 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -1123,13 +1123,15 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	mtd->name = "nand";
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret)
-		goto release_dma_write_chan;
+		goto cleanup_nand;
 
 	platform_set_drvdata(pdev, host);
 	dev_info(&pdev->dev, "FSMC NAND driver registration successful\n");
 
 	return 0;
 
+cleanup_nand:
+	nand_cleanup(nand);
 release_dma_write_chan:
 	if (host->mode == USE_DMA_ACCESS)
 		dma_release_channel(host->write_dma_chan);
-- 
2.14.1

[PATCH v3 06/11] mtd: rawnand: hisi504: clean the probe function error path

[Miquel Raynal]

There is not need for a goto statement when the only action to take is
to return.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/hisi504_nand.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/raw/hisi504_nand.c b/drivers/mtd/nand/raw/hisi504_nand.c
index 27558a67fa41..3c64a6281b54 100644
--- a/drivers/mtd/nand/raw/hisi504_nand.c
+++ b/drivers/mtd/nand/raw/hisi504_nand.c
@@ -731,23 +731,19 @@ static int hisi_nfc_probe(struct platform_device *pdev)
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(dev, "no IRQ resource defined\n");
-		ret = -ENXIO;
-		goto err_res;
+		return -ENXIO;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->iobase = devm_ioremap_resource(dev, res);
-	if (IS_ERR(host->iobase)) {
-		ret = PTR_ERR(host->iobase);
-		goto err_res;
-	}
+	if (IS_ERR(host->iobase))
+		return PTR_ERR(host->iobase);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	host->mmio = devm_ioremap_resource(dev, res);
 	if (IS_ERR(host->mmio)) {
-		ret = PTR_ERR(host->mmio);
 		dev_err(dev, "devm_ioremap_resource[1] fail\n");
-		goto err_res;
+		return PTR_ERR(host->mmio);
 	}
 
 	mtd->name		= "hisi_nand";
@@ -770,19 +766,17 @@ static int hisi_nfc_probe(struct platform_device *pdev)
 	ret = devm_request_irq(dev, irq, hinfc_irq_handle, 0x0, "nandc", host);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ\n");
-		goto err_res;
+		return ret;
 	}
 
 	ret = nand_scan_ident(mtd, max_chips, NULL);
 	if (ret)
-		goto err_res;
+		return ret;
 
 	host->buffer = dmam_alloc_coherent(dev, mtd->writesize + mtd->oobsize,
 		&host->dma_buffer, GFP_KERNEL);
-	if (!host->buffer) {
-		ret = -ENOMEM;
-		goto err_res;
-	}
+	if (!host->buffer)
+		return -ENOMEM;
 
 	host->dma_oob = host->dma_buffer + mtd->writesize;
 	memset(host->buffer, 0xff, mtd->writesize + mtd->oobsize);
@@ -798,8 +792,7 @@ static int hisi_nfc_probe(struct platform_device *pdev)
 	 */
 	default:
 		dev_err(dev, "NON-2KB page size nand flash\n");
-		ret = -EINVAL;
-		goto err_res;
+		return -EINVAL;
 	}
 	hinfc_write(host, flag, HINFC504_CON);
 
@@ -809,21 +802,17 @@ static int hisi_nfc_probe(struct platform_device *pdev)
 	ret = nand_scan_tail(mtd);
 	if (ret) {
 		dev_err(dev, "nand_scan_tail failed: %d\n", ret);
-		goto err_res;
+		return ret;
 	}
 
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret) {
 		dev_err(dev, "Err MTD partition=%d\n", ret);
-		goto err_mtd;
+		nand_release(mtd);
+		return ret;
 	}
 
 	return 0;
-
-err_mtd:
-	nand_release(mtd);
-err_res:
-	return ret;
 }
 
 static int hisi_nfc_remove(struct platform_device *pdev)
-- 
2.14.1

[PATCH v3 07/11] mtd: rawnand: hisi504: fix the probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup() and not
a nand_release(). The latter doing an mtd_device_unregister() which is
not needed if mtd_device_register() failed.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/hisi504_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/hisi504_nand.c b/drivers/mtd/nand/raw/hisi504_nand.c
index 3c64a6281b54..a1e009c8e556 100644
--- a/drivers/mtd/nand/raw/hisi504_nand.c
+++ b/drivers/mtd/nand/raw/hisi504_nand.c
@@ -808,7 +808,7 @@ static int hisi_nfc_probe(struct platform_device *pdev)
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret) {
 		dev_err(dev, "Err MTD partition=%d\n", ret);
-		nand_release(mtd);
+		nand_cleanup(chip);
 		return ret;
 	}
 
-- 
2.14.1

[PATCH v3 08/11] mtd: rawnand: lpc32xx_mlc: clean the probe function

[Miquel Raynal]

Before fixing the error path of the probe function, fix the style of the
entire function and particularly the goto labels: they should indicate
what the next cleanup to do is.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/lpc32xx_mlc.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/raw/lpc32xx_mlc.c b/drivers/mtd/nand/raw/lpc32xx_mlc.c
index e357948a7505..6e31faf2f07f 100644
--- a/drivers/mtd/nand/raw/lpc32xx_mlc.c
+++ b/drivers/mtd/nand/raw/lpc32xx_mlc.c
@@ -673,7 +673,7 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	host->io_base = devm_ioremap_resource(&pdev->dev, rc);
 	if (IS_ERR(host->io_base))
 		return PTR_ERR(host->io_base);
-	
+
 	host->io_base_phy = rc->start;
 
 	nand_chip = &host->nand_chip;
@@ -706,11 +706,11 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	if (IS_ERR(host->clk)) {
 		dev_err(&pdev->dev, "Clock initialization failure\n");
 		res = -ENOENT;
-		goto err_exit1;
+		goto free_gpio;
 	}
 	res = clk_prepare_enable(host->clk);
 	if (res)
-		goto err_put_clk;
+		goto put_clk;
 
 	nand_chip->cmd_ctrl = lpc32xx_nand_cmd_ctrl;
 	nand_chip->dev_ready = lpc32xx_nand_device_ready;
@@ -744,7 +744,7 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 		res = lpc32xx_dma_setup(host);
 		if (res) {
 			res = -EIO;
-			goto err_exit2;
+			goto unprepare_clk;
 		}
 	}
 
@@ -754,18 +754,18 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	 */
 	res = nand_scan_ident(mtd, 1, NULL);
 	if (res)
-		goto err_exit3;
+		goto release_dma_chan;
 
 	host->dma_buf = devm_kzalloc(&pdev->dev, mtd->writesize, GFP_KERNEL);
 	if (!host->dma_buf) {
 		res = -ENOMEM;
-		goto err_exit3;
+		goto release_dma_chan;
 	}
 
 	host->dummy_buf = devm_kzalloc(&pdev->dev, mtd->writesize, GFP_KERNEL);
 	if (!host->dummy_buf) {
 		res = -ENOMEM;
-		goto err_exit3;
+		goto release_dma_chan;
 	}
 
 	nand_chip->ecc.mode = NAND_ECC_HW;
@@ -783,14 +783,14 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	if (host->irq < 0) {
 		dev_err(&pdev->dev, "failed to get platform irq\n");
 		res = -EINVAL;
-		goto err_exit3;
+		goto release_dma_chan;
 	}
 
 	if (request_irq(host->irq, (irq_handler_t)&lpc3xxx_nand_irq,
 			IRQF_TRIGGER_HIGH, DRV_NAME, host)) {
 		dev_err(&pdev->dev, "Error requesting NAND IRQ\n");
 		res = -ENXIO;
-		goto err_exit3;
+		goto release_dma_chan;
 	}
 
 	/*
@@ -799,27 +799,29 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	 */
 	res = nand_scan_tail(mtd);
 	if (res)
-		goto err_exit4;
+		goto free_irq;
 
 	mtd->name = DRV_NAME;
 
 	res = mtd_device_register(mtd, host->ncfg->parts,
 				  host->ncfg->num_parts);
-	if (!res)
-		return res;
+	if (res)
+		goto release_nand;
 
+	return 0;
+
+release_nand:
 	nand_release(mtd);
-
-err_exit4:
+free_irq:
 	free_irq(host->irq, host);
-err_exit3:
+release_dma_chan:
 	if (use_dma)
 		dma_release_channel(host->dma_chan);
-err_exit2:
+unprepare_clk:
 	clk_disable_unprepare(host->clk);
-err_put_clk:
+put_clk:
 	clk_put(host->clk);
-err_exit1:
+free_gpio:
 	lpc32xx_wp_enable(host);
 	gpio_free(host->ncfg->wp_gpio);
 
-- 
2.14.1

[PATCH v3 09/11] mtd: rawnand: lpc32xx_mlc: fix the probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup() and not
a nand_release(). The latter doing an mtd_device_unregister() which is
not needed if mtd_device_register() failed.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/lpc32xx_mlc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/lpc32xx_mlc.c b/drivers/mtd/nand/raw/lpc32xx_mlc.c
index 6e31faf2f07f..052d123a8304 100644
--- a/drivers/mtd/nand/raw/lpc32xx_mlc.c
+++ b/drivers/mtd/nand/raw/lpc32xx_mlc.c
@@ -806,12 +806,12 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	res = mtd_device_register(mtd, host->ncfg->parts,
 				  host->ncfg->num_parts);
 	if (res)
-		goto release_nand;
+		goto cleanup_nand;
 
 	return 0;
 
-release_nand:
-	nand_release(mtd);
+cleanup_nand:
+	nand_cleanup(nand_chip);
 free_irq:
 	free_irq(host->irq, host);
 release_dma_chan:
-- 
2.14.1

[PATCH v3 10/11] mtd: rawnand: lpc32xx_slc: clean the probe function

[Miquel Raynal]

Before fixing the error path of the probe function, fix the style of the
entire function and particularly the goto labels: they should indicate
what the next cleanup to do is.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/lpc32xx_slc.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/lpc32xx_slc.c b/drivers/mtd/nand/raw/lpc32xx_slc.c
index 5f7cc6da0a7f..342f666ad435 100644
--- a/drivers/mtd/nand/raw/lpc32xx_slc.c
+++ b/drivers/mtd/nand/raw/lpc32xx_slc.c
@@ -831,11 +831,11 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	if (IS_ERR(host->clk)) {
 		dev_err(&pdev->dev, "Clock failure\n");
 		res = -ENOENT;
-		goto err_exit1;
+		goto release_gpio;
 	}
 	res = clk_prepare_enable(host->clk);
 	if (res)
-		goto err_exit1;
+		goto release_gpio;
 
 	/* Set NAND IO addresses and command/ready functions */
 	chip->IO_ADDR_R = SLC_DATA(host->io_base);
@@ -874,19 +874,19 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 				      GFP_KERNEL);
 	if (host->data_buf == NULL) {
 		res = -ENOMEM;
-		goto err_exit2;
+		goto release_clk;
 	}
 
 	res = lpc32xx_nand_dma_setup(host);
 	if (res) {
 		res = -EIO;
-		goto err_exit2;
+		goto release_clk;
 	}
 
 	/* Find NAND device */
 	res = nand_scan_ident(mtd, 1, NULL);
 	if (res)
-		goto err_exit3;
+		goto release_dma;
 
 	/* OOB and ECC CPU and DMA work areas */
 	host->ecc_buf = (uint32_t *)(host->data_buf + LPC32XX_DMA_DATA_SIZE);
@@ -920,21 +920,23 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	 */
 	res = nand_scan_tail(mtd);
 	if (res)
-		goto err_exit3;
+		goto release_dma;
 
 	mtd->name = "nxp_lpc3220_slc";
 	res = mtd_device_register(mtd, host->ncfg->parts,
 				  host->ncfg->num_parts);
-	if (!res)
-		return res;
+	if (res)
+		goto release_nand;
 
+	return 0;
+
+release_nand:
 	nand_release(mtd);
-
-err_exit3:
+release_dma:
 	dma_release_channel(host->dma_chan);
-err_exit2:
+release_clk:
 	clk_disable_unprepare(host->clk);
-err_exit1:
+release_gpio:
 	lpc32xx_wp_enable(host);
 
 	return res;
-- 
2.14.1

Re: [PATCH v3 10/11] mtd: rawnand: lpc32xx_slc: clean the probe function

[Boris Brezillon]

On Sat, 21 Apr 2018 20:00:42 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Before fixing the error path of the probe function, fix the style of the
> entire function and particularly the goto labels: they should indicate
> what the next cleanup to do is.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/lpc32xx_slc.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/lpc32xx_slc.c b/drivers/mtd/nand/raw/lpc32xx_slc.c
> index 5f7cc6da0a7f..342f666ad435 100644
> --- a/drivers/mtd/nand/raw/lpc32xx_slc.c
> +++ b/drivers/mtd/nand/raw/lpc32xx_slc.c
> @@ -831,11 +831,11 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
>  	if (IS_ERR(host->clk)) {
>  		dev_err(&pdev->dev, "Clock failure\n");
>  		res = -ENOENT;
> -		goto err_exit1;
> +		goto release_gpio;
>  	}
>  	res = clk_prepare_enable(host->clk);
>  	if (res)
> -		goto err_exit1;
> +		goto release_gpio;
>  
>  	/* Set NAND IO addresses and command/ready functions */
>  	chip->IO_ADDR_R = SLC_DATA(host->io_base);
> @@ -874,19 +874,19 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
>  				      GFP_KERNEL);
>  	if (host->data_buf == NULL) {
>  		res = -ENOMEM;
> -		goto err_exit2;
> +		goto release_clk;
>  	}
>  
>  	res = lpc32xx_nand_dma_setup(host);
>  	if (res) {
>  		res = -EIO;
> -		goto err_exit2;
> +		goto release_clk;
>  	}
>  
>  	/* Find NAND device */
>  	res = nand_scan_ident(mtd, 1, NULL);
>  	if (res)
> -		goto err_exit3;
> +		goto release_dma;
>  
>  	/* OOB and ECC CPU and DMA work areas */
>  	host->ecc_buf = (uint32_t *)(host->data_buf + LPC32XX_DMA_DATA_SIZE);
> @@ -920,21 +920,23 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
>  	 */
>  	res = nand_scan_tail(mtd);
>  	if (res)
> -		goto err_exit3;
> +		goto release_dma;
>  
>  	mtd->name = "nxp_lpc3220_slc";
>  	res = mtd_device_register(mtd, host->ncfg->parts,
>  				  host->ncfg->num_parts);
> -	if (!res)
> -		return res;
> +	if (res)
> +		goto release_nand;
>  
> +	return 0;
> +
> +release_nand:
>  	nand_release(mtd);
> -
> -err_exit3:
> +release_dma:
>  	dma_release_channel(host->dma_chan);
> -err_exit2:
> +release_clk:

You mean unprepare_clk.

>  	clk_disable_unprepare(host->clk);
> -err_exit1:
> +release_gpio:

And I'd go for enable_wp here since it's not releasing the gpio at all.

No need to resend, I'll fix it when applying

>  	lpc32xx_wp_enable(host);
>  
>  	return res;

[PATCH v3 11/11] mtd: rawnand: lpc32xx_slc: fix the probe function error path

[Miquel Raynal]

An error after nand_scan_tail() should trigger a nand_cleanup() and not
a nand_release(). The latter doing an mtd_device_unregister() which is
not needed if mtd_device_register() failed.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/lpc32xx_slc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/lpc32xx_slc.c b/drivers/mtd/nand/raw/lpc32xx_slc.c
index 342f666ad435..143e1ec3a90a 100644
--- a/drivers/mtd/nand/raw/lpc32xx_slc.c
+++ b/drivers/mtd/nand/raw/lpc32xx_slc.c
@@ -926,12 +926,12 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	res = mtd_device_register(mtd, host->ncfg->parts,
 				  host->ncfg->num_parts);
 	if (res)
-		goto release_nand;
+		goto cleanup_nand;
 
 	return 0;
 
-release_nand:
-	nand_release(mtd);
+cleanup_nand:
+	nand_cleanup(chip);
 release_dma:
 	dma_release_channel(host->dma_chan);
 release_clk:
-- 
2.14.1

Re: [PATCH v3 00/11] Fix NAND controllers probe functions error path

[Boris Brezillon]

On Sat, 21 Apr 2018 20:00:32 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello all,
> 
> In order to remove the limitation that prevents dynamic allocations
> during the identification phase (nand_scan_ident()), we need to get rid
> of the nand_scan_ident()/nand_scan_tail() calls from drivers and only
> export nand_scan().
> 
> This series prepares the migration to nand_scan() by first fixing (and
> enhancing some times) the probe function error path of the drivers that
> do not use nand_scan() yet.
> 
> The three main problems addressed in the series are:
> 0/ unproper error path (fuzzy, styling issues, undescriptive goto's).
> 1/ a wrong error path that does not free/disable the resources
>    correctly.
> 2/ nand_cleanup() is not called upon error after a successful
>    nand_scan_tail().
> 3/ nand_release() is called instead of nand_cleanup(). nand_release()
>    does call nand_cleanup() and also mtd_device_unregister(), which
>    should not be called while mtd_device_register() as succeeded.
> 
> Thanks,
> Miquèl

Applied.

Thanks,

Boris

> 
> Changes since v2:
> =================
>  * Reworked the few remaining patches that needed small changes.
>  * docg4: changed the commit message as proposed.
>  * fsl_elbc/fsl_ifc: moved the wrong nand_release() out of the
>    'remove' function to avoid double free possible issues.
>  * fsmc/hisi504/lpc32xx_mlc/lpc32xx_slc: split the changes in two
>    commits, one to fix the goto labels/style, one that actually fixes
>    the function.
> 
> Changes since v1:
> =================
>  * Extracted only the patches fixing the error path from the first huge
>    series (50+ patches) that also converted the drivers to nand_scan().
>  * Changed wrong nand_release() calls to nand_cleanup().
> 
> 
> Miquel Raynal (11):
>   mtd: rawnand: docg4: fix the probe function error path
>   mtd: rawnand: fsl_elbc: fix probe function error path
>   mtd: rawnand: fsl_ifc: fix probe function error path
>   mtd: rawnand: fsmc: clean the probe function style
>   mtd: rawnand: fsmc: fix the probe function error path
>   mtd: rawnand: hisi504: clean the probe function error path
>   mtd: rawnand: hisi504: fix the probe function error path
>   mtd: rawnand: lpc32xx_mlc: clean the probe function
>   mtd: rawnand: lpc32xx_mlc: fix the probe function error path
>   mtd: rawnand: lpc32xx_slc: clean the probe function
>   mtd: rawnand: lpc32xx_slc: fix the probe function error path
> 
>  drivers/mtd/nand/raw/docg4.c         | 22 +++++++++++----------
>  drivers/mtd/nand/raw/fsl_elbc_nand.c | 13 ++++++++----
>  drivers/mtd/nand/raw/fsl_ifc_nand.c  | 12 +++++++++---
>  drivers/mtd/nand/raw/fsmc_nand.c     | 27 +++++++++++++------------
>  drivers/mtd/nand/raw/hisi504_nand.c  | 35 ++++++++++++---------------------
>  drivers/mtd/nand/raw/lpc32xx_mlc.c   | 38 +++++++++++++++++++-----------------
>  drivers/mtd/nand/raw/lpc32xx_slc.c   | 26 ++++++++++++------------
>  7 files changed, 91 insertions(+), 82 deletions(-)
>