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

[Max Krummenacher]

Hi Benoît,

2016-06-09 1:41 GMT+02:00 Benoît Thébaudeau <benoit.thebaudeau.dev at gmail.com>:
> Hi Max,
>
> On Tue, Jun 7, 2016 at 1:46 PM, Max Krummenacher <max.oss.09 at 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