[PATCH v2] dfu: mtd: mark bad the MTD block on erase error

[Michael Nazzareno Trimarchi]

Hi

On Fri, Jun 2, 2023 at 3:23 PM Patrick Delaunay
<patrick.delaunay at 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 at 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 at amarulasolutions.com>

> --
> 2.25.1
>