Re: [U-Boot] [PATCH v2 2/2] nand: extend nand torture

Hi Benoît,

2016-06-09 1:41 GMT+02:00 Benoît Thébaudeau benoit.thebaudeau.dev@gmail.com:

Hi Max,

On Tue, Jun 7, 2016 at 1:46 PM, Max Krummenacher max.oss.09@gmail.com wrote:

...

diff --git a/cmd/nand.c b/cmd/nand.c index 583a18f..8ade5e2 100644 --- a/cmd/nand.c +++ b/cmd/nand.c

...

...

•           return failed == 0 ? 0 : 1;
  

The given offset could also start anywhere, so it's better to auto-align it like the size.

If the arguments extend beyond the end of the flash, then nand_torture() will return an error at each iteration, so it's better to break the loop or not to start it in this case.

It's better to print the range actually tortured than the arguments from the user.

Agreed.

So what about the following?

@@ -647,6 +647,9 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

#ifdef CONFIG_CMD_NAND_TORTURE if (strcmp(cmd, "torture") == 0) {

•    loff_t endoff;
  

•    unsigned int failed = 0, passed = 0;
  

•    if (argc < 3)
         goto usage;
  
  

@@ -655,12 +658,36 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1; }

•    printf("\nNAND torture: device %d offset 0x%llx size 0x%x\n",
  

•        dev, off, mtd->erasesize);
  

•    ret = nand_torture(mtd, off);
  

•    printf(" %s\n", ret ? "Failed" : "Passed");
  

•    size = mtd->erasesize;
  

•    if (argc > 3 && !str2off(argv[3], &size)) {
  

Here I prefer having that in 2 if() as the stuff tested is only loosely related. I guess keeping it like this would also require parantheses around (argc > 3). Will revert to two if's in v3

•        puts("Size is not a valid number\n");
  

•        return 1;
  

•    }
  
  

•    return ret == 0 ? 0 : 1;
  

•    endoff = off + size;
  

•    if (endoff > mtd->size) {
  

•        puts("Arguments beyond end of NAND\n");
  

•        return 1;
  

•    }
  

•    off = round_down(off, mtd->erasesize);
  

•    endoff = round_up(endoff, mtd->erasesize);
  

•    size = endoff - off;
  

•    printf("\nNAND torture: device %d offset 0x%llx size 0x%llx "
  

•        "(block size 0x%x)\n",
  

patman.py/checkpatch.py warn here to keep quoted strings on one line even when the line length exceeds 80 characters. Will remove the line break / string concatenation for v3.

•        dev, off, size, mtd->erasesize);
  

•    while (off < endoff) {
  

•        ret = nand_torture(mtd, off);
  

•        if (ret) {
  

•            failed++;
  

•            printf("  block at 0x%llx failed\n", off);
  

•        } else {
  

•            passed++;
  

•        }
  

•        off += mtd->erasesize;
  

•    }
  

•    printf(" Passed: %u, failed: %u\n", passed, failed);
  

•    return failed != 0;
  
  

...

    }

#endif

Apart from above comments I merged your proposal.

...

@@ -775,7 +792,8 @@ static char nand_help_text[] =

...

...

diff --git a/doc/README.nand b/doc/README.nand index 96ffc48..5136f31 100644 --- a/doc/README.nand +++ b/doc/README.nand @@ -307,7 +307,7 @@ Miscellaneous and testing commands: DANGEROUS!!! Factory set bad blocks will be lost. Use only to remove artificial bad blocks created with the "markbad" command.

• "torture offset"

• "torture offset [size]" Torture block to determine if it is still reliable. Enabled by the CONFIG_CMD_NAND_TORTURE configuration option. This command returns 0 if the block is still reliable, else 1.

@@ -324,6 +324,10 @@ Miscellaneous and testing commands: automate actions following a nand->write() error. This would e.g. be required in order to program or update safely firmware to NAND, especially for the UBI part of such firmware.

• Optionally a second parameter size can be given to test multiple blocks with

"Optionally,"

...

• one call. If size is not a multiple of the NAND's erasesize then the block

"erase size, then"

...

• which contains offset + size will be tested in full. If used with size this

"that", not "which". "size, this"

Agreed. Will add that to v3.

Regards Max