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
