Fixup in NAND bad block management + fix of misspring . (nand_base.c)

Hi all,

I faced some issues with bad block marking on some NAND devices which have non-default bad block pattern. For the such devices I was unable to mark Bad blocks.
I made small changes in nand_block_bad function to cover the case of devices with non-default bad block pattern.

Also I found a missprint in nand_prepare_oobbuf() function :
- ofs += mtd->oobavail;
+ ofs += mtd->oobsize;
which could affect if somebody tries to write several pages with oob data via nand_write_ecc call.

Please see the patch bellow:
If nobody complains, I would be very much appreciate if somebody put this patch into MTD repository.

Thanks a lot,
Alexey

==================================

@@ -410,6 +410,7 @@

static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)

{
    int page, chipnr, res = 0;
+    int badblockpos;
    struct nand_chip *this = mtd->priv;
    u16 bad;

@@ -425,15 +426,22 @@

    } else
        page = (int) ofs;

+ /* If pattern is given we must use offset from badblock_pattern structure
+ else we should use badblockpos which is filled by default values */
+ if (this->badblock_pattern)
+ badblockpos=this->badblock_pattern->offs;
+ else
+ badblockpos=this->badblockpos;
+

    if (this->options & NAND_BUSWIDTH_16) {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE, page & this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page & this->pagemask);
        bad = cpu_to_le16(this->read_word(mtd));
        if (this->badblockpos & 0x1)
-            bad >>= 8;
+            bad >>= 1;
        if ((bad & 0xFF) != 0xff)
            res = 1;
    } else {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page & this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page & this->pagemask);
        if (this->read_byte(mtd) != 0xff)
            res = 1;
    }
@@ -470,8 +478,11 @@
    if (this->options & NAND_USE_FLASH_BBT)
        return nand_update_bbt (mtd, ofs);

-    /* We write two bytes, so we dont have to mess with 16 bit access */
-    ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+    if (this->badblock_pattern)

+ ofs += (this->badblock_pattern->offs & ~0x01);
+ else
+ ofs += (this->badblockpos & ~0x01);
+

    return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

@@ -1700,7 +1711,7 @@

            len += num;
            fsbuf += num;
        }
-        ofs += mtd->oobavail;
+        ofs += mtd->oobsize;
    }
    return this->oob_buf;
}

Thomas,

Couple of week ago, I sent this patch to list serever.
There weren't any feedback on this code. I don't know acceptable it or not. I would be very much appreciate if you review this patch.

Here is some notes to the patch:
At present time we have some issues with bad block management on ST NAND devices on Linux.
It is impossible to mark Bad block. The reason of it code doesn't work properly if bad block pattern is given. I made small fix in nand_block_bad function to cover the case of
devices with non-default bad block pattern.

Also there is a missprint in nand_prepare_oobbuf() function :

- ofs += mtd->oobavail;
+ ofs += mtd->oobsize;
which could affect if somebody tries to write several pages with oob data via nand_write_ecc call.

Please see the patch bellow:

Thanks,
Alexey

==================================

@@ -410,6 +410,7 @@

static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)

{
    int page, chipnr, res = 0;
+    int badblockpos;
    struct nand_chip *this = mtd->priv;
    u16 bad;

@@ -425,15 +426,22 @@

    } else
        page = (int) ofs;

+ /* If pattern is given we must use offset from badblock_pattern
structure
+ else we should use badblockpos which is filled by default values */
+ if (this->badblock_pattern)
+ badblockpos=this->badblock_pattern->offs;
+ else
+ badblockpos=this->badblockpos;
+

    if (this->options & NAND_BUSWIDTH_16) {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE,
page & this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page
& this->pagemask);
        bad = cpu_to_le16(this->read_word(mtd));
        if (this->badblockpos & 0x1)
-            bad >>= 8;
+            bad >>= 1;
        if ((bad & 0xFF) != 0xff)
            res = 1;
    } else {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page &
this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page &
this->pagemask);
        if (this->read_byte(mtd) != 0xff)
            res = 1;
    }
@@ -470,8 +478,11 @@
    if (this->options & NAND_USE_FLASH_BBT)
        return nand_update_bbt (mtd, ofs);

-    /* We write two bytes, so we dont have to mess with 16 bit access */
-    ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+    if (this->badblock_pattern)

+ ofs += (this->badblock_pattern->offs & ~0x01);
+ else
+ ofs += (this->badblockpos & ~0x01);
+

    return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

@@ -1700,7 +1711,7 @@

            len += num;
            fsbuf += num;
        }
