Skip to contentSkip to conversation

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

PATCH Applied Brian Norris 4 participants 22 messages May 2, 2017 – May 15, 2017

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Brian Norris

author
#0 lore raw

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

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 <[email protected]>
Signed-off-by Brian Norris <[email protected]>

---
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(-)
drivers/mtd/nand/nand_base.c +5 -1
index 978242b1213f..e4919f9dece4 100644
@@ -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);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
author
#1 lore raw

PATCH 1/1 mtd: nand: samsung: warn about un-parseable ECC info

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 <[email protected]>
Fixes 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
Signed-off-by Brian Norris <[email protected]>

---
Compile tested only

 drivers/mtd/nand/nand_samsung.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mtd/nand/nand_samsung.c +3 -0
index 9cfc4035a420..1e0755997762 100644
@@ -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 {
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Acked-by on v1 1/1
#2 lore raw

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

Quoted 7 lines · On Mon, 1 May 2017 17:04:55 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Fixes 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
Signed-off-by Brian Norris <[email protected]>
Acked-by Boris Brezillon <[email protected]>
Quoted 20 lines

---
Compile tested only

 drivers/mtd/nand/nand_samsung.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mtd/nand/nand_samsung.c +3 -0
index 9cfc4035a420..1e0755997762 100644
@@ -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 {
Applied
#3 lore raw

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

In reply to Brian Norris
Quoted 7 lines · On Mon, 1 May 2017 17:04:55 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Fixes 8fc82d456e40 ("mtd: nand: samsung: Retrieve ECC requirements from extended ID")
Signed-off-by Brian Norris <[email protected]>

Applied to nand/fixes.

Thanks,

Boris

Quoted 20 lines

---
Compile tested only

 drivers/mtd/nand/nand_samsung.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/mtd/nand/nand_samsung.c +3 -0
index 9cfc4035a420..1e0755997762 100644
@@ -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 {
author
#4 lore raw

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

In reply to 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 <[email protected]>

---
Compile tested only

 drivers/mtd/nand/nand_base.c  | 11 +++++++++--
 drivers/mtd/nand/nand_hynix.c |  1 -
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/mtd/nand/nand_base.c +9 -2
index ed49a1d634b0..2adcc8cdedf1 100644
@@ -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);
+	}
 }
 
 /*
drivers/mtd/nand/nand_hynix.c +0 -1
index b12dc7325378..54d99f90aa9f 100644
@@ -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)
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
#5 lore raw

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

Quoted 3 lines · On Mon, 1 May 2017 17:04:51 -0700 Brian Norris <[email protected]>…

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.

Quoted 55 lines
Signed-off-by Brian Norris <[email protected]>

---
Compile tested only

 drivers/mtd/nand/nand_base.c  | 11 +++++++++--
 drivers/mtd/nand/nand_hynix.c |  1 -
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/mtd/nand/nand_base.c +9 -2
index ed49a1d634b0..2adcc8cdedf1 100644
@@ -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);
+	}
 }
 
 /*
drivers/mtd/nand/nand_hynix.c +0 -1
index b12dc7325378..54d99f90aa9f 100644
@@ -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)
author
#6 lore raw

PATCH 1/1 mtd: nand: drop unneeded module.h include

In reply to Brian Norris

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

Signed-off-by Brian Norris <[email protected]>
---
 drivers/mtd/nand/nand_ids.c | 1 -
 1 file changed, 1 deletion(-)
drivers/mtd/nand/nand_ids.c +0 -1
index 9d5ca0e540b5..92e2cf8e9ff9 100644
@@ -6,7 +6,6 @@
  * published by the Free Software Foundation.
  *
  */
-#include <linux/module.h>
 #include <linux/mtd/nand.h>
 #include <linux/sizes.h>
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Acked-by on v1 1/1
#7 lore raw

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

Quoted 3 lines · On Mon, 1 May 2017 17:04:52 -0700 Brian Norris <[email protected]>…

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

Signed-off-by Brian Norris <[email protected]>
Acked-by Boris Brezillon <[email protected]>
Quoted 16 lines
---
 drivers/mtd/nand/nand_ids.c | 1 -
 1 file changed, 1 deletion(-)
drivers/mtd/nand/nand_ids.c +0 -1
index 9d5ca0e540b5..92e2cf8e9ff9 100644
@@ -6,7 +6,6 @@
  * published by the Free Software Foundation.
  *
  */
-#include <linux/module.h>
 #include <linux/mtd/nand.h>
 #include <linux/sizes.h>
Applied
#8 lore raw

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

In reply to Brian Norris
Quoted 3 lines · On Mon, 1 May 2017 17:04:52 -0700 Brian Norris <[email protected]>…

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

Signed-off-by Brian Norris <[email protected]>

Applied to nand/fixes.

Thanks,

Boris

Quoted 16 lines
---
 drivers/mtd/nand/nand_ids.c | 1 -
 1 file changed, 1 deletion(-)
