[PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Shmulik Ladkani]

The code responsible for reading the version of the mirror bbt was
incorrectly using the descriptor of the main bbt.

Pass the mirror bbt descriptor to 'scan_read_raw' when reading the
version of the mirror bbt.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
- Spotted while reviewing BBT code.
- Introduced by 7cba7b1 [mtd: nand: add support for BBT without OOB].
- Looks like a typo.
- Not runtime tested.
- Sebastian, care to take a look and Ack?

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 30d1319..c126469 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -390,7 +390,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 	/* Read the mirror version, if available */
 	if (md && (md->options & NAND_BBT_VERSION)) {
 		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
-			      mtd->writesize, td);
+			      mtd->writesize, md);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
 		pr_info("Bad block table at page %d, version 0x%02X\n",
 			 md->pages[0], md->version[0]);

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Sebastian Andrzej Siewior]

On 06/10/2012 12:58 PM, Shmulik Ladkani wrote:
> The code responsible for reading the version of the mirror bbt was
> incorrectly using the descriptor of the main bbt.
>
> Pass the mirror bbt descriptor to 'scan_read_raw' when reading the
> version of the mirror bbt.
>
> Signed-off-by: Shmulik Ladkani<shmulik.ladkani@gmail.com>
> ---
> - Spotted while reviewing BBT code.
> - Introduced by 7cba7b1 [mtd: nand: add support for BBT without OOB].
> - Looks like a typo.
> - Not runtime tested.
> - Sebastian, care to take a look and Ack?

Looks good.

Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index 30d1319..c126469 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -390,7 +390,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
>   	/* Read the mirror version, if available */
>   	if (md&&  (md->options&  NAND_BBT_VERSION)) {
>   		scan_read_raw(mtd, buf, (loff_t)md->pages[0]<<  this->page_shift,
> -			      mtd->writesize, td);
> +			      mtd->writesize, md);
>   		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
>   		pr_info("Bad block table at page %d, version 0x%02X\n",
>   			 md->pages[0], md->version[0]);

Sebastian

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Brian Norris]

Hi,

On Sun, Jun 10, 2012 at 3:58 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> - Not runtime tested.

Patch looks fine, but this makes me curious: does anyone use
NAND_BBT_ABSPAGE? It looked broken to me when I tried it not tool long
ago, but I didn't look too deeply. (I doubt that this typo was its
only problem, but I may try again...)

Brian

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On Sun, 2012-06-10 at 22:59 -0700, Brian Norris wrote:
> Hi,
> 
> On Sun, Jun 10, 2012 at 3:58 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > - Not runtime tested.
> 
> Patch looks fine, but this makes me curious: does anyone use
> NAND_BBT_ABSPAGE? It looked broken to me when I tried it not tool long
> ago, but I didn't look too deeply. (I doubt that this typo was its
> only problem, but I may try again...)

Looks like this is used in diskonchip.c.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Brian Norris]

On Tue, Jun 12, 2012 at 3:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-06-10 at 22:59 -0700, Brian Norris wrote:
>> On Sun, Jun 10, 2012 at 3:58 AM, Shmulik Ladkani
>> <shmulik.ladkani@gmail.com> wrote:
>> > - Not runtime tested.
>>
>> Patch looks fine, but this makes me curious: does anyone use
>> NAND_BBT_ABSPAGE? It looked broken to me when I tried it not too long
>> ago, but I didn't look too deeply. (I doubt that this typo was its
>> only problem, but I may try again...)
>
> Looks like this is used in diskonchip.c.

Hmm, OK. This driver seems to be very stale (very few real
fixes/improvements in git history). Anyone know if diskonchip.c even
works with the current BBT code?

I retried NAND_BBT_ABSPAGE on my own driver, and it seems that this
code-path has no ability to *create* a bad block table where one
didn't exist previously. It simply reads whatever data is present at
the given page(s) (according to td->pages[]), regardless of ECC
errors, junk data, lack of BBT markers (i.e., "Bbt0" or "1tbB"), and
versioning.

So when I use NAND_BBT_ABSPAGE with an erased block at page 1024, I
get the following:

   Bad block table at page 1024, version 0xFF

