[PATCH 1/3] mtd: mxc-nand: Improve comment about vendor BBM and address verschwurbelung

[PATCH 1/3] mtd: 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 | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 2774b6bb4f01..da1c426d1951 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)
 {

base-commit: 8ca910b9218822e7122ac9bcb62d0c50acb971ff
-- 
2.43.0

[PATCH 2/3] mtd: 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.

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

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index da1c426d1951..a72275480144 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1583,31 +1583,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(chip, 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 3/3] mtd: 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 the first 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 | 58 ++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index a72275480144..fd5ae447a198 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1555,30 +1555,6 @@ 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)
-{
-	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;
-
-	ret = mtd_read_oob(mtd, ofs, &ops);
-	if (ret < 0)
-		return ret;
-
-	if (buf[2000] != 0xff)
-		return 1;
-
-	return 0;
-}
-
 static int imxnd_create_bbt(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
@@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
 
 	for (i = 0; i < numblocks; ++i) {
 		loff_t ofs = i << chip->bbt_erase_shift;
+		uint8_t buf[mtd->writesize + 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 = checkbad(chip, ofs);
-		if (ret < 0)
+		ret = mtd_read_oob(mtd, ofs, &ops);
+		if (ret < 0) {
+			dev_err(mtd->dev.parent, "Failed to read page at 0x%08x\n", (unsigned int)ofs);
 			goto out;
+		}
 
-		if (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 (ofs == 0 && is_barebox_arm_head(buf)) {
+			dev_err(mtd->dev.parent, "Flash seems to contain a barebox image, refusing\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
+			dev_err(mtd->dev.parent, "Flash seems to contain a UBI, refusing\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (buf[2000] != 0xff) {
 			bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
 			dev_info(mtd->dev.parent, "Bad eraseblock %d at 0x%08x\n",
 				 i, (unsigned int)ofs);
-- 
2.43.0

Re: [PATCH 3/3] mtd: mxc-nand: Only automatically create BBT if NAND seems to be pristine

[Sascha Hauer]

On Tue, Apr 30, 2024 at 11:44:54AM +0200, Uwe Kleine-König wrote:
> 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 the first 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 | 58 ++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index a72275480144..fd5ae447a198 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1555,30 +1555,6 @@ 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)
> -{
> -	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;
> -
> -	ret = mtd_read_oob(mtd, ofs, &ops);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (buf[2000] != 0xff)
> -		return 1;
> -
> -	return 0;
> -}
> -
>  static int imxnd_create_bbt(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> @@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
>  
>  	for (i = 0; i < numblocks; ++i) {
>  		loff_t ofs = i << chip->bbt_erase_shift;
> +		uint8_t buf[mtd->writesize + 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 = checkbad(chip, ofs);
> -		if (ret < 0)
> +		ret = mtd_read_oob(mtd, ofs, &ops);
> +		if (ret < 0) {
> +			dev_err(mtd->dev.parent, "Failed to read page at 0x%08x\n", (unsigned int)ofs);
>  			goto out;
> +		}
>  
> -		if (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 (ofs == 0 && is_barebox_arm_head(buf)) {
> +			dev_err(mtd->dev.parent, "Flash seems to contain a barebox image, refusing\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
> +			dev_err(mtd->dev.parent, "Flash seems to contain a UBI, refusing\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		if (buf[2000] != 0xff) {
>  			bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
>  			dev_info(mtd->dev.parent, "Bad eraseblock %d at 0x%08x\n",
>  				 i, (unsigned int)ofs);

Could you add the new code to checkbad() instead of inlining it? That
way it seems easier to adjust the code in case we have to change the way
how we detect useful data on a page. Rename checkbad() in case the name
doesn't feel appropriate anymore.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] mtd: mxc-nand: Only automatically create BBT if NAND seems to be pristine

[Uwe Kleine-König]

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

Hallo Sascha,

On Fri, May 03, 2024 at 09:44:24AM +0200, Sascha Hauer wrote:
> On Tue, Apr 30, 2024 at 11:44:54AM +0200, Uwe Kleine-König wrote:
> > 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 the first 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 | 58 ++++++++++++++++++---------------
> >  1 file changed, 31 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index a72275480144..fd5ae447a198 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1555,30 +1555,6 @@ 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)
> > -{
> > -	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;
> > -
> > -	ret = mtd_read_oob(mtd, ofs, &ops);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	if (buf[2000] != 0xff)
> > -		return 1;
> > -
> > -	return 0;
> > -}
> > -
> >  static int imxnd_create_bbt(struct nand_chip *chip)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > @@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
> >  
> >  	for (i = 0; i < numblocks; ++i) {
> >  		loff_t ofs = i << chip->bbt_erase_shift;
> > +		uint8_t buf[mtd->writesize + 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 = checkbad(chip, ofs);
> > -		if (ret < 0)
> > +		ret = mtd_read_oob(mtd, ofs, &ops);
> > +		if (ret < 0) {
> > +			dev_err(mtd->dev.parent, "Failed to read page at 0x%08x\n", (unsigned int)ofs);
> >  			goto out;
> > +		}
> >  
> > -		if (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 (ofs == 0 && is_barebox_arm_head(buf)) {
> > +			dev_err(mtd->dev.parent, "Flash seems to contain a barebox image, refusing\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
> > +			dev_err(mtd->dev.parent, "Flash seems to contain a UBI, refusing\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		if (buf[2000] != 0xff) {
> >  			bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
> >  			dev_info(mtd->dev.parent, "Bad eraseblock %d at 0x%08x\n",
> >  				 i, (unsigned int)ofs);
> 
> Could you add the new code to checkbad() instead of inlining it? That
> way it seems easier to adjust the code in case we have to change the way
> how we detect useful data on a page. Rename checkbad() in case the name
> doesn't feel appropriate anymore.

I hesitated to add the check for the chip being pristine to checkbad().
Then maybe read page 0 and check its contents before the check_bad()
loop? Then page 0 will be read twice but checkbad() can stay around and
only do what its name promises.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]