[PATCH v3] cmd: mtd: check if a block has to be skipped or erased

[Dario Binacchi]

Hi Mikhail,

On Mon, Oct 24, 2022 at 1:24 PM Mikhail Kshevetskiy
<mikhail.kshevetskiy at iopsys.eu> wrote:
>
>
> On 24.10.2022 12:44, Dario Binacchi wrote:
> > [External email]
> >
> >
> >
> >
> >
> > From: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> >
> > As reported by patch [1], the `mtd erase' command should not erase bad
> > blocks.
> > To force bad block erasing you have to use the `mtd erase.dontskipbad'
> > command.
> >
> > This patch tries to fix the same issue without modifying code taken
> > from the linux kernel, in order to make further upgrades easier.
> >
> > [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20221006031501.110290-2-mikhail.kshevetskiy%40iopsys.eu%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C0ce3c4f03458440084dd08dab5a44a0e%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638022014468655016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=VMQov1%2FRUmymW2G452jlOveOu2tFCE5MtzssuP2Ri3Y%3D&reserved=0
> > Suggested-by: Michael Trimarchi <michael at amarulasolutions.com>
> > Co-developed-by: Michael Trimarchi <michael at amarulasolutions.com>
> > Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> > Co-developed-by: Dario Binacchi <dario.binacchi at amarulasolutions.com>
> > Signed-off-by: Dario Binacchi <dario.binacchi at amarulasolutions.com>
> > Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
> >
> > ---
> >
> > Changes in v3:
> > - Simplify the code. mtd_erase() can't return a bad block error. Print
> >   the messaged where the bad block is found.
> >
> > Changes in v2:
> > - Change the commit author
> > - Do not continue to erase if scrub option is enabled and a bad block
> >   was found but return from the function.
> > - Update the patch tags.
> >
> >  cmd/mtd.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/cmd/mtd.c b/cmd/mtd.c
> > index ad5cc9827d55..29b2a9c04c0c 100644
> > --- a/cmd/mtd.c
> > +++ b/cmd/mtd.c
> > @@ -434,19 +434,27 @@ static int do_mtd_erase(struct cmd_tbl *cmdtp, int flag, int argc,
> >         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)
> > +               if (!scrub) {
> > +                       ret = mtd_block_isbad(mtd, erase_op.addr);
> > +                       if (ret < 0) {
> > +                               printf("Failed to get bad block at 0x%08llx\n",
> > +                                      erase_op.addr);
> > +                               ret = CMD_RET_FAILURE;
> > +                               goto out_put_mtd;
> > +                       } else if (ret > 0) {
> > +                               printf("Skipping bad block at 0x%08llx\n",
> > +                                      erase_op.addr);
> > +                               ret = -EIO;
> >                                 break;
> > -                       printf("Skipping bad block at 0x%08llx\n",
> > -                              erase_op.addr);
> > +                       }
> >                 }
> >
> > +               ret = mtd_erase(mtd, &erase_op);
> > +               if (ret)
> > +                       break;
> > +
>
> mtd_erase() can return -EIO, see drivers/mtd/nand/spi/core.c function
> spinand_mtd_erase()

If I compare my original patch [1] with yours [2], I see no difference
in behavior except for ret checking after calling
mtd_erase() which, in my case, was wrong.
Do you agree?

Further, checking for a bad block inside the do_mtd_erase(), the
mtd_erase() can return -EIO only in the case of a protected
block. In case of the scrub option enabled the bad block is erased,
otherwise the block is jumped in the do_mtd_erase()
without calling the mtd_erase().

I think that now we can distinguish between a bad block and a
protected block inside the do_mtd_erase(), always if it's true
that the -EIO error is only returned for bad/protected block by low
level operations.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20221021152929.3598415-1-dario.binacchi@amarulasolutions.com/
[2] https://patchwork.amarulasolutions.com/patch/2458/

Thanks and regards,
Dario

>
>
>
> >                 len -= mtd->erasesize;
> >                 erase_op.addr += mtd->erasesize;
> >         }
> > --
> > 2.32.0
> >



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi at amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info at amarulasolutions.com

www.amarulasolutions.com