[PATCH 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[PATCH 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[Baruch Siach]

This patch series fixes mxc_nand bugs that I encountered while trying to use 8
bit ECC mode on i.MX25 with the Micron MT29F8G08ABABA flash chip.

Many thanks to Uwe Kleine-König for guiding me through the process of
debugging and fixing, and for contributing a clean-up patch to this series.

This series applies and tested on current Linus tree as of d19d133e432248.

Baruch Siach (3):
  mtd: mxc_nand: limit the size of used oob
  mtd: mxc_nand: fix truncate of unaligned oob copying
  mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Uwe Kleine-König (1):
  mtd: nand: mxc_nand: cleanup copy_spare function

 drivers/mtd/nand/mxc_nand.c | 74 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCH 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[Baruch Siach]

This patch series fixes mxc_nand bugs that I encountered while trying to use 8
bit ECC mode on i.MX25 with the Micron MT29F8G08ABABA flash chip.

Many thanks to Uwe Kleine-K?nig for guiding me through the process of
debugging and fixing, and for contributing a clean-up patch to this series.

This series applies and tested on current Linus tree as of d19d133e432248.

Baruch Siach (3):
  mtd: mxc_nand: limit the size of used oob
  mtd: mxc_nand: fix truncate of unaligned oob copying
  mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Uwe Kleine-K?nig (1):
  mtd: nand: mxc_nand: cleanup copy_spare function

 drivers/mtd/nand/mxc_nand.c | 74 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCH 1/4] mtd: nand: mxc_nand: cleanup copy_spare function

[Baruch Siach]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

To give people without the reference manual at hand a chance to
understand how spare area is handled in the i.MX nand controller,
improve commenting, naming of variables and coding style.

No functional change intended.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
[baruch: declare oob_chunk_size; update comments; reword commit log]
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 46 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 372e0e38f59b..33b22b9c0b30 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -807,32 +807,48 @@ static void mxc_nand_select_chip_v2(struct mtd_info *mtd, int chip)
 }
 
 /*
- * Function to transfer data to/from spare area.
+ * The controller splits a page into data chunks of 512 bytes + partial oob.
+ * There are writesize / 512 such chunks, the size of the partial oob parts is
+ * oobsize / #chunks rounded down to a multiple of 2. The last oob chunk then
+ * contains additionally the byte lost by rounding (if any).
+ * This function handles the needed shuffling between host->data_buf (which
+ * holds a page in natural order, i.e. writesize bytes data + oobsize bytes
+ * spare) and the NFC buffer.
  */
 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, j;
-	u16 n = mtd->writesize >> 9;
+	u16 i, oob_chunk_size;
+	u16 num_chunks = mtd->writesize / 512;
+
 	u8 *d = host->data_buf + mtd->writesize;
 	u8 __iomem *s = host->spare0;
-	u16 t = host->devtype_data->spare_len;
+	u16 sparebuf_size = host->devtype_data->spare_len;
 
-	j = (mtd->oobsize / n >> 1) << 1;
+	/* size of oob chunk for all but possibly the last one */
+	oob_chunk_size = (mtd->oobsize / num_chunks) & ~1;
 
 	if (bfrom) {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_fromio(d + i * j, s + i * t, j);
-
-		/* the last section */
-		memcpy32_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_fromio(d + i * oob_chunk_size,
+					s + i * sparebuf_size,
+					oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_fromio(d + i * oob_chunk_size,
+				s + i * sparebuf_size,
+				mtd->oobsize - i * oob_chunk_size);
 	} else {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_toio(&s[i * t], &d[i * j], j);
-
-		/* the last section */
-		memcpy32_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_toio(&s[i * sparebuf_size],
+				      &d[i * oob_chunk_size],
+				      oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
+			      &d[i * oob_chunk_size],
+			      mtd->oobsize - i * oob_chunk_size);
 	}
 }
 
-- 
2.1.4

[PATCH 1/4] mtd: nand: mxc_nand: cleanup copy_spare function

[Baruch Siach]

From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

To give people without the reference manual at hand a chance to
understand how spare area is handled in the i.MX nand controller,
improve commenting, naming of variables and coding style.

