[PATCH 0/8] NAND and NAND-BBT improvements

[PATCH 0/8] NAND and NAND-BBT improvements

[Brian Norris]

Hi,

This series includes several assorted changes to MTD, NAND, and NAND-BBT code.
I wanted to get these out there now, as I will be off the radar for the next
week. These aren't too thoroughly tested, but for those that are somewhat
obvious, feel free to merge them. Others are meant for some discussion; feel
free to improve them and resubmit yourself.

Of note: the first two patches address a problem with mtd_read_oob() and the
recent max_bitflips changes by Mike Dunn. If they are satisfactory, it may be
worth merging them in the 3.5-rc cycle, as they help avoid an API inconsistency
that recently emerged.

"mtd: nand: use ECC, if present, when scanning OOB"
This patch may not be needed/wanted, as it affects the NAND_BBT_ABSPAGE
codepath. Refer to previous discussions with Mike Dunn regarding diskonchip.c.

Regards,
Brian

Brian Norris (8):
  mtd: move mtd_read_oob() definition out of mtd.h
  mtd: check for max_bitflips in mtd_read_oob()
  mtd: nand: rename "no_bbt" descriptors to "no_oob"
  mtd: nand: remove unused 'int' return codes
  mtd: nand: rename '_raw' BBT scan functions
  mtd: nand_bbt: refactor check_pattern_no_oob()
  mtd: nand_bbt: use string library
  mtd: nand: use ECC, if present, when scanning OOB

 drivers/mtd/mtdcore.c       |   21 +++++++++
 drivers/mtd/nand/nand_bbt.c |  108 +++++++++++++++++++++----------------------
 include/linux/mtd/mtd.h     |    9 +---
 3 files changed, 75 insertions(+), 63 deletions(-)

-- 
1.7.10

[PATCH 1/8] mtd: move mtd_read_oob() definition out of mtd.h

[Brian Norris]

mtd_read_oob() will be expanded a little, so don't leave it in the header
as a static inline function.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/mtdcore.c   |    9 +++++++++
 include/linux/mtd/mtd.h |    9 +--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5757307..fcfce24 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -858,6 +858,15 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 }
 EXPORT_SYMBOL_GPL(mtd_panic_write);
 
+int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
+{
+	ops->retlen = ops->oobretlen = 0;
+	if (!mtd->_read_oob)
+		return -EOPNOTSUPP;
+	return mtd->_read_oob(mtd, from, ops);
+}
+EXPORT_SYMBOL_GPL(mtd_read_oob);
+
 /*
  * Method to access the protection register area, present in some flash
  * devices. The user data is one time programmable but the factory data is read
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 63dadc0..81d61e7 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -265,14 +265,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		    const u_char *buf);
 
-static inline int mtd_read_oob(struct mtd_info *mtd, loff_t from,
-			       struct mtd_oob_ops *ops)
-{
-	ops->retlen = ops->oobretlen = 0;
-	if (!mtd->_read_oob)
-		return -EOPNOTSUPP;
-	return mtd->_read_oob(mtd, from, ops);
-}
+int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
 
 static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
 				struct mtd_oob_ops *ops)
-- 
1.7.10

[PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Brian Norris]

mtd_read_oob() has some unexpected similarities to mtd_read(). For
instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
however, when ops->datbuf == NULL, nand_base's code potentially could
return -EUCLEAN (no in-tree drivers do this yet). In any case where the
driver might return max_bitflips, we should translate this into an
appropriate return code using the bitflip_threshold.

Essentially, mtd_read_oob() duplicates the logic from mtd_read().

This prevents users of mtd_read_oob() from receiving a positive return
value (i.e., from max_bitflips) and interpreting it as an unknown error.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/mtdcore.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index fcfce24..75288d3 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -860,10 +860,22 @@ EXPORT_SYMBOL_GPL(mtd_panic_write);
 
 int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 {
+	int ret_code;
 	ops->retlen = ops->oobretlen = 0;
 	if (!mtd->_read_oob)
 		return -EOPNOTSUPP;
-	return mtd->_read_oob(mtd, from, ops);
+	/*
+	 * In cases where ops->datbuf != NULL, mtd->_read_oob() can have
+	 * semantics similar to mtd->_read(), regarding max bitflips. In other
+	 * cases, mtd->_read_oob() may return -EUCLEAN. In all cases, perform
+	 * similar logic to mtd_read() (see above).
+	 */
+	ret_code = mtd->_read_oob(mtd, from, ops);
+	if (unlikely(ret_code < 0))
+		return ret_code;
+	if (mtd->ecc_strength == 0)
+		return 0;	/* device lacks ecc */
+	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read_oob);
 
-- 
1.7.10

[PATCH 3/8] mtd: nand: rename "no_bbt" descriptors to "no_oob"

[Brian Norris]

These descriptors are for BBT's that don't use OOB; the "no_bbt" name doesn't
really make sense.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index c126469..dff24fa 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1274,7 +1274,7 @@ static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = mirror_pattern
 };
 
