[PATCH v2] mtd: nand: write bad block marker even with BBT

[PATCH v2] mtd: nand: write bad block marker even with BBT

[Brian Norris]

Currently, the flash-based BBT implementation writes bad block data only
to its flash-based table and not to the OOB marker area. Then, as new
bad blocks are marked over time, the OOB markers become out of date and
the flash-based table becomes the only source of current bad block
information. This can be a problem when:

 * bootloader cannot read the flash-based BBT format
 * BBT is corrupted and the flash must be rescanned for bad
   blocks; we want to remember bad blocks that were marked from Linux

In an attempt to keep the bad block markers in sync with the flash-based
BBT, this patch changes the default so that we write bad block markers
to the proper OOB area on each block in addition to flash-based BBT.

Theoretically, the bad block table and the OOB markers can still get out
of sync if the system experiences a power cut between writing the BBT to
flash and writing the OOB marker to a newly-marked bad block. However,
this is a relatively unlikely event, as new bad blocks shouldn't appear
frequently.

Note that this is a change from the previous default flash-based BBT
behavior. If any contributors rely on the old behavior, they are welcome
to introduce an option flag for it.

Adapted from code by Matthieu Castet.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: Explain potential power cut issues and remove option for retaining
    old behavior. I CC'd various MTD contributors; speak up if the new
    default is unacceptable!

drivers/mtd/nand/nand_base.c |   59 +++++++++++++++++++++++------------------
 1 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 35b4565..dfa017e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -393,6 +393,7 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	struct nand_chip *chip = mtd->priv;
 	uint8_t buf[2] = { 0, 0 };
 	int block, ret, i = 0;
+	struct mtd_oob_ops ops;
 
 	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
 		ofs += mtd->erasesize - mtd->writesize;
@@ -403,34 +404,40 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 		chip->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);
 
 	/* Do we have a flash based bad block table? */
-	if (chip->bbt_options & NAND_BBT_USE_FLASH)
+	if (chip->bbt_options & NAND_BBT_USE_FLASH) {
 		ret = nand_update_bbt(mtd, ofs);
-	else {
-		struct mtd_oob_ops ops;
+		if (ret)
+			return ret;
+	}
 
-		nand_get_device(chip, mtd, FL_WRITING);
+	/*
+	 * Write bad block marker to OOB
+	 * Note that a flash-based BBT (when used) can become out of sync with
+	 * OOB markers if a power cut occurs here. See:
+	 *   http://lists.infradead.org/pipermail/linux-mtd/2011-December/038851.html
+	 */
 
-		/*
-		 * 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.
-		 */
-		ops.len = ops.ooblen = 2;
-		ops.datbuf = NULL;
-		ops.oobbuf = buf;
-		ops.ooboffs = chip->badblockpos & ~0x01;
-		ops.mode = MTD_OPS_PLACE_OOB;
-		do {
-			ret = nand_do_write_oob(mtd, ofs, &ops);
-
-			i++;
-			ofs += mtd->writesize;
-		} while (!ret && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE) &&
-				i < 2);
+	nand_get_device(chip, mtd, FL_WRITING);
+
+	/*
+	 * 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.
+	 */
+	ops.len = ops.ooblen = 2;
+	ops.datbuf = NULL;
+	ops.oobbuf = buf;
+	ops.ooboffs = chip->badblockpos & ~0x01;
+	ops.mode = MTD_OPS_PLACE_OOB;
+	do {
+		ret = nand_do_write_oob(mtd, ofs, &ops);
+
+		i++;
+		ofs += mtd->writesize;
+	} while (!ret && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
+
+	nand_release_device(mtd);
 
-		nand_release_device(mtd);
-	}
 	if (!ret)
 		mtd->ecc_stats.badblocks++;
 
-- 
1.7.5.4

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Brian Norris]

Sorry for two e-mails; apparently several addresses I reached were no
longer valid. I'm removing Randy Dunlap and Sukumar Ghorai from the CC
list, and I updated Barry Song and Adrian Hunter's addresses.

Hopefully this can prevent other people from receiving too many
bounces :) Patch description copied below.

On Mon, Dec 19, 2011 at 2:03 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Currently, the flash-based BBT implementation writes bad block data only
> to its flash-based table and not to the OOB marker area. Then, as new
> bad blocks are marked over time, the OOB markers become out of date and
> the flash-based table becomes the only source of current bad block
> information. This can be a problem when:
>
>  * bootloader cannot read the flash-based BBT format
>  * BBT is corrupted and the flash must be rescanned for bad
>   blocks; we want to remember bad blocks that were marked from Linux
>
> In an attempt to keep the bad block markers in sync with the flash-based
> BBT, this patch changes the default so that we write bad block markers
> to the proper OOB area on each block in addition to flash-based BBT.
>
> Theoretically, the bad block table and the OOB markers can still get out
> of sync if the system experiences a power cut between writing the BBT to
> flash and writing the OOB marker to a newly-marked bad block. However,
> this is a relatively unlikely event, as new bad blocks shouldn't appear
> frequently.
>
> Note that this is a change from the previous default flash-based BBT
> behavior. If any contributors rely on the old behavior, they are welcome
> to introduce an option flag for it.
>
> Adapted from code by Matthieu Castet.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Brian

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Sebastian Andrzej Siewior]