-        ofs += mtd->oobavail;
+        ofs += mtd->oobsize;
    }
    return this->oob_buf;
}

===================================================

Hi Alexey,

Hi Vitaly,

Thanks a lot for feedback.
I agree for most of your comments. I missed some lines when I was merging the patch to the latest snapshot. I corrected it. Here is the second version at the end of the letter. Your comments are welcome.

You wrote

We definetely get here and badblock_pattern is given for the case of ST Nand. This patch should fix bad block marking issues found on ST Nand. Why we shouldn't be here if bad block pattern is given?

Please see patch below. I believe there shouldn't be tab issues. For just a case I attached diff file to the letter.

Thanks
Alexey
=======================

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
@@ -409,7 +409,7 @@
 */

static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip) {
- int page, chipnr, res = 0;
+ int page, badblockpos, chipnr, res = 0;

    struct nand_chip *this = mtd->priv;
    u16 bad;

@@ -425,15 +425,22 @@

    } else
        page = (int) ofs;

+ /* If pattern is given use offset from badblock_pattern structure
+ else use badblockpos which take default values */
+ if (this->badblock_pattern)
+ badblockpos=this->badblock_pattern->offs;
+ else
+ badblockpos=this->badblockpos;
+

    if (this->options & NAND_BUSWIDTH_16) {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE, page & this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page & this->pagemask);
        bad = cpu_to_le16(this->read_word(mtd));
        if (this->badblockpos & 0x1)
            bad >>= 8;
        if ((bad & 0xFF) != 0xff)
            res = 1;
    } else {
-        this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page & this->pagemask);
+        this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page & this->pagemask);
        if (this->read_byte(mtd) != 0xff)
            res = 1;
    }
@@ -470,8 +477,11 @@
    if (this->options & NAND_USE_FLASH_BBT)
        return nand_update_bbt (mtd, ofs);

-    /* We write two bytes, so we dont have to mess with 16 bit access */
-    ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+    if (this->badblock_pattern)

+ ofs += (this->badblock_pattern->offs & ~0x01);
+ else
+ ofs += (this->badblockpos & ~0x01);
+

    return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

@@ -1700,7 +1710,7 @@

            len += num;
            fsbuf += num;
        }
-        ofs += mtd->oobavail;
+        ofs += mtd->oobsize;
    }
    return this->oob_buf;
}

=======================

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
@@ -409,7 +409,7 @@
  */
 static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 {
-	int page, chipnr, res = 0;
+	int page, badblockpos, chipnr, res = 0;
 	struct nand_chip *this = mtd->priv;
 	u16 bad;
 

@@ -425,15 +425,22 @@
 	} else
 		page = (int) ofs;
 
+	/* If pattern is given use offset from badblock_pattern structure
+	   else use badblockpos which take default values */
+	if (this->badblock_pattern)
+		badblockpos=this->badblock_pattern->offs;
+	else
+		badblockpos=this->badblockpos;
+
 	if (this->options & NAND_BUSWIDTH_16) {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page & this->pagemask);
 		bad = cpu_to_le16(this->read_word(mtd));
 		if (this->badblockpos & 0x1)
 			bad >>= 8;
 		if ((bad & 0xFF) != 0xff)
 			res = 1;
 	} else {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page & this->pagemask);
 		if (this->read_byte(mtd) != 0xff)
 			res = 1;
 	}

@@ -470,8 +477,11 @@
 	if (this->options & NAND_USE_FLASH_BBT)
 		return nand_update_bbt (mtd, ofs);
 
-	/* We write two bytes, so we dont have to mess with 16 bit access */
-	ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+	if (this->badblock_pattern)
+		ofs += (this->badblock_pattern->offs & ~0x01);
+	else
+		ofs += (this->badblockpos & ~0x01);
+
 	return nand_write_oob (mtd, ofs , 2, &retlen, buf);
 }
 

@@ -1700,7 +1710,7 @@
 			len += num;
 			fsbuf += num;
 		}
-		ofs += mtd->oobavail;
+		ofs += mtd->oobsize;
 	}
 	return this->oob_buf;
 }

I still do not understand what this whole crap is for. This patch will not make it anywhere.

As I said before, this problem can simply be solved by a check for the manufacturer ID in nand_scan() and a small tweak of nand_default_bbt().

Your solution requires board driver support, but this is a chip level not a board level problem.

tglx

Hi Vitaly,

Thanks a lot for feedback.
I agree for most of your comments. I missed some lines when I was merging the patch to the latest snapshot. I corrected it. Here is the second version at the end of the letter. Your comments are welcome.

You wrote

