[PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Brian Norris]

This bug seems to have been here forever, although we came close to
fixing all of them in [1]!

[1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")

Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
The goto isn't *really* necessary, but I thought it'd be more consistent.

Compile tested only

 drivers/mtd/nand/nand_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 978242b1213f..e4919f9dece4 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4794,7 +4794,11 @@ int nand_scan_tail(struct mtd_info *mtd)
 		return 0;
 
 	/* Build bad block table */
-	return chip->scan_bbt(mtd);
+	ret = chip->scan_bbt(mtd);
+	if (ret)
+		goto err_free;
+	return 0;
+
 err_free:
 	if (nbuf) {
 		kfree(nbuf->databuf);
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH] mtd: nand: don't make vendor-specific code un-set their data pointer

[Brian Norris]

It makes sense to do this in nand_base.

(Alternatively: we don't really need to do this at all.)

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Compile tested only

 drivers/mtd/nand/nand_base.c  | 11 +++++++++--
 drivers/mtd/nand/nand_hynix.c |  1 -
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ed49a1d634b0..2adcc8cdedf1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3910,11 +3910,16 @@ static void nand_manufacturer_detect(struct nand_chip *chip)
  */
 static int nand_manufacturer_init(struct nand_chip *chip)
 {
+	int ret;
+
 	if (!chip->manufacturer.desc || !chip->manufacturer.desc->ops ||
 	    !chip->manufacturer.desc->ops->init)
 		return 0;
 
-	return chip->manufacturer.desc->ops->init(chip);
+	ret = chip->manufacturer.desc->ops->init(chip);
+	if (ret)
+		nand_set_manufacturer_data(chip, NULL);
+	return ret;
 }
 
 /*
@@ -3927,8 +3932,10 @@ static void nand_manufacturer_cleanup(struct nand_chip *chip)
 {
 	/* Release manufacturer private data */
 	if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
-	    chip->manufacturer.desc->ops->cleanup)
+	    chip->manufacturer.desc->ops->cleanup) {
 		chip->manufacturer.desc->ops->cleanup(chip);
+		nand_set_manufacturer_data(chip, NULL);
+	}
 }
 
 /*
diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c
index b12dc7325378..54d99f90aa9f 100644
--- a/drivers/mtd/nand/nand_hynix.c
+++ b/drivers/mtd/nand/nand_hynix.c
@@ -598,7 +598,6 @@ static void hynix_nand_cleanup(struct nand_chip *chip)
 
 	kfree(hynix->read_retry);
 	kfree(hynix);
-	nand_set_manufacturer_data(chip, NULL);
 }
 
 static int hynix_nand_init(struct nand_chip *chip)
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH] mtd: nand: don't make vendor-specific code un-set their data pointer

[Boris Brezillon]

On Mon,  1 May 2017 17:04:51 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> It makes sense to do this in nand_base.
> 
> (Alternatively: we don't really need to do this at all.)

Actually, I was just trying to handle a potential 'double-free' error
nicely (see the 'if (hynix) return;' statement at the beginning of
hynix_nand_cleanup(), but I'm not sure this was a good idea.

Let's drop this assignment.

> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c  | 11 +++++++++--
>  drivers/mtd/nand/nand_hynix.c |  1 -
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ed49a1d634b0..2adcc8cdedf1 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3910,11 +3910,16 @@ static void nand_manufacturer_detect(struct nand_chip *chip)
>   */
>  static int nand_manufacturer_init(struct nand_chip *chip)
>  {
> +	int ret;
> +
>  	if (!chip->manufacturer.desc || !chip->manufacturer.desc->ops ||
>  	    !chip->manufacturer.desc->ops->init)
>  		return 0;
>  
> -	return chip->manufacturer.desc->ops->init(chip);
> +	ret = chip->manufacturer.desc->ops->init(chip);
> +	if (ret)
> +		nand_set_manufacturer_data(chip, NULL);
> +	return ret;
>  }
>  
>  /*
> @@ -3927,8 +3932,10 @@ static void nand_manufacturer_cleanup(struct nand_chip *chip)
>  {
>  	/* Release manufacturer private data */
>  	if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
> -	    chip->manufacturer.desc->ops->cleanup)
> +	    chip->manufacturer.desc->ops->cleanup) {
>  		chip->manufacturer.desc->ops->cleanup(chip);
> +		nand_set_manufacturer_data(chip, NULL);
> +	}
>  }
>  
>  /*
> diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c
> index b12dc7325378..54d99f90aa9f 100644
> --- a/drivers/mtd/nand/nand_hynix.c
> +++ b/drivers/mtd/nand/nand_hynix.c
> @@ -598,7 +598,6 @@ static void hynix_nand_cleanup(struct nand_chip *chip)
>  
>  	kfree(hynix->read_retry);
>  	kfree(hynix);
> -	nand_set_manufacturer_data(chip, NULL);
>  }
>  
>  static int hynix_nand_init(struct nand_chip *chip)

[PATCH] mtd: nand: drop unneeded module.h include

[Brian Norris]

nand_ids isn't a separate module anymore and doesn't need this header.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_ids.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 9d5ca0e540b5..92e2cf8e9ff9 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -6,7 +6,6 @@
  * published by the Free Software Foundation.
  *
  */
