[PATCH 1/8] mtd: nand: remove NAND_BBT_SCANBYTE1AND6 option

[PATCH 1/8] mtd: nand: remove NAND_BBT_SCANBYTE1AND6 option

[Brian Norris]

This patch reverts most of:
    commit 58373ff0afff4cc8ac40608872995f4d87eb72ec
    mtd: nand: more BB Detection refactoring and dynamic scan options

According to the discussion at:
    http://lists.infradead.org/pipermail/linux-mtd/2011-May/035696.html
the NAND_BBT_SCANBYTE1AND6 flag, although technically valid, can break
some existing ECC layouts that use the 6th byte in the OOB for ECC data.
Furthermore, we apparently do not need to scan both bytes 1 and 6 in
the OOB region of the devices under consideration; instead, we only need
to scan one or the other.

Thus, the NAND_BBT_SCANBYTE1AND6 flag is at best unnecessary and at
worst a regression.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   29 +++++------------------------
 drivers/mtd/nand/nand_bbt.c  |   32 +-------------------------------
 include/linux/mtd/bbm.h      |    2 --
 3 files changed, 6 insertions(+), 57 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a46e9bb..456fb34 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -410,10 +410,11 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	else {
 		nand_get_device(chip, mtd, FL_WRITING);
 
-		/* Write to first two pages and to byte 1 and 6 if necessary.
-		 * If we write to more than one location, the first error
-		 * encountered quits the procedure. We write two bytes per
-		 * location, so we dont have to mess with 16 bit access.
+		/*
+		 * Write to first two pages if necessary. If we write to more
+		 * than one location, the first error encountered quits the
+		 * procedure. We write two bytes per location, so we dont have
+		 * to mess with 16 bit access.
 		 */
 		do {
 			chip->ops.len = chip->ops.ooblen = 2;
@@ -423,11 +424,6 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 
 			ret = nand_do_write_oob(mtd, ofs, &chip->ops);
 
-			if (!ret && (chip->options & NAND_BBT_SCANBYTE1AND6)) {
-				chip->ops.ooboffs = NAND_SMALL_BADBLOCK_POS
-					& ~0x01;
-				ret = nand_do_write_oob(mtd, ofs, &chip->ops);
-			}
 			i++;
 			ofs += mtd->writesize;
 		} while (!ret && (chip->options & NAND_BBT_SCAN2NDPAGE) &&
@@ -3131,16 +3127,6 @@ ident_done:
 			 *maf_id == NAND_MFR_MICRON))
 		chip->options |= NAND_BBT_SCAN2NDPAGE;
 
-	/*
-	 * Numonyx/ST 2K pages, x8 bus use BOTH byte 1 and 6
-	 */
-	if (!(busw & NAND_BUSWIDTH_16) &&
-			*maf_id == NAND_MFR_STMICRO &&
-			mtd->writesize == 2048) {
-		chip->options |= NAND_BBT_SCANBYTE1AND6;
-		chip->badblockpos = 0;
-	}
-
 	/* Check for AND chips with 4 page planes */
 	if (chip->options & NAND_4PAGE_ARRAY)
 		chip->erase_cmd = multi_erase_cmd;
@@ -3538,11 +3524,6 @@ void nand_release(struct mtd_info *mtd)
 	kfree(chip->bbt);
 	if (!(chip->options & NAND_OWN_BUFFERS))
 		kfree(chip->buffers);
-
-	/* Free bad block descriptor memory */
-	if (chip->badblock_pattern && chip->badblock_pattern->options
-			& NAND_BBT_DYNAMICSTRUCT)
-		kfree(chip->badblock_pattern);
 }
 EXPORT_SYMBOL_GPL(nand_release);
 
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ccbeaa1..5ffb9a4 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -114,28 +114,6 @@ static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
 			return -1;
 	}
 
