| Message ID | 4bd5da1f7d57aa32c5682297f7aa0c768e8c6e49.1430034929.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 26, 2015 at 11:16:49AM +0300, Baruch Siach wrote: > For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or > 8bit ECC data, respectively. Larger oobsize confuses the logic of copy_spare(). > Limit the size of used oob size to avoid that. > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > index 33b22b9c0b30..c650f0950b20 100644 > --- a/drivers/mtd/nand/mxc_nand.c > +++ b/drivers/mtd/nand/mxc_nand.c > @@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) > { > struct nand_chip *this = mtd->priv; > struct mxc_nand_host *host = this->priv; > - u16 i, oob_chunk_size; > + u16 i, oob_chunk_size, used_oobsize; > u16 num_chunks = mtd->writesize / 512; > > u8 *d = host->data_buf + mtd->writesize; > u8 __iomem *s = host->spare0; > u16 sparebuf_size = host->devtype_data->spare_len; > > + /* hardware can only use 218 or 128 oob bytes for ecc */ > + if (mtd->oobsize >= 218) IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?) Moreover the comment is misleading as it only applies to 4K flashes. At least the driver works well with (2ki + 128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?). > + used_oobsize = 218; > + else if (mtd->oobsize >= 128) > + used_oobsize = 128; > + else > + used_oobsize = mtd->oobsize; Is it worth to put this check into _probe and put the used size into driver data? Best regards Uwe
Hi Uwe, On Sun, Apr 26, 2015 at 10:07:25PM +0200, Uwe Kleine-König wrote: > On Sun, Apr 26, 2015 at 11:16:49AM +0300, Baruch Siach wrote: > > For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or > > 8bit ECC data, respectively. Larger oobsize confuses the logic of copy_spare(). > > Limit the size of used oob size to avoid that. > > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > --- > > drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > index 33b22b9c0b30..c650f0950b20 100644 > > --- a/drivers/mtd/nand/mxc_nand.c > > +++ b/drivers/mtd/nand/mxc_nand.c > > @@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) > > { > > struct nand_chip *this = mtd->priv; > > struct mxc_nand_host *host = this->priv; > > - u16 i, oob_chunk_size; > > + u16 i, oob_chunk_size, used_oobsize; > > u16 num_chunks = mtd->writesize / 512; > > > > u8 *d = host->data_buf + mtd->writesize; > > u8 __iomem *s = host->spare0; > > u16 sparebuf_size = host->devtype_data->spare_len; > > > > + /* hardware can only use 218 or 128 oob bytes for ecc */ > > + if (mtd->oobsize >= 218) > IMHO this is the wrong check. What if your part (with 224 bytes spare) > is used but the machine only uses the small layout e.g. for booting? > (That would work, wouldn't it?) Yes, but how would I know? I am following here the assumption of get_eccsize() that enables 8 bit ECC when there is enough oobsize. > Moreover the comment is misleading as it > only applies to 4K flashes. At least the driver works well with (2ki + > 128) bytes pages (while there are only 64 bytes spare used?? Maybe there > are still more bugs?). I suspect there are more bugs. A simple way to trigger the bug I encountered is by doing a sub-page write, that is: dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2 and then try reading this page. You may need to dirty the (useless; random) content of ecccalc to see the bug (you can conveniently use mxc_nand_calculate_ecc() for that). What copy_spare() currently do (spread ECC chunks evenly over oob) does not match nandv2_hw_eccoob_largepage. > > + used_oobsize = 218; > > + else if (mtd->oobsize >= 128) > > + used_oobsize = 128; > > + else > > + used_oobsize = mtd->oobsize; > Is it worth to put this check into _probe and put the used size into > driver data? I think you are right. I'll also add checks for smaller page/oob size. Thanks for reviewing, baruch
Hello, On Mon, Apr 27, 2015 at 07:39:06AM +0300, Baruch Siach wrote: > On Sun, Apr 26, 2015 at 10:07:25PM +0200, Uwe Kleine-König wrote: > > On Sun, Apr 26, 2015 at 11:16:49AM +0300, Baruch Siach wrote: > > > For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or > > > 8bit ECC data, respectively. Larger oobsize confuses the logic of copy_spare(). > > > Limit the size of used oob size to avoid that. > > > > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > > --- > > > drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > > > index 33b22b9c0b30..c650f0950b20 100644 > > > --- a/drivers/mtd/nand/mxc_nand.c > > > +++ b/drivers/mtd/nand/mxc_nand.c > > > @@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) > > > { > > > struct nand_chip *this = mtd->priv; > > > struct mxc_nand_host *host = this->priv; > > > - u16 i, oob_chunk_size; > > > + u16 i, oob_chunk_size, used_oobsize; > > > u16 num_chunks = mtd->writesize / 512; > > > > > > u8 *d = host->data_buf + mtd->writesize; > > > u8 __iomem *s = host->spare0; > > > u16 sparebuf_size = host->devtype_data->spare_len; > > > > > > + /* hardware can only use 218 or 128 oob bytes for ecc */ > > > + if (mtd->oobsize >= 218) > > IMHO this is the wrong check. What if your part (with 224 bytes spare) > > is used but the machine only uses the small layout e.g. for booting? > > (That would work, wouldn't it?) > > Yes, but how would I know? I am following here the assumption of get_eccsize() > that enables 8 bit ECC when there is enough oobsize. Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to 0x70 for 224 bytes oob? Optimally this would result in the last 6 bytes being written but not protected by hardware ecc. > > Moreover the comment is misleading as it > > only applies to 4K flashes. At least the driver works well with (2ki + > > 128) bytes pages (while there are only 64 bytes spare used?? Maybe there > > are still more bugs?). > > I suspect there are more bugs. A simple way to trigger the bug I encountered > is by doing a sub-page write, that is: > > dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 > dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2 > > and then try reading this page. You may need to dirty the (useless; random) > content of ecccalc to see the bug (you can conveniently use > mxc_nand_calculate_ecc() for that). What copy_spare() currently do (spread ECC > chunks evenly over oob) does not match nandv2_hw_eccoob_largepage. I didn't follow, but would be willing to review a fix :-) Best regards Uwe
Hi Uwe, On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > On Mon, Apr 27, 2015 at 07:39:06AM +0300, Baruch Siach wrote: > > On Sun, Apr 26, 2015 at 10:07:25PM +0200, Uwe Kleine-König wrote: > > > On Sun, Apr 26, 2015 at 11:16:49AM +0300, Baruch Siach wrote: > > > > + /* hardware can only use 218 or 128 oob bytes for ecc */ > > > > + if (mtd->oobsize >= 218) > > > IMHO this is the wrong check. What if your part (with 224 bytes spare) > > > is used but the machine only uses the small layout e.g. for booting? > > > (That would work, wouldn't it?) > > > > Yes, but how would I know? I am following here the assumption of get_eccsize() > > that enables 8 bit ECC when there is enough oobsize. > Hmm I rechecked the reference manual and found a register to specify the > size of the spare area (I didn't notice that one before). Did you try > what happens if you set this to 0x70 for 224 bytes oob? Which register is that? > Optimally this would result in the last 6 bytes being written but not > protected by hardware ecc. Last 6 bytes of what? AFAIK, hardware ECC does not protect oob at all. > > > Moreover the comment is misleading as it > > > only applies to 4K flashes. At least the driver works well with (2ki + > > > 128) bytes pages (while there are only 64 bytes spare used?? Maybe there > > > are still more bugs?). > > > > I suspect there are more bugs. A simple way to trigger the bug I encountered > > is by doing a sub-page write, that is: > > > > dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 > > dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2 > > > > and then try reading this page. You may need to dirty the (useless; random) > > content of ecccalc to see the bug (you can conveniently use > > mxc_nand_calculate_ecc() for that). What copy_spare() currently do (spread ECC > > chunks evenly over oob) does not match nandv2_hw_eccoob_largepage. > I didn't follow, but would be willing to review a fix :-) OK. I'll try something. baruch
Hello Baruch, On Mon, Apr 27, 2015 at 10:20:57AM +0300, Baruch Siach wrote: > On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 27, 2015 at 07:39:06AM +0300, Baruch Siach wrote: > > > On Sun, Apr 26, 2015 at 10:07:25PM +0200, Uwe Kleine-König wrote: > > > > On Sun, Apr 26, 2015 at 11:16:49AM +0300, Baruch Siach wrote: > > > > > + /* hardware can only use 218 or 128 oob bytes for ecc */ > > > > > + if (mtd->oobsize >= 218) > > > > IMHO this is the wrong check. What if your part (with 224 bytes spare) > > > > is used but the machine only uses the small layout e.g. for booting? > > > > (That would work, wouldn't it?) > > > > > > Yes, but how would I know? I am following here the assumption of get_eccsize() > > > that enables 8 bit ECC when there is enough oobsize. > > Hmm I rechecked the reference manual and found a register to specify the > > size of the spare area (I didn't notice that one before). Did you try > > what happens if you set this to 0x70 for 224 bytes oob? > > Which register is that? Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's what you're using, don't you?). > > Optimally this would result in the last 6 bytes being written but not > > protected by hardware ecc. > > Last 6 bytes of what? AFAIK, hardware ECC does not protect oob at all. I thought it did, might be wrong here. > > > > Moreover the comment is misleading as it > > > > only applies to 4K flashes. At least the driver works well with (2ki + > > > > 128) bytes pages (while there are only 64 bytes spare used?? Maybe there > > > > are still more bugs?). > > > > > > I suspect there are more bugs. A simple way to trigger the bug I encountered > > > is by doing a sub-page write, that is: > > > > > > dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 > > > dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2 > > > > > > and then try reading this page. You may need to dirty the (useless; random) > > > content of ecccalc to see the bug (you can conveniently use > > > mxc_nand_calculate_ecc() for that). What copy_spare() currently do (spread ECC > > > chunks evenly over oob) does not match nandv2_hw_eccoob_largepage. > > I didn't follow, but would be willing to review a fix :-) > > OK. I'll try something. \o/ Uwe
Hi Uwe, On Mon, Apr 27, 2015 at 09:50:23AM +0200, Uwe Kleine-König wrote: > On Mon, Apr 27, 2015 at 10:20:57AM +0300, Baruch Siach wrote: > > On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > > > Hmm I rechecked the reference manual and found a register to specify the > > > size of the spare area (I didn't notice that one before). Did you try > > > what happens if you set this to 0x70 for 224 bytes oob? > > > > Which register is that? > Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's > what you're using, don't you?). Yes, that's what I'm using. I tried setting the SPAS register to oobsize/2 (0x70 in my case), but I see no change in behaviour. Moreover, it turns out the previously Barebox set this register (apparently wrongly) to 0x20 for spare size of 64. Current Barebox master still do. For v3 Barebox limits CONFIG2_SPAS to 218 bytes spare size since Eric Bénard's commit 632c45795065 (nand_imx: update to support onfi & 4k flashs, 2012-07-05). As you can see, the kernel doesn't touch this register for v2 NFC. I have no idea what is the effect of the SPAS in v2 (or any other) NFC. baruch
Hello, On Mon, Apr 27, 2015 at 02:43:41PM +0300, Baruch Siach wrote: > On Mon, Apr 27, 2015 at 09:50:23AM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 27, 2015 at 10:20:57AM +0300, Baruch Siach wrote: > > > On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > > > > Hmm I rechecked the reference manual and found a register to specify the > > > > size of the spare area (I didn't notice that one before). Did you try > > > > what happens if you set this to 0x70 for 224 bytes oob? > > > > > > Which register is that? > > Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's > > what you're using, don't you?). > > Yes, that's what I'm using. > > I tried setting the SPAS register to oobsize/2 (0x70 in my case), but I see no > change in behaviour. Moreover, it turns out the previously Barebox set this > register (apparently wrongly) to 0x20 for spare size of 64. Current Barebox > master still do. For v3 Barebox limits CONFIG2_SPAS to 218 bytes spare size I guess for v21 writing to SPAS is just what the controller assumes after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand has 64. > since Eric Bénard's commit 632c45795065 (nand_imx: update to support onfi & 4k > flashs, 2012-07-05). As you can see, the kernel doesn't touch this register > for v2 NFC. right. > I have no idea what is the effect of the SPAS in v2 (or any other) NFC. I didn't try, but I'm surprised it doesn't make a difference. How did you test? I'd do in barebox: memset -w 0xbb000000 0x55 0x1200 set SPAS to 218/2 trigger page read check how much of the 0x55 was overwritten set SPAS to 224/2 trigger page read compare with above Best regards Uwe
Hi Uwe, On Mon, Apr 27, 2015 at 09:31:27PM +0200, Uwe Kleine-König wrote: > On Mon, Apr 27, 2015 at 02:43:41PM +0300, Baruch Siach wrote: > > On Mon, Apr 27, 2015 at 09:50:23AM +0200, Uwe Kleine-König wrote: > > > On Mon, Apr 27, 2015 at 10:20:57AM +0300, Baruch Siach wrote: > > > > On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > > > > > Hmm I rechecked the reference manual and found a register to specify the > > > > > size of the spare area (I didn't notice that one before). Did you try > > > > > what happens if you set this to 0x70 for 224 bytes oob? > > > > > > > > Which register is that? > > > Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's > > > what you're using, don't you?). > > > > Yes, that's what I'm using. > > > > I tried setting the SPAS register to oobsize/2 (0x70 in my case), but I see no > > change in behaviour. Moreover, it turns out the previously Barebox set this > > register (apparently wrongly) to 0x20 for spare size of 64. Current Barebox > > master still do. For v3 Barebox limits CONFIG2_SPAS to 218 bytes spare size > I guess for v21 writing to SPAS is just what the controller assumes > after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand > has 64. > > > since Eric Bénard's commit 632c45795065 (nand_imx: update to support onfi & 4k > > flashs, 2012-07-05). As you can see, the kernel doesn't touch this register > > for v2 NFC. > right. > > > I have no idea what is the effect of the SPAS in v2 (or any other) NFC. > I didn't try, but I'm surprised it doesn't make a difference. How did > you test? I'd do in barebox: > > memset -w 0xbb000000 0x55 0x1200 > set SPAS to 218/2 > trigger page read > check how much of the 0x55 was overwritten > set SPAS to 224/2 > trigger page read > compare with above Just did this test. No matter how I set SPAS, Only first 26 bytes of each 64 bytes spare data buffer chunk are overwritten. I tried setting SPAS to 224/2 (0x70), 218/2 (0x6d), and 128/2 (0x40). baruch
Hello Baruch, Cc += Fabio On Wed, Apr 29, 2015 at 09:18:53AM +0300, Baruch Siach wrote: > On Mon, Apr 27, 2015 at 09:31:27PM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 27, 2015 at 02:43:41PM +0300, Baruch Siach wrote: > > > On Mon, Apr 27, 2015 at 09:50:23AM +0200, Uwe Kleine-König wrote: > > > > On Mon, Apr 27, 2015 at 10:20:57AM +0300, Baruch Siach wrote: > > > > > On Mon, Apr 27, 2015 at 09:12:38AM +0200, Uwe Kleine-König wrote: > > > > > > Hmm I rechecked the reference manual and found a register to specify the > > > > > > size of the spare area (I didn't notice that one before). Did you try > > > > > > what happens if you set this to 0x70 for 224 bytes oob? > > > > > > > > > > Which register is that? > > > > Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's > > > > what you're using, don't you?). > > > > > > Yes, that's what I'm using. > > > > > > I tried setting the SPAS register to oobsize/2 (0x70 in my case), but I see no > > > change in behaviour. Moreover, it turns out the previously Barebox set this > > > register (apparently wrongly) to 0x20 for spare size of 64. Current Barebox > > > master still do. For v3 Barebox limits CONFIG2_SPAS to 218 bytes spare size > > I guess for v21 writing to SPAS is just what the controller assumes > > after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand > > has 64. > > > > > since Eric Bénard's commit 632c45795065 (nand_imx: update to support onfi & 4k > > > flashs, 2012-07-05). As you can see, the kernel doesn't touch this register > > > for v2 NFC. > > right. > > > > > I have no idea what is the effect of the SPAS in v2 (or any other) NFC. > > I didn't try, but I'm surprised it doesn't make a difference. How did > > you test? I'd do in barebox: > > > > memset -w 0xbb000000 0x55 0x1200 > > set SPAS to 218/2 > > trigger page read > > check how much of the 0x55 was overwritten > > set SPAS to 224/2 > > trigger page read > > compare with above > > Just did this test. No matter how I set SPAS, Only first 26 bytes of each 64 > bytes spare data buffer chunk are overwritten. I tried setting SPAS to 224/2 > (0x70), 218/2 (0x6d), and 128/2 (0x40). That's surprising. It would be interesting to hear from Freescale what is wrong here (Fabio? Maybe contact support?). I quickly skimmed through the errata documents but didn't found anything. So it's not only that you need to handle a pristine flash differently on i.MX than on all other controllers but also that you cannot make full use of the flash's capacity. *sigh* Uwe
Hi Baruch, On Wed, Apr 29, 2015 at 3:35 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > That's surprising. It would be interesting to hear from Freescale what > is wrong here (Fabio? Maybe contact support?). I quickly skimmed through > the errata documents but didn't found anything. So it's not only that Yes, that's something I am not aware of either. Please contact the FSL support folks so that the hardware guys could assist on this topic. Regards, Fabio Estevam
Hi Fabio, On Thu, Apr 30, 2015 at 12:20:32PM -0300, Fabio Estevam wrote: > On Wed, Apr 29, 2015 at 3:35 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > That's surprising. It would be interesting to hear from Freescale what > > is wrong here (Fabio? Maybe contact support?). I quickly skimmed through > > the errata documents but didn't found anything. So it's not only that > > Yes, that's something I am not aware of either. > > Please contact the FSL support folks so that the hardware guys could > assist on this topic. Our past experience with hardware support at the micro level was not all that great. Anyway, I dropped the limit to 128 for oobsize < 218, since this is only a theoretic bug, and I can't really test it. Thanks, baruch
diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c index 33b22b9c0b30..c650f0950b20 100644 --- a/drivers/mtd/nand/mxc_nand.c +++ b/drivers/mtd/nand/mxc_nand.c @@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) { struct nand_chip *this = mtd->priv; struct mxc_nand_host *host = this->priv; - u16 i, oob_chunk_size; + u16 i, oob_chunk_size, used_oobsize; u16 num_chunks = mtd->writesize / 512; u8 *d = host->data_buf + mtd->writesize; u8 __iomem *s = host->spare0; u16 sparebuf_size = host->devtype_data->spare_len; + /* hardware can only use 218 or 128 oob bytes for ecc */ + if (mtd->oobsize >= 218) + used_oobsize = 218; + else if (mtd->oobsize >= 128) + used_oobsize = 128; + else + used_oobsize = mtd->oobsize; /* size of oob chunk for all but possibly the last one */ - oob_chunk_size = (mtd->oobsize / num_chunks) & ~1; + oob_chunk_size = (used_oobsize / num_chunks) & ~1; if (bfrom) { for (i = 0; i < num_chunks - 1; i++) @@ -838,7 +845,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) /* the last chunk */ memcpy32_fromio(d + i * oob_chunk_size, s + i * sparebuf_size, - mtd->oobsize - i * oob_chunk_size); + used_oobsize - i * oob_chunk_size); } else { for (i = 0; i < num_chunks - 1; i++) memcpy32_toio(&s[i * sparebuf_size], @@ -848,7 +855,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) /* the last chunk */ memcpy32_toio(&s[oob_chunk_size * sparebuf_size], &d[i * oob_chunk_size], - mtd->oobsize - i * oob_chunk_size); + used_oobsize - i * oob_chunk_size); } }
For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or 8bit ECC data, respectively. Larger oobsize confuses the logic of copy_spare(). Limit the size of used oob size to avoid that. Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)