[PATCH v3] cmd: mtd: check if a block has to be skipped or erased
Mikhail Kshevetskiy
mikhail.kshevetskiy at iopsys.eu
Wed Oct 26 14:56:04 CEST 2022
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%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=YRmQ5XcWRw8gcy4Hy3nK29J%2FnttlD10lQvH%2B5YnBrxU%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
> 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&data=05%7C01%7Cmikhail.kshevetskiy%40iopsys.eu%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=BK5CkLi6MBMaByX8%2FK0vKhAMKciyFE0TuOnNqsljrLc%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%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wzYLYjzuTOn8t7aHWxxUyT6IoKM72C%2FTTmC2nNzxVck%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%7C02dff372bf00435cd94208dab71b6aed%7C7ff78d652de440f586750569e5c7a65d%7C0%7C0%7C638023625630435867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HhmwzN7An3%2FyWcgkO7uORGfVKmlzdMp9cwFjaD4nLHs%3D&reserved=0
More information about the U-Boot
mailing list