-	/* Check both positions 1 and 6 for pattern? */
-	if (td->options & NAND_BBT_SCANBYTE1AND6) {
-		if (td->options & NAND_BBT_SCANEMPTY) {
-			p += td->len;
-			end += NAND_SMALL_BADBLOCK_POS - td->offs;
-			/* Check region between positions 1 and 6 */
-			for (i = 0; i < NAND_SMALL_BADBLOCK_POS - td->offs - td->len;
-					i++) {
-				if (*p++ != 0xff)
-					return -1;
-			}
-		}
-		else {
-			p += NAND_SMALL_BADBLOCK_POS - td->offs;
-		}
-		/* Compare the pattern */
-		for (i = 0; i < td->len; i++) {
-			if (p[i] != td->pattern[i])
-				return -1;
-		}
-	}
-
 	if (td->options & NAND_BBT_SCANEMPTY) {
 		p += td->len;
 		end += td->len;
@@ -167,13 +145,6 @@ static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
 		if (p[td->offs + i] != td->pattern[i])
 			return -1;
 	}
-	/* Need to check location 1 AND 6? */
-	if (td->options & NAND_BBT_SCANBYTE1AND6) {
-		for (i = 0; i < td->len; i++) {
-			if (p[NAND_SMALL_BADBLOCK_POS + i] != td->pattern[i])
-				return -1;
-		}
-	}
 	return 0;
 }
 
@@ -1330,8 +1301,7 @@ static struct nand_bbt_descr bbt_mirror_no_bbt_descr = {
 	.pattern = mirror_pattern
 };
 
-#define BBT_SCAN_OPTIONS (NAND_BBT_SCANLASTPAGE | NAND_BBT_SCAN2NDPAGE | \
-		NAND_BBT_SCANBYTE1AND6)
+#define BBT_SCAN_OPTIONS (NAND_BBT_SCANLASTPAGE | NAND_BBT_SCAN2NDPAGE)
 /**
  * nand_create_default_bbt_descr - [Internal] Creates a BBT descriptor structure
  * @this:	NAND chip to create descriptor for
diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
index 57cc0e6..08ffa21 100644
--- a/include/linux/mtd/bbm.h
+++ b/include/linux/mtd/bbm.h
@@ -98,8 +98,6 @@ struct nand_bbt_descr {
 #define NAND_BBT_SCAN2NDPAGE	0x00004000
 /* Search good / bad pattern on the last page of the eraseblock */
 #define NAND_BBT_SCANLASTPAGE	0x00008000
-/* Chip stores bad block marker on BOTH 1st and 6th bytes of OOB */
-#define NAND_BBT_SCANBYTE1AND6 0x00100000
 /* The nand_bbt_descr was created dynamicaly and must be freed */
 #define NAND_BBT_DYNAMICSTRUCT 0x00200000
 /* The bad block table does not OOB for marker */
-- 
1.7.0.4

Re: [PATCH 1/8] mtd: nand: remove NAND_BBT_SCANBYTE1AND6 option

[Angus CLARK]

Hi Brian,

On Tue, 31 May 2011 16:31:20 -0700, Brian Norris
<computersforpeace@gmail.com> wrote:
> This patch reverts most of:
>     commit 58373ff0afff4cc8ac40608872995f4d87eb72ec
>     mtd: nand: more BB Detection refactoring and dynamic scan options
> 
> According to the discussion at:
>     http://lists.infradead.org/pipermail/linux-mtd/2011-May/035696.html
> the NAND_BBT_SCANBYTE1AND6 flag, although technically valid, can break
> some existing ECC layouts that use the 6th byte in the OOB for ECC data.
> Furthermore, we apparently do not need to scan both bytes 1 and 6 in
> the OOB region of the devices under consideration; instead, we only need
> to scan one or the other.
> 
> Thus, the NAND_BBT_SCANBYTE1AND6 flag is at best unnecessary and at
> worst a regression.
> 

Sorry to bring this up again, but I just thought you might be interested
in the following clarification I received from Micron (who now own the
ST/Numonyx NAND portfolio):

