[PATCH v3] cmd: mtd: check if a block has to be skipped or erased
Dario Binacchi
dario.binacchi at amarulasolutions.com
Sun Oct 30 15:06:18 CET 2022
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%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
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&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
--
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
More information about the U-Boot
mailing list