On 12/19/2011 11:03 PM, Brian Norris wrote:
> Currently, the flash-based BBT implementation writes bad block data only
> to its flash-based table and not to the OOB marker area. Then, as new
> bad blocks are marked over time, the OOB markers become out of date and
> the flash-based table becomes the only source of current bad block
> information. This can be a problem when:
>
>   * bootloader cannot read the flash-based BBT format
>   * BBT is corrupted and the flash must be rescanned for bad
>     blocks; we want to remember bad blocks that were marked from Linux
>
> In an attempt to keep the bad block markers in sync with the flash-based
> BBT, this patch changes the default so that we write bad block markers
> to the proper OOB area on each block in addition to flash-based BBT.
>
> Theoretically, the bad block table and the OOB markers can still get out
> of sync if the system experiences a power cut between writing the BBT to
> flash and writing the OOB marker to a newly-marked bad block. However,
> this is a relatively unlikely event, as new bad blocks shouldn't appear
> frequently.
>

The marker and BBT may get out of sync. You should use either the one 
_or_ the other but not both. Why use both anyway? I use BBT because I
have no room left in OOB or updating a single area in OOB is too
complicated / hardly possible.
In the former case, reading that area again and interpreting it as
something as it was once is a bad thing.

Why is the BBT corrupted? You have two tables. If one is broken you
have still the other. If you lose one block which was about to be
marked bad you find soon enough.

Sebastian

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Brian Norris]

Hi Sebastian,

On Tue, Dec 20, 2011 at 12:49 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 12/19/2011 11:03 PM, Brian Norris wrote:
>>
>> Currently, the flash-based BBT implementation writes bad block data only
>> to its flash-based table and not to the OOB marker area. Then, as new
>> bad blocks are marked over time, the OOB markers become out of date and
>> the flash-based table becomes the only source of current bad block
>> information. This can be a problem when:
>>
>>  * bootloader cannot read the flash-based BBT format
>>  * BBT is corrupted and the flash must be rescanned for bad
>>    blocks; we want to remember bad blocks that were marked from Linux
>>
>> In an attempt to keep the bad block markers in sync with the flash-based
>> BBT, this patch changes the default so that we write bad block markers
>> to the proper OOB area on each block in addition to flash-based BBT.
>>
>> Theoretically, the bad block table and the OOB markers can still get out
>> of sync if the system experiences a power cut between writing the BBT to
>> flash and writing the OOB marker to a newly-marked bad block. However,
>> this is a relatively unlikely event, as new bad blocks shouldn't appear
>> frequently.
>>
>
> The marker and BBT may get out of sync. You should use either the one _or_
> the other but not both. Why use both anyway?

Two reasons for using OOB area were stated above. There are cases where:
* bootloader cannot read the flash-based BBT format [... but it still
may need to recognize bad blocks]
* BBT is corrupted and the flash must be rescanned for bad blocks; we
want to remember bad blocks that were marked from Linux

In the above cases, one might still want to use flash-based BBT for
boot-time performance.

> I use BBT because I
> have no room left in OOB or updating a single area in OOB is too
> complicated / hardly possible.
> In the former case, reading that area again and interpreting it as
> something as it was once is a bad thing.

Well, then that is one case where you want an option to avoid writing
to markers back to OOB. We may need an option flag to enforce the old
behavior.

However, your use case is not the only use case. I use flash-based BBT
because of the boot-time performance improvement when I don't have to
rescan the whole flash. I believe there are others who use flash-based
BBT who want the ability to fall back to OOB bad block markers.

> Why is the BBT corrupted? You have two tables. If one is broken you
> have still the other. If you lose one block which was about to be
> marked bad you find soon enough.

There are certainly cases where the table could be erased/overwritten
(and therefore "corrupted"), like by a bootloader that doesn't
recognize the BBT, by a software bug, etc. Also, a table entry that
was written once and then read back over a lifetime might wear out and
start to have errors. Recent patches have tried to solve this with
"scrubbing" and, if necessary, rescanning the flash for bad blocks to
rebuild the table.

Also, mirroring is not a requirement for flash-based BBT. A driver
could provide a custom, non-mirrored configuration.

Brian

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Artem Bityutskiy]