-static struct nand_bbt_descr bbt_main_no_bbt_descr = {
+static struct nand_bbt_descr bbt_main_no_oob_descr = {
 	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
 		| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP
 		| NAND_BBT_NO_OOB,
@@ -1284,7 +1284,7 @@ static struct nand_bbt_descr bbt_main_no_bbt_descr = {
 	.pattern = bbt_pattern
 };
 
-static struct nand_bbt_descr bbt_mirror_no_bbt_descr = {
+static struct nand_bbt_descr bbt_mirror_no_oob_descr = {
 	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
 		| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP
 		| NAND_BBT_NO_OOB,
@@ -1355,8 +1355,8 @@ int nand_default_bbt(struct mtd_info *mtd)
 		/* Use the default pattern descriptors */
 		if (!this->bbt_td) {
 			if (this->bbt_options & NAND_BBT_NO_OOB) {
-				this->bbt_td = &bbt_main_no_bbt_descr;
-				this->bbt_md = &bbt_mirror_no_bbt_descr;
+				this->bbt_td = &bbt_main_no_oob_descr;
+				this->bbt_md = &bbt_mirror_no_oob_descr;
 			} else {
 				this->bbt_td = &bbt_main_descr;
 				this->bbt_md = &bbt_mirror_descr;
-- 
1.7.10

[PATCH 4/8] mtd: nand: remove unused 'int' return codes

[Brian Norris]

The return codes for read_abs_bbts() and search_read_bbts() are always
non-zero, and so don't have much meaning. Just remove them.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index dff24fa..4123705 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -373,8 +373,8 @@ static u32 bbt_get_ver_offs(struct mtd_info *mtd, struct nand_bbt_descr *td)
  * Read the bad block table(s) for all chips starting at a given page. We
  * assume that the bbt bits are in consecutive order.
  */
-static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
-			 struct nand_bbt_descr *td, struct nand_bbt_descr *md)
+static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
+			  struct nand_bbt_descr *td, struct nand_bbt_descr *md)
 {
 	struct nand_chip *this = mtd->priv;
 
@@ -395,7 +395,6 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		pr_info("Bad block table at page %d, version 0x%02X\n",
 			 md->pages[0], md->version[0]);
 	}
-	return 1;
 }
 
 /* Scan a given block full */
@@ -626,7 +625,9 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
  *
  * Search and read the bad block table(s).
  */
-static int search_read_bbts(struct mtd_info *mtd, uint8_t * buf, struct nand_bbt_descr *td, struct nand_bbt_descr *md)
+static void search_read_bbts(struct mtd_info *mtd, uint8_t * buf,
+			     struct nand_bbt_descr *td,
+			     struct nand_bbt_descr *md)
 {
 	/* Search the primary table */
 	search_bbt(mtd, buf, td);
@@ -634,9 +635,6 @@ static int search_read_bbts(struct mtd_info *mtd, uint8_t * buf, struct nand_bbt
 	/* Search the mirror table */
 	if (md)
 		search_bbt(mtd, buf, md);
-
-	/* Force result check */
-	return 1;
 }
 
 /**
@@ -1162,14 +1160,13 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 
 	/* Is the bbt at a given page? */
 	if (td->options & NAND_BBT_ABSPAGE) {
-		res = read_abs_bbts(mtd, buf, td, md);
+		read_abs_bbts(mtd, buf, td, md);
 	} else {
 		/* Search the bad block table using a pattern in oob */
-		res = search_read_bbts(mtd, buf, td, md);
+		search_read_bbts(mtd, buf, td, md);
 	}
 
-	if (res)
-		res = check_create(mtd, buf, bd);
+	res = check_create(mtd, buf, bd);
 
 	/* Prevent the bbt regions from erasing / writing */
 	mark_bbt_region(mtd, td);
-- 
1.7.10

[PATCH 5/8] mtd: nand: rename '_raw' BBT scan functions

[Brian Norris]

None of these scanning functions use MTD_OPS_RAW mode any more, so there's
really nothing 'raw' about them. Rename them to (hopefully) make the code
a little clearer.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 4123705..35e8cdf 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -288,7 +288,7 @@ static int read_abs_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_desc
 }
 
 /* BBT marker is in the first page, no OOB */
-static int scan_read_raw_data(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
+static int scan_read_data(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 			 struct nand_bbt_descr *td)
 {
 	size_t retlen;
@@ -301,8 +301,8 @@ static int scan_read_raw_data(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 	return mtd_read(mtd, offs, len, &retlen, buf);
 }
 
-/* Scan read raw data from flash */
-static int scan_read_raw_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
+/* Scan read data from flash */
+static int scan_read_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 			 size_t len)
 {
 	struct mtd_oob_ops ops;
@@ -329,13 +329,13 @@ static int scan_read_raw_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 	return 0;
 }
 
-static int scan_read_raw(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
+static int scan_read(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 			 size_t len, struct nand_bbt_descr *td)
 {
 	if (td->options & NAND_BBT_NO_OOB)
-		return scan_read_raw_data(mtd, buf, offs, td);
+		return scan_read_data(mtd, buf, offs, td);
 	else
-		return scan_read_raw_oob(mtd, buf, offs, len);
+		return scan_read_oob(mtd, buf, offs, len);
 }
 
 /* Scan write data with oob to flash */
@@ -380,7 +380,7 @@ static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 
 	/* Read the primary version, if available */
 	if (td->options & NAND_BBT_VERSION) {
-		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
+		scan_read(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
 		pr_info("Bad block table at page %d, version 0x%02X\n",
@@ -389,7 +389,7 @@ static void read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 
 	/* Read the mirror version, if available */
 	if (md && (md->options & NAND_BBT_VERSION)) {
-		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
+		scan_read(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
 			      mtd->writesize, md);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
 		pr_info("Bad block table at page %d, version 0x%02X\n",
@@ -404,7 +404,7 @@ static int scan_block_full(struct mtd_info *mtd, struct nand_bbt_descr *bd,
 {
 	int ret, j;
 
-	ret = scan_read_raw_oob(mtd, buf, offs, readlen);
+	ret = scan_read_oob(mtd, buf, offs, readlen);
 	/* Ignore ECC errors when checking for BBM */
 	if (ret && !mtd_is_bitflip_or_eccerr(ret))
 		return ret;
@@ -593,7 +593,7 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
 			loff_t offs = (loff_t)actblock << this->bbt_erase_shift;
 
 			/* Read first page */
-			scan_read_raw(mtd, buf, offs, mtd->writesize, td);
+			scan_read(mtd, buf, offs, mtd->writesize, td);
 			if (!check_pattern(buf, scanlen, mtd->writesize, td)) {
 				td->pages[i] = actblock << blocktopage;
 				if (td->options & NAND_BBT_VERSION) {
-- 
1.7.10

[PATCH 6/8] mtd: nand_bbt: refactor check_pattern_no_oob()

[Brian Norris]

This function only returns 0 or -1, so make that clear.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 35e8cdf..ca07230 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -71,12 +71,9 @@
 
 static int check_pattern_no_oob(uint8_t *buf, struct nand_bbt_descr *td)
 {
-	int ret;
-
-	ret = memcmp(buf, td->pattern, td->len);
-	if (!ret)
-		return ret;
-	return -1;
+	if (memcmp(buf, td->pattern, td->len))
+		return -1;
+	return 0;
 }
 
 /**
-- 
1.7.10

[PATCH 7/8] mtd: nand_bbt: use string library

[Brian Norris]

Some nand_bbt code can be shortened by using memcmp() and memchr_inv().
As an added bonus, there is a possbile performance benefit.

Borrowed some code from Akinobu Mita.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ca07230..45cdb97 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -68,6 +68,7 @@
 #include <linux/delay.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
+#include <linux/string.h>
 
 static int check_pattern_no_oob(uint8_t *buf, struct nand_bbt_descr *td)
 {
@@ -89,19 +90,16 @@ static int check_pattern_no_oob(uint8_t *buf, struct nand_bbt_descr *td)
  */
 static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
 {
-	int i, end = 0;
+	int end = 0;
 	uint8_t *p = buf;
 
 	if (td->options & NAND_BBT_NO_OOB)
 		return check_pattern_no_oob(buf, td);
 
 	end = paglen + td->offs;
-	if (td->options & NAND_BBT_SCANEMPTY) {
-		for (i = 0; i < end; i++) {
-			if (p[i] != 0xff)
-				return -1;
-		}
-	}
+	if (td->options & NAND_BBT_SCANEMPTY)
+		if (memchr_inv(p, 0xff, end))
+			return -1;
 	p += end;
 
 	/* Compare the pattern */
@@ -111,10 +109,8 @@ static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
 	if (td->options & NAND_BBT_SCANEMPTY) {
 		p += td->len;
 		end += td->len;
-		for (i = end; i < len; i++) {
-			if (*p++ != 0xff)
-				return -1;
-		}
+		if (memchr_inv(p, 0xff, len - end))
+			return -1;
 	}
 	return 0;
 }
@@ -130,14 +126,9 @@ static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
  */
 static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
 {
-	int i;
-	uint8_t *p = buf;
-
 	/* Compare the pattern */
-	for (i = 0; i < td->len; i++) {
-		if (p[td->offs + i] != td->pattern[i])
-			return -1;
-	}
+	if (memcmp(buf + td->offs, td->pattern, td->len))
+		return -1;
 	return 0;
 }
 
-- 
1.7.10

[PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Brian Norris]

scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
added bonus of error correction.

This brings scan_block_full() in line with scan_block_fast() so that they
both read bad block markers with MTD_OPS_PLACE_OOB. This can help in
preventing 0xff markers (in good blocks) from being interpreted as bad
block indicators in the presence of a single bitflip.

Note that ECC error codes (EUCLEAN or EBADMSG) are already silently
ignored in all users of scan_read_raw_oob().

[1] Few  drivers perform proper error correction on OOB data. In those
    cases, the use of MTD_OPS_RAW vs. MTD_OPS_PLACE_OOB is not
    significant.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Mike Dunn <mikedunn@newsguy.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mtd/nand/nand_bbt.c |   27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 45cdb97..7b9a0a7 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -289,14 +289,24 @@ static int scan_read_data(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 	return mtd_read(mtd, offs, len, &retlen, buf);
 }
 
-/* Scan read data from flash */
+/**
+ * scan_read_oob - [GENERIC] Scan data+OOB region to buffer
+ * @mtd: MTD device structure
+ * @buf: temporary buffer
+ * @offs: offset at which to scan
+ * @len: length of data region to read
+ *
+ * Scan read data from data+OOB. May traverse multiple pages, interleaving
+ * page,OOB,page,OOB,... in buf. Completes transfer and returns the "strongest"
+ * ECC condition (error or bitflip). May quit on the first (non-ECC) error.
+ */
 static int scan_read_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 			 size_t len)
 {
 	struct mtd_oob_ops ops;
-	int res;
+	int res, ret = 0;
 
-	ops.mode = MTD_OPS_RAW;
+	ops.mode = MTD_OPS_PLACE_OOB;
 	ops.ooboffs = 0;
 	ops.ooblen = mtd->oobsize;
 
@@ -306,15 +316,18 @@ static int scan_read_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
 		ops.oobbuf = buf + ops.len;
 
 		res = mtd_read_oob(mtd, offs, &ops);
-
-		if (res)
-			return res;
+		if (res) {
+			if (!mtd_is_bitflip_or_eccerr(res))
+				return res;
+			else if (mtd_is_eccerr(res) || !ret)
+				ret = res;
+		}
 
 		buf += mtd->oobsize + mtd->writesize;
 		len -= mtd->writesize;
 		offs += mtd->writesize;
 	}
-	return 0;
+	return ret;
 }
 
 static int scan_read(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
-- 
1.7.10

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Shmulik Ladkani]

Hi Brian,

On Fri, 22 Jun 2012 16:35:39 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index fcfce24..75288d3 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -860,10 +860,22 @@ EXPORT_SYMBOL_GPL(mtd_panic_write);
>  
>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>  {
> +	int ret_code;
>  	ops->retlen = ops->oobretlen = 0;
>  	if (!mtd->_read_oob)
>  		return -EOPNOTSUPP;
> -	return mtd->_read_oob(mtd, from, ops);
> +	/*
> +	 * In cases where ops->datbuf != NULL, mtd->_read_oob() can have

s/can have/has/

> +	 * semantics similar to mtd->_read(), regarding max bitflips. In other

Please consider:
s/regarding max bitflips/returning a non-negative integer representing max bitflips/

Other than these tiny comment amendments,

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Thanks,
Shmulik

Re: [PATCH 4/8] mtd: nand: remove unused 'int' return codes

[Shmulik Ladkani]

On Fri, 22 Jun 2012 16:35:41 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> The return codes for read_abs_bbts() and search_read_bbts() are always
> non-zero, and so don't have much meaning. Just remove them.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Re: [PATCH 5/8] mtd: nand: rename '_raw' BBT scan functions

[Shmulik Ladkani]

Hi Brian,

On Fri, 22 Jun 2012 16:35:42 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> None of these scanning functions use MTD_OPS_RAW mode any more, so there's
> really nothing 'raw' about them. Rename them to (hopefully) make the code
> a little clearer.

Minor nitpick: I guess this patch should be the last in your patchset.

This is since 'scan_read_raw_oob()' (renamed to 'scan_read_oob' by this
patch) still uses MTD_OPS_RAW; hence the statement "None of these
scanning functions use MTD_OPS_RAW mode any more" is incorrect at the
time this patch is introduced.

Regards,
Shmulik

Re: [PATCH 7/8] mtd: nand_bbt: use string library

[Shmulik Ladkani]

Hi Brian,

On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>  {
> -	int i, end = 0;
> +	int end = 0;
>  	uint8_t *p = buf;
>  
>  	if (td->options & NAND_BBT_NO_OOB)
>  		return check_pattern_no_oob(buf, td);
>  
>  	end = paglen + td->offs;
> -	if (td->options & NAND_BBT_SCANEMPTY) {
> -		for (i = 0; i < end; i++) {
> -			if (p[i] != 0xff)
> -				return -1;
> -		}
> -	}
> +	if (td->options & NAND_BBT_SCANEMPTY)
> +		if (memchr_inv(p, 0xff, end))
> +			return -1;
>  	p += end;
>  

A question regarding the original code:
Why the region validated for 0xff is [0 , paglen+td->of) ?
I assume this buffer region contains the inband data. Why verify inband
data is all 0xff?
Shouldn't the region validated be [paglen , paglen+td->of) ?

Regards,
Shmulik

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Shmulik Ladkani]

Hi,

On Fri, 22 Jun 2012 16:35:45 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.

Please consider rephrasing, text is bit confusing.

> + * Scan read data from data+OOB. May traverse multiple pages, interleaving
> + * page,OOB,page,OOB,... in buf.

In the case 'len' is less than 'mtd->writesize' (possible from a
'scan_block_full' call), the read oob will be placed immediately after
the partially read page, quoting scan_read_raw_oob:

		ops.len = min(len, (size_t)mtd->writesize);
		ops.oobbuf = buf + ops.len;

Any idea if this is intentional or a bug?

Regards,
Shmulik

Re: [PATCH 4/8] mtd: nand: remove unused 'int' return codes (SPAM)

[William F.]

Em 26-06-2012 09:29, Shmulik Ladkani escreveu:
> On Fri, 22 Jun 2012 16:35:41 -0700 Brian Norris<computersforpeace@gmail.com>  wrote:
>> The return codes for read_abs_bbts() and search_read_bbts() are always
>> non-zero, and so don't have much meaning. Just remove them.
>>
>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
> Reviewed-by: Shmulik Ladkani<shmulik.ladkani@gmail.com>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Mike Dunn]

On 06/22/2012 04:35 PM, Brian Norris wrote:
> mtd_read_oob() has some unexpected similarities to mtd_read(). For
> instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
> however, when ops->datbuf == NULL, nand_base's code potentially could
> return -EUCLEAN (no in-tree drivers do this yet). In any case where the
> driver might return max_bitflips, we should translate this into an
> appropriate return code using the bitflip_threshold.
> 
> Essentially, mtd_read_oob() duplicates the logic from mtd_read().
> 
> This prevents users of mtd_read_oob() from receiving a positive return
> value (i.e., from max_bitflips) and interpreting it as an unknown error.


Reviewed-by Mike Dunn <mikedunn@newsguy.com>

This should fix the problem, but it's still confusing and inconsistent; see below...


> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> Cc: Mike Dunn <mikedunn@newsguy.com>
> ---
>  drivers/mtd/mtdcore.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index fcfce24..75288d3 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -860,10 +860,22 @@ EXPORT_SYMBOL_GPL(mtd_panic_write);
>  
>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>  {
> +	int ret_code;
>  	ops->retlen = ops->oobretlen = 0;
>  	if (!mtd->_read_oob)
>  		return -EOPNOTSUPP;
> -	return mtd->_read_oob(mtd, from, ops);
> +	/*
> +	 * In cases where ops->datbuf != NULL, mtd->_read_oob() can have
> +	 * semantics similar to mtd->_read(), regarding max bitflips. In other
> +	 * cases, mtd->_read_oob() may return -EUCLEAN. In all cases, perform
> +	 * similar logic to mtd_read() (see above).
> +	 */
> +	ret_code = mtd->_read_oob(mtd, from, ops);
> +	if (unlikely(ret_code < 0))
> +		return ret_code;


As Brian explains in the comment, here the return code propagated up could be
-EUCLEAN for an oob-only read (ops->databuf == NULL), which is unlike the
mtd_read() case, where here only a hard error will be propagated up.  To be
consistent, nand_do_read_oob(), and non-nand drivers' implementation of
mtd->_read_oob(), should not return -EUCLEAN.  When (or if) a policy is decided
for reporting bitflips within the oob area, this will need to be revisited.

Thanks Brian!


> +	if (mtd->ecc_strength == 0)
> +		return 0;	/* device lacks ecc */
> +	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_read_oob);
>  

Re: [PATCH 0/8] NAND and NAND-BBT improvements

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

Pushed these patches to l2-mtd.git, thanks!

mtd: move mtd_read_oob() definition out of mtd.h
mtd: nand_bbt: refactor check_pattern_no_oob()
mtd: nand: rename "no_bbt" descriptors to "no_oob"


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/8] mtd: nand: rename '_raw' BBT scan functions

[Brian Norris]

On Tue, Jun 26, 2012 at 5:39 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Fri, 22 Jun 2012 16:35:42 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>> None of these scanning functions use MTD_OPS_RAW mode any more, so there's
>> really nothing 'raw' about them. Rename them to (hopefully) make the code
>> a little clearer.
>
> Minor nitpick: I guess this patch should be the last in your patchset.

You're correct. I think I somewhat-arbitrarily rearranged (rebased)
some of these patches before submission. Will fix if/when I resubmit.

Thanks,
Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Brian Norris]

On Tue, Jun 26, 2012 at 7:09 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Fri, 22 Jun 2012 16:35:45 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
>> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
>
> Please consider rephrasing, text is bit confusing.

I realized I was missing the word "to". So the edit would read:

scan_read_raw_oob() is used in only in places where the
MTD_OPS_PLACE_OOB mode is preferable to MTD_OPS_RAW mode, so use
MTD_OPS_PLACE_OOB instead.

I can rephrase more, regarding what each mode does, but I'm wondering
if this clears up your understanding.

>> + * Scan read data from data+OOB. May traverse multiple pages, interleaving
>> + * page,OOB,page,OOB,... in buf.
>
> In the case 'len' is less than 'mtd->writesize' (possible from a
> 'scan_block_full' call), the read oob will be placed immediately after
> the partially read page, quoting scan_read_raw_oob:
>
>                 ops.len = min(len, (size_t)mtd->writesize);
>                 ops.oobbuf = buf + ops.len;
>
> Any idea if this is intentional or a bug?

Not really. I honestly don't fully understand many of the options in
NAND_BBT, but I'm trying to work through some of it. I think there's a
lot of half-baked stuff in here with not-very-good code structure.
Some of the code is written as if it were generic, but it's only meant
for some very specific circumstances.

With that said, here's a thought: it looks like your hypothetical "len
< mtd->writesize" would only occur with NAND_BBT_SCANEMPTY disabled
and NAND_BBT_SCANALLPAGES enabled. But I only see them used together:
NAND_BBT_SCANEMPTY | NAND_BBT_SCANALLPAGES (omap2.c and with
agand_flashbased in nand_bbt.c). So maybe it's a kind of "bug" for a
case that was never really thought out. And if they must be used
together, maybe we should document that.

Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Matthieu CASTET]

Hi Brian,

Brian Norris a écrit :
> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
> MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
> added bonus of error correction.
> 
> This brings scan_block_full() in line with scan_block_fast() so that they
> both read bad block markers with MTD_OPS_PLACE_OOB. This can help in
> preventing 0xff markers (in good blocks) from being interpreted as bad
> block indicators in the presence of a single bitflip.
As far I understand the code, this work when "chip->ecc.read_oob" (used in
nand_do_read_oob) correct bit flip.

But I see no "chip->ecc.read_oob" implementation that can return bit flip. Is
that expected ?

This can also work when nand_do_read_ops is used (ops->datbuf != NULL). But it
is hard to see case where it can correct bit flip in bad block marker. Do you
have any exemple ?

For example the default chip->ecc.read_page (nand_read_page_hwecc) doesn't read
oob with ecc [1].




Matthieu

PS : Did you have any comment on
http://thread.gmane.org/gmane.linux.drivers.mtd/42243 ?

[1]
   for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
        chip->ecc.hwctl(mtd, NAND_ECC_READ);
        chip->read_buf(mtd, p, eccsize);
        chip->ecc.calculate(mtd, p, &ecc_calc[i]);
    }
    chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);

> 
> Note that ECC error codes (EUCLEAN or EBADMSG) are already silently
> ignored in all users of scan_read_raw_oob().
> 
> [1] Few  drivers perform proper error correction on OOB data. In those
>     cases, the use of MTD_OPS_RAW vs. MTD_OPS_PLACE_OOB is not
>     significant.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> Cc: Mike Dunn <mikedunn@newsguy.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/mtd/nand/nand_bbt.c |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index 45cdb97..7b9a0a7 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -289,14 +289,24 @@ static int scan_read_data(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
>  	return mtd_read(mtd, offs, len, &retlen, buf);
>  }
>  
> -/* Scan read data from flash */
> +/**
> + * scan_read_oob - [GENERIC] Scan data+OOB region to buffer
> + * @mtd: MTD device structure
> + * @buf: temporary buffer
> + * @offs: offset at which to scan
> + * @len: length of data region to read
> + *
> + * Scan read data from data+OOB. May traverse multiple pages, interleaving
> + * page,OOB,page,OOB,... in buf. Completes transfer and returns the "strongest"
> + * ECC condition (error or bitflip). May quit on the first (non-ECC) error.
> + */
>  static int scan_read_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
>  			 size_t len)
>  {
>  	struct mtd_oob_ops ops;
> -	int res;
> +	int res, ret = 0;
>  
> -	ops.mode = MTD_OPS_RAW;
> +	ops.mode = MTD_OPS_PLACE_OOB;
>  	ops.ooboffs = 0;
>  	ops.ooblen = mtd->oobsize;
>  
> @@ -306,15 +316,18 @@ static int scan_read_oob(struct mtd_info *mtd, uint8_t *buf, loff_t offs,
>  		ops.oobbuf = buf + ops.len;
>  
>  		res = mtd_read_oob(mtd, offs, &ops);
> -
> -		if (res)
> -			return res;
> +		if (res) {
> +			if (!mtd_is_bitflip_or_eccerr(res))
> +				return res;
> +			else if (mtd_is_eccerr(res) || !ret)
> +				ret = res;
> +		}
>  
>  		buf += mtd->oobsize + mtd->writesize;
>  		len -= mtd->writesize;
>  		offs += mtd->writesize;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  static int scan_read(struct mtd_info *mtd, uint8_t *buf, loff_t offs,

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Brian Norris]

On Tue, Jun 26, 2012 at 11:23 AM, Mike Dunn <mikedunn@newsguy.com> wrote:
> Reviewed-by Mike Dunn <mikedunn@newsguy.com>

I'll add your/Shmulik's tags in a resend. This is getting a bit late
in the rc cycle, but it should still go in on 3.5 right?

> This should fix the problem, but it's still confusing and inconsistent; see below...
>
> On 06/22/2012 04:35 PM, Brian Norris wrote:
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index fcfce24..75288d3 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -860,10 +860,22 @@ EXPORT_SYMBOL_GPL(mtd_panic_write);
>>
>>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>>  {
>> +     int ret_code;
>>       ops->retlen = ops->oobretlen = 0;
>>       if (!mtd->_read_oob)
>>               return -EOPNOTSUPP;
>> -     return mtd->_read_oob(mtd, from, ops);
>> +     /*
>> +      * In cases where ops->datbuf != NULL, mtd->_read_oob() can have
>> +      * semantics similar to mtd->_read(), regarding max bitflips. In other
>> +      * cases, mtd->_read_oob() may return -EUCLEAN. In all cases, perform
>> +      * similar logic to mtd_read() (see above).
>> +      */
>> +     ret_code = mtd->_read_oob(mtd, from, ops);
>> +     if (unlikely(ret_code < 0))
>> +             return ret_code;
>
> As Brian explains in the comment, here the return code propagated up could be
> -EUCLEAN for an oob-only read (ops->databuf == NULL), which is unlike the
> mtd_read() case, where here only a hard error will be propagated up.  To be
> consistent, nand_do_read_oob(), and non-nand drivers' implementation of
> mtd->_read_oob(), should not return -EUCLEAN.  When (or if) a policy is decided
> for reporting bitflips within the oob area, this will need to be revisited.

Yeah, this seems to be a recurring theme: that no in-tree drivers
actually implement ECC for the OOB region. If no other drivers need to
report ECC flips/errors from OOB, then I can't really establish a
policy here. Perhaps I can rethink my driver sometime...

> Thanks Brian!

You're welcome,
Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Brian Norris]

On Tue, Jul 10, 2012 at 12:45 AM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> Brian Norris a écrit :
>> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
>> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
>> MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
>> added bonus of error correction.
>>
>> This brings scan_block_full() in line with scan_block_fast() so that they
>> both read bad block markers with MTD_OPS_PLACE_OOB. This can help in
>> preventing 0xff markers (in good blocks) from being interpreted as bad
>> block indicators in the presence of a single bitflip.
>
> As far I understand the code, this work when "chip->ecc.read_oob" (used in
> nand_do_read_oob) correct bit flip.
>
> But I see no "chip->ecc.read_oob" implementation that can return bit flip. Is
> that expected ?

I have an out-of-tree driver that corrects OOB bitflips. Is there
really no other HW out there that corrects OOB errors?

Anyway, I understand that my driver is an outlier here, but I don't
see a real disadvantage in these changes. But on the positive side, I
expect that in the future, more drivers/HW will either want to stop
using OOB for anything at all or will want ECC protection for OOB.

> This can also work when nand_do_read_ops is used (ops->datbuf != NULL). But it
> is hard to see case where it can correct bit flip in bad block marker. Do you
> have any exemple ?

First of all, this has no effect if the driver does not protect OOB
with ECC (i.e., for OOB-only reads, MTD_OPS_PLACE_OOB == MTD_OPS_RAW).
So the following argument only applies when OOB is ECC-protected.

Consider a *good* block that is written with filesystem data. On
bootup, Linux may scan this block's BBM to check if it is bad. If a
bitflip occurs in the bad block marker, then it may be erroneously
considered bad.

Similarly, if a block was marked bad from wear (not factory-marked),
then its BBM may be written along with ECC protection. Then, when we
scan for bad blocks, it will be protected from bitflips that could
possibly cause 0x00 to appear non-zero. (This is not a big issue,
since 'non-zero' is still bad, as long as 0x00 didn't flip to 0xff -
quite unlikely...)

> PS : Did you have any comment on
> http://thread.gmane.org/gmane.linux.drivers.mtd/42243 ?

I read it, and it seems promising. I agree with much of the premise
(that nand_bbt.c is ugly and repetitive at times) but haven't had
enough time to review properly. Sorry. I'm a bit backlogged and will
be for a few weeks, I think. But I'll see what I can do.

Thanks,
Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Mike Dunn]

On 07/13/2012 10:39 AM, Brian Norris wrote:
> 
> I have an out-of-tree driver that corrects OOB bitflips. Is there
> really no other HW out there that corrects OOB errors?


The diskonchip g3 and g4 devices use a hw-generated hamming byte to protect the
first eight oob bytes (those not used for page data ecc) with ecc of one bit
strength, but neither driver implements the correcting algorithm yet.


> 
> Anyway, I understand that my driver is an outlier here, but I don't
> see a real disadvantage in these changes. But on the positive side, I
> expect that in the future, more drivers/HW will either want to stop
> using OOB for anything at all or will want ECC protection for OOB.
> 
>> This can also work when nand_do_read_ops is used (ops->datbuf != NULL). But it
>> is hard to see case where it can correct bit flip in bad block marker. Do you
>> have any exemple ?
> 
> First of all, this has no effect if the driver does not protect OOB
> with ECC (i.e., for OOB-only reads, MTD_OPS_PLACE_OOB == MTD_OPS_RAW).
> So the following argument only applies when OOB is ECC-protected.
> 
> Consider a *good* block that is written with filesystem data. On
> bootup, Linux may scan this block's BBM to check if it is bad. If a
> bitflip occurs in the bad block marker, then it may be erroneously
> considered bad.


Yeah, this is a strong argument for ecc on oob-only reads.

Thanks,
Mike

Re: [PATCH 7/8] mtd: nand_bbt: use string library

[Brian Norris]

Add Ivan, as omap2.c is one of the only NAND_BBT_SCANEMPTY users.

On Tue, Jun 26, 2012 at 6:37 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>>  {
>> -     int i, end = 0;
>> +     int end = 0;
>>       uint8_t *p = buf;
>>
>>       if (td->options & NAND_BBT_NO_OOB)
>>               return check_pattern_no_oob(buf, td);
>>
>>       end = paglen + td->offs;
>> -     if (td->options & NAND_BBT_SCANEMPTY) {
>> -             for (i = 0; i < end; i++) {
>> -                     if (p[i] != 0xff)
>> -                             return -1;
>> -             }
>> -     }
>> +     if (td->options & NAND_BBT_SCANEMPTY)
>> +             if (memchr_inv(p, 0xff, end))
>> +                     return -1;
>>       p += end;
>>
>
> A question regarding the original code:

This is another instance of a NAND_BBT_* option that I don't really
understand, but I'll provide my thoughts...

> Why the region validated for 0xff is [0 , paglen+td->of) ?
> I assume this buffer region contains the inband data. Why verify inband
> data is all 0xff?

If used on a flash-based BBT page, then it checks for an empty table
(why would you need this?), and if used for factory-marked bad blocks,
then it checks that all the non-marker locations are 0xff. I guess the
latter is at least more reasonable, but that still doesn't really
answer "why?" So I'm guessing this another ill-conceived option.

> Shouldn't the region validated be [paglen , paglen+td->of) ?

Dunno.

Anyway, it's only used in a single driver (omap2.c) in
"bb_descrip_flashbased" which, despite its name, does not use a
flash-based BBT - also used in "agand_flashbased". So the option is
*only* used for OOB bad block markers, ensuring that the non-marker
positions are 0xff. But I don't see how it's valid to assume that the
data will be 0xff, nor do I know why someone would want to check that.

Finally, I think that some of the "use" of NAND_BBT_SCANEMPTY is
obscured by the fact that omap2.c's main codepath involves
(permanently) clearing NAND_BBT_SCANEMPTY in nand_memory_bbt(). See:
bd->options &= ~NAND_BBT_SCANEMPTY;

So, is this another candidate for removal, as it is practically unused
and unmaintained? Or any comments on its purpose, Ivan?

Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Ivan Djelic]

On Sun, Jul 15, 2012 at 10:01:24PM +0200, Mike Dunn wrote:
> On 07/13/2012 10:39 AM, Brian Norris wrote:
> > 
> > I have an out-of-tree driver that corrects OOB bitflips. Is there
> > really no other HW out there that corrects OOB errors?
> 
> 
> The diskonchip g3 and g4 devices use a hw-generated hamming byte to protect the
> first eight oob bytes (those not used for page data ecc) with ecc of one bit
> strength, but neither driver implements the correcting algorithm yet.
> 
> 
> > 
> > Anyway, I understand that my driver is an outlier here, but I don't
> > see a real disadvantage in these changes. But on the positive side, I
> > expect that in the future, more drivers/HW will either want to stop
> > using OOB for anything at all or will want ECC protection for OOB.
> > 
> >> This can also work when nand_do_read_ops is used (ops->datbuf != NULL). But it
> >> is hard to see case where it can correct bit flip in bad block marker. Do you
> >> have any exemple ?
> > 
> > First of all, this has no effect if the driver does not protect OOB
> > with ECC (i.e., for OOB-only reads, MTD_OPS_PLACE_OOB == MTD_OPS_RAW).
> > So the following argument only applies when OOB is ECC-protected.
> > 
> > Consider a *good* block that is written with filesystem data. On
> > bootup, Linux may scan this block's BBM to check if it is bad. If a
> > bitflip occurs in the bad block marker, then it may be erroneously
> > considered bad.
> 
> 
> Yeah, this is a strong argument for ecc on oob-only reads.

Hello Mike,

I think it is a strong argument for a robust reading of BBM, rather than an argument
for ECC on OOB-only reads. By "robust reading", I mean simply looking at the Hamming weight of the
marker (the number of 1s in the BBM) rather than its value, as done in nand_block_bad() by setting chip->badblockbits.

This robust reading is trivially implemented, does not depend on OOB ecc availability,
and benefits all drivers. Even if your driver implements OOB ECC, it may not work
on an erased block with a bitflip in its BBM (because erased data may not have a valid
ECC). Moreover, reading just the OOB region with ECC may require a full page read on some drivers
(when OOB and data are parts of the same codeword).

To me, the only strong reason for wanting OOB ECC is the implementation of YAFFS2 or similar filesystems
which require OOB metadata protection. But maybe I'm missing some other use cases ?

What do you think ?

BR,
-- 
Ivan

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Mike Dunn]

Hi Ivan, thanks again for the comments.

On 07/16/2012 07:01 AM, Ivan Djelic wrote:
> On Sun, Jul 15, 2012 at 10:01:24PM +0200, Mike Dunn wrote:
>
>> Yeah, this is a strong argument for ecc on oob-only reads.
> 
> Hello Mike,
> 
> I think it is a strong argument for a robust reading of BBM, rather than an argument
> for ECC on OOB-only reads. By "robust reading", I mean simply looking at the Hamming weight of the
> marker (the number of 1s in the BBM) rather than its value, as done in nand_block_bad() by setting chip->badblockbits.
> 
> This robust reading is trivially implemented, does not depend on OOB ecc availability,
> and benefits all drivers. Even if your driver implements OOB ECC, it may not work
> on an erased block with a bitflip in its BBM (because erased data may not have a valid
> ECC). 


This is a certainty, no?  Erased, by definition, includes any ecc bytes.


> Moreover, reading just the OOB region with ECC may require a full page read on some drivers
> (when OOB and data are parts of the same codeword).
> 
> To me, the only strong reason for wanting OOB ECC is the implementation of YAFFS2 or similar filesystems
> which require OOB metadata protection. But maybe I'm missing some other use cases ?
> 
> What do you think ?


If we assume the oob bytes on the first page of a good block can contain
anything, won't simply counting the bits make the risk of falsly identifying a
bb marker unacceptably high?

Thanks,
Mike

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Ivan Djelic]

On Mon, Jul 16, 2012 at 08:36:56PM +0200, Mike Dunn wrote:
> Hi Ivan, thanks again for the comments.
> 
> On 07/16/2012 07:01 AM, Ivan Djelic wrote:
> > On Sun, Jul 15, 2012 at 10:01:24PM +0200, Mike Dunn wrote:
> >
> >> Yeah, this is a strong argument for ecc on oob-only reads.
> > 
> > Hello Mike,
> > 
> > I think it is a strong argument for a robust reading of BBM, rather than an argument
> > for ECC on OOB-only reads. By "robust reading", I mean simply looking at the Hamming weight of the
> > marker (the number of 1s in the BBM) rather than its value, as done in nand_block_bad() by setting chip->badblockbits.
> > 
> > This robust reading is trivially implemented, does not depend on OOB ecc availability,
> > and benefits all drivers. Even if your driver implements OOB ECC, it may not work
> > on an erased block with a bitflip in its BBM (because erased data may not have a valid
> > ECC). 
> 
> 
> This is a certainty, no?  Erased, by definition, includes any ecc bytes.

If you add the right polynomial[1] to your Hamming or BCH code, then you can make
sure the ECC of an erased page (possibly with OOB bytes) is actually a sequence of 0xff bytes.
This trick enables bitflip correction on data that has never been programmed.
Take a look at nand_ecc.c:63 (or nand_bch.c:62) for an example.
This trick is not possible on all hardware ECC engines.
 
> 
> > Moreover, reading just the OOB region with ECC may require a full page read on some drivers
> > (when OOB and data are parts of the same codeword).
> > 
> > To me, the only strong reason for wanting OOB ECC is the implementation of YAFFS2 or similar filesystems
> > which require OOB metadata protection. But maybe I'm missing some other use cases ?
> > 
> > What do you think ?
> 
> 
> If we assume the oob bytes on the first page of a good block can contain
> anything, won't simply counting the bits make the risk of falsly identifying a
> bb marker unacceptably high?

Counting bits on a BBM marker byte basically gives a 4-bitflip protection on a _single_ byte, which is
*extremely* unlikely. If you assume an erased good block can contain garbage in its OOB region, then
it can indeed be wrongly identified as a bad block; but this is also true if you use OOB ECC
(e.g. exceeding correction capacity).
In practice, since the bad block marker byte is normally never programmed to anything other than 0xff,
there is no reason why we should find garbage in it (even if a power failure occurs during an
erase/program operation).
BR,

-- 
Ivan

[1] this polynomial is simply the inverted ecc of an erased ecc block

Re: [PATCH 7/8] mtd: nand_bbt: use string library

[Ivan Djelic]

On Mon, Jul 16, 2012 at 08:06:31AM +0200, Brian Norris wrote:
> Add Ivan, as omap2.c is one of the only NAND_BBT_SCANEMPTY users.
> 
> On Tue, Jun 26, 2012 at 6:37 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> >>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> >>  {
> >> -     int i, end = 0;
> >> +     int end = 0;
> >>       uint8_t *p = buf;
> >>
> >>       if (td->options & NAND_BBT_NO_OOB)
> >>               return check_pattern_no_oob(buf, td);
> >>
> >>       end = paglen + td->offs;
> >> -     if (td->options & NAND_BBT_SCANEMPTY) {
> >> -             for (i = 0; i < end; i++) {
> >> -                     if (p[i] != 0xff)
> >> -                             return -1;
> >> -             }
> >> -     }
> >> +     if (td->options & NAND_BBT_SCANEMPTY)
> >> +             if (memchr_inv(p, 0xff, end))
> >> +                     return -1;
> >>       p += end;
> >>
> >
> > A question regarding the original code:
> 
> This is another instance of a NAND_BBT_* option that I don't really
> understand, but I'll provide my thoughts...
> 
> > Why the region validated for 0xff is [0 , paglen+td->of) ?
> > I assume this buffer region contains the inband data. Why verify inband
> > data is all 0xff?
> 
> If used on a flash-based BBT page, then it checks for an empty table
> (why would you need this?), and if used for factory-marked bad blocks,
> then it checks that all the non-marker locations are 0xff. I guess the
> latter is at least more reasonable, but that still doesn't really
> answer "why?" So I'm guessing this another ill-conceived option.
> 
> > Shouldn't the region validated be [paglen , paglen+td->of) ?
> 
> Dunno.
> 
> Anyway, it's only used in a single driver (omap2.c) in
> "bb_descrip_flashbased" which, despite its name, does not use a
> flash-based BBT - also used in "agand_flashbased". So the option is
> *only* used for OOB bad block markers, ensuring that the non-marker
> positions are 0xff. But I don't see how it's valid to assume that the
> data will be 0xff, nor do I know why someone would want to check that.
> 
> Finally, I think that some of the "use" of NAND_BBT_SCANEMPTY is
> obscured by the fact that omap2.c's main codepath involves
> (permanently) clearing NAND_BBT_SCANEMPTY in nand_memory_bbt(). See:
> bd->options &= ~NAND_BBT_SCANEMPTY;
> 
> So, is this another candidate for removal, as it is practically unused
> and unmaintained? Or any comments on its purpose, Ivan?

Hi Brian,

I haven't really had a detailed look at this bbt structure while writing my ECC patches.
I think it is indeed a candidate for removal.

My guess is that a specific OOB layout compatible with the OMAP ROM ECC was added at some point,
and this legacy bbt structure found its way into the same commit... ?

Since omap2.c also disables bbt scan in omap_nand_probe():
      info->nand.options    |= NAND_SKIP_BBTSCAN;
I guess it does not really make a difference ?

Adding Sukumar Ghorai in CC, as he introduced this in f040d33253b2daf6f305fc35fca2399047ced028:

    omap3: nand: making ecc layout as compatible with romcode ecc
    
    This patch overrides nand ecc layout and bad block descriptor (for 8-bit
    device) to support hw ecc in romcode layout. So as to have in sync with ecc
    layout throughout; i.e. x-loader, u-boot and kernel.
    
    This enables to flash x-loader, u-boot, kernel, FS images from kernel itself
    and compatiable with other tools.
    
    This patch does not enables this feature by default and need to pass from
    board file to enable for any board.

Ghorai, do you remember why option NAND_BBT_SCANEMPTY is used ?
Thanks,

BR,
-- 
Ivan

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Mike Dunn]

On 07/16/2012 02:34 PM, Ivan Djelic wrote:
> On Mon, Jul 16, 2012 at 08:36:56PM +0200, Mike Dunn wrote:
>>
>> This is a certainty, no?  Erased, by definition, includes any ecc bytes.
> 
> If you add the right polynomial[1] to your Hamming or BCH code, then you can make
> sure the ECC of an erased page (possibly with OOB bytes) is actually a sequence of 0xff bytes.
> This trick enables bitflip correction on data that has never been programmed.
> Take a look at nand_ecc.c:63 (or nand_bch.c:62) for an example.
> This trick is not possible on all hardware ECC engines.


Ah, yes, I forgot.  You explained this when you were helping me with ecc on the
diskonchip.


>  
>>
>>> Moreover, reading just the OOB region with ECC may require a full page read on some drivers
>>> (when OOB and data are parts of the same codeword).
>>>
>>> To me, the only strong reason for wanting OOB ECC is the implementation of YAFFS2 or similar filesystems
>>> which require OOB metadata protection. But maybe I'm missing some other use cases ?
>>>
>>> What do you think ?
>>
>>
>> If we assume the oob bytes on the first page of a good block can contain
>> anything, won't simply counting the bits make the risk of falsly identifying a
>> bb marker unacceptably high?
> 
> Counting bits on a BBM marker byte basically gives a 4-bitflip protection on a _single_ byte, which is
> *extremely* unlikely. If you assume an erased good block can contain garbage in its OOB region, then
> it can indeed be wrongly identified as a bad block; but this is also true if you use OOB ECC
> (e.g. exceeding correction capacity).
> In practice, since the bad block marker byte is normally never programmed to anything other than 0xff,
> there is no reason why we should find garbage in it (even if a power failure occurs during an
> erase/program operation).


This is what I was missing.  I was assuming that the bb marker byte could be
programmed to an arbitrary value by the user.  I see now that e.g., davinci
defines the bb marker position in its ecc.layout definition, so it will never be
written as long as MTD_OPS_AUTO_OOB mode is used.  I need to fix that in the docg4.

Are you trying to gently nudge me on this? :)  I am loathe to touch that bbm
code.  OTOH, the docg4 would benefit, since it uses an oob bb marker.

Thanks again Ivan,
Mike

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> mtd_read_oob() has some unexpected similarities to mtd_read(). For
> instance, when ops->datbuf != NULL, nand_base.c might return max_bitflips;
> however, when ops->datbuf == NULL, nand_base's code potentially could
> return -EUCLEAN (no in-tree drivers do this yet). In any case where the
> driver might return max_bitflips, we should translate this into an
> appropriate return code using the bitflip_threshold.
> 
> Essentially, mtd_read_oob() duplicates the logic from mtd_read().
> 
> This prevents users of mtd_read_oob() from receiving a positive return
> value (i.e., from max_bitflips) and interpreting it as an unknown error.

Pushed this patch to l2-mtd.git, thanks. I've amended the commentary as
Smulik requested, and also removed "(see above)", because "above" is (a)
unclear, and (b) if it means 'mtd_read()', we'd need to make sure
'mtd_read()' is always above 'mtd_read_oob()'.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/8] mtd: nand: remove unused 'int' return codes

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> The return codes for read_abs_bbts() and search_read_bbts() are always
> non-zero, and so don't have much meaning. Just remove them.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/8] mtd: nand_bbt: use string library

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> Some nand_bbt code can be shortened by using memcmp() and memchr_inv().
> As an added bonus, there is a possbile performance benefit.
> 
> Borrowed some code from Akinobu Mita.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
> MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
> added bonus of error correction.

Amended the commit message to satisfy Smulik's request, and pushed to
l2-mtd.git, thanks Brian!

You are doing great job de-mystifying the NAND/BBT code! Sometimes I
feel like you are playing too safe and you could be a bit more
aggressive removing half-baked or obviously broken stuff.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/8] mtd: nand: rename '_raw' BBT scan functions

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> None of these scanning functions use MTD_OPS_RAW mode any more, so there's
> really nothing 'raw' about them. Rename them to (hopefully) make the code
> a little clearer.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Pushed this one as the last patch. I think now the entire patch-set is
in the l2 tree.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Shmulik Ladkani]

Hi Artem,

On Wed, 15 Aug 2012 15:05:17 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> > scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
> > mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
> > MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
> > added bonus of error correction.
> 
> Amended the commit message to satisfy Smulik's request, and pushed to
> l2-mtd.git, thanks Brian!

I had some second thoughts WRT the move from MTD_OPS_RAW to
MTD_OPS_PLACE_OOB.

When reading BBT content, MTD_OPS_PLACE_OOB is preferred.

But when scanning the OOBs for BBMs (when creating the table), I guess
MTD_OPS_RAW should be used - since manufacturer bad blocks are not
protected by ECC (as Matthieu noted in [1], and I elaborated in [2]).

Can you please review and comment regarding the ideas discussed there?

Thanks,
Shmulik

[1]
http://thread.gmane.org/gmane.linux.drivers.mtd/42243/focus=42365
[2]
http://thread.gmane.org/gmane.linux.drivers.mtd/42243/focus=42669

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Brian Norris]

