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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |