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

[Patrick DELAUNAY]

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 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


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 at amarulasolutions.com>
>
>> -- 
>> 2.25.1
>>
> _______________________________________________
> Uboot-stm32 mailing list
> Uboot-stm32 at st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32