On Wed, Aug 15, 2012 at 4:24 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Pushed this patch to l2-mtd.git, thanks. I've amended the commentary as
> Smulik requested, and also removed "(see above)", because "above" is (a)
> unclear, and (b) if it means 'mtd_read()', we'd need to make sure
> 'mtd_read()' is always above 'mtd_read_oob()'.

Good idea, removing "(see above)". Readers should be smart enough to
locate mtd_read() and its accompanying comments themselves anyway.

Note that there was a v2 of this patch, although the difference was
very minor (the comments and the Reviewed-by's). The end result is
almost the same as your amendments.

Thanks,
Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

Hi Shmulik,

On Wed, 2012-08-15 at 17:31 +0300, Shmulik Ladkani wrote:
> > Amended the commit message to satisfy Smulik's request, and pushed to
> > l2-mtd.git, thanks Brian!
> 
> I had some second thoughts WRT the move from MTD_OPS_RAW to
> MTD_OPS_PLACE_OOB.
> 
> When reading BBT content, MTD_OPS_PLACE_OOB is preferred.
> 
> But when scanning the OOBs for BBMs (when creating the table), I guess
> MTD_OPS_RAW should be used - since manufacturer bad blocks are not
> protected by ECC (as Matthieu noted in [1], and I elaborated in [2]).
> 
> Can you please review and comment regarding the ideas discussed there?
> 
> Thanks,
> Shmulik
> 
> [1]
> http://thread.gmane.org/gmane.linux.drivers.mtd/42243/focus=42365
> [2]
> http://thread.gmane.org/gmane.linux.drivers.mtd/42243/focus=42669

I cannot spend much time reading the threads carefully, so I went
through the discussion only very briefly. Apologies. But I took the
patches because:

1. They do not break existing (in-tree) systems which do not have OOB
protected with ECC. This means the patches are harmless and while useful
because they clean up the code.

2. For systems like Brian has where OOB is protected with ECC, the BBM
should just not be covered by the OOB ECC. And the OOB scanning code
should not read non-BBM bytes, and we are done. This seems to be the
only logical setup for me.

If it is not the case, I just consider it is Brian's problem, he'll deal
with it and send us another set of patches.

Or to put it differently: he sent a good clean-up, which does not seem t
break anything, why would we not take it?

If you object merging these patches, I'd appreciate if you could
elaborate why they would be _harmful_ to have.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Wed, 2012-08-15 at 12:15 -0700, Brian Norris wrote:
> Note that there was a v2 of this patch, although the difference was
> very minor (the comments and the Reviewed-by's). The end result is
> almost the same as your amendments.

Reviewed-by tags are important for me, because people spent time and
they should be credited in some form in git log. I actually collected
all the tags from the thread and added them when applying your patches. 

Thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/8] mtd: check for max_bitflips in mtd_read_oob()

[Brian Norris]

On Thu, Aug 16, 2012 at 3:48 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2012-08-15 at 12:15 -0700, Brian Norris wrote:
>> Note that there was a v2 of this patch, although the difference was
>> very minor (the comments and the Reviewed-by's). The end result is
>> almost the same as your amendments.
>
> Reviewed-by tags are important for me, because people spent time and
> they should be credited in some form in git log.

Right, I understood this, and those tags were reflected in my v2.

> I actually collected
> all the tags from the thread and added them when applying your patches.

Yes, I was only commenting that while you took and amended my v1, it
ended up being nearly identical to my v2 - including the Reviewed-by
tags. They only differed by some punctuation or something else
trivial.

Thanks!

Brian

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Shmulik Ladkani]

Hi Artem,

On Thu, 16 Aug 2012 13:40:02 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> If you object merging these patches, I'd appreciate if you could
> elaborate why they would be _harmful_ to have.

Thanks. I understand your point.

Took another look, and no, these are _not_ harmful to have.

Regards,
Shmulik

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Angus Clark]

