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

From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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://lore.kernel.org/all/20221006031501.110290-2-mikhail.kshevetskiy@iopsys.eu/
Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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(-)
  

On 24.10.2022 12:44, Dario Binacchi wrote:
> [External email]
>
>
>
>
>
> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C0ce3c4f03458440084dd08dab5a44a0e%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638022014468655016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=VMQov1%2FRUmymW2G452jlOveOu2tFCE5MtzssuP2Ri3Y%3D&amp;reserved=0
> Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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;

That is bad.//Skipping bad block is an expected behavior. We should not fail on it.


> -                       printf("Skipping bad block at 0x%08llx\n",
> -                              erase_op.addr);
> +                       }
>                 }
>
> +               ret = mtd_erase(mtd, &erase_op);
> +               if (ret)
> +                       break;
> +
>                 len -= mtd->erasesize;
>                 erase_op.addr += mtd->erasesize;
>         }
> --
> 2.32.0
>
  

On 24.10.2022 12:44, Dario Binacchi wrote:
> [External email]
>
>
>
>
>
> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C0ce3c4f03458440084dd08dab5a44a0e%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638022014468655016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=VMQov1%2FRUmymW2G452jlOveOu2tFCE5MtzssuP2Ri3Y%3D&amp;reserved=0
> Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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()


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

Hi Mikhail,

On Mon, Oct 24, 2022 at 1:24 PM Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
>
> On 24.10.2022 12:44, Dario Binacchi wrote:
> > [External email]
> >
> >
> >
> >
> >
> > From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C0ce3c4f03458440084dd08dab5a44a0e%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638022014468655016%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=VMQov1%2FRUmymW2G452jlOveOu2tFCE5MtzssuP2Ri3Y%3D&amp;reserved=0
> > Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
> > Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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
> >
  

On 26.10.2022 09:29, Dario Binacchi wrote:

> [External email]
>
>
>
>
>
> Hi Mikhail,
>
> On Mon, Oct 24, 2022 at 1:24 PM Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>>
>> On 24.10.2022 12:44, Dario Binacchi wrote:
>>> [External email]
>>>
>>>
>>>
>>>
>>>
>>> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=YRmQ5XcWRw8gcy4Hy3nK29J%2FnttlD10lQvH%2B5YnBrxU%3D&amp;reserved=0
>>> Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
>>> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>> Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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?

yes

> 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 see -EIO error in the following cases

a) physical bad block

b) hardware failures (ex: some timeout expired)

maybe it also returned for protected block as well.

> 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.

Could you describe your idea in more details. I do not see a way to
distinguish bad and protected blocks.

The main problem with your v3 patch is a termination of erasing loop
on any mtd_erase() failure. It becomes worse if mtd_erase() returns -EIO.
In this case you'll stop erasing due to error, but do_mtd_erase() will
return success.

Mikhail

[1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20221021152929.3598415-1-dario.binacchi%40amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=BK5CkLi6MBMaByX8%2FK0vKhAMKciyFE0TuOnNqsljrLc%3D&amp;reserved=0
[2] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F2458%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=wzYLYjzuTOn8t7aHWxxUyT6IoKM72C%2FTTmC2nNzxVck%3D&amp;reserved=0

Thanks and regards,
Dario

>>
>>
>>>                 len -= mtd->erasesize;
>>>                 erase_op.addr += mtd->erasesize;
>>>         }
>>> --
>>> 2.32.0
>>>
>
>
> --
>
> Dario Binacchi
>
> Embedded Linux Developer
>
> dario.binacchi@amarulasolutions.com
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> 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%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HhmwzN7An3%2FyWcgkO7uORGfVKmlzdMp9cwFjaD4nLHs%3D&amp;reserved=0
  

Hi Mikhail,

On Wed, Oct 26, 2022 at 2:56 PM Mikhail Kshevetskiy
<mikhail.kshevetskiy@iopsys.eu> wrote:
>
> On 26.10.2022 09:29, Dario Binacchi wrote:
>
> > [External email]
> >
> >
> >
> >
> >
> > Hi Mikhail,
> >
> > On Mon, Oct 24, 2022 at 1:24 PM Mikhail Kshevetskiy
> > <mikhail.kshevetskiy@iopsys.eu> wrote:
> >>
> >> On 24.10.2022 12:44, Dario Binacchi wrote:
> >>> [External email]
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=YRmQ5XcWRw8gcy4Hy3nK29J%2FnttlD10lQvH%2B5YnBrxU%3D&amp;reserved=0
> >>> Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
> >>> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> >>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >>> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>> Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> >>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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?
>
> yes

So I'll submit version 4 which reverts to version 1 and changes the
check for the return value
of the mtd_erase(). I will also change the author of the commit. I
added your signed-off. Please
retest the patch on your hardware.

>
> > 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 see -EIO error in the following cases
>
> a) physical bad block
>
> b) hardware failures (ex: some timeout expired)
>
> maybe it also returned for protected block as well.

So,  in case (b) we may have another type of problem which is not
caused by this patch.
As we can see by the current code:

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;
 }

if (ret && ret != -EIO)
    ret = CMD_RET_FAILURE;
else
    ret = CMD_RET_SUCCESS;

>
> > 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.
>
> Could you describe your idea in more details. I do not see a way to
> distinguish bad and protected blocks.

Could we use mtd_block_isreserved() ?

Thanks and regards
Dario