On Tue, 2011-12-20 at 10:17 -0800, Brian Norris wrote:
> > I use BBT because I
> > have no room left in OOB or updating a single area in OOB is too
> > complicated / hardly possible.
> > In the former case, reading that area again and interpreting it as
> > something as it was once is a bad thing.
> 
> Well, then that is one case where you want an option to avoid writing
> to markers back to OOB. We may need an option flag to enforce the old
> behavior. 

I guess there should be some way of detecting this case (no space in
OOB) and just not writing.

Artem.

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Brian Norris]

On Tue, Dec 20, 2011 at 12:49 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2011-12-20 at 10:17 -0800, Brian Norris wrote:
>> > I use BBT because I
>> > have no room left in OOB or updating a single area in OOB is too
>> > complicated / hardly possible.
>> > In the former case, reading that area again and interpreting it as
>> > something as it was once is a bad thing.
>>
>> Well, then that is one case where you want an option to avoid writing
>> to markers back to OOB. We may need an option flag to enforce the old
>> behavior.
>
> I guess there should be some way of detecting this case (no space in
> OOB) and just not writing.

Any suggestions on how to detect no space in OOB? ECC layout structure
is not necessarily reliable, as its fields are designed for
determining what's available apart from BBM and ECC (for userspace or
filesystem data).

My idea: instead of adding more layout info about free space for
writing BBM, we can add a flag to avoid writing BBM to OOB
(NAND_NO_WRITE_OOB?). Would this suffice for you, Sebastian? It would
effectively be a reversal of my v1 patch; we write to both the
flash-BBT and the OOB BBM by default and provide an option that
prevents writing to OOB.

If we use NAND_NO_WRITE_OOB, are there other reasonable situations in
which we should prevent OOB writes according to this flag?

Another question: I noticed that when writing bad block markers to
OOB, we simply overwrite without erasing first. This is a problem on
MLC, which aren't designed to be written twice; they usually can write
some of the bits to 0, but not all of them. Is it reasonable to change
nand_default_block_markbad() to always have it erase the block before
writing? We can just ignore errors (if this is a really bad block that
can't be erased properly). This would be subject of a second patch of
course.

Brian

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Shmulik Ladkani]

On Wed, 21 Dec 2011 17:15:29 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> Another question: I noticed that when writing bad block markers to
> OOB, we simply overwrite without erasing first. This is a problem on
> MLC, which aren't designed to be written twice; they usually can write
> some of the bits to 0, but not all of them. Is it reasonable to change
> nand_default_block_markbad() to always have it erase the block before
> writing?

Not an MLC expert, but could it be that for these MLC chips,
NAND_BBT_USE_FLASH is usually set, hence the complexities invloved in
re-programming the OOB are simply avoided?

If so, it stregthens your NAND_BBT_WRITE_BBM/NAND_NO_WRITE_OOB approach,
which goes hand-in-hand with Sebastian's scenario where there's no room
in the OOB.

Re: [PATCH v2] mtd: nand: write bad block marker even with BBT

[Brian Norris]

On Fri, Dec 23, 2011 at 12:13 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Wed, 21 Dec 2011 17:15:29 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
>> Another question: I noticed that when writing bad block markers to
>> OOB, we simply overwrite without erasing first. This is a problem on
>> MLC, which aren't designed to be written twice; they usually can write
>> some of the bits to 0, but not all of them. Is it reasonable to change
>> nand_default_block_markbad() to always have it erase the block before
>> writing?
>
> Not an MLC expert, but could it be that for these MLC chips,
> NAND_BBT_USE_FLASH is usually set, hence the complexities invloved in
> re-programming the OOB are simply avoided?

I can't comment on others' use of MLC + NAND_BBT_USE_FLASH, but I use
NAND_BBT_USE_FLASH for all NAND.

NAND_BBT_USE_FLASH only applies to the bad block *table* location -
that it will be stored in flash. This does not exclude the possibility
of still writing/recognizing bad block markers in OOB. Thus, I do not
understand your how "the complexities invloved in re-programming the
OOB are simply avoided". In fact, I feel that (when possible) it's
best to write to *both* a flash-based BBT and the OOB region of the
bad block, so I do not plan to avoid the complexities of reprogramming
the OOB. Please correct me if I'm missing your point.

> If so, it stregthens your NAND_BBT_WRITE_BBM/NAND_NO_WRITE_OOB approach,
> which goes hand-in-hand with Sebastian's scenario where there's no room
> in the OOB.

I will proceed with v2 adding a NAND_NO_WRITE_OOB flag to provide an
option for cases like Sebastian's, where there is no writeable OOB
space whatsoever.

Perhaps the paragraph beginning with "Another question:" should be
relegated to a separate patch/thread to avoid confusion between two
related but distinct issues. I will try to write a patch soon that
implements my suggestion and tag it onto a v2 patch series.

Brian