---
"In the factory, we mark bad blocks by programming (in the spare area of
the first page of the block) SPARE[0] = 00h and SPARE[5] = 00h.  On x16
devices we program SPARE[0] = 0000h.

Due to the fact that the block is bad, it may be impossible to program
some of the bits.  The most reliable test for a bad block is therefore:

x8 device: (SPARE[0] != FFh) OR (SPARE[5] != FFh)
x16 device: (SPARE[0] != FFFFh)

Having said that, it is very unlikely that all 8 bits of a location will
be stuck at '1', so if you just want to test SPARE[0] OR SPARE[5], that
should be reliable enough for most applications."
---

So, I believe this confirms your original interpretation of the
ST/Numonyx datasheets and that the NAND_BBT_SCANBYTE1AND6 is valid.  The
question still remains what to do with this information, in view of the
potential clash with existing ECC layouts (including the default S/W ECC
layout).  A couple of options you proposed previously:

1) Retain the NAND_BBT_SCANBYTE1AND6 flag, and add some code to mask out
the flag if necessary based on the ECC layout (possibly in
nand_scan_tail()?).

2) Revert the flag, as done in this patch, and accept the very small
possibility of missing some factory-marked bad-blocks.

My preference would be 1), since it is technically correct, and would
allow drivers to make use of the information if possible, while avoiding
regressions associated with ECC layout clashes.

I appreciate the patch for 2) has already made it to l2-mtd2.6 so it may
be too late anyway...

Cheers,

Angus

Re: [PATCH 1/8] mtd: nand: remove NAND_BBT_SCANBYTE1AND6 option

[Brian Norris]

On Wed, Oct 12, 2011 at 2:49 AM, Angus CLARK <angus.clark@st.com> wrote:
> Sorry to bring this up again, but I just thought you might be interested
> in the following clarification I received from Micron (who now own the
> ST/Numonyx NAND portfolio):

Nice to have a real manufacturer response.

> So, I believe this confirms your original interpretation of the
> ST/Numonyx datasheets and that the NAND_BBT_SCANBYTE1AND6 is valid.

I love being right :) But I feel this is really a matter of theory,
not practice.

> The
> question still remains what to do with this information, in view of the
> potential clash with existing ECC layouts (including the default S/W ECC
> layout).  A couple of options you proposed previously:

Just a memory recall: are the clashes only on 2KB page, 64B OOB
devices with "SCANBYTE1AND6" and certain ECC layouts? It's not an
issue with any small page (512B+16B) NAND with
NAND_SMALL_BADBLOCK_POS, is it? I suppose small page NAND uses smaller
ECC?

> 1) Retain the NAND_BBT_SCANBYTE1AND6 flag, and add some code to mask out
> the flag if necessary based on the ECC layout (possibly in
> nand_scan_tail()?).
>
> 2) Revert the flag, as done in this patch, and accept the very small
> possibility of missing some factory-marked bad-blocks.
>
> My preference would be 1), since it is technically correct, and would
> allow drivers to make use of the information if possible, while avoiding
> regressions associated with ECC layout clashes.

I thought (and still believe) that (1) is technically correct, but as
Micron states, it's not necessary. Option (2) is easier, and in my
experience, bad blocks are actually successfully written with all
zeros (whole block, not just the special OOB indicator). So really,
this is a matter of how much code should be added for an exception
that (arguably) provides no benefit.

> I appreciate the patch for 2) has already made it to l2-mtd2.6 so it may
> be too late anyway...

It's not too late, but even if we want to re-implement my patch, I
wouldn't recommend doing it exactly the same way. I think it was a
little ugly and could cause two consecutive writes to the same page,
which is sometimes a problem for newer flash (but may not be a problem
on the old flash to which this option would actually apply).

So I'm fine with leaving the code as it stands in l2-mtd-2.6. If
there's a consensus formed around reverting my revert, then I can
probably help, but I'm unlikely to take the initiative myself.

Brian