| Message ID | 20230602152232.v2.1.I20e8d74ea2ff0a99c6c741846b46af89c4ee136a@changeid |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] dfu: mtd: mark bad the MTD block on erase error | expand |
Hi On Fri, Jun 2, 2023 at 3:23 PM Patrick Delaunay <patrick.delaunay@foss.st.com> wrote: > > In the MTD DFU backend, it is needed to mark the NAND block bad when the > erase failed with the -EIO error, as it is done in UBI and JFFS2 code. > > This operation is not done in the MTD framework, but the bad block > tag (in BBM or in BBT) is required to avoid to write data on this block > in the next DFU_OP_WRITE loop in mtd_block_op(): the code skip the bad > blocks, tested by mtd_block_isbad(). > > Without this patch, when the NAND block become bad on DFU write operation > - low probability on new NAND - the DFU write operation will always failed > because the failing block is never marked bad. > > This patch also adds a test to avoid to request an erase operation on a > block already marked bad; this test is not performed in MTD framework > in mtd_erase(). > > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > --- > > Changes in v2: > - fixe mtd_block_isbad offset parameter for erase check > - Add trace when bad block are skipped in erase loop > - Add remaining byte in trace "Limit reached" > > drivers/dfu/dfu_mtd.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c > index c7075f12eca9..a4a3e91be23e 100644 > --- a/drivers/dfu/dfu_mtd.c > +++ b/drivers/dfu/dfu_mtd.c > @@ -86,27 +86,39 @@ static int mtd_block_op(enum dfu_op op, struct dfu_entity *dfu, > > while (remaining) { > if (erase_op.addr + remaining > lim) { > - printf("Limit reached 0x%llx while erasing at offset 0x%llx\n", > - lim, off); > + printf("Limit reached 0x%llx while erasing at offset 0x%llx, remaining 0x%llx\n", > + lim, erase_op.addr, remaining); This can be in a different change > return -EIO; > } > > + /* Skip the block if it is bad, don't erase it again */ > + if (mtd_block_isbad(mtd, erase_op.addr)) { > + printf("Skipping bad block at 0x%08llx\n", > + erase_op.addr); This print is wrong. If ret is 1 it's a bad block if it's 2 the block is reserved > + erase_op.addr += mtd->erasesize; > + continue; > + } > + > ret = mtd_erase(mtd, &erase_op); > > if (ret) { > - /* Abort if its not a bad block error */ > - if (ret != -EIO) { > - printf("Failure while erasing at offset 0x%llx\n", > - erase_op.fail_addr); > - return 0; > + /* If this is not -EIO, we have no idea what to do. */ > + if (ret == -EIO) { > + printf("Marking bad block at 0x%08llx (%d)\n", > + erase_op.fail_addr, ret); > + ret = mtd_block_markbad(mtd, erase_op.addr); > + } > + /* Abort if it is not -EIO or can't mark bad */ > + if (ret) { > + printf("Failure while erasing at offset 0x%llx (%d)\n", > + erase_op.fail_addr, ret); > + return ret; > } > - printf("Skipping bad block at 0x%08llx\n", > - erase_op.addr); > } else { > remaining -= mtd->erasesize; > } > > - /* Continue erase behind bad block */ > + /* Continue erase behind the current block */ > erase_op.addr += mtd->erasesize; > } > } Otherwise Reviewed-by: Michael Trimarchi <michael@amarulasolutions.com> > -- > 2.25.1 >
Hi, On 6/2/23 17:27, Michael Nazzareno Trimarchi wrote: > Hi > > On Fri, Jun 2, 2023 at 3:23 PM Patrick Delaunay > <patrick.delaunay@foss.st.com> wrote: >> In the MTD DFU backend, it is needed to mark the NAND block bad when the >> erase failed with the -EIO error, as it is done in UBI and JFFS2 code. >> >> This operation is not done in the MTD framework, but the bad block >> tag (in BBM or in BBT) is required to avoid to write data on this block >> in the next DFU_OP_WRITE loop in mtd_block_op(): the code skip the bad >> blocks, tested by mtd_block_isbad(). >> >> Without this patch, when the NAND block become bad on DFU write >> operation >> - low probability on new NAND - the DFU write operation will always >> failed >> because the failing block is never marked bad. >> >> This patch also adds a test to avoid to request an erase operation on a >> block already marked bad; this test is not performed in MTD framework >> in mtd_erase(). >> >> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> >> --- >> >> Changes in v2: >> - fixe mtd_block_isbad offset parameter for erase check >> - Add trace when bad block are skipped in erase loop >> - Add remaining byte in trace "Limit reached" >> >> drivers/dfu/dfu_mtd.c | 32 ++++++++++++++++++++++---------- >> 1 file changed, 22 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c >> index c7075f12eca9..a4a3e91be23e 100644 >> --- a/drivers/dfu/dfu_mtd.c >> +++ b/drivers/dfu/dfu_mtd.c >> @@ -86,27 +86,39 @@ static int mtd_block_op(enum dfu_op op, struct >> dfu_entity *dfu, >> >> while (remaining) { >> if (erase_op.addr + remaining > lim) { >> - printf("Limit reached 0x%llx while >> erasing at offset 0x%llx\n", >> - lim, off); >> + printf("Limit reached 0x%llx while >> erasing at offset 0x%llx, remaining 0x%llx\n", >> + lim, erase_op.addr, remaining); > This can be in a different change ok > >> return -EIO; >> } >> >> + /* Skip the block if it is bad, don't erase >> it again */ >> + if (mtd_block_isbad(mtd, erase_op.addr)) { >> + printf("Skipping bad block at >> 0x%08llx\n", >> + erase_op.addr); > This print is wrong. If ret is 1 it's a bad block if it's 2 the block > is reserved Ok I did the same trace than drivers/mtd/nand/raw/nand_util.c:nand_erase_opts() cmd/mtd.c:do_mtd_bad() but on old branch before you commits d9fa61f54e7f9a ("mtd: nand: Show reserved block in chip.erase") cfb82f7c123e4 ("mtd: nand: Mark reserved blocks") thanks to point it, I prepare a V3 > >> + erase_op.addr += mtd->erasesize; >> + continue; >> + } >> + >> ret = mtd_erase(mtd, &erase_op); >> >> if (ret) { >> - /* Abort if its not a bad block error */ >> - if (ret != -EIO) { >> - printf("Failure while erasing >> at offset 0x%llx\n", >> - erase_op.fail_addr); >> - return 0; >> + /* If this is not -EIO, we have no >> idea what to do. */ >> + if (ret == -EIO) { >> + printf("Marking bad block at >> 0x%08llx (%d)\n", >> + erase_op.fail_addr, ret); >> + ret = mtd_block_markbad(mtd, >> erase_op.addr); >> + } >> + /* Abort if it is not -EIO or can't >> mark bad */ >> + if (ret) { >> + printf("Failure while erasing >> at offset 0x%llx (%d)\n", >> + erase_op.fail_addr, ret); >> + return ret; >> } >> - printf("Skipping bad block at >> 0x%08llx\n", >> - erase_op.addr); >> } else { >> remaining -= mtd->erasesize; >> } >> >> - /* Continue erase behind bad block */ >> + /* Continue erase behind the current block */ >> erase_op.addr += mtd->erasesize; >> } >> } > Otherwise > Reviewed-by: Michael Trimarchi <michael@amarulasolutions.com> > >> -- >> 2.25.1 >> > _______________________________________________ > Uboot-stm32 mailing list > Uboot-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32
diff --git a/drivers/dfu/dfu_mtd.c b/drivers/dfu/dfu_mtd.c index c7075f12eca9..a4a3e91be23e 100644 --- a/drivers/dfu/dfu_mtd.c +++ b/drivers/dfu/dfu_mtd.c @@ -86,27 +86,39 @@ static int mtd_block_op(enum dfu_op op, struct dfu_entity *dfu, while (remaining) { if (erase_op.addr + remaining > lim) { - printf("Limit reached 0x%llx while erasing at offset 0x%llx\n", - lim, off); + printf("Limit reached 0x%llx while erasing at offset 0x%llx, remaining 0x%llx\n", + lim, erase_op.addr, remaining); return -EIO; } + /* Skip the block if it is bad, don't erase it again */ + if (mtd_block_isbad(mtd, erase_op.addr)) { + printf("Skipping bad block at 0x%08llx\n", + erase_op.addr); + erase_op.addr += mtd->erasesize; + continue; + } + ret = mtd_erase(mtd, &erase_op); if (ret) { - /* Abort if its not a bad block error */ - if (ret != -EIO) { - printf("Failure while erasing at offset 0x%llx\n", - erase_op.fail_addr); - return 0; + /* If this is not -EIO, we have no idea what to do. */ + if (ret == -EIO) { + printf("Marking bad block at 0x%08llx (%d)\n", + erase_op.fail_addr, ret); + ret = mtd_block_markbad(mtd, erase_op.addr); + } + /* Abort if it is not -EIO or can't mark bad */ + if (ret) { + printf("Failure while erasing at offset 0x%llx (%d)\n", + erase_op.fail_addr, ret); + return ret; } - printf("Skipping bad block at 0x%08llx\n", - erase_op.addr); } else { remaining -= mtd->erasesize; } - /* Continue erase behind bad block */ + /* Continue erase behind the current block */ erase_op.addr += mtd->erasesize; } }
In the MTD DFU backend, it is needed to mark the NAND block bad when the erase failed with the -EIO error, as it is done in UBI and JFFS2 code. This operation is not done in the MTD framework, but the bad block tag (in BBM or in BBT) is required to avoid to write data on this block in the next DFU_OP_WRITE loop in mtd_block_op(): the code skip the bad blocks, tested by mtd_block_isbad(). Without this patch, when the NAND block become bad on DFU write operation - low probability on new NAND - the DFU write operation will always failed because the failing block is never marked bad. This patch also adds a test to avoid to request an erase operation on a block already marked bad; this test is not performed in MTD framework in mtd_erase(). Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> --- Changes in v2: - fixe mtd_block_isbad offset parameter for erase check - Add trace when bad block are skipped in erase loop - Add remaining byte in trace "Limit reached" drivers/dfu/dfu_mtd.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)