Patchwork mtd: nand: write bad block marker even with BBT

login
register
mail settings
Submitter Brian Norris
Date Dec. 7, 2011, 9:08 p.m.
Message ID <1323292100-19280-1-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/130039/
State New
Headers show

Comments

Brian Norris - Dec. 7, 2011, 9:08 p.m.
Add and option (NAND_BBT_WRITE_BBM) for writing bad block markers to
proper OOB area on each block in addition to flash-based BBT. This is
useful 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

Adapted from code by Matthieu CASTET.

Cc: Matthieu CASTET <matthieu.castet@parrot.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Last year's discussion thread:
    http://lists.infradead.org/pipermail/linux-mtd/2010-October/032942.html

I'll address the criticisms:

"add a BUG_ON to catch misuse cases when NAND_WRITE_BB [now named
NAND_BBT_WRITE_BBM] is used without NAND_USE_FLASH_BBT - they should go
in pair"

NAND_BBT_WRITE_BBM has no special effect when used without
NAND_BBT_USE_FLASH; it's already the default behavior. So I don't expect
it to be an issue.

"And also, how about cases when BBT is updated, but OOB did not because
of a power cut"

Not a very likely case as bad blocks are written infrequently, plus for
the situations where we're writing BBM simply because the bootloader
doesn't understand our BBT, having 99% of blocks marked properly is
better than none.

 drivers/mtd/nand/nand_base.c |   11 +++++++++--
 include/linux/mtd/bbm.h      |    9 +++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)
Artem Bityutskiy - Dec. 12, 2011, 8:49 p.m.
On Wed, 2011-12-07 at 13:08 -0800, Brian Norris wrote:
> Add and option (NAND_BBT_WRITE_BBM) for writing bad block markers to
> proper OOB area on each block in addition to flash-based BBT. This is
> useful 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
> 
> Adapted from code by Matthieu CASTET.
> 
> Cc: Matthieu CASTET <matthieu.castet@parrot.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

2 questions, perhaps silly, but still:

1. Why wouldn't you make this the only and the default behavior? Why
adding more options instead?

2. Would the case of inconsistency between BBT and OOB markers be
possible to fix-up after the power cut?

Artem.
Brian Norris - Dec. 12, 2011, 9:53 p.m.
On Mon, Dec 12, 2011 at 12:49 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-12-07 at 13:08 -0800, Brian Norris wrote:
>> Add and option (NAND_BBT_WRITE_BBM) for writing bad block markers to
>> proper OOB area on each block in addition to flash-based BBT. This is
>> useful 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
>>
>> Adapted from code by Matthieu CASTET.
>>
>> Cc: Matthieu CASTET <matthieu.castet@parrot.com>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>
> 2 questions, perhaps silly, but still:
>
> 1. Why wouldn't you make this the only and the default behavior? Why
> adding more options instead?

Personally, I'd like it to be default behavior. I guessed that it may
cause incompatibility with some other system which depended on the
current behavior, but if that's not a realistic concern, then I don't
mind dropping the option. It's also possible to add a flag to restore
the old behavior if needed in the future.

> 2. Would the case of inconsistency between BBT and OOB markers be
> possible to fix-up after the power cut?

Not trivially :) I suppose we could assume that any bad block markers
in the OOB are in the BBT (when it exists) but not vice versa, then
when we rebuild the BBT at boot time, we rescan the OOB only on the
bad blocks, rewriting to OOB if necessary. Eventually, this kind of
degrades to usefulness of the BBT though, if you're always checking
back with the OOB markers.

Brian
Shmulik Ladkani - Dec. 13, 2011, 7:48 a.m.
On Mon, 12 Dec 2011 13:53:08 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Dec 12, 2011 at 12:49 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > 2. Would the case of inconsistency between BBT and OOB markers be
> > possible to fix-up after the power cut?
> 
> Not trivially :) I suppose we could assume that any bad block markers
> in the OOB are in the BBT (when it exists) but not vice versa, then
> when we rebuild the BBT at boot time, we rescan the OOB only on the
> bad blocks, rewriting to OOB if necessary. Eventually, this kind of
> degrades to usefulness of the BBT though, if you're always checking
> back with the OOB markers.

What about the following scheme for 'nand_default_block_markbad' in case
NAND_BBT_WRITE_BBM is on:

1. Invalidate exising on-flash BBT
2. Update the BI marker on the OOB
3. Update the on-flash BBT

If a power cut occurs just before (1), then we failed to update either
BBT or OOB with the newly detected bad block. Can't do much about that.

If a power cut occurs between (1) and (2), then upon next boot the BBT
will be rebuilt by scanning the device.
Assuming all former bad blocks were indeed marked in their OOB (since
NAND_BBT_WRITE_BBM was on), then the scan would lead to a BBT identical
to the old BBT that was invalidated.
Again, the "new" bad block would not be marked - same case as if the
power cut occured before (1).

If a power cut occurs between (2) and (3), then upon next boot the BBT
will be rebuilt by scanning the device - this time the new BBT will
contain the newly detected bad block, since it was already OOB marked.

Invalidating the on-flash BBT might be implemented by erasing its block,
for example.

I guess this is a bigger change than the patch suggested, since
'nand_bbt.c' should expose interface for invalidating the BBT.

