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

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://lore.kernel.org/all/20221006031501.110290-2-mikhail.kshevetskiy@iops... 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 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/mtd.c b/cmd/mtd.c index ad5cc9827d55..a314745e95e1 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -434,11 +434,24 @@ 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 (!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) { + /* simulate bad block behavior */ + ret = -EIO; + goto skip_block_erasing; + } + }
+ ret = mtd_erase(mtd, &erase_op); +skip_block_erasing: if (ret) { /* Abort if its not a bad block error */ if (ret != -EIO)

What patch will be finally applied? I am preparing a patch that will fix Dario Binacchi issues with non-BBT bad block.
From my point of view this patch have wrong lines in 'Changes in v2'
- Do not continue to erase if scrub option is enabled and a bad block was found but return from the function.
Issues of original Dario Binacchi patch:
Non-BBT registered bad block will break 'mtd erase'.
In this case * mtd_erase() will returns with -EIO * erasing will be stopped due to error * 'mtd erase' returns CMD_RET_SUCCESS as -EIO is expected result
Fix this by: a) return back -EIO check after mtd_erase() b) simulate bad block behavior and jump to mtd_erase() results check in the case of BBT registered bad block.
On 24.10.2022 12:35, 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 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/cmd/mtd.c b/cmd/mtd.c index ad5cc9827d55..a314745e95e1 100644 --- a/cmd/mtd.c +++ b/cmd/mtd.c @@ -434,11 +434,24 @@ 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 (!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) {
/* simulate bad block behavior */
ret = -EIO;
goto skip_block_erasing;
}
}
ret = mtd_erase(mtd, &erase_op);
+skip_block_erasing: if (ret) { /* Abort if its not a bad block error */ if (ret != -EIO) -- 2.32.0

Hi,
On Mon, 24 Oct 2022 at 03:35, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
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://lore.kernel.org/all/20221006031501.110290-2-mikhail.kshevetskiy@iops... 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 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
Can we get some tests in test/dm/mtd.c?
Regards, Simon

Hi Simon,
On Wed, Oct 26, 2022 at 1:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Mon, 24 Oct 2022 at 03:35, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
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://lore.kernel.org/all/20221006031501.110290-2-mikhail.kshevetskiy@iops... 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 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
Can we get some tests in test/dm/mtd.c?
We are thinking about it. Probably not soon, but we are thinking about what and how to add them.
Thanks and regards, Dario
Regards, Simon
participants (3)
-
Dario Binacchi
-
Mikhail Kshevetskiy
-
Simon Glass