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

[Benoît Thébaudeau]

Hi Max,

On Thu, Jun 9, 2016 at 3:19 PM, Max Krummenacher <max.oss.09 at gmail.com> wrote:
> Hi Benoît,
>
> 2016-06-09 1:41 GMT+02:00 Benoît Thébaudeau <benoit.thebaudeau.dev at gmail.com>:
>> 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
[...]
>> +        if (argc > 3 && !str2off(argv[3], &size)) {
>
> Here I prefer having that in 2 if() as the stuff tested is only loosely related.

Usually, we keep things as compact as possible, which also limits the
number of indentation levels, but that's fine if you prefer otherwise.
I don't think it's a strong rule.

> I guess keeping it like this would also require parantheses around (argc > 3).

No: `>` has higher precedence than `&&`.

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

Normally, this rule is for grep-ability. Here, it's more complicated
with the '%' in-between, but it's still makes sense with a regular
expression, so OK.

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

Thanks.

Best regards,
Benoît