And no blocks are detected bad (simply because there was 0xff "table
data"). But then, I get even worse results if the uninitialized BBT
block has arbitrary (non-0xff) data! nand_bbt would just mark random
blocks as bad...

So, this "feature" seems severely limited - designed for a somewhat
static, pre-initialized BBT. I can probably survive by continuing to
ignore this eyesore, but I'd rather just fix it or kill it.

Brian

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

Let's probe Mike and Robert and see what they say.

On Tue, 2012-06-12 at 17:42 -0700, Brian Norris wrote:
> On Tue, Jun 12, 2012 at 3:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Sun, 2012-06-10 at 22:59 -0700, Brian Norris wrote:
> >> On Sun, Jun 10, 2012 at 3:58 AM, Shmulik Ladkani
> >> <shmulik.ladkani@gmail.com> wrote:
> >> > - Not runtime tested.
> >>
> >> Patch looks fine, but this makes me curious: does anyone use
> >> NAND_BBT_ABSPAGE? It looked broken to me when I tried it not too long
> >> ago, but I didn't look too deeply. (I doubt that this typo was its
> >> only problem, but I may try again...)
> >
> > Looks like this is used in diskonchip.c.
> 
> Hmm, OK. This driver seems to be very stale (very few real
> fixes/improvements in git history). Anyone know if diskonchip.c even
> works with the current BBT code?
> 
> I retried NAND_BBT_ABSPAGE on my own driver, and it seems that this
> code-path has no ability to *create* a bad block table where one
> didn't exist previously. It simply reads whatever data is present at
> the given page(s) (according to td->pages[]), regardless of ECC
> errors, junk data, lack of BBT markers (i.e., "Bbt0" or "1tbB"), and
> versioning.
> 
> So when I use NAND_BBT_ABSPAGE with an erased block at page 1024, I
> get the following:
> 
>    Bad block table at page 1024, version 0xFF
> 
> And no blocks are detected bad (simply because there was 0xff "table
> data"). But then, I get even worse results if the uninitialized BBT
> block has arbitrary (non-0xff) data! nand_bbt would just mark random
> blocks as bad...
> 
> So, this "feature" seems severely limited - designed for a somewhat
> static, pre-initialized BBT. I can probably survive by continuing to
> ignore this eyesore, but I'd rather just fix it or kill it.
> 
> Brian

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> Let's probe Mike and Robert and see what they say.
Sure.

>> I retried NAND_BBT_ABSPAGE on my own driver, and it seems that this
>> code-path has no ability to *create* a bad block table where one
>> didn't exist previously. It simply reads whatever data is present at
>> the given page(s) (according to td->pages[]), regardless of ECC
>> errors, junk data, lack of BBT markers (i.e., "Bbt0" or "1tbB"), and
>> versioning.

OK, I can explain the driver behaviour, as it looks like it is the same on the
docg3.

The way I understand things are handled on diskonchip devices :
 (1) the chip is manufactured
 (2) the chip is factory-tested to check if there are any initial bad blocks
 (3) the information which blocks are good/bad is written into the "One Time
 Programmable" area of the chip
 (4) the chip is shipped

Now, this 'Bad Block Table' (in the DoC meaning) is a table of factory bad
blocks. It doesn't include worn out blocks, as the OTP area is ... read-only.

To store worn out blocks, each filesystem implementation has to do something
smart. UBI has it way, SAFTL (docg3 fs) has its way, etc ...

But in the end, the bad block table is immutable, and represents factory bad
blocks, not up-to-date list of bad blocks.

And of course, as it is factory written, no ECC is required (it looks to me as a
fuse system there rather that pure NAND thing).

>> And no blocks are detected bad (simply because there was 0xff "table
>> data"). But then, I get even worse results if the uninitialized BBT
>> block has arbitrary (non-0xff) data! nand_bbt would just mark random
>> blocks as bad...
All "0xff" is the nominal situation, ie. you had a perfect chip shipped.

>> So, this "feature" seems severely limited - designed for a somewhat
>> static, pre-initialized BBT. I can probably survive by continuing to
>> ignore this eyesore, but I'd rather just fix it or kill it.
As you say, static pre-initialized BBT, that fits.

As to whether you should kill it or not, it's up to the maintainer of diskonchip
I suppose.

Cheers.

-- 
Robert

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Mon, 2012-06-18 at 19:59 +0200, Robert Jarzmik wrote:
> To store worn out blocks, each filesystem implementation has to do something
> smart. UBI has it way, SAFTL (docg3 fs) has its way, etc ...

UBI does not do anything about storing the bad block table. UBI relies
on the MTD layer - it may mark a block as bad and MTD should take care
of storing this information either in the OOB or in the BBT on in both
places.

> But in the end, the bad block table is immutable, and represents factory bad
> blocks, not up-to-date list of bad blocks.

How do you store the information about the later developed bad blocks?

> As to whether you should kill it or not, it's up to the maintainer of diskonchip
> I suppose.

There is no DoC maintainer, you can consider yourself to be the
maintainer, because you and Mike are the only people who care about this
AFAICS.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Mike Dunn]

On 06/18/2012 01:28 PM, Artem Bityutskiy wrote:

> There is no DoC maintainer, you can consider yourself to be the
> maintainer, because you and Mike are the only people who care about this
> AFAICS.


Actually, I don't really care ;)  The docg4 only shares a brand name with the
legacy DOCs.

If they have been orphaned, I'd be willing to be the maintainer if I could get
my hands on at least one variant for testing.  Anyone know the names of any
products that contain them?  I believe some were used as a SSD in PCs, but a big
part of the business was to the embedded market.

Thanks,
Mike

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Mon, 2012-06-18 at 19:59 +0200, Robert Jarzmik wrote:
>> To store worn out blocks, each filesystem implementation has to do something
>> smart. UBI has it way, SAFTL (docg3 fs) has its way, etc ...
>
> UBI does not do anything about storing the bad block table. UBI relies
> on the MTD layer - it may mark a block as bad and MTD should take care
> of storing this information either in the OOB or in the BBT on in both
> places.
Urg ... I was under the impression UBI stored that information in its control
data, too bad for me ...

>> But in the end, the bad block table is immutable, and represents factory bad
>> blocks, not up-to-date list of bad blocks.
> How do you store the information about the later developed bad blocks?

Well, I think ... I don't actually. I should review a bit docg3 driver, as there
is a spare byte in the OOB area to store information. The trick is, if the block
is already worn out, can I rely on OOB to store the bad block info ?

I remember seeing people on this mailing list discussing this topic, but I can't
remember the outcome ...

>> As to whether you should kill it or not, it's up to the maintainer of diskonchip
>> I suppose.
> There is no DoC maintainer, you can consider yourself to be the
> maintainer, because you and Mike are the only people who care about this
> AFAICS.
Same as Mike, I don't have the hardware. If somebody has some spare hardware to
provide, I'd gladly take over the maintenership.

Cheers.

-- 
Robert

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Mike Dunn]