Hi Brian,

Firstly, apologies for dragging up an issue that dates back well over a year!

While preparing to upstream an out-of-tree NAND driver, we have fallen foul of
the change from MTD_OPS_RAW to MTD_OPS_PLACE_OOB in nand_bbt.c:scan_read_oob().

One issue relates to what is at best a hack within our own driver, and that is
for us to deal with :-)  However, I also have a concern that the patch could
result in genuine bad blocks escaping detection.

As I understand it, the patch was attempting to address the following situation:
    - NAND-resident BBTs are not used.
    - The BBT is re-created on each boot by scanning for MBBM.
    - A page write yields one or more bit-flips in the location used for the
MBBM, resulting in non-0xff data being present.
    - The non-0xff data is then misinterpreted as a MBBM on a subsequent boot,
giving a false bad-block.

In cases where the ECC scheme covers the MBBM location, then I can see that
enabling the ECC would cause the non-0xff data to be corrected, and therefore
avoid the block being falsely identified as bad.

However, I can also construct a situation where a genuine MBBM gets "corrected"
to 0xff.  Consider, for example, an 8-bit ECC scheme covering the MBBM location,
where the ECC for a sector of all 0xff data is also all 0xff.  In this case, a
MBBM of 0x00, with the remaining data all 0xff, would get "corrected" to 0xff.
Although perhaps a slightly contrived example, the S/W BCH ECC included in the
kernel scheme can be driven in this way, and I have seen blocks marked as bad
with this pattern in the past.