-#include <linux/module.h>
 #include <linux/mtd/nand.h>
 #include <linux/sizes.h>
 
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH] mtd: nand: drop unneeded module.h include

[Boris Brezillon]

On Mon,  1 May 2017 17:04:52 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> nand_ids isn't a separate module anymore and doesn't need this header.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/nand/nand_ids.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> index 9d5ca0e540b5..92e2cf8e9ff9 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -6,7 +6,6 @@
>   * published by the Free Software Foundation.
>   *
>   */
> -#include <linux/module.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/sizes.h>
>  

Re: [PATCH] mtd: nand: drop unneeded module.h include

[Boris Brezillon]

On Mon,  1 May 2017 17:04:52 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> nand_ids isn't a separate module anymore and doesn't need this header.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Applied to nand/fixes.

Thanks,

Boris

> ---
>  drivers/mtd/nand/nand_ids.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> index 9d5ca0e540b5..92e2cf8e9ff9 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -6,7 +6,6 @@
>   * published by the Free Software Foundation.
>   *
>   */
> -#include <linux/module.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/sizes.h>
>  

[PATCH] mtd: nand: free vendor-specific resources in init failure paths

[Brian Norris]

If we fail any time after calling nand_detect(), then we don't call the
vendor-specific ->cleanup() callback, and we'll leak any resources the
vendor-specific code might have allocated.

Mark the "fix" against the first commit that started allocating anything
in ->init().

Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Fixes: 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Compile tested only

 drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 2adcc8cdedf1..978242b1213f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4300,7 +4300,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	/* Initialize the ->data_interface field. */
 	ret = nand_init_data_interface(chip);
 	if (ret)
-		return ret;
+		goto err_nand_init;
 
 	/*
 	 * Setup the data interface correctly on the chip and controller side.
@@ -4312,7 +4312,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	 */
 	ret = nand_setup_data_interface(chip);
 	if (ret)
-		return ret;
+		goto err_nand_init;
 
 	nand_maf_id = chip->id.data[0];
 	nand_dev_id = chip->id.data[1];
@@ -4343,6 +4343,12 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	mtd->size = i * chip->chipsize;
 
 	return 0;
+
+err_nand_init:
+	/* Free manufacturer priv data. */
+	nand_manufacturer_cleanup(chip);
+
+	return ret;
 }
 EXPORT_SYMBOL(nand_scan_ident);
 
@@ -4513,18 +4519,23 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
 	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
