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.kerne... 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.... [2] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork....
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.amarula...