We definetely get here and badblock_pattern is given for the case of ST Nand. This patch should fix bad block marking issues found on ST Nand. Why we shouldn't be here if bad block pattern is given?

Please see patch below. I believe there shouldn't be tab issues.

Thanks
Alexey
=======================
diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c

@@ -409,7 +409,7 @@
 */

static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip) {
- int page, chipnr, res = 0;
+ int page, badblockpos, chipnr, res = 0;

	struct nand_chip *this = mtd->priv;
	u16 bad;

@@ -425,15 +425,22 @@

	} else
		page = (int) ofs;

+ /* If pattern is given use offset from badblock_pattern structure
+ else use badblockpos which take default values */
+ if (this->badblock_pattern)
+ badblockpos=this->badblock_pattern->offs;
+ else
+ badblockpos=this->badblockpos;
+

	if (this->options & NAND_BUSWIDTH_16) {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page & this->pagemask);
		bad = cpu_to_le16(this->read_word(mtd));
		if (this->badblockpos & 0x1)
			bad >>= 8;
		if ((bad & 0xFF) != 0xff)
			res = 1;
	} else {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page & this->pagemask);
		if (this->read_byte(mtd) != 0xff)
			res = 1;
	}
@@ -470,8 +477,11 @@
	if (this->options & NAND_USE_FLASH_BBT)
		return nand_update_bbt (mtd, ofs);

- /* We write two bytes, so we dont have to mess with 16 bit access */
- ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+ if (this->badblock_pattern)
+ ofs += (this->badblock_pattern->offs & ~0x01);
+ else
+ ofs += (this->badblockpos & ~0x01);
+

	return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

@@ -1700,7 +1710,7 @@

			len += num;
			fsbuf += num;
		}
-		ofs += mtd->oobavail;
+		ofs += mtd->oobsize;
	}
	return this->oob_buf;
}

====================

Hi Alexey,

Because if bad block pattern is used, we should use bad block table. This is how I see it, though I can be wrong.

I'm afraid there still are. It's probably due to your mailer converting tabs to spaces.

Best regards,
Vitaly

Please take a look at page 29 (we use x16 device) Here is specification of NAND device.
http://www.st.com/stonline/products/literature/ds/10058/nand512r3a.pdf

According to this specification :
Size of oobblock is 512b and bad block position should be 0.

According to the nand_base.c code
There are only two cases for bad block positions:
a) If oobblock size is greater than 512b this case bad block position offset should be 0
b) If oobblock size is lower or equal than 512b this case offset pointing to the bad block should be 5

It contradicts to the specification.

To avoid contradictions I added bad block pattern to the low level driver for ST NAND device.
Then I faced issue: nand_block_bad and block_markbad doesn't parse case when bad block pattern is given. So I added parsing too.

I want to use memory based BBT (not flash based).
I wonder if there any way to have correct Bad block management and memory based BBT for NAND with non default bad block positions. The problem in the current implementation that it's impossible to save information about bad block marking if you are going to use memory based BBT and you have NAND without default bad block positioning.

Please take a look at the bad block marking code:
If NAND_USE_FLASH_BBT option is not given we have to write BB data at the mtd->oobsize + (this->badblockpos & ~0x01) offset.
For some NAND devices this is incorrect.

static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs) {
struct nand_chip *this = mtd->priv;
u_char buf[2] = {0, 0};
size_t retlen;
int block;

   /* Get block number */
   block = ((int) ofs) >> this->bbt_erase_shift;
   if (this->bbt)
       this->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);

/* Do we have a flash based bad block table ? */ if (this->options & NAND_USE_FLASH_BBT)

       return nand_update_bbt (mtd, ofs);

/* We write two bytes, so we dont have to mess with 16 bit access */ ofs += mtd->oobsize + (this->badblockpos & ~0x01);
return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

The patch I sent modifies offset according to the bad block pattern information.

======================================================

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
@@ -409,7 +409,7 @@
 */

static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip) {
- int page, chipnr, res = 0;
+ int page, badblockpos, chipnr, res = 0;

	struct nand_chip *this = mtd->priv;
	u16 bad;

@@ -425,15 +425,22 @@

	} else
		page = (int) ofs;

