On Thu, 6 Oct 2022 20:09:11 +0200 Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote:
> [External email] > > > > > > Hi Mikhail > > On Thu, Oct 6, 2022 at 6:17 PM Mikhail Kshevetskiy > <mikhail.kshevets...@iopsys.eu> wrote: > > > > On Thu, 6 Oct 2022 18:03:17 +0200 > > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote: > > > > > [External email] > > > > > > > > > > > > > > > > > > On Thu, Oct 6, 2022 at 5:52 PM Mikhail Kshevetskiy > > > <mikhail.kshevets...@iopsys.eu> wrote: > > > > > > > > On Thu, 6 Oct 2022 08:56:08 +0200 > > > > Michael Nazzareno Trimarchi <mich...@amarulasolutions.com> wrote: > > > > > > > > > [External email] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi > > > > > > > > > > On Thu, Oct 6, 2022 at 5:15 AM <mikhail.kshevets...@iopsys.eu> wrote: > > > > > > > > > > > > From: Mikhail Kshevetskiy <mikhail.kshevets...@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.kshevets...@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 > > > > > mich...@amarulasolutions.com > > > > > __________________________________ > > > > > > > > > > Amarula Solutions BV > > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > > > > T. +31 (0)85 111 9172 > > > > > i...@amarulasolutions.com > > > > > https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&reserved=0 > > > > > > > > > > > > -- > > > Michael Nazzareno Trimarchi > > > Co-Founder & Chief Executive Officer > > > M. +39 347 913 2170 > > > mich...@amarulasolutions.com > > > __________________________________ > > > > > > Amarula Solutions BV > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > > > T. +31 (0)85 111 9172 > > > i...@amarulasolutions.com > > > https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&reserved=0 > > > > -- > Michael Nazzareno Trimarchi > Co-Founder & Chief Executive Officer > M. +39 347 913 2170 > mich...@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > i...@amarulasolutions.com > https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C315a2da790414f5cb22108daa7c5e5e2%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638006765651063852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ouhzZ4pEofl7llK8OQHpatkrdksZozIgzY5IY9P%2BIL8%3D&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