-		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
-		return -EINVAL;
+		   !(chip->bbt_options & NAND_BBT_USE_FLASH))) {
+		ret = -EINVAL;
+		goto err_ident;
+	}
 
 	if (invalid_ecc_page_accessors(chip)) {
 		pr_err("Invalid ECC page accessors setup\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_ident;
 	}
 
 	if (!(chip->options & NAND_OWN_BUFFERS)) {
 		nbuf = kzalloc(sizeof(*nbuf), GFP_KERNEL);
-		if (!nbuf)
-			return -ENOMEM;
+		if (!nbuf) {
+			ret = -ENOMEM;
+			goto err_ident;
+		}
 
 		nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
 		if (!nbuf->ecccalc) {
@@ -4547,8 +4558,10 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 		chip->buffers = nbuf;
 	} else {
-		if (!chip->buffers)
-			return -ENOMEM;
+		if (!chip->buffers) {
+			ret = -ENOMEM;
+			goto err_ident;
+		}
 	}
 
 	/* Set the internal oob buffer location, just after the page data */
@@ -4789,6 +4802,13 @@ int nand_scan_tail(struct mtd_info *mtd)
 		kfree(nbuf->ecccalc);
 		kfree(nbuf);
 	}
+
+err_ident:
+	/* Clean up nand_scan_ident(). */
+
+	/* Free manufacturer priv data. */
+	nand_manufacturer_cleanup(chip);
+
 	return ret;
 }
 EXPORT_SYMBOL(nand_scan_tail);
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH] mtd: nand: free vendor-specific resources in init failure paths

[Boris Brezillon]

On Mon,  1 May 2017 17:04:53 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> If we fail any time after calling nand_detect(), then we don't call the
> vendor-specific ->cleanup() callback, and we'll leak any resources the
> vendor-specific code might have allocated.
> 
> Mark the "fix" against the first commit that started allocating anything
> in ->init().

Yep, I noticed this leak while reviewing/applying Masahiro's series
[1], and forgot to send a fix for this bug.

Actually, I'm not sure we should keep nand_manufacturer_init() in
nand_scan_ident(), especially if we consider that nand_scan_ident() is
not supposed to allocate resources and does not require a
nand_cleanup() when something fails between nand_scan_ident() and
nand_scan_tail().

Note that nand_scan_ident() is already allocating resources through
nand_init_data_interface() which are also leaked if nand_cleanup() is
not called. Again, we could solve the problem by moving data-iface
initialization steps in nand_scan_tail() (which shouldn't be a problem
AFAICS). Alternatively, we could could consider that nand_cleanup() is
smart enough to know what to not release (which seems to be the case
already), and force drivers to call nand_cleanup() as soon as
nand_scan_ident() has returned 0.

Brian, any opinion?

Anyway, this is a bit off-topic, so for this patch

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Fixes: 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 2adcc8cdedf1..978242b1213f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4300,7 +4300,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	/* Initialize the ->data_interface field. */
>  	ret = nand_init_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	/*
>  	 * Setup the data interface correctly on the chip and controller side.
> @@ -4312,7 +4312,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	 */
>  	ret = nand_setup_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	nand_maf_id = chip->id.data[0];
>  	nand_dev_id = chip->id.data[1];
> @@ -4343,6 +4343,12 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	mtd->size = i * chip->chipsize;
>  
>  	return 0;
> +
> +err_nand_init:
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_ident);
>  
> @@ -4513,18 +4519,23 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
>  	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> -		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
> -		return -EINVAL;
> +		   !(chip->bbt_options & NAND_BBT_USE_FLASH))) {
> +		ret = -EINVAL;
> +		goto err_ident;
> +	}
>  
>  	if (invalid_ecc_page_accessors(chip)) {
>  		pr_err("Invalid ECC page accessors setup\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_ident;
>  	}
>  
>  	if (!(chip->options & NAND_OWN_BUFFERS)) {
>  		nbuf = kzalloc(sizeof(*nbuf), GFP_KERNEL);
> -		if (!nbuf)
> -			return -ENOMEM;
> +		if (!nbuf) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  
>  		nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
>  		if (!nbuf->ecccalc) {
> @@ -4547,8 +4558,10 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  		chip->buffers = nbuf;
>  	} else {
> -		if (!chip->buffers)
> -			return -ENOMEM;
> +		if (!chip->buffers) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  	}
>  
>  	/* Set the internal oob buffer location, just after the page data */
> @@ -4789,6 +4802,13 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		kfree(nbuf->ecccalc);
>  		kfree(nbuf);
>  	}
> +
> +err_ident:
> +	/* Clean up nand_scan_ident(). */
> +
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_tail);

Re: [PATCH] mtd: nand: free vendor-specific resources in init failure paths

[Boris Brezillon]

On Tue, 2 May 2017 09:52:30 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon,  1 May 2017 17:04:53 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > If we fail any time after calling nand_detect(), then we don't call the
> > vendor-specific ->cleanup() callback, and we'll leak any resources the
> > vendor-specific code might have allocated.
> > 
> > Mark the "fix" against the first commit that started allocating anything
> > in ->init().  
> 
> Yep, I noticed this leak while reviewing/applying Masahiro's series
> [1], and forgot to send a fix for this bug.
> 
> Actually, I'm not sure we should keep nand_manufacturer_init() in
> nand_scan_ident(), especially if we consider that nand_scan_ident() is
> not supposed to allocate resources and does not require a
> nand_cleanup() when something fails between nand_scan_ident() and
> nand_scan_tail().
> 

Just for the record, this is the kind of fix I had in mind, but it can
still be applied on top of your patch.

--->8---
From fd918ec7ed960792f020227b675838fee6f710d9 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Tue, 2 May 2017 18:10:18 +0200
Subject: [PATCH] mtd: nand: Fix various memory leaks in core

The nand_scan_ident() function is not expected to allocate resources,
and people are usually not calling nand_cleanup() if something fails
between nand_scan_ident() and nand_scan_tail().

Move all functions that may allocate resource to the nand_scan_tail()
path to prevent such resource leaks.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Only compile tested.
---
 drivers/mtd/nand/nand_base.c | 79 ++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ed49a1d634b0..6ba03921eddb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3939,7 +3939,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 	const struct nand_manufacturer *manufacturer;
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	int busw;
-	int i, ret;
+	int i;
 	u8 *id_data = chip->id.data;
 	u8 maf_id, dev_id;
 
@@ -4080,10 +4080,6 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
-	ret = nand_manufacturer_init(chip);
-	if (ret)
-		return ret;
-
 	pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
 		maf_id, dev_id);
 
@@ -4290,23 +4286,6 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		return ret;
 	}
 
-	/* Initialize the ->data_interface field. */
-	ret = nand_init_data_interface(chip);
-	if (ret)
-		return ret;
-
-	/*
-	 * Setup the data interface correctly on the chip and controller side.
-	 * This explicit call to nand_setup_data_interface() is only required
-	 * for the first die, because nand_reset() has been called before
-	 * ->data_interface and ->default_onfi_timing_mode were set.
-	 * For the other dies, nand_reset() will automatically switch to the
-	 * best mode for us.
-	 */
-	ret = nand_setup_data_interface(chip);
-	if (ret)
-		return ret;
-
 	nand_maf_id = chip->id.data[0];
 	nand_dev_id = chip->id.data[1];
 
@@ -4502,7 +4481,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct nand_ecc_ctrl *ecc = &chip->ecc;
 	struct nand_buffers *nbuf = NULL;
-	int ret;
+	int ret, i;
 
 	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
 	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
@@ -4522,20 +4501,20 @@ int nand_scan_tail(struct mtd_info *mtd)
 		nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
 		if (!nbuf->ecccalc) {
 			ret = -ENOMEM;
-			goto err_free;
+			goto err_free_nbuf;
 		}
 
 		nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
 		if (!nbuf->ecccode) {
 			ret = -ENOMEM;
-			goto err_free;
+			goto err_free_nbuf;
 		}
 
 		nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
 					GFP_KERNEL);
 		if (!nbuf->databuf) {
 			ret = -ENOMEM;
-			goto err_free;
+			goto err_free_nbuf;
 		}
 
 		chip->buffers = nbuf;
@@ -4544,6 +4523,10 @@ int nand_scan_tail(struct mtd_info *mtd)
 			return -ENOMEM;
 	}
 
+	ret = nand_manufacturer_init(chip);
+	if (ret)
+		goto err_free_nbuf;
+
 	/* Set the internal oob buffer location, just after the page data */
 	chip->oob_poi = chip->buffers->databuf + mtd->writesize;
 
@@ -4565,7 +4548,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			WARN(1, "No oob scheme defined for oobsize %d\n",
 				mtd->oobsize);
 			ret = -EINVAL;
-			goto err_free;
+			goto err_nand_manuf_cleanup;
 		}
 	}
 
@@ -4580,7 +4563,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		if (!ecc->calculate || !ecc->correct || !ecc->hwctl) {
 			WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
 			ret = -EINVAL;
-			goto err_free;
+			goto err_nand_manuf_cleanup;
 		}
 		if (!ecc->read_page)
 			ecc->read_page = nand_read_page_hwecc_oob_first;
@@ -4612,7 +4595,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		     ecc->write_page == nand_write_page_hwecc)) {
 			WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
 			ret = -EINVAL;
-			goto err_free;
+			goto err_nand_manuf_cleanup;
 		}
 		/* Use standard syndrome read/write page function? */
 		if (!ecc->read_page)
@@ -4632,7 +4615,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			if (!ecc->strength) {
 				WARN(1, "Driver must set ecc.strength when using hardware ECC\n");
 				ret = -EINVAL;
-				goto err_free;
+				goto err_nand_manuf_cleanup;
 			}
 			break;
 		}
@@ -4645,7 +4628,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		ret = nand_set_ecc_soft_ops(mtd);
 		if (ret) {
 			ret = -EINVAL;
-			goto err_free;
+			goto err_nand_manuf_cleanup;
 		}
 		break;
 
@@ -4665,7 +4648,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	default:
 		WARN(1, "Invalid NAND_ECC_MODE %d\n", ecc->mode);
 		ret = -EINVAL;
-		goto err_free;
+		goto err_nand_manuf_cleanup;
 	}
 
 	/* For many systems, the standard OOB write also works for raw */
@@ -4686,7 +4669,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	if (ecc->steps * ecc->size != mtd->writesize) {
 		WARN(1, "Invalid ECC parameters\n");
 		ret = -EINVAL;
-		goto err_free;
+		goto err_nand_manuf_cleanup;
 	}
 	ecc->total = ecc->steps * ecc->bytes;
 
@@ -4769,13 +4752,39 @@ int nand_scan_tail(struct mtd_info *mtd)
 	if (!mtd->bitflip_threshold)
 		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3, 4);
 
+	/* Initialize the ->data_interface field. */
+	ret = nand_init_data_interface(chip);
+	if (ret)
+		goto err_nand_manuf_cleanup;
+
+	/* Enter fastest possible mode on all dies. */
+	for (i = 0; i < chip->numchips; i++) {
+		chip->select_chip(mtd, i);
+		ret = nand_setup_data_interface(chip);
+		chip->select_chip(mtd, -1);
+
+		if (ret)
+			goto err_nand_data_iface_cleanup;
+	}
+
 	/* Check, if we should skip the bad block table scan */
 	if (chip->options & NAND_SKIP_BBTSCAN)
 		return 0;
 
 	/* Build bad block table */