+ /* If pattern is given use offset from badblock_pattern structure
+ else use badblockpos which take default values */
+ if (this->badblock_pattern)
+ badblockpos=this->badblock_pattern->offs;
+ else
+ badblockpos=this->badblockpos;
+

	if (this->options & NAND_BUSWIDTH_16) {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos & 0xFE, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos & 0xFE, page & this->pagemask);
		bad = cpu_to_le16(this->read_word(mtd));
		if (this->badblockpos & 0x1)
			bad >>= 8;
		if ((bad & 0xFF) != 0xff)
			res = 1;
	} else {
-		this->cmdfunc (mtd, NAND_CMD_READOOB, this->badblockpos, page & this->pagemask);
+		this->cmdfunc (mtd, NAND_CMD_READOOB, badblockpos, page & this->pagemask);
		if (this->read_byte(mtd) != 0xff)
			res = 1;
	}
@@ -470,8 +477,11 @@
	if (this->options & NAND_USE_FLASH_BBT)
		return nand_update_bbt (mtd, ofs);

- /* We write two bytes, so we dont have to mess with 16 bit access */
- ofs += mtd->oobsize + (this->badblockpos & ~0x01);
+ if (this->badblock_pattern)
+ ofs += (this->badblock_pattern->offs & ~0x01);
+ else
+ ofs += (this->badblockpos & ~0x01);
+

	return nand_write_oob (mtd, ofs , 2, &retlen, buf);
}

@@ -1700,7 +1710,7 @@

			len += num;
			fsbuf += num;
		}
-		ofs += mtd->oobavail;
+		ofs += mtd->oobsize;
	}
	return this->oob_buf;
}

Thomas,

Fixing this->badblockpos in low level driver will not help because, according to the code this->badblockpos updates in nand_base.c unconditionally.
Moreover it's rather hard to define the conditions, because by default structure "this" is filled by zeroes. (We can't distinguish advisedly setting of badblockpos from the default value).

Alexey,

Well, maybe I'm missting something, but what prevents you from setting this->badblockpos to 0 *after* call to nand_scan??

Best regards,
Vitaly

Did I say low level driver ?

And you believe that your patch is the only way to solve that trivial problem ?

Without thinking too much about the problem, there _are_ at least two sane places to fix that.

1. nand_scan() can handle this based on chip id and/or manufacturer id
2. nand_scan_bbt() can do the fixup as well

When the ST chips have the bad block pos at offset 0 in general then we want a generic solution which fixes up the nand_scan_bbt code as well instead of requiring a seperate board driver supplied badblock_pattern.

Your patch is bogus anyway, as the else path will _NEVER_ be executed. this->badblock_pattern is never NULL.

tglx

Thomas,

Generic solution would be great. But this solution will require inforamtion about BB positioning for all chips. At present time I don't have info about BB positioning for all chips. I wonder if BB positioning is somehow standardized? I'm not sure that it will be possible to avoid board driver supplied badblock_patterns at all.

Imo the latest nand_base code has some inconsistence.
If mem based BBT is used, this case Bad block scanning is based on data from bad block pattern, but writing is based on data of badblockpos variable.
I guess it's not good to have two variables responsible for one thing if they are used at the same time. Moreover this code doesn't work if somebody specified BB pattern different from default. My patch resolves this inconsistence, logically it works rather clear a) if pattern is specified we will use pattern for both read and write.
b) if not we will use the "badblockpos" variable.
I'd like to know do you have any objections to the patch? IMO it should not break anything. But it resolves the described inconsistence.

That is incorrect. This->badblock_pattern can be NULL. According to the nand_base code if NAND_SKIP_BBTSCAN option is given and pattern is not defined in low level driver the nand_scan_bbt function will not be executed and no code will define badblock_pattern.

Thanks,
Alexey

Yes. And no other solution will make it into the kernel. Board level fixes for chip level problems are not acceptable.

Yes it is. And according to the ST datasheet it is simply at offset 0x05 for 8 bit devices and offset 0x00 for 16 bit devices.

Fixup the offsets in nand_scan and tweak nand_bbt_default. Again this is a chip problem not a board problem. Chips are handled by the generic nand_base and nand_bbt code to avoid code duplication all over the place.

Well, thats true, but this can be fixed with two lines of code.

Did not think about this weird scenario as I never imagined a usecase. It still does not need all that hacks. A simple fixup of badblockpos is enough.

tglx

Hello Thomas,

I appologize for delay. I was away on vacation.

I your point of view regarding BB mabnagement is clear. Fixup of badblockpos is rather suitable.

There was another very small fix for a missprint in nand_base.c. I think it shouldn't raise many objections. If you don't have any would you please check in the patch below ?

Thanks,
Alexey

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
@@ -1700,7 +1710,7 @@
 			len += num;
 			fsbuf += num;
 		}
-		ofs += mtd->oobavail;
+		ofs += mtd->oobsize;
 	}
 	return this->oob_buf;
 }