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

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

[Alexey, Korolev]

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


==================================
--- a/drivers/mtd/nand/nand_base.c    2006-01-20 18:13:49.657859296 +0300
+++ b/drivers/mtd/nand/nand_base.c    2006-01-20 18:38:50.281729792 +0300
@@ -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;
 }

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Alexey, Korolev]

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

==================================
--- a/drivers/mtd/nand/nand_base.c    2006-01-20 18:13:49.657859296 +0300
+++ b/drivers/mtd/nand/nand_base.c    2006-01-20 18:38:50.281729792 +0300
@@ -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;
 }

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

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Vitaly Wool]

Hi Alexey,

Alexey, Korolev wrote:

>
> ==================================
> --- a/drivers/mtd/nand/nand_base.c    2006-01-20 18:13:49.657859296 +0300
> +++ b/drivers/mtd/nand/nand_base.c    2006-01-20 18:38:50.281729792 +0300
> @@ -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;

Bah, alignment/tabs problem...

>
> +    /* 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;
> +

I'm not sure this is right. If badblock_pattern is set, we shouldn't 
ever be here.

>         if (this->badblockpos & 0x1)
> -            bad >>= 8;
> +            bad >>= 1;

And here you do revert the bugfix committed long ago.

>     @@ -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);
> +

See above.

Vitaly

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Alexey, Korolev]

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

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
 >> +    /* 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;
 >> +

 > I'm not sure this is right. If badblock_pattern is set, we shouldn't
 > ever be here.

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
--- b/drivers/mtd/nand/nand_base.c    2006-02-09 20:05:28.447558688 +0300
+++ c/drivers/mtd/nand/nand_base.c    2006-02-09 20:14:58.182945744 +0300
@@ -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;
 }
=======================

[-- Attachment #2: badblock_pattern.diff --]
[-- Type: text/plain, Size: 1866 bytes --]

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
--- b/drivers/mtd/nand/nand_base.c	2006-02-09 20:05:28.447558688 +0300
+++ c/drivers/mtd/nand/nand_base.c	2006-02-09 20:14:58.182945744 +0300
@@ -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;
 }

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Alexey, Korolev]

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
 >> +    /* 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;
 >> +

 > I'm not sure this is right. If badblock_pattern is set, we shouldn't
 > ever be here.

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

--- b/drivers/mtd/nand/nand_base.c	2006-02-09 20:05:28.447558688 +0300
+++ c/drivers/mtd/nand/nand_base.c	2006-02-09 20:14:58.182945744 +0300
@@ -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;
 }
====================

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Vitaly Wool]

Hi Alexey,

> You wrote
> >> +    /* 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;
> >> +
>
> > I'm not sure this is right. If badblock_pattern is set, we shouldn't
> > ever be here.
>
> 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?
>
Because if bad block pattern is used, we should use bad block table. 
This is how I see it, though I can be wrong.

> Please see patch below. I believe there shouldn't be tab issues.
>
I'm afraid there still are. It's probably due to your mailer converting 
tabs to spaces.

Best regards,
    Vitaly

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Alexey, Korolev]

Vitaly Wool wrote:

> Hi Alexey,
>
> > You wrote
> > >> +    /* 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;
> > >> +
> >
> > > I'm not sure this is right. If badblock_pattern is set, we shouldn't
> > > ever be here.
> >
> > 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?
> >
> Because if bad block pattern is used, we should use bad block table.
> This is how I see it, though I can be wrong.
>
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
--- b/drivers/mtd/nand/nand_base.c	2006-02-09 20:05:28.447558688 +0300
+++ c/drivers/mtd/nand/nand_base.c	2006-02-09 20:14:58.182945744 +0300
@@ -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;
 }

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Thomas Gleixner]

On Mon, 2006-02-20 at 13:53 +0300, Alexey, Korolev wrote:
> +	/* 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;

I still do not get why you need all this magic instead of fixing up
this->badblockpos in the first place.

	tglx

Re: [PATCH] Fixup in NAND bad block management + fix of misspring.(nand_base.c)

[Alexey, Korolev]

Thomas,

> I still do not get why you need all this magic instead of fixing up
> this->badblockpos in the first place.
>
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).

Re: [PATCH] Fixup in NAND bad block management + fix of misspring.(nand_base.c)

[Vitaly Wool]

Alexey,

Alexey, Korolev wrote:

> Thomas,
>
>> I still do not get why you need all this magic instead of fixing up
>> this->badblockpos in the first place.
>>
> 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).

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

Best regards,
   Vitaly

Re: [PATCH] Fixup in NAND bad block management + fix of misspring.(nand_base.c)

[Thomas Gleixner]

On Mon, 2006-02-20 at 14:56 +0300, Alexey, Korolev wrote:
> Thomas,
> 
> > I still do not get why you need all this magic instead of fixing up
> > this->badblockpos in the first place.
> >
> Fixing this->badblockpos in low level driver will not help because, 
> according to the code this->badblockpos updates in nand_base.c 
> unconditionally.

Did I say low level driver ?

> 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).

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

Re: [PATCH] Fixup in NAND bad block management + fix ofmisspring.(nand_base.c)

[Alexey, Korolev]

Thomas,

> 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.
>
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.

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

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.

> Your patch is bogus anyway, as the else path will _NEVER_ be executed.
> this->badblock_pattern is never NULL.
>
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

Re: [PATCH] Fixup in NAND bad block management + fix of misspring .(nand_base.c)

[Thomas Gleixner]

On Thu, 2006-02-09 at 20:54 +0300, Alexey, Korolev wrote:
> 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.

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

Re: [PATCH] Fixup in NAND bad block management + fix ofmisspring.(nand_base.c)

[Thomas Gleixner]

On Thu, 2006-03-02 at 20:29 +0300, Alexey, Korolev wrote:
> > 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.
> >
> Generic solution would be great. 

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

> 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?

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.

> I'm not sure that it will be possible to avoid board driver supplied 
> badblock_patterns at all.

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.

>  > And you believe that your patch is the only way to solve that trivial
>  > problem ?
> 
> 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.

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

> 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.
> 
> > Your patch is bogus anyway, as the else path will _NEVER_ be executed.
> > this->badblock_pattern is never NULL.
> >
> 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.

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

Re: [PATCH] Fixup in NAND bad block management + fixofmisspring.(nand_base.c)

[Alexey, Korolev]

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

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








[-- Attachment #2: miss_print_fixup.diff --]
[-- Type: text/plain, Size: 355 bytes --]

diff -aur b/drivers/mtd/nand/nand_base.c c/drivers/mtd/nand/nand_base.c
--- b/drivers/mtd/nand/nand_base.c	2006-02-09 20:05:28.447558688 +0300
+++ c/drivers/mtd/nand/nand_base.c	2006-02-09 20:14:58.182945744 +0300
@@ -1700,7 +1710,7 @@
 			len += num;
 			fsbuf += num;
 		}
-		ofs += mtd->oobavail;
+		ofs += mtd->oobsize;
 	}
 	return this->oob_buf;
 }