drivers/mtd/nand/nand_ids.c +0 -1
index 9d5ca0e540b5..92e2cf8e9ff9 100644
@@ -6,7 +6,6 @@
  * published by the Free Software Foundation.
  *
  */
-#include <linux/module.h>
 #include <linux/mtd/nand.h>
 #include <linux/sizes.h>
author
#9 lore raw

PATCH 1/1 mtd: nand: free vendor-specific resources in init failure paths

In reply to 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 <[email protected]>
Fixes 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
Signed-off-by Brian Norris <[email protected]>

---
Compile tested only

 drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)
drivers/mtd/nand/nand_base.c +29 -9
index 2adcc8cdedf1..978242b1213f 100644
@@ -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);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Acked-by on v1 1/1
#10 lore raw

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

Quoted 6 lines · On Mon, 1 May 2017 17:04:53 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Quoted 101 lines
Cc Boris Brezillon <[email protected]>
Fixes 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
Signed-off-by Brian Norris <[email protected]>

---
Compile tested only

 drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)
drivers/mtd/nand/nand_base.c +29 -9
index 2adcc8cdedf1..978242b1213f 100644
@@ -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);
#11 lore raw

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

Quoted 10 lines · On Tue, 2 May 2017 09:52:30 +0200 Boris Brezillon…
Quoted 6 lines · On Mon, 1 May 2017 17:04:53 -0700 Brian Norris <[email protected]>…

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().

Quoted 9 lines

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 <[email protected]>
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 <[email protected]>

---
Only compile tested.

---
 drivers/mtd/nand/nand_base.c | 79 ++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 35 deletions(-)
drivers/mtd/nand/nand_base.c +44 -35
index ed49a1d634b0..6ba03921eddb 100644
@@ -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);
Applied
#12 lore raw

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

In reply to Brian Norris
Quoted 10 lines · On Mon, 1 May 2017 17:04:53 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Fixes 626994e07480 ("mtd: nand: hynix: Add read-retry support for 1x nm MLC NANDs")
Signed-off-by Brian Norris <[email protected]>

Applied to nand/fixes.

Thanks,

Boris

Quoted 97 lines

---
Compile tested only

 drivers/mtd/nand/nand_base.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)
drivers/mtd/nand/nand_base.c +29 -9
index 2adcc8cdedf1..978242b1213f 100644
@@ -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);
author
#13 lore raw

PATCH 1/1 mtd: nand: orion: don't complain for probe deferral

In reply to 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 <[email protected]>
Signed-off-by Brian Norris <[email protected]>

---
Compile tested only

 drivers/mtd/nand/orion_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mtd/nand/orion_nand.c +2 -1
index f8e463a97b9e..e2bfb37f11df 100644
@@ -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;
 		}
 	}
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Acked-by on v1 1/1
#14 lore raw

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

Quoted 6 lines · On Mon, 1 May 2017 17:04:54 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Acked-by Boris Brezillon <[email protected]>
Quoted 20 lines

---
Compile tested only

 drivers/mtd/nand/orion_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mtd/nand/orion_nand.c +2 -1
index f8e463a97b9e..e2bfb37f11df 100644
@@ -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;
 		}
 	}
#15 lore raw

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

In reply to Brian Norris

Hi Brian,

Quoted 4 lines · 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.

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

author
#16 lore raw

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

Quoted 8 lines · Simon Baatz

Hi Brian,

Quoted 4 lines · 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.

Quoted 4 lines

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

#17 lore raw
In reply to Brian Norris
Quoted 5 lines · 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")

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

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
author
#18 lore raw
Quoted 9 lines · Ezequiel Garcia
Quoted 5 lines · 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")

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?

Quoted 2 lines

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

Haha, well bad strategy I guess :)

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 5 lines

[1] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066834.html,
[2] http://lists.infradead.org/pipermail/linux-mtd/2016-April/066835.html

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
Acked-by
#19 lore raw
Quoted 5 lines · Brian Norris
Quoted 4 lines · Ezequiel Garcia
Quoted 3 lines · Brian Norris

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

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

[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 <[email protected]>
Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
Acked-by on v1 1/1
#20 lore raw
In reply to Brian Norris
Quoted 7 lines · On Mon, 1 May 2017 17:04:50 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Signed-off-by Brian Norris <[email protected]>
Acked-by Boris Brezillon <[email protected]>
Quoted 25 lines

---
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(-)
drivers/mtd/nand/nand_base.c +5 -1
index 978242b1213f..e4919f9dece4 100644
@@ -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);
Applied
#21 lore raw
In reply to Brian Norris
Quoted 7 lines · On Mon, 1 May 2017 17:04:50 -0700 Brian Norris <[email protected]>…

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 <[email protected]>
Signed-off-by Brian Norris <[email protected]>

Applied to nand/fixes.

Thanks,

Boris

Quoted 25 lines

---
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(-)
drivers/mtd/nand/nand_base.c +5 -1
index 978242b1213f..e4919f9dece4 100644
@@ -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);