| Message ID | 20120610135812.01dce4e7@pixies.home.jungo.com |
|---|---|
| State | Accepted |
| Commit | 7bb9c75436212813b38700c34df4bbb6eb82debe |
| Headers | show |
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
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
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.
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
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
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!
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.
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.
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
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
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.
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
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.
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]);
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?