It is difficult to know if your particular system could suffer in this way.  It
all depends on the nature of your ECC scheme.  I guess my concern is that the
patch deviates from what is recommended by the NAND manufacturers, and that it
makes certain assumptions on how the ECC scheme operates.

My own view is that the only safe way to record and track bad blocks is to use
NAND-resident BBTs; after all, if a block is bad then there is no guarantee that
an attempt to write a MBBM would succeed.  NAND-resident BBTs would also avoid
the problem the patch was attempting fix in the first place.

Cheers,

Angus


On 07/13/2012 06:39 PM, Brian Norris wrote:
> On Tue, Jul 10, 2012 at 12:45 AM, Matthieu CASTET
> <matthieu.castet@parrot.com> wrote:
>> Brian Norris a écrit :
>>> scan_read_raw_oob() is used in only in places where the MTD_OPS_PLACE_OOB
>>> mode is preferable MTD_OPS_RAW mode, so use MTD_OPS_PLACE_OOB instead.
>>> MTD_OPS_PLACE_OOB provides the same functionality with the potential[1]
>>> added bonus of error correction.
>>>
>>> This brings scan_block_full() in line with scan_block_fast() so that they
>>> both read bad block markers with MTD_OPS_PLACE_OOB. This can help in
>>> preventing 0xff markers (in good blocks) from being interpreted as bad
>>> block indicators in the presence of a single bitflip.
>>
>> As far I understand the code, this work when "chip->ecc.read_oob" (used in
>> nand_do_read_oob) correct bit flip.
>>
>> But I see no "chip->ecc.read_oob" implementation that can return bit flip. Is
>> that expected ?
> 
> I have an out-of-tree driver that corrects OOB bitflips. Is there
> really no other HW out there that corrects OOB errors?
> 
> Anyway, I understand that my driver is an outlier here, but I don't
> see a real disadvantage in these changes. But on the positive side, I
> expect that in the future, more drivers/HW will either want to stop
> using OOB for anything at all or will want ECC protection for OOB.
> 
>> This can also work when nand_do_read_ops is used (ops->datbuf != NULL). But it
>> is hard to see case where it can correct bit flip in bad block marker. Do you
>> have any exemple ?
> 
> First of all, this has no effect if the driver does not protect OOB
> with ECC (i.e., for OOB-only reads, MTD_OPS_PLACE_OOB == MTD_OPS_RAW).
> So the following argument only applies when OOB is ECC-protected.
> 
> Consider a *good* block that is written with filesystem data. On
> bootup, Linux may scan this block's BBM to check if it is bad. If a
> bitflip occurs in the bad block marker, then it may be erroneously
> considered bad.
> 
> Similarly, if a block was marked bad from wear (not factory-marked),
> then its BBM may be written along with ECC protection. Then, when we
> scan for bad blocks, it will be protected from bitflips that could
> possibly cause 0x00 to appear non-zero. (This is not a big issue,
> since 'non-zero' is still bad, as long as 0x00 didn't flip to 0xff -
> quite unlikely...)
> 
>> PS : Did you have any comment on
>> http://thread.gmane.org/gmane.linux.drivers.mtd/42243 ?
> 
> I read it, and it seems promising. I agree with much of the premise
> (that nand_bbt.c is ugly and repetitive at times) but haven't had
> enough time to review properly. Sorry. I'm a bit backlogged and will
> be for a few weeks, I think. But I'll see what I can do.
> 
> Thanks,
> Brian
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

