Re: [PATCH 2/7] mtd/nand: try to erase bad blocks only if scrub flag is provided

[Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>]

On Thu, 6 Oct 2022 20:09:11 +0200
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> [External email]
> 
> 
> 
> 
> 
> Hi Mikhail
> 
> On Thu, Oct 6, 2022 at 6:17 PM Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
> >
> > On Thu, 6 Oct 2022 18:03:17 +0200
> > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> >
> > > [External email]
> > >
> > >
> > >
> > >
> > >
> > > On Thu, Oct 6, 2022 at 5:52 PM Mikhail Kshevetskiy
> > > <mikhail.kshevetskiy@iopsys.eu> wrote:
> > > >
> > > > On Thu, 6 Oct 2022 08:56:08 +0200
> > > > Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:
> > > >
> > > > > [External email]
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Hi
> > > > >
> > > > > On Thu, Oct 6, 2022 at 5:15 AM <mikhail.kshevetskiy@iopsys.eu> wrote:
> > > > > >
> > > > > > From: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> > > > > >
> > > > > > 'mtd erase' command should not erase bad blocks. To force bad block
> > > > > > erasing there is 'mtd erase.dontskipbad' command (this command sets
> > > > > > 'scrub' flag to true in the erase_info structure). Unfortunately
> > > > > > nand layer ignore scrub flag and try to erases bad blocks
> > > > > > unconditionally. This is wrong.
> > > > > >
> > > > > > Add checks to allow bad block erasing only if scrub flag is set.
> > > > > >
> > > > > > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> > > > > > ---
> > > > > >  drivers/mtd/nand/core.c | 5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > > > > > index 99c29670c7..a4fb7602c9 100644
> > > > > > --- a/drivers/mtd/nand/core.c
> > > > > > +++ b/drivers/mtd/nand/core.c
> > > > > > @@ -174,7 +174,10 @@ int nanddev_mtd_erase(struct mtd_info *mtd,
> > > > > > struct erase_info *einfo) nanddev_offs_to_pos(nand, einfo->addr +
> > > > > > einfo->len - 1, &last); while (nanddev_pos_cmp(&pos, &last) <= 0) {
> > > > > >                 schedule();
> > > > > > -               ret = nanddev_erase(nand, &pos);
> > > > > > +               if (!einfo->scrub && nanddev_isbad(nand, &pos))
> > > > >
> > > > > The nandev_erase already check it here:
> > > > >
> > > > > if (nanddev_isbad(nand, pos) || nanddev_isreserved(nand, pos)) {
> > > > >
> > > >
> > > > no it does not work. see nanddev_erase() code
> > > >
> > >
> > > Let me re-formulate it. What execution path are you taking into account?
> > >
> > > The nand are erased using the cmd/nand interface and the erase command
> > > there calls nand_erase_opts that take in account it.
> >
> > spi-nand flash
> >
> > cmd/mtd.c -> do_mtd_erase() -> .... -> nanddev_mtd_erase() ->
> > nanddev_erase()
> >
> 
> Ok now it's clear. I'm thinking and i have create a patch if we can
> use somenthing like this
> 
> Not tested
> 
> 
> @@ -393,7 +394,7 @@ out_put_mtd:
>  static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc,
>                         char *const argv[])
>  {
> -       struct erase_info erase_op = {};
> +       nand_erase_options_t opts = {};
>         struct mtd_info *mtd;
>         u64 off, len;
>         bool scrub;
> @@ -431,26 +432,11 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp,
> int flag, int argc,
>         printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
>                off, off + len - 1, mtd_div_by_eb(len, mtd));
> 
> -       erase_op.mtd = mtd;
> -       erase_op.addr = off;
> -       erase_op.len = mtd->erasesize;
> -       erase_op.scrub = scrub;
> -
> -       while (len) {
> -               ret = mtd_erase(mtd, &erase_op);
> -
> -               if (ret) {
> -                       /* Abort if its not a bad block error */
> -                       if (ret != -EIO)
> -                               break;
> -                       printf("Skipping bad block at 0x%08llx\n",
> -                              erase_op.addr);
> -               }
> -
> -               len -= mtd->erasesize;
> -               erase_op.addr += mtd->erasesize;
> -       }
> +       opts.offset = off;
> +       opts.length = len;
> +       opts.scrub = scrub;
> 
> +       ret = nand_erase_opts(mtd, &opts);
>         if (ret && ret != -EIO)
>                 ret = CMD_RET_FAILURE;
>         else
> 
> Michael

no, we can't do it.

cmd/mtd.c can be used without CONFIG_MTD_RAW_NAND and I think your fix will
break mtd command for non-nand flashes.

spi-nand flashes directly registers itself with mtd layer (nand_register()
function is not used), so it much more simple use cmd/mtd than cmd/nand.



> 
> 
> >
> > >
> > > Michael
> > >
> > >
> > >
> > > The nand is erased using the
> > > > if block is bad or reserverved, than warning is printed, than block is
> > > > erased.
> > > >
> > > >
> > > > > > +                       ret = -EIO;
> > > > > > +               else
> > > > > > +                       ret = nanddev_erase(nand, &pos);
> > > > >
> > > > > erase opt should already take in account scrub.
> > > > >
> > > > > Please extend the problem
> > > > >
> > > > > Michael
> > > > > >                 if (ret) {
> > > > > >                         einfo->fail_addr = nanddev_pos_to_offs(nand,
> > > > > > &pos);
> > > > > >
> > > > > > --
> > > > > > 2.35.1
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Michael Nazzareno Trimarchi
> > > > > Co-Founder & Chief Executive Officer
> > > > > M. +39 347 913 2170
> > > > > michael@amarulasolutions.com
> > > > > __________________________________
> > > > >
> > > > > Amarula Solutions BV
> > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > > T. +31 (0)85 111 9172
> > > > > info@amarulasolutions.com
> > > > > https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&amp;reserved=0
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&amp;reserved=0
> 
> 
> 
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&amp;reserved=0


Maybe it's better accept my change and fix nand_erase_opts() instead ?
Bad block table is autocleaned in nanddev_erase() function.

diff --git a/drivers/mtd/nand/raw/nand_util.c b/drivers/mtd/nand/raw/nand_util.c
index b2345dca7f..b35571dfd5 100644
--- a/drivers/mtd/nand/raw/nand_util.c
+++ b/drivers/mtd/nand/raw/nand_util.c
@@ -75,6 +75,7 @@ int nand_erase_opts(struct mtd_info *mtd,
 	erase.mtd = mtd;
 	erase.len = mtd->erasesize;
 	erase.addr = opts->offset;
+	erase.scrub = opts->scrub;
 	erase_length = lldiv(opts->length + mtd->erasesize - 1,
 			     mtd->erasesize);
 
@@ -82,23 +83,6 @@ int nand_erase_opts(struct mtd_info *mtd,
 	cleanmarker.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
 	cleanmarker.totlen = cpu_to_je32(8);
 
-	/* scrub option allows to erase badblock. To prevent internal
-	 * check from erase() method, set block check method to dummy
-	 * and disable bad block table while erasing.
-	 */
-	if (opts->scrub) {
-		erase.scrub = opts->scrub;
-		/*
-		 * We don't need the bad block table anymore...
-		 * after scrub, there are no bad blocks left!
-		 */
-		if (chip->bbt) {
-			kfree(chip->bbt);
-		}
-		chip->bbt = NULL;
-		chip->options &= ~NAND_BBT_SCANNED;
-	}
-
 	for (erased_length = 0;
 	     erased_length < erase_length;
 	     erase.addr += mtd->erasesize) {
@@ -109,34 +93,24 @@ int nand_erase_opts(struct mtd_info *mtd,
 			puts("Size of erase exceeds limit\n");
 			return -EFBIG;
 		}
-		if (!opts->scrub) {
-			int ret = mtd_block_isbad(mtd, erase.addr);
-			if (ret > 0) {
+
+		erased_length++;
+
+		result = mtd_erase(mtd, &erase);
+		if (result != 0) {
+			if (!opts->scrub && result == -EIO) {
 				if (!opts->quiet)
 					printf("\rSkipping bad block at  "
 					       "0x%08llx                 "
 					       "                         \n",
 					       erase.addr);
 
-				if (!opts->spread)
-					erased_length++;
-
-				continue;
-
-			} else if (ret < 0) {
-				printf("\n%s: MTD get bad block failed: %d\n",
-				       mtd_device,
-				       ret);
-				return -1;
+				if (opts->spread)
+					erased_length--;
+			} else {
+				printf("\n%s: MTD Erase failure: %d\n",
+				       mtd_device, result);
 			}
-		}
-
-		erased_length++;
-
-		result = mtd_erase(mtd, &erase);
-		if (result != 0) {
-			printf("\n%s: MTD Erase failure: %d\n",
-			       mtd_device, result);
 			continue;
 		}
 
---
Mikhail Kshevetskiy