-	return chip->scan_bbt(mtd);
-err_free:
+	ret = chip->scan_bbt(mtd);
+	if (ret)
+		goto err_nand_data_iface_cleanup;
+
+	return 0;
+
+err_nand_data_iface_cleanup:
+	nand_release_data_interface(chip);
+
+err_nand_manuf_cleanup:
+	nand_manufacturer_cleanup(chip);
+
+err_free_nbuf:
 	if (nbuf) {
 		kfree(nbuf->databuf);
 		kfree(nbuf->ecccode);

Re: [PATCH] mtd: nand: free vendor-specific resources in init failure paths

[Boris Brezillon]

On Mon,  1 May 2017 17:04:53 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> If we fail any time after calling nand_detect(), then we don't call the
> vendor-specific ->cleanup() callback, and we'll leak any resources the
> vendor-specific code might have allocated.
> 
> Mark the "fix" against the first commit that started allocating anything
> in ->init().
> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Fixes: 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Applied to nand/fixes.

Thanks,

Boris

> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 2adcc8cdedf1..978242b1213f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4300,7 +4300,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	/* Initialize the ->data_interface field. */
>  	ret = nand_init_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	/*
>  	 * Setup the data interface correctly on the chip and controller side.
> @@ -4312,7 +4312,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	 */
>  	ret = nand_setup_data_interface(chip);
>  	if (ret)
> -		return ret;
> +		goto err_nand_init;
>  
>  	nand_maf_id = chip->id.data[0];
>  	nand_dev_id = chip->id.data[1];
> @@ -4343,6 +4343,12 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	mtd->size = i * chip->chipsize;
>  
>  	return 0;
> +
> +err_nand_init:
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_ident);
>  
> @@ -4513,18 +4519,23 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
>  	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> -		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
> -		return -EINVAL;
> +		   !(chip->bbt_options & NAND_BBT_USE_FLASH))) {
> +		ret = -EINVAL;
> +		goto err_ident;
> +	}
>  
>  	if (invalid_ecc_page_accessors(chip)) {
>  		pr_err("Invalid ECC page accessors setup\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_ident;
>  	}
>  
>  	if (!(chip->options & NAND_OWN_BUFFERS)) {
>  		nbuf = kzalloc(sizeof(*nbuf), GFP_KERNEL);
> -		if (!nbuf)
> -			return -ENOMEM;
> +		if (!nbuf) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  
>  		nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
>  		if (!nbuf->ecccalc) {
> @@ -4547,8 +4558,10 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  		chip->buffers = nbuf;
>  	} else {
> -		if (!chip->buffers)
> -			return -ENOMEM;
> +		if (!chip->buffers) {
> +			ret = -ENOMEM;
> +			goto err_ident;
> +		}
>  	}
>  
>  	/* Set the internal oob buffer location, just after the page data */
> @@ -4789,6 +4802,13 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		kfree(nbuf->ecccalc);
>  		kfree(nbuf);
>  	}
> +
> +err_ident:
> +	/* Clean up nand_scan_ident(). */
> +
> +	/* Free manufacturer priv data. */
> +	nand_manufacturer_cleanup(chip);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_tail);

[PATCH] mtd: nand: orion: don't complain for probe deferral

[Brian Norris]

Recent patches to this driver changed the error handling for missing
clocks. Now we'll print an error if the clock causes us to defer
probing. Let's not do that.

Cc: Simon Baatz <gmbnomis@gmail.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Compile tested only

 drivers/mtd/nand/orion_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
index f8e463a97b9e..e2bfb37f11df 100644
--- a/drivers/mtd/nand/orion_nand.c
+++ b/drivers/mtd/nand/orion_nand.c
@@ -161,7 +161,8 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 		if (ret == -ENOENT) {
 			info->clk = NULL;
 		} else {
-			dev_err(&pdev->dev, "failed to get clock!\n");
+			if (ret != -EPROBE_DEFER)
+				dev_err(&pdev->dev, "failed to get clock!\n");
 			return ret;
 		}
 	}
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH] mtd: nand: orion: don't complain for probe deferral

[Boris Brezillon]

On Mon,  1 May 2017 17:04:54 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Recent patches to this driver changed the error handling for missing
> clocks. Now we'll print an error if the clock causes us to defer
> probing. Let's not do that.
> 
> Cc: Simon Baatz <gmbnomis@gmail.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> Compile tested only
> 
>  drivers/mtd/nand/orion_nand.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
> index f8e463a97b9e..e2bfb37f11df 100644
> --- a/drivers/mtd/nand/orion_nand.c
> +++ b/drivers/mtd/nand/orion_nand.c
> @@ -161,7 +161,8 @@ static int __init orion_nand_probe(struct platform_device *pdev)
>  		if (ret == -ENOENT) {
>  			info->clk = NULL;
>  		} else {
> -			dev_err(&pdev->dev, "failed to get clock!\n");
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(&pdev->dev, "failed to get clock!\n");
>  			return ret;
>  		}
>  	}

Re: [PATCH] mtd: nand: orion: don't complain for probe deferral

[Simon Baatz]

Hi Brian,

On Mon, May 01, 2017 at 05:04:54PM -0700, Brian Norris wrote:
> Recent patches to this driver changed the error handling for missing
> clocks. Now we'll print an error if the clock causes us to defer
> probing. Let's not do that.
> 

This is by intention.  Probe deferral is not supported by drivers
using module_platform_driver_probe().  EPROBE_DEFER results in not
being able to get the clock and, thus, is handled like any other
error.


- Simon

Re: [PATCH] mtd: nand: orion: don't complain for probe deferral

[Brian Norris]

On Tue, May 02, 2017 at 10:07:31AM +0200, Simon Baatz wrote:
> Hi Brian,
> 
> On Mon, May 01, 2017 at 05:04:54PM -0700, Brian Norris wrote:
> > Recent patches to this driver changed the error handling for missing
> > clocks. Now we'll print an error if the clock causes us to defer
> > probing. Let's not do that.
> > 
> 
> This is by intention.  Probe deferral is not supported by drivers
> using module_platform_driver_probe().  EPROBE_DEFER results in not
> being able to get the clock and, thus, is handled like any other
> error.

Ah, good point. I overlooked that part.

Seems kind of inconsistent driver writing (what happens if someone wants
to use this driver on a more complex system, where some of the dependent
resources are not available at init time?), but I see in the git logs
that apparently this is intentional (to save memory?).

So, I'll NAK this patch.

Brian

[PATCH] mtd: nand: samsung: warn about un-parseable ECC info

[Brian Norris]

We don't handle cases larger than 7. We probably shouldn't pretend we
know the ECC step size in this case, and it's probably also good to
WARN() like we do in many other similar cases.

Cc: Hans de Goede <hdegoede@redhat.com>
Fixes: 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Compile tested only

 drivers/mtd/nand/nand_samsung.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c
index 9cfc4035a420..1e0755997762 100644
--- a/drivers/mtd/nand/nand_samsung.c
+++ b/drivers/mtd/nand/nand_samsung.c
@@ -84,6 +84,9 @@ static void samsung_nand_decode_id(struct nand_chip *chip)
 			case 7:
 				chip->ecc_strength_ds = 60;
 				break;
+			default:
+				WARN(1, "Could not decode ECC info");
+				chip->ecc_step_ds = 0;
 			}
 		}
 	} else {
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH] mtd: nand: samsung: warn about un-parseable ECC info

[Boris Brezillon]

On Mon,  1 May 2017 17:04:55 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> We don't handle cases larger than 7. We probably shouldn't pretend we
> know the ECC step size in this case, and it's probably also good to
> WARN() like we do in many other similar cases.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Fixes: 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_samsung.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c
> index 9cfc4035a420..1e0755997762 100644
> --- a/drivers/mtd/nand/nand_samsung.c
> +++ b/drivers/mtd/nand/nand_samsung.c
> @@ -84,6 +84,9 @@ static void samsung_nand_decode_id(struct nand_chip *chip)
>  			case 7:
>  				chip->ecc_strength_ds = 60;
>  				break;
> +			default:
> +				WARN(1, "Could not decode ECC info");
> +				chip->ecc_step_ds = 0;
>  			}
>  		}
>  	} else {

Re: [PATCH] mtd: nand: samsung: warn about un-parseable ECC info

[Boris Brezillon]

On Mon,  1 May 2017 17:04:55 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> We don't handle cases larger than 7. We probably shouldn't pretend we
> know the ECC step size in this case, and it's probably also good to
> WARN() like we do in many other similar cases.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Fixes: 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Applied to nand/fixes.

Thanks,

Boris

> ---
> Compile tested only
> 
>  drivers/mtd/nand/nand_samsung.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c
> index 9cfc4035a420..1e0755997762 100644
> --- a/drivers/mtd/nand/nand_samsung.c
> +++ b/drivers/mtd/nand/nand_samsung.c
> @@ -84,6 +84,9 @@ static void samsung_nand_decode_id(struct nand_chip *chip)
>  			case 7:
>  				chip->ecc_strength_ds = 60;
>  				break;
> +			default:
> +				WARN(1, "Could not decode ECC info");
> +				chip->ecc_step_ds = 0;
>  			}
>  		}
>  	} else {

Re: [PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Ezequiel Garcia]

On 1 May 2017 at 21:04, Brian Norris <computersforpeace@gmail.com> wrote:
> This bug seems to have been here forever, although we came close to
> fixing all of them in [1]!
>
> [1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")
>

Well, we came even closer. See [1] for a patch that fixes this by cleaning
BBT init and release.

Back then Boris suggested on IRC to wait for Peter Pan's BBT, which would
address this too.

It's interesting to note that: (1) the patch never got any formal feedback,
despite it was a fix; and (2) Peter Pan's work is still under development.

Guess I should have pressed the upstream buttons harder :-)

[1] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066834.html,
[2] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066835.html
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Brian Norris]