-- 
-------------------------------------
Angus Clark
ST Microelectronics (R&D) Ltd.
1000 Aztec West, Bristol, BS32 4SQ
email: angus.clark@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-------------------------------------

Re: [PATCH 8/8] mtd: nand: use ECC, if present, when scanning OOB

[Brian Norris]

Hi Angus,

On Thu, Nov 07, 2013 at 02:56:50PM +0000, Angus Clark wrote:
> Firstly, apologies for dragging up an issue that dates back well over a year!

No problem. Some of these same issues have been nagging at me in the
back of my head, actually. I don't think this problem is solved
completely correctly, so it's good you bring it up!

> While preparing to upstream an out-of-tree NAND driver, we have fallen foul of
> the change from MTD_OPS_RAW to MTD_OPS_PLACE_OOB in nand_bbt.c:scan_read_oob().
> 
> One issue relates to what is at best a hack within our own driver, and that is
> for us to deal with :-)  However, I also have a concern that the patch could
> result in genuine bad blocks escaping detection.
> 
> As I understand it, the patch was attempting to address the following situation:
>     - NAND-resident BBTs are not used.
>     - The BBT is re-created on each boot by scanning for MBBM.
>     - A page write yields one or more bit-flips in the location used for the
> MBBM, resulting in non-0xff data being present.
>     - The non-0xff data is then misinterpreted as a MBBM on a subsequent boot,
> giving a false bad-block.
> 
> In cases where the ECC scheme covers the MBBM location, then I can see that
> enabling the ECC would cause the non-0xff data to be corrected, and therefore
> avoid the block being falsely identified as bad.

