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

Mikhail Kshevetskiy mikhail.kshevetskiy at iopsys.eu
Mon Oct 31 00:31:27 CET 2022


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 at 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 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%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TwMrP%2FpIz5R1Dfd1iXr8u2wZvDj3sdyl%2F8TADkPEDpQ%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?
>> 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&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=EZiou2JpcMoHgVxNhjtBmmnJWcX39byooOlQHuLH4Z0%3D&reserved=0
>> [2] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F2458%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=8zifHAz7im8uMEXrFBOEl9wDviXcH%2B2RNUKkvjh%2FUR0%3D&reserved=0
>>
>> 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
>>>
>>> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=nBKZLjQtxq8u83xF968C73piVKRpJ1k7iVXAuMWjD5o%3D&reserved=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
>
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7Cfe44aaf264764836295e08daba7ff1f7%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638027355932818989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=nBKZLjQtxq8u83xF968C73piVKRpJ1k7iVXAuMWjD5o%3D&reserved=0


More information about the U-Boot mailing list