No functional change intended.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
[baruch: declare oob_chunk_size; update comments; reword commit log]
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 46 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 372e0e38f59b..33b22b9c0b30 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -807,32 +807,48 @@ static void mxc_nand_select_chip_v2(struct mtd_info *mtd, int chip)
 }
 
 /*
- * Function to transfer data to/from spare area.
+ * The controller splits a page into data chunks of 512 bytes + partial oob.
+ * There are writesize / 512 such chunks, the size of the partial oob parts is
+ * oobsize / #chunks rounded down to a multiple of 2. The last oob chunk then
+ * contains additionally the byte lost by rounding (if any).
+ * This function handles the needed shuffling between host->data_buf (which
+ * holds a page in natural order, i.e. writesize bytes data + oobsize bytes
+ * spare) and the NFC buffer.
  */
 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, j;
-	u16 n = mtd->writesize >> 9;
+	u16 i, oob_chunk_size;
+	u16 num_chunks = mtd->writesize / 512;
+
 	u8 *d = host->data_buf + mtd->writesize;
 	u8 __iomem *s = host->spare0;
-	u16 t = host->devtype_data->spare_len;
+	u16 sparebuf_size = host->devtype_data->spare_len;
 
-	j = (mtd->oobsize / n >> 1) << 1;
+	/* size of oob chunk for all but possibly the last one */
+	oob_chunk_size = (mtd->oobsize / num_chunks) & ~1;
 
 	if (bfrom) {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_fromio(d + i * j, s + i * t, j);
-
-		/* the last section */
-		memcpy32_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_fromio(d + i * oob_chunk_size,
+					s + i * sparebuf_size,
+					oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_fromio(d + i * oob_chunk_size,
+				s + i * sparebuf_size,
+				mtd->oobsize - i * oob_chunk_size);
 	} else {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_toio(&s[i * t], &d[i * j], j);
-
-		/* the last section */
-		memcpy32_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_toio(&s[i * sparebuf_size],
+				      &d[i * oob_chunk_size],
+				      oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
+			      &d[i * oob_chunk_size],
+			      mtd->oobsize - i * oob_chunk_size);
 	}
 }
 
-- 
2.1.4

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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)
+		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);
 	}
 }
 
-- 
2.1.4

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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)
+		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);
 	}
 }
 
-- 
2.1.4

[PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Copy to/from oob io area might not be aligned to 4 bytes. When 8 bit ECC is
used, the buffer size is 26. Make sure we pass 4 bytes aligned buffer size to
memcpy32_{to,from}io to avoid truncating the buffer.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index c650f0950b20..c05f5e8fef17 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 		for (i = 0; i < num_chunks - 1; i++)
 			memcpy32_fromio(d + i * oob_chunk_size,
 					s + i * sparebuf_size,
-					oob_chunk_size);
+					ALIGN(oob_chunk_size, 4));
 
 		/* the last chunk */
 		memcpy32_fromio(d + i * oob_chunk_size,
 				s + i * sparebuf_size,
-				used_oobsize - i * oob_chunk_size);
+				ALIGN(used_oobsize - i * oob_chunk_size, 4));
 	} else {
 		for (i = 0; i < num_chunks - 1; i++)
 			memcpy32_toio(&s[i * sparebuf_size],
 				      &d[i * oob_chunk_size],
-				      oob_chunk_size);
+				      ALIGN(oob_chunk_size, 4));
 
 		/* the last chunk */
 		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
 			      &d[i * oob_chunk_size],
-			      used_oobsize - i * oob_chunk_size);
+			      ALIGN(used_oobsize - i * oob_chunk_size, 4));
 	}
 }
 
-- 
2.1.4

[PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Copy to/from oob io area might not be aligned to 4 bytes. When 8 bit ECC is
used, the buffer size is 26. Make sure we pass 4 bytes aligned buffer size to
memcpy32_{to,from}io to avoid truncating the buffer.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index c650f0950b20..c05f5e8fef17 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 		for (i = 0; i < num_chunks - 1; i++)
 			memcpy32_fromio(d + i * oob_chunk_size,
 					s + i * sparebuf_size,
-					oob_chunk_size);
+					ALIGN(oob_chunk_size, 4));
 
 		/* the last chunk */
 		memcpy32_fromio(d + i * oob_chunk_size,
 				s + i * sparebuf_size,
-				used_oobsize - i * oob_chunk_size);
+				ALIGN(used_oobsize - i * oob_chunk_size, 4));
 	} else {
 		for (i = 0; i < num_chunks - 1; i++)
 			memcpy32_toio(&s[i * sparebuf_size],
 				      &d[i * oob_chunk_size],
-				      oob_chunk_size);
+				      ALIGN(oob_chunk_size, 4));
 
 		/* the last chunk */
 		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
 			      &d[i * oob_chunk_size],