Also, need to examine if this scheme fits a scenario where there are
both main and mirror BBTs.
Brian Norris - Dec. 14, 2011, 8:46 p.m.
On Mon, Dec 12, 2011 at 11:48 PM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> What about the following scheme for 'nand_default_block_markbad' in case
> NAND_BBT_WRITE_BBM is on:
>
> 1. Invalidate exising on-flash BBT
> 2. Update the BI marker on the OOB
> 3. Update the on-flash BBT

Generally looks OK, but it complicates things unnecessarily IMO...

> I guess this is a bigger change than the patch suggested, since
> 'nand_bbt.c' should expose interface for invalidating the BBT.

Yes, and I don't want to rewrite nand_bbt just to expose an interface
simply for an esoteric power cut case.

> Also, need to examine if this scheme fits a scenario where there are
> both main and mirror BBTs.

This is one of the problems. We would need to invalidate both BBTs in
step 1, making our attempts at mirroring pointless. In fact, in some
power cut cases, we could be left in a worse condition than if we
didn't try to compensate for power cuts, since we just killed our
backup table.

Brian
Artem Bityutskiy - Dec. 17, 2011, 2:54 p.m.
On Mon, 2011-12-12 at 13:53 -0800, Brian Norris wrote:
> On Mon, Dec 12, 2011 at 12:49 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-12-07 at 13:08 -0800, Brian Norris wrote:
> >> Add and option (NAND_BBT_WRITE_BBM) for writing bad block markers to
> >> proper OOB area on each block in addition to flash-based BBT. This is
> >> useful 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
> >>
> >> Adapted from code by Matthieu CASTET.
> >>
> >> Cc: Matthieu CASTET <matthieu.castet@parrot.com>
> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >
> > 2 questions, perhaps silly, but still:
> >
> > 1. Why wouldn't you make this the only and the default behavior? Why
> > adding more options instead?
> 
> Personally, I'd like it to be default behavior. I guessed that it may
> cause incompatibility with some other system which depended on the
> current behavior, but if that's not a realistic concern, then I don't
> mind dropping the option. It's also possible to add a flag to restore
> the old behavior if needed in the future.

Right, I do not see that it will likely break older systems. I suggest
making this the only behavior, unless someone spots a problem with this.

You could send a new patch, and put into CC major MTD contributors
obtained with git-log, for example. If they have concerns, they can
rise the flag. How does this sound to you?
Artem Bityutskiy - Dec. 17, 2011, 3:06 p.m.
On Wed, 2011-12-14 at 12:46 -0800, Brian Norris wrote:
> This is one of the problems. We would need to invalidate both BBTs in
> step 1, making our attempts at mirroring pointless. In fact, in some
> power cut cases, we could be left in a worse condition than if we
> didn't try to compensate for power cuts, since we just killed our
> backup table.

I agree that it is not probably worth the effort, but would be nice to
do, if someone has time and thinks this is useful. But I think it would
be fair to request to add a comment which describes this hypothetical
issue and this possible solution, would it?
Brian Norris - Dec. 19, 2011, 6:51 p.m.
On Sat, Dec 17, 2011 at 7:06 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-12-14 at 12:46 -0800, Brian Norris wrote:
>> This is one of the problems. We would need to invalidate both BBTs in
>> step 1, making our attempts at mirroring pointless. In fact, in some
>> power cut cases, we could be left in a worse condition than if we
>> didn't try to compensate for power cuts, since we just killed our
>> backup table.
>
> I agree that it is not probably worth the effort, but would be nice to
> do, if someone has time and thinks this is useful. But I think it would
> be fair to request to add a comment which describes this hypothetical
> issue and this possible solution, would it?

Sure, I will add a comment, remove the option flag, and CC more
contributors (per your other recommendations) in a v2 patch.

Brian

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 35b4565..6d619f7 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -392,7 +392,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;
+	int block, ret = 0, i = 0;
 
 	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
 		ofs += mtd->erasesize - mtd->writesize;
@@ -405,7 +405,14 @@  static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	/* Do we have a flash based bad block table? */
 	if (chip->bbt_options & NAND_BBT_USE_FLASH)
 		ret = nand_update_bbt(mtd, ofs);
-	else {
+	/*
+	 * Write bad block marker to OOB, in one of two cases:
+	 *  (1) we don't have flash-based BBT
+	 *  (2) we have flash-based BBT but also want to keep markers in OOB in
+	 *      each block
+	 */
+	if (!(chip->bbt_options & NAND_BBT_USE_FLASH) ||
+			(chip->bbt_options & NAND_BBT_WRITE_BBM)) {
 		struct mtd_oob_ops ops;
 
 		nand_get_device(chip, mtd, FL_WRITING);
diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
index c4eec22..61fcb30 100644
--- a/include/linux/mtd/bbm.h
+++ b/include/linux/mtd/bbm.h
@@ -112,6 +112,15 @@  struct nand_bbt_descr {
 #define NAND_BBT_USE_FLASH	0x00020000
 /* Do not store flash based bad block table in OOB area; store it in-band */
 #define NAND_BBT_NO_OOB		0x00040000
+/*
+ * Write bad block marker to OOB when marking new bad blocks. This is the
+ * default behavior without NAND_USE_FLASH_BBT, so this is only useful in
+ * conjunction with NAND_USE_FLASH_BBT.
+ * One might need bad blocks marked in each block as well as in BBT when
+ * bootloader cannot read the flash-based BBT format or when the BBT is
+ * corrupted and the flash must be rescanned for bad blocks.
+ */
+#define NAND_BBT_WRITE_BBM	0x00080000
 
 /*
  * Flag set by nand_create_default_bbt_descr(), marking that the nand_bbt_descr