>
> The main problem with your v3 patch is a termination of erasing loop
> on any mtd_erase() failure. It becomes worse if mtd_erase() returns -EIO.
> In this case you'll stop erasing due to error, but do_mtd_erase() will
> return success.
>
> Mikhail
>
> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20221021152929.3598415-1-dario.binacchi%40amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=BK5CkLi6MBMaByX8%2FK0vKhAMKciyFE0TuOnNqsljrLc%3D&amp;reserved=0
> [2] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F2458%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=wzYLYjzuTOn8t7aHWxxUyT6IoKM72C%2FTTmC2nNzxVck%3D&amp;reserved=0
>
> Thanks and regards,
> Dario
>
> >>
> >>
> >>>                 len -= mtd->erasesize;
> >>>                 erase_op.addr += mtd->erasesize;
> >>>         }
> >>> --
> >>> 2.32.0
> >>>
> >
> >
> > --
> >
> > Dario Binacchi
> >
> > Embedded Linux Developer
> >
> > dario.binacchi@amarulasolutions.com
> >
> > __________________________________
> >
> >
> > Amarula Solutions SRL
> >
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> >
> > T. +39 042 243 5310
> > 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%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HhmwzN7An3%2FyWcgkO7uORGfVKmlzdMp9cwFjaD4nLHs%3D&amp;reserved=0
  

On 30.10.2022 17:06, Dario Binacchi wrote:

> [External email]
>
>
>
>
>
> Hi Mikhail,
>
> On Wed, Oct 26, 2022 at 2:56 PM Mikhail Kshevetskiy
> <mikhail.kshevetskiy@iopsys.eu> wrote:
>> On 26.10.2022 09:29, Dario Binacchi wrote:
>>
>>> [External email]
>>>
>>>
>>>
>>>
>>>
>>> Hi Mikhail,
>>>
>>> On Mon, Oct 24, 2022 at 1:24 PM Mikhail Kshevetskiy
>>> <mikhail.kshevetskiy@iopsys.eu> wrote:
>>>> On 24.10.2022 12:44, Dario Binacchi wrote:
>>>>> [External email]
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> From: Mikhail Kshevetskiy <mikhail.kshevetskiy@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&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TwMrP%2FpIz5R1Dfd1iXr8u2wZvDj3sdyl%2F8TADkPEDpQ%3D&amp;reserved=0
>>>>> Suggested-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>>> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>>> Co-developed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>>>> Tested-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>>>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@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?
>> yes
> So I'll submit version 4 which reverts to version 1 and changes the
> check for the return value
> of the mtd_erase(). I will also change the author of the commit. I
> added your signed-off. Please
> retest the patch on your hardware.

v4 looks good for me.

>
>>> 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 see -EIO error in the following cases
>>
>> a) physical bad block
>>
>> b) hardware failures (ex: some timeout expired)
>>
>> maybe it also returned for protected block as well.
> So,  in case (b) we may have another type of problem which is not
> caused by this patch.
> As we can see by the current code:
>
> 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;
>  }
>
> if (ret && ret != -EIO)
>     ret = CMD_RET_FAILURE;
> else
>     ret = CMD_RET_SUCCESS;
>
>>> 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.
>> Could you describe your idea in more details. I do not see a way to
>> distinguish bad and protected blocks.
> Could we use mtd_block_isreserved() ?

Probably no. As I remember reserved block is not very good tested.
I try to use it once to protect blocks from erasing and it does not help.
The code looks good, but something goes wrong. I don't remember details :-(

The issue exists both u-boot and linux.

If I have some time during a week, I'll provide mode details.

Mikhail. 

>
> Thanks and regards
> Dario
>
>> The main problem with your v3 patch is a termination of erasing loop
>> on any mtd_erase() failure. It becomes worse if mtd_erase() returns -EIO.
>> In this case you'll stop erasing due to error, but do_mtd_erase() will
>> return success.
>>
>> Mikhail
>>
>> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20221021152929.3598415-1-dario.binacchi%40amarulasolutions.com%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=EZiou2JpcMoHgVxNhjtBmmnJWcX39byooOlQHuLH4Z0%3D&amp;reserved=0
>> [2] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F2458%2F&amp;data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=8zifHAz7im8uMEXrFBOEl9wDviXcH%2B2RNUKkvjh%2FUR0%3D&amp;reserved=0
>>
>> Thanks and regards,
>> Dario
>>
>>>>
>>>>>                 len -= mtd->erasesize;
>>>>>                 erase_op.addr += mtd->erasesize;
>>>>>         }
>>>>> --
>>>>> 2.32.0
>>>>>
>>>
>>> --
>>>
>>> Dario Binacchi
>>>
>>> Embedded Linux Developer
>>>
>>> dario.binacchi@amarulasolutions.com
>>>
>>> __________________________________
>>>
>>>
>>> Amarula Solutions SRL
>>>
>>> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>>>
>>> T. +39 042 243 5310
>>> 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%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nBKZLjQtxq8u83xF968C73piVKRpJ1k7iVXAuMWjD5o%3D&amp;reserved=0
>
>
> --
>
> Dario Binacchi
>
> Embedded Linux Developer
>
> dario.binacchi@amarulasolutions.com
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> 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%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nBKZLjQtxq8u83xF968C73piVKRpJ1k7iVXAuMWjD5o%3D&amp;reserved=0