This is more or less the situation that was being addressed. But my
particular situation was actually targeting situations in which a BBT is
used "most of the time", but sometimes (for various reasons) the BBT may
be erased/corrupted and need rebuilt on an unclean flash. But it's a
similar scenario either way, IMO.

> However, I can also construct a situation where a genuine MBBM gets "corrected"
> to 0xff.  Consider, for example, an 8-bit ECC scheme covering the MBBM location,
> where the ECC for a sector of all 0xff data is also all 0xff.  In this case, a
> MBBM of 0x00, with the remaining data all 0xff, would get "corrected" to 0xff.
> Although perhaps a slightly contrived example, the S/W BCH ECC included in the
> kernel scheme can be driven in this way, and I have seen blocks marked as bad
> with this pattern in the past.

Yes, I suspected that this may be possible. But I never observed it
myself (I don't use SW ECC; I only use my HW ECC).

> It is difficult to know if your particular system could suffer in this way.  It
> all depends on the nature of your ECC scheme.  I guess my concern is that the
> patch deviates from what is recommended by the NAND manufacturers, and that it
> makes certain assumptions on how the ECC scheme operates.

My system cannot suffer in this way, because (unfortunately) my HW ECC
does not consider all-0xff to be valid ECC. So it cannot correct
bitflips in erased pages, nor can it "correct" a bad block marker from
0x00 to 0xff.

I'm not sure it's a deviation from manufacturer recommendations, since
manufacturers make no statement about what to do if the BBT fails. But
they do seem to imply that you should build the BBT once and never
revisit the bad block markers.

I agree that my patch does make a few assumptions about the ECC scheme.

> My own view is that the only safe way to record and track bad blocks is to use
> NAND-resident BBTs; after all, if a block is bad then there is no guarantee that
> an attempt to write a MBBM would succeed.  NAND-resident BBTs would also avoid
> the problem the patch was attempting fix in the first place.

I agree that BBTs should be used (I use them). However, they do not
solve my original problem completely: how can a BBT be rebuilt robustly?

I think that my original approach (use ECC while scanning BBMs) is not
100% correct, but I don't think dropping it is 100% correct either.
Rather, I would prefer taking some form of Matthieu's patch (linked in
the reply that you're bringing up from > 1 year ago!) so that we use the
'badblockbits' parameter appropriately. Then I would be fine with
scanning BBMs in RAW mode, and I think we can satisfy both my use case
and yours safely.

> On 07/13/2012 06:39 PM, Brian Norris wrote:
> > On Tue, Jul 10, 2012 at 12:45 AM, Matthieu CASTET <matthieu.castet@parrot.com> wrote:
> >> http://thread.gmane.org/gmane.linux.drivers.mtd/42243 ?

BTW, this patch came into discussion again recently for other reasons:

  http://lists.infradead.org/pipermail/linux-mtd/2013-November/049692.html

Anything more you can contribute to this general conversation would be
more than welcome! I think Ezequiel and I are still interested in
unifying some of nand_bbt and nand_base, regarding BBM scanning, and I
think we can resolve several pending issues by doing so.

Thanks,
Brian