-			      used_oobsize - i * oob_chunk_size);
+			      ALIGN(used_oobsize - i * oob_chunk_size, 4));
 	}
 }
 
-- 
2.1.4

[PATCH 4/4] mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

[Baruch Siach]

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet
another static struct nand_ecclayout, generate it in code.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index c05f5e8fef17..07493f0d9eb4 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -934,6 +934,22 @@ static int get_eccsize(struct mtd_info *mtd)
 		return 8;
 }
 
+static void ecc_8bit_layout(struct nand_ecclayout *layout)
+{
+	int i, j;
+
+	for (i = 0; i < 8; i++)
+		for (j = 0; j < 18; j++)
+			layout->eccpos[i*18 + j] = i*26 + j + 7;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = 4;
+	for (i = 1; i < 8; i++) {
+		layout->oobfree[i].offset = i*26;
+		layout->oobfree[i].length = 7;
+	}
+}
+
 static void preset_v1(struct mtd_info *mtd)
 {
 	struct nand_chip *nand_chip = mtd->priv;
@@ -1610,8 +1626,11 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	if (mtd->writesize == 2048)
 		this->ecc.layout = host->devtype_data->ecclayout_2k;
-	else if (mtd->writesize == 4096)
+	else if (mtd->writesize == 4096) {
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout(this->ecc.layout);
+	}
 
 	if (this->ecc.mode == NAND_ECC_HW) {
 		if (is_imx21_nfc(host) || is_imx27_nfc(host))
-- 
2.1.4

[PATCH 4/4] mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

[Baruch Siach]

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet
another static struct nand_ecclayout, generate it in code.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index c05f5e8fef17..07493f0d9eb4 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -934,6 +934,22 @@ static int get_eccsize(struct mtd_info *mtd)
 		return 8;
 }
 
+static void ecc_8bit_layout(struct nand_ecclayout *layout)
+{
+	int i, j;
+
+	for (i = 0; i < 8; i++)
+		for (j = 0; j < 18; j++)
+			layout->eccpos[i*18 + j] = i*26 + j + 7;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = 4;
+	for (i = 1; i < 8; i++) {
+		layout->oobfree[i].offset = i*26;
+		layout->oobfree[i].length = 7;
+	}
+}
+
 static void preset_v1(struct mtd_info *mtd)
 {
 	struct nand_chip *nand_chip = mtd->priv;
@@ -1610,8 +1626,11 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	if (mtd->writesize == 2048)
 		this->ecc.layout = host->devtype_data->ecclayout_2k;
-	else if (mtd->writesize == 4096)
+	else if (mtd->writesize == 4096) {
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout(this->ecc.layout);
+	}
 
 	if (this->ecc.mode == NAND_ECC_HW) {
 		if (is_imx21_nfc(host) || is_imx27_nfc(host))
-- 
2.1.4

Re: [PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

Hello Baruch,

On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index c650f0950b20..c05f5e8fef17 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
>  		for (i = 0; i < num_chunks - 1; i++)
>  			memcpy32_fromio(d + i * oob_chunk_size,
>  					s + i * sparebuf_size,
> -					oob_chunk_size);
> +					ALIGN(oob_chunk_size, 4));
If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
either for uneven i. That's not nice. I suggest to use memcpy16_fromio.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

Hello Baruch,

On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index c650f0950b20..c05f5e8fef17 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
>  		for (i = 0; i < num_chunks - 1; i++)
>  			memcpy32_fromio(d + i * oob_chunk_size,
>  					s + i * sparebuf_size,
> -					oob_chunk_size);
> +					ALIGN(oob_chunk_size, 4));
If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
either for uneven i. That's not nice. I suggest to use memcpy16_fromio.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Sun, Apr 26, 2015 at 09:52:11PM +0200, Uwe Kleine-König wrote:
> On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index c650f0950b20..c05f5e8fef17 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
> >  		for (i = 0; i < num_chunks - 1; i++)
> >  			memcpy32_fromio(d + i * oob_chunk_size,
> >  					s + i * sparebuf_size,
> > -					oob_chunk_size);
> > +					ALIGN(oob_chunk_size, 4));
> If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
> either for uneven i. That's not nice. I suggest to use memcpy16_fromio.

memcpy32_fromio() was introduced by Sascha in 096bcc231fd2 (mtd: mxc_nand: use 
32bit copy functions, 2012-05-29), replacing memcpy_fromio() to force 32bit io 
access. Are you sure the hypothetical memcpy16_fromio() would work?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Sun, Apr 26, 2015 at 09:52:11PM +0200, Uwe Kleine-K?nig wrote:
> On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index c650f0950b20..c05f5e8fef17 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
> >  		for (i = 0; i < num_chunks - 1; i++)
> >  			memcpy32_fromio(d + i * oob_chunk_size,
> >  					s + i * sparebuf_size,
> > -					oob_chunk_size);
> > +					ALIGN(oob_chunk_size, 4));
> If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
> either for uneven i. That's not nice. I suggest to use memcpy16_fromio.

