[PATCH] mtd: nand: omap: save Bad-Block-Table (BBT) on device

[Brian Norris]

On Wed, Jul 23, 2014 at 05:27:46PM -0300, ezequiel at vanguardiasur.com.ar wrote:
> On 23 Jul 04:26 PM, Gupta, Pekon wrote:
> > >From: Ezequiel Garcia [mailto:ezequiel at vanguardiasur.com.ar]
> > >>On 23 Jul 05:24 PM, Pekon Gupta wrote:
> > >> This patch makes OMAP NAND driver to
> > >> - save Bad-Block-Table (BBT) on NAND Flash device
> > >> - scan on device BBT during probe
> > >>
> > >
> > >Doesn't this break backward compatibility? Please correct me if I'm wrong
> > >here, but if I apply this patch, and boot with my current NAND flash
> > >contents, the kernel won't have any place to store the BBT (assuming
> > >there are no blocks reserved).
> > 
> > Good point. I'll rather like your opinion to judge my below understanding
> > as you were recently involved in some NAND BBT features additions..
> > 
> > When bbt_options "NAND_BBT_USE_FLASH" is enabled, default BBT
> > options will be used which are:
> > 
> > $KERNEL/drivers/mtd/nand_bbt.c
> > static struct nand_bbt_descr bbt_main_descr = {
> > 	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
> > 		| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> > 
> > As default options have NAND_BBT_CREATE and NAND_BBT_WRITE,
> > so kernel will try to reserve erase-blocks for main-BBT and mirror-BBT
> > towards end of NAND device.
> > 
> > - Case-1: If there is un-allocated space (not allocated to any partition)

Partitioning has no effect on the BBT process. BBT is run only for the
'master' MTD, and it has no regard for previous data on the flash. Given
that...

> >   at end of Flash device then it will be used. And there is no problem.
> > 
> 
> Yes, so far, so good.

Agreed. It is especially safe because that area was not partitioned, and
therefore should not contain important data.

> > - Case-2: If there is no free space towards the end of flash, then ...
> > As per my understanding, the create_bbt() would try scanning whole
> > NAND to find empty blocks and then it creates BBT wherever it finds
> > empty blocks. And those blocks are marked as BAD with BBT signature
> > to prevent getting erased or over-written by any user-data.
> > 
> 
> OK, I just went through the BBT code, trying to see if I was missing
> something. I can't see any place where the kernel scans the device
> and searches for empty space for the BBT.

Right, on-flash BBT has no notion of "empty space", really; it just
knows what region is reserved for its use (the last few blocks of the
device).

> Instead, what create_bbt() seem to do is scan the whole device, searching
> for bad blocks to fill an in-memory BBT. I presume that if I have some
> blocks marked as bad in the OOB region, they are identified as bad
> and the BBT is filled.
> 
> I think that write_bbt() is where the (previously created, in-memory) BBT
> is written to flash. It goes like this:
> 
> write_bbt()
> {
> [..]
>                 /*
>                  * Automatic placement of the bad block table. Search direction
>                  * top -> down?
>                  */
>                 if (td->options & NAND_BBT_LASTBLOCK) {
>                         startblock = numblocks * (chip + 1) - 1;
>                         dir = -1;
>                 } else {
>                         startblock = chip * numblocks;
>                         dir = 1;
>                 }
> 
>                 for (i = 0; i < td->maxblocks; i++) {
>                         int block = startblock + dir * i;
>                         /* Check, if the block is bad */
>                         switch (bbt_get_entry(this, block)) {
>                         case BBT_BLOCK_WORN:
>                         case BBT_BLOCK_FACTORY_BAD:
>                                 continue;
>                         }
>                         page = block <<
>                                 (this->bbt_erase_shift - this->page_shift);
>                         /* Check, if the block is used by the mirror table */
>                         if (!md || md->pages[chip] != page)
>                                 goto write;
>                 }
>                 pr_err("No space left to write bad block table\n");
>                 return -ENOSPC;
> 		[..]
> }
> 
> which fails if there's no room left for the BBT. This will happen on every
> boot, so it's not an ideal situation.

BTW, td->maxblocks==4 by default. There's no 'scanning the whole
device'; it only scans the last 4 blocks. But you're correct that it's
not ideal, since it will fail if there are 4 bad blocks at the end.

> > Is my understanding correct?
> > If yes, then this will not break backward compatibility, as on upgrading
> > the kernel new BBT will be automatically written on first boot. And
> > it will be used in subsequent boots.
> > Need feedbacks ...
> > 
> 
> In fact, you do have a simple way to solve this. Just support BBT through
> the "nand-on-flash-bbt" devicetree property, so a user can tell if his flash
> has a BBT or not. See pxa3xx-nand.c, which should be correct.

Yes, you can use the 'nand-on-flash-bbt' property, and that probably
makes the most sense.

The only real backwards-compatibility concern you'd have for
unconditionally enabling on-flash BBT (like in this patch) is if you had
previous file system data in the last 4 blocks; nand_bbt will just
clobber it, breaking your file system. For this reason, using DT is
probably a good idea -- you're opting in, rather than being forced in by
a kernel upgrade.

Beyond the backwards-compatibility concern, you still have other
concerns about on-flash BBT's robustness. Limiting yourself to a region
of 4 blocks is one potential issue. There are others (e.g., lack of CRC
protection), but none that have been real show-stoppers. I have a few
generations of products running it here.

BTW, there's a recent thread about GPMI and its "bad block mark
swapping", which has led to somebody wanting a new DT binding for
'on-flash-bbt-no-oob-bbm' or something like that, to mirror the
(excellently named -- by me) NAND_BBT_NO_OOB_BBM option. I'm not real
happy with adding a random assortment of configurable BBT flags into the
DT ABI, without fully describing and standardizing the BBT format. It's
kind of a vacuous target right now, which is just defined by the
70%-baked solution in our current implementation... (I guess I need to
go reply to the GPMI/NO_OOB_BBM thread soon!)

Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2014-March/052666.html
    http://lists.infradead.org/pipermail/linux-mtd/2014-March/052796.html
    http://lists.infradead.org/pipermail/linux-mtd/2014-March/052988.html