[PATCH v2 0/4] mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine

[PATCH v2 0/4] mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine

[Uwe Kleine-König]

Hello,

this is v2 of this series. The changes are:

 - More cleanups
 - Keep checkbad() function (Sascha)
 - Check each block if it contains a barebox image or a UBI

(implicit) v1 available at
https://lore.barebox.org/barebox/20240430094451.1038256-4-u.kleine-koenig@pengutronix.de/T/#t

This is based on master.

Best regards
Uwe

Uwe Kleine-König (4):
  mtd: nand: mxc_nand: Improve comment about vendor BBM and address verschwurbelung
  mtd: nand: mxc_nand: Cleanup code creating bad block table
  mtd: nand: mxc_nand: Add error message if BBT creation fails
  mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine

 drivers/mtd/nand/raw/mxc_nand.c | 95 ++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 31 deletions(-)

base-commit: 302db826b67c44a925a25a7b7508bc82a0fcd6a8
-- 
2.43.0

[PATCH v2 1/4] mtd: nand: mxc_nand: Improve comment about vendor BBM and address verschwurbelung

[Uwe Kleine-König]

To better describe why the BBM is at offset 2000 describe the full
misinterpretation^Wmapping of the NAND memory that the i.MX hardware
implements.

Also adapt the comment to reality: A BBT is created automatically since
commit 2ad441bb7e78 ("mtd: nand-imx: Create BBT automatically when
necessary") which was included in v2020.03.0.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index c6533b20fc0f..f7ef2e4374a9 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1528,20 +1528,32 @@ static const struct nand_controller_ops mxcnd_controller_ops = {
  * 512b data + 16b OOB +
  * 512b data + 16b OOB
  *
+ * So the mapping between original NAND addressing (as intended by the chip
+ * vendor) and interpretation when accessed via the i.MX NAND controller is as
+ * follows:
+ *
+ *       original       |        i.MX
+ * ---------------------+---------------------
+ * data 0x0000 - 0x0200 | data 0x0000 - 0x0200
+ * data 0x0200 - 0x0210 | oob  0x0000 - 0x0010
+ * data 0x0210 - 0x0410 | data 0x0200 - 0x0400
+ * data 0x0410 - 0x0420 | oob  0x0010 - 0x0020
+ * data 0x0420 - 0x0620 | data 0x0400 - 0x0600
+ * data 0x0620 - 0x0630 | oob  0x0020 - 0x0030
+ * data 0x0630 - 0x0800 | data 0x0600 - 0x07d0
+ * oob  0x0000 - 0x0030 | data 0x07d0 - 0x0800
+ * oob  0x0030 - 0x0040 | oob  0x0030 - 0x0040
+ *
  * This means that the factory provided bad block marker ends up
- * in the page data at offset 2000 instead of in the OOB data.
+ * in the page data at offset 2000 = 0x7d0 instead of in the OOB data.
  *
- * To preserve the factory bad block information we take the following
- * strategy:
- *
- * - If the NAND driver detects that no flash BBT is present on 2k NAND
- *   chips it will not create one because it would do so based on the wrong
- *   BBM position
- * - This command is used to create a flash BBT then.
+ * If the NAND driver detects that no flash BBT is present on a 2k NAND
+ * chip it will create one automatically in the assumption that the NAND is
+ * pristine (that is completely erased with only vendor BBMs in the OOB) to
+ * preserve factory bad block information.
  *
  * From this point on we can forget about the BBMs and rely completely
  * on the flash BBT.
- *
  */
 static int checkbad(struct nand_chip *chip, loff_t ofs)
 {
@@ -1562,8 +1574,10 @@ static int checkbad(struct nand_chip *chip, loff_t ofs)
 		return ret;
 
 	if (buf[2000] != 0xff)
+		/* block considered bad */
 		return 1;
 
+	/* block considered good */
 	return 0;
 }
 
-- 
2.43.0

[PATCH v2 2/4] mtd: nand: mxc_nand: Cleanup code creating bad block table

[Uwe Kleine-König]

Let the variable "numblocks" contain the number of blocks of the NAND
device (and not twice this value). Also the loop variable counts blocks
now instead of the doubled value. Further calculate the offset of the
i-th block from i instead of incrementing in each step and pick a more
sensbile variable name for it.

Also checkbad() only makes use of *mtd which is already available in the
caller, so pass that one directly. Additionally initialize the struct
mtd_oob_ops in the declarator (which has the nice side effect of zeroing
the members not mentioned) and improve code comments.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 41 ++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index f7ef2e4374a9..8e564fa76a33 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1555,19 +1555,18 @@ static const struct nand_controller_ops mxcnd_controller_ops = {
  * From this point on we can forget about the BBMs and rely completely
  * on the flash BBT.
  */
-static int checkbad(struct nand_chip *chip, loff_t ofs)
+static int checkbad(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_info *mtd = nand_to_mtd(chip);
 	int ret;
 	uint8_t buf[mtd->writesize + mtd->oobsize];
-	struct mtd_oob_ops ops;
-
-	ops.mode = MTD_OPS_RAW;
-	ops.ooboffs = 0;
-	ops.datbuf = buf;
-	ops.len = mtd->writesize;
-	ops.oobbuf = buf + mtd->writesize;
-	ops.ooblen = mtd->oobsize;
+	struct mtd_oob_ops ops = {
+		.mode = MTD_OPS_RAW,
+		.ooboffs = 0,
+		.datbuf = buf,
+		.len = mtd->writesize,
+		.oobbuf = buf + mtd->writesize,
+		.ooblen = mtd->oobsize,
+	};
 
 	ret = mtd_read_oob(mtd, ofs, &ops);
 	if (ret < 0)
@@ -1585,31 +1584,31 @@ static int imxnd_create_bbt(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	int len, i, numblocks, ret;
-	loff_t from = 0;
 	uint8_t *bbt;
 
-	len = mtd->size >> (chip->bbt_erase_shift + 2);
+	numblocks = mtd->size >> chip->bbt_erase_shift;
 
-	/* Allocate memory (2bit per block) and clear the memory bad block table */
+	/*
+	 * Allocate memory (2bit per block = 1 byte per 4 blocks) and clear the
+	 * memory bad block table
+	 */
+	len = (numblocks + 3) >> 2;
 	bbt = kzalloc(len, GFP_KERNEL);
 	if (!bbt)
 		return -ENOMEM;
 
-	numblocks = mtd->size >> (chip->bbt_erase_shift - 1);
+	for (i = 0; i < numblocks; ++i) {
+		loff_t ofs = i << chip->bbt_erase_shift;
 
-	for (i = 0; i < numblocks;) {
-		ret = checkbad(chip, from);
+		ret = checkbad(mtd, ofs);
 		if (ret < 0)
 			goto out;
 
 		if (ret) {
-			bbt[i >> 3] |= 0x03 << (i & 0x6);
+			bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
 			dev_info(mtd->dev.parent, "Bad eraseblock %d at 0x%08x\n",
-				 i >> 1, (unsigned int)from);
+				 i, (unsigned int)ofs);
 		}
-
-		i += 2;
-		from += (1 << chip->bbt_erase_shift);
 	}
 
 	chip->bbt_td->options |= NAND_BBT_CREATE;
-- 
2.43.0

[PATCH v2 3/4] mtd: nand: mxc_nand: Add error message if BBT creation fails

[Uwe Kleine-König]

Then reading from the chip and thus aborting creation of a bad block
table, at inform the user about that with an error message.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 8e564fa76a33..faea1c95f95e 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1569,8 +1569,11 @@ static int checkbad(struct mtd_info *mtd, loff_t ofs)
 	};
 
 	ret = mtd_read_oob(mtd, ofs, &ops);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(mtd->dev.parent, "Failed to read page at 0x%08x\n",
+			(unsigned int)ofs);
 		return ret;
+	}
 
 	if (buf[2000] != 0xff)
 		/* block considered bad */
-- 
2.43.0

[PATCH v2 4/4] mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine

[Uwe Kleine-König]

Automatically creating a BBT is the right thing to do if the NAND is
factory new. However when migrating from a barebox older than commit
v2020.03.0~28^2~1 ("mtd: nand-imx: Create BBT automatically when
necessary") on a used machine, this automatism is really bad because it
most likely marks the blocks containing the barebox image (and possibly
more) as bad. On such a system the vendor BBMs are gone, but it was
operated without that information before, so continuing to do so is a
sane option.

Add a light check for the NAND to be really pristine: If any block looks
like containing a barebox image or a UBI refuse to create a BBT.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index faea1c95f95e..f8517df823e5 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1575,6 +1575,23 @@ static int checkbad(struct mtd_info *mtd, loff_t ofs)
 		return ret;
 	}
 
+	/*
+	 * Automatically adding a BBT based on factory BBTs is only
+	 * sensible if the NAND is pristine. Abort if the first page
+	 * looks like a bootloader or UBI block.
+	 */
+	if (is_barebox_arm_head(buf)) {
+		dev_err(mtd->dev.parent,
+			"Flash seems to contain a barebox image, refusing to create BBT\n");
+		return -EINVAL;
+	}
+
+	if (!memcmp(buf, "UBI#", 4)) {
+		dev_err(mtd->dev.parent,
+			"Flash seems to contain a UBI, refusing to create BBT\n");
+		return -EINVAL;
+	}
+
 	if (buf[2000] != 0xff)
 		/* block considered bad */
 		return 1;
-- 
2.43.0

Re: [PATCH v2 0/4] mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine

[Sascha Hauer]

On Mon, 06 May 2024 16:46:07 +0200, Uwe Kleine-König wrote:
> this is v2 of this series. The changes are:
> 
>  - More cleanups
>  - Keep checkbad() function (Sascha)
>  - Check each block if it contains a barebox image or a UBI
> 
> (implicit) v1 available at
> https://lore.barebox.org/barebox/20240430094451.1038256-4-u.kleine-koenig@pengutronix.de/T/#t
> 
> [...]

Applied, thanks!

[1/4] mtd: nand: mxc_nand: Improve comment about vendor BBM and address verschwurbelung
      https://git.pengutronix.de/cgit/barebox/commit/?id=4bad7cc26bbd (link may not be stable)
[2/4] mtd: nand: mxc_nand: Cleanup code creating bad block table
      https://git.pengutronix.de/cgit/barebox/commit/?id=23686460eb8b (link may not be stable)
[3/4] mtd: nand: mxc_nand: Add error message if BBT creation fails
      https://git.pengutronix.de/cgit/barebox/commit/?id=8695b02ffa13 (link may not be stable)
[4/4] mtd: nand: mxc_nand: Only automatically create BBT if NAND seems to be pristine
      https://git.pengutronix.de/cgit/barebox/commit/?id=30130286232a (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>