On Mon, May 01, 2017 at 09:22:03PM -0300, Ezequiel Garcia wrote:
> On 1 May 2017 at 21:04, Brian Norris <computersforpeace@gmail.com> wrote:
> > This bug seems to have been here forever, although we came close to
> > fixing all of them in [1]!
> >
> > [1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")
> >
> 
> Well, we came even closer. See [1] for a patch that fixes this by cleaning
> BBT init and release.

That's a different bug(fix), no?

> Back then Boris suggested on IRC to wait for Peter Pan's BBT, which would
> address this too.

Haha, well bad strategy I guess :)

> It's interesting to note that: (1) the patch never got any formal feedback,
> despite it was a fix; and (2) Peter Pan's work is still under development.

Yes, well you kinda nacked yourself, so (1) isn't that surprising. And
(2) is well...no comment.

> Guess I should have pressed the upstream buttons harder :-)

I suppose.

I'd be happy to see you resubmit, since I definitely would prioritize
bugfixes over years-long refactoring. But I guess I'd defer to Boris, et
al, who are paying much more attention to NAND stuff these days than I
am. I just noticed this when bugfixing some things I noticed in Boris's
pull request.

Brian

> [1] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066834.html,
> [2] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066835.html
> -- 
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Ezequiel Garcia]

On 1 May 2017 at 22:33, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, May 01, 2017 at 09:22:03PM -0300, Ezequiel Garcia wrote:
>> On 1 May 2017 at 21:04, Brian Norris <computersforpeace@gmail.com> wrote:
>> > This bug seems to have been here forever, although we came close to
>> > fixing all of them in [1]!
>> >
>> > [1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")
>> >
>>
>> Well, we came even closer. See [1] for a patch that fixes this by cleaning
>> BBT init and release.
>
> That's a different bug(fix), no?
>

Oops, I really jumped on the gun here. The patch looks good.

Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Boris Brezillon]

On Mon,  1 May 2017 17:04:50 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> This bug seems to have been here forever, although we came close to
> fixing all of them in [1]!
> 
> [1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")
> 
> Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> The goto isn't *really* necessary, but I thought it'd be more consistent.
> 
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 978242b1213f..e4919f9dece4 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4794,7 +4794,11 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		return 0;
>  
>  	/* Build bad block table */
> -	return chip->scan_bbt(mtd);
> +	ret = chip->scan_bbt(mtd);
> +	if (ret)
> +		goto err_free;
> +	return 0;
> +
>  err_free:
>  	if (nbuf) {
>  		kfree(nbuf->databuf);

Re: [PATCH] mtd: nand: don't leak buffers when ->scan_bbt() fails

[Boris Brezillon]

On Mon,  1 May 2017 17:04:50 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> This bug seems to have been here forever, although we came close to
> fixing all of them in [1]!
> 
> [1] 11eaf6df1cce ("mtd: nand: Remove BUG() abuse in nand_scan_tail")
> 
> Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Applied to nand/fixes.

Thanks,

Boris

> ---
> The goto isn't *really* necessary, but I thought it'd be more consistent.
> 
> Compile tested only
> 
>  drivers/mtd/nand/nand_base.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 978242b1213f..e4919f9dece4 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4794,7 +4794,11 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		return 0;
>  
>  	/* Build bad block table */
> -	return chip->scan_bbt(mtd);
> +	ret = chip->scan_bbt(mtd);
> +	if (ret)
> +		goto err_free;
> +	return 0;
> +
>  err_free:
>  	if (nbuf) {
>  		kfree(nbuf->databuf);