| Message ID | 1323292100-19280-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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?
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?
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
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
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(-)