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.kshevets...@iopsys.eu> wrote: >> >> On 24.10.2022 12:44, Dario Binacchi wrote: >>> [External email] >>> >>> >>> >>> >>> >>> From: Mikhail Kshevetskiy <mikhail.kshevets...@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 <mich...@amarulasolutions.com> >>> Co-developed-by: Michael Trimarchi <mich...@amarulasolutions.com> >>> Signed-off-by: Michael Trimarchi <mich...@amarulasolutions.com> >>> Co-developed-by: Dario Binacchi <dario.binac...@amarulasolutions.com> >>> Signed-off-by: Dario Binacchi <dario.binac...@amarulasolutions.com> >>> Tested-by: Mikhail Kshevetskiy <mikhail.kshevets...@iopsys.eu> >>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevets...@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.binac...@amarulasolutions.com > > __________________________________ > > > Amarula Solutions SRL > > Via Le Canevare 30, 31100 Treviso, Veneto, IT > > T. +39 042 243 5310 > i...@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