On 06/24/2012 11:52 AM, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
> 
>>> But in the end, the bad block table is immutable, and represents factory bad
>>> blocks, not up-to-date list of bad blocks.
>> How do you store the information about the later developed bad blocks?
> 
> Well, I think ... I don't actually. I should review a bit docg3 driver, as there
> is a spare byte in the OOB area to store information. The trick is, if the block
> is already worn out, can I rely on OOB to store the bad block info ?


It's imperfect for that reason, but mitigated by the fact that the block is
considered bad if the expected pattern in oob does not match exactly.

Another problem with this method is that if the flash contains data not written
by mtd, the bb scan falsely identifies most (if not all) of the non-blank blocks
as "bad".  This is the reason the docg4 driver has a module parameter
'ignore_badblocks'.  I have to use this parameter in order to read the flash in
a phone that still contains the data from PalmOS.

And currently the bb scan is done over the whole device, regardless of the
partitioning scheme.  This makes a dual boot setup, where the docg4 is divided
into PalmOS and Linux partitions, impossible without a blizzard of warnings
during initialization about bad blocks within the PalmOS partition.

Mike

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

On Sun, 2012-06-24 at 20:52 +0200, Robert Jarzmik wrote:
> > UBI does not do anything about storing the bad block table. UBI relies
> > on the MTD layer - it may mark a block as bad and MTD should take care
> > of storing this information either in the OOB or in the BBT on in both
> > places.
> Urg ... I was under the impression UBI stored that information in its control
> data, too bad for me ...

No, UBI calls 'mtd_block_markbad()' and relies on MTD marking the block
as bad - either by writing to OOB or using a bad block table.

> > There is no DoC maintainer, you can consider yourself to be the
> > maintainer, because you and Mike are the only people who care about this
> > AFAICS.
> Same as Mike, I don't have the hardware. If somebody has some spare hardware to
> provide, I'd gladly take over the maintenership.

I do not have any HW with those, unfortunately.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Mike Dunn]

On 06/18/2012 04:12 AM, Artem Bityutskiy wrote:
> Let's probe Mike and Robert and see what they say.


To be honest, I did not pay much attention to the NAND_BBT_ABSPAGE code path.
The docg4 uses the oob marker scheme for marking bad blocks, though it also has
a read-only "factory" bbt...


>>>> <shmulik.ladkani@gmail.com> wrote:
>>>>> - Not runtime tested.
>>>>
>>>> Patch looks fine, but this makes me curious: does anyone use
>>>> NAND_BBT_ABSPAGE? It looked broken to me when I tried it not too long
>>>> ago, but I didn't look too deeply. (I doubt that this typo was its
>>>> only problem, but I may try again...)


As Robert pointed out, docg3 and docg4 ship with a read-only factory bbt at a
fixed location.  If this is what NAND_BBT_ABSPAGE refers to, the docg4 doesn't
use it.  For one thing, I assumed that the format of the table on the docg4
would be different from what is assumed in the nand code for the
NAND_BBT_ABSPAGE case.  I also assumed (without much investigation) that the
fixed location table would be dynamically updated as bad blocks are detected
(which docg4 can't do), though doing this at a fixed location doesn't make much
sense now that I think about it.

FYI... in its probe function, the docg4 reads this factory bbt after
nand_scan_bbt() has built the memory-based bbt. (In the docg4 case,
nand_scan_bbt() builds a memory-based table by looking for the bb marker in the
oob bytes of the first page of each block.)  Then the docg4 driver ensures that
the memory based bbt includes the bad blocks marked in the factory bbt, and
updates it if not.  This, and the NAND_BBT_ABSPAGE code, can probably be
improved.  I'll take a closer look.

Thanks,
Mike

Re: [PATCH] mtd: nand: Use the mirror BBT descriptor when reading its version

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Sun, 2012-06-10 at 13:58 +0300, Shmulik Ladkani wrote:
> The code responsible for reading the version of the mirror bbt was
> incorrectly using the descriptor of the main bbt.
> 
> Pass the mirror bbt descriptor to 'scan_read_raw' when reading the
> version of the mirror bbt.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Added

Cc: stable@vger.kernel.org [v2.6.37+]

and pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]