memcpy32_fromio() was introduced by Sascha in 096bcc231fd2 (mtd: mxc_nand: use 
32bit copy functions, 2012-05-29), replacing memcpy_fromio() to force 32bit io 
access. Are you sure the hypothetical memcpy16_fromio() would work?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

Hello Baruch,

On Mon, Apr 27, 2015 at 07:46:18AM +0300, Baruch Siach wrote:
> On Sun, Apr 26, 2015 at 09:52:11PM +0200, Uwe Kleine-König wrote:
> > On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > index c650f0950b20..c05f5e8fef17 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
> > >  		for (i = 0; i < num_chunks - 1; i++)
> > >  			memcpy32_fromio(d + i * oob_chunk_size,
> > >  					s + i * sparebuf_size,
> > > -					oob_chunk_size);
> > > +					ALIGN(oob_chunk_size, 4));
> > If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
> > either for uneven i. That's not nice. I suggest to use memcpy16_fromio.
> 
> memcpy32_fromio() was introduced by Sascha in 096bcc231fd2 (mtd: mxc_nand: use 
> 32bit copy functions, 2012-05-29), replacing memcpy_fromio() to force 32bit io 
> access. Are you sure the hypothetical memcpy16_fromio() would work?
according to the reference manual you can use 16-bit or 32-bit accesses.
And giving that in some cases the amount of valid data (in bytes) is ≡ 2
(mod 4) 16-bit access seem to be the obvious thing. Also Sascha only
wrote about byte accesses being problematic in 096bcc231fd2. So I'm
expecting that 16-bit operators should be fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

Hello Baruch,

On Mon, Apr 27, 2015 at 07:46:18AM +0300, Baruch Siach wrote:
> On Sun, Apr 26, 2015 at 09:52:11PM +0200, Uwe Kleine-K?nig wrote:
> > On Sun, Apr 26, 2015 at 11:16:50AM +0300, Baruch Siach wrote:
> > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > index c650f0950b20..c05f5e8fef17 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
> > >  		for (i = 0; i < num_chunks - 1; i++)
> > >  			memcpy32_fromio(d + i * oob_chunk_size,
> > >  					s + i * sparebuf_size,
> > > -					oob_chunk_size);
> > > +					ALIGN(oob_chunk_size, 4));
> > If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't
> > either for uneven i. That's not nice. I suggest to use memcpy16_fromio.
> 
> memcpy32_fromio() was introduced by Sascha in 096bcc231fd2 (mtd: mxc_nand: use 
> 32bit copy functions, 2012-05-29), replacing memcpy_fromio() to force 32bit io 
> access. Are you sure the hypothetical memcpy16_fromio() would work?
according to the reference manual you can use 16-bit or 32-bit accesses.
And giving that in some cases the amount of valid data (in bytes) is ? 2
(mod 4) 16-bit access seem to be the obvious thing. Also Sascha only
wrote about byte accesses being problematic in 096bcc231fd2. So I'm
expecting that 16-bit operators should be fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Fabio Estevam]

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

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Fabio Estevam]

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

Re: [PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mtd: mxc_nand: limit the size of used oob

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -