[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Max Krummenacher]

follow parameter name change (nand to mtd) to fix compiler error.

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

---

Changes in v2:
- Patch v1 1/1 went into master, but Scott's patch series syncing
  with kernel v4.6 introduced an additional compile time error.

 drivers/mtd/nand/nand_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index 5bba66a..e8bcc34 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd, loff_t offset)
 {
 	u_char patterns[] = {0xa5, 0x5a, 0x00};
 	struct erase_info instr = {
-		.mtd = nand,
+		.mtd = mtd,
 		.addr = offset,
 		.len = mtd->erasesize,
 	};
-- 
2.5.5

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

[Max Krummenacher]

nand torture currently works on exactly one nand block which is specified
by giving the byteoffset to the beginning of the block.

Extend this by allowing for a second parameter specifying the byte size
to be tested.

e.g.
==> nand torture 1000000

NAND torture: device 0 offset 0x1000000 size 0x20000 (nand block size 0x20000)
passed 1, failed 0

==> nand torture 1000000 40000

NAND torture: device 0 offset 0x1000000 size 0x40000 (nand block size 0x20000)
passed 2, failed 0

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

---

Changes in v2:
- findings from Beno?t:
  - change interface to be offset/size
  - change the output to include both 'size tested' and 'nand block size'
  - updated doc/README.nand accordingly
  - I did not implement the suggestion to move the code into the
    nand_torture() function. Likely one uses the extended functionality
    only during HW bringup interactively. If one would want to test
    multiple blocks from code one would also want to know the testresult
    of each individual block rather than only having a return parameter
    indicating a 'all good' or 'at least one block failed'.

 cmd/nand.c      | 34 ++++++++++++++++++++++++++--------
 doc/README.nand |  6 +++++-
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/cmd/nand.c b/cmd/nand.c
index 583a18f..8ade5e2 100644
--- a/cmd/nand.c
+++ b/cmd/nand.c
@@ -647,6 +647,8 @@ 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 failed = 0, passed = 0;
 		if (argc < 3)
 			goto usage;
 
@@ -654,13 +656,28 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			puts("Offset is not a valid number\n");
 			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");
-
-		return ret == 0 ? 0 : 1;
+		size = mtd->erasesize;
+		if (argc > 3)
+			if (!str2off(argv[3], &size)) {
+				puts("Size is not a valid number\n");
+				return 1;
+			}
+		printf("\nNAND torture: device %d offset 0x%llx size 0x%llx (nand block size 0x%x)\n",
+		       dev, off, size, mtd->erasesize);
+
+		endoff = off + size;
+		while (off < endoff) {
+			ret = nand_torture(mtd, off);
+			if (ret) {
+				failed++;
+				printf(" off 0x%llx %s\n", off, "Failed");
+			} else {
+				passed++;
+			}
+			off += mtd->erasesize;
+		}
+		printf("passed %u, failed %u\n", passed, failed);
+		return failed == 0 ? 0 : 1;
 	}
 #endif
 
@@ -775,7 +792,8 @@ static char nand_help_text[] =
 	"nand bad - show bad blocks\n"
 	"nand dump[.oob] off - dump page\n"
 #ifdef CONFIG_CMD_NAND_TORTURE
-	"nand torture off - torture block at offset\n"
+	"nand torture off - torture one block at offset\n"
+	"nand torture off size - torture blocks from off to off+size\n"
 #endif
 	"nand scrub [-y] off size | scrub.part partition | scrub.chip\n"
 	"    really clean NAND erasing bad blocks (UNSAFE)\n"
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
+  one call. If size is not a multiple of the NAND's erasesize then the block
+  which contains offset + size will be tested in full. If used with size this
+  command returns 0 if all tested blocks have been found reliable, else 1.
 
 
 NAND locking command (for chips with active LOCKPRE pin)
-- 
2.5.5

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

[Benoît Thébaudeau]

Hi Max,

On Tue, Jun 7, 2016 at 1:46 PM, Max Krummenacher <max.oss.09@gmail.com> wrote:
> nand torture currently works on exactly one nand block which is specified
> by giving the byteoffset to the beginning of the block.
>
> Extend this by allowing for a second parameter specifying the byte size
> to be tested.
>
> e.g.
> ==> nand torture 1000000
>
> NAND torture: device 0 offset 0x1000000 size 0x20000 (nand block size 0x20000)
> passed 1, failed 0
>
> ==> nand torture 1000000 40000
>
> NAND torture: device 0 offset 0x1000000 size 0x40000 (nand block size 0x20000)
> passed 2, failed 0
>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>
> ---
>
> Changes in v2:
> - findings from Beno?t:
>   - change interface to be offset/size
>   - change the output to include both 'size tested' and 'nand block size'
>   - updated doc/README.nand accordingly
>   - I did not implement the suggestion to move the code into the
>     nand_torture() function. Likely one uses the extended functionality
>     only during HW bringup interactively. If one would want to test
>     multiple blocks from code one would also want to know the testresult
>     of each individual block rather than only having a return parameter
>     indicating a 'all good' or 'at least one block failed'.
>
>  cmd/nand.c      | 34 ++++++++++++++++++++++++++--------
>  doc/README.nand |  6 +++++-
>  2 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/cmd/nand.c b/cmd/nand.c
> index 583a18f..8ade5e2 100644
> --- a/cmd/nand.c
> +++ b/cmd/nand.c
> @@ -647,6 +647,8 @@ 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 failed = 0, passed = 0;
>                 if (argc < 3)
>                         goto usage;
>
> @@ -654,13 +656,28 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                         puts("Offset is not a valid number\n");
>                         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");
> -
> -               return ret == 0 ? 0 : 1;
> +               size = mtd->erasesize;
> +               if (argc > 3)
> +                       if (!str2off(argv[3], &size)) {
> +                               puts("Size is not a valid number\n");
> +                               return 1;
> +                       }
> +               printf("\nNAND torture: device %d offset 0x%llx size 0x%llx (nand block size 0x%x)\n",
> +                      dev, off, size, mtd->erasesize);
> +
> +               endoff = off + size;
> +               while (off < endoff) {
> +                       ret = nand_torture(mtd, off);
> +                       if (ret) {
> +                               failed++;
> +                               printf(" off 0x%llx %s\n", off, "Failed");
> +                       } else {
> +                               passed++;
> +                       }
> +                       off += mtd->erasesize;
> +               }
> +               printf("passed %u, failed %u\n", passed, failed);
> +               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.

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)) {
+            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",
+            dev, off, size, mtd->erasesize);
+        while (off < endoff) {
+            ret = nand_torture(mtd, off);
+            if (ret) {
+                failed++;
+                printf("  block@0x%llx failed\n", off);
+            } else {
+                passed++;
+            }
+            off += mtd->erasesize;
+        }
+        printf(" Passed: %u, failed: %u\n", passed, failed);
+        return failed != 0;

>         }
>  #endif
>
> @@ -775,7 +792,8 @@ static char nand_help_text[] =
>         "nand bad - show bad blocks\n"
>         "nand dump[.oob] off - dump page\n"
>  #ifdef CONFIG_CMD_NAND_TORTURE
> -       "nand torture off - torture block at offset\n"
> +       "nand torture off - torture one block at offset\n"
> +       "nand torture off size - torture blocks from off to off+size\n"
>  #endif
>         "nand scrub [-y] off size | scrub.part partition | scrub.chip\n"
>         "    really clean NAND erasing bad blocks (UNSAFE)\n"
> 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"

> +  command returns 0 if all tested blocks have been found reliable, else 1.
>
>
>  NAND locking command (for chips with active LOCKPRE pin)
> --
> 2.5.5
>

Best regards,
Beno?t

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Scott Wood]

On 06/07/2016 06:47 AM, Max Krummenacher wrote:
> follow parameter name change (nand to mtd) to fix compiler error.
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> 
> ---
> 
> Changes in v2:
> - Patch v1 1/1 went into master, but Scott's patch series syncing
>   with kernel v4.6 introduced an additional compile time error.
> 
>  drivers/mtd/nand/nand_util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
> index 5bba66a..e8bcc34 100644
> --- a/drivers/mtd/nand/nand_util.c
> +++ b/drivers/mtd/nand/nand_util.c
> @@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd, loff_t offset)
>  {
>  	u_char patterns[] = {0xa5, 0x5a, 0x00};
>  	struct erase_info instr = {
> -		.mtd = nand,
> +		.mtd = mtd,
>  		.addr = offset,
>  		.len = mtd->erasesize,
>  	};

This got missed because no boards enable CONFIG_CMD_NAND_TORTURE.  If
you use this option could you enable it in the relevant board?

Or maybe we need to add an "allyesconfig"-type build (user-tunable
options only) to buildman?  And/or some other test configs that add up
to decent build coverage of options that are only enabled by users.

-Scott

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Max Krummenacher]

Hi Scott

2016-06-09 1:47 GMT+02:00 Scott Wood <scott.wood@nxp.com>:
> On 06/07/2016 06:47 AM, Max Krummenacher wrote:
>> follow parameter name change (nand to mtd) to fix compiler error.
>>
>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>
>> ---
>>
>> Changes in v2:
>> - Patch v1 1/1 went into master, but Scott's patch series syncing
>>   with kernel v4.6 introduced an additional compile time error.
>>
>>  drivers/mtd/nand/nand_util.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
>> index 5bba66a..e8bcc34 100644
>> --- a/drivers/mtd/nand/nand_util.c
>> +++ b/drivers/mtd/nand/nand_util.c
>> @@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd, loff_t offset)
>>  {
>>       u_char patterns[] = {0xa5, 0x5a, 0x00};
>>       struct erase_info instr = {
>> -             .mtd = nand,
>> +             .mtd = mtd,
>>               .addr = offset,
>>               .len = mtd->erasesize,
>>       };
>
> This got missed because no boards enable CONFIG_CMD_NAND_TORTURE.  If
> you use this option could you enable it in the relevant board?

I believe this makes option makes only sense if you do HW bringup or
have issues with
your NAND driver. (Which I currently have with an i.MX 7 design)
So likely one would not want to enable this for production code.

On the other hand people switching on the option should be able to fix
whatever issue
arises.

And, considering that it was broken since January 2013
(dfe64e2c89731a3f9950d7acd8681b68df2bae03) it can not be that important.

>
> Or maybe we need to add an "allyesconfig"-type build (user-tunable
> options only) to buildman?  And/or some other test configs that add up
> to decent build coverage of options that are only enabled by users.
>
> -Scott
>

Regards
Max

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

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Scott Wood]

On Thu, 2016-06-09 at 10:35 +0200, Max Krummenacher wrote:
> Hi Scott
> 
> 2016-06-09 1:47 GMT+02:00 Scott Wood <scott.wood@nxp.com>:
> > On 06/07/2016 06:47 AM, Max Krummenacher wrote:
> > > follow parameter name change (nand to mtd) to fix compiler error.
> > > 
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Patch v1 1/1 went into master, but Scott's patch series syncing
> > >   with kernel v4.6 introduced an additional compile time error.
> > > 
> > >  drivers/mtd/nand/nand_util.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
> > > index 5bba66a..e8bcc34 100644
> > > --- a/drivers/mtd/nand/nand_util.c
> > > +++ b/drivers/mtd/nand/nand_util.c
> > > @@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd, loff_t
> > > offset)
> > >  {
> > >       u_char patterns[] = {0xa5, 0x5a, 0x00};
> > >       struct erase_info instr = {
> > > -             .mtd = nand,
> > > +             .mtd = mtd,
> > >               .addr = offset,
> > >               .len = mtd->erasesize,
> > >       };
> > 
> > This got missed because no boards enable CONFIG_CMD_NAND_TORTURE.  If
> > you use this option could you enable it in the relevant board?
> 
> I believe this makes option makes only sense if you do HW bringup or
> have issues with
> your NAND driver. (Which I currently have with an i.MX 7 design)
> So likely one would not want to enable this for production code.

That's why I suggested the alternative of adding one or more targets aimed at
build coverage.

> On the other hand people switching on the option should be able to fix
> whatever issue
> arises.
> 
> And, considering that it was broken since January 2013
> (dfe64e2c89731a3f9950d7acd8681b68df2bae03) it can not be that important.

This patch fixes a breakage that was merged within the past week.  What was
broken by the 2013 sync?

-Scott

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Max Krummenacher]

Am Donnerstag, den 09.06.2016, 12:10 -0500 schrieb Scott Wood:
> On Thu, 2016-06-09 at 10:35 +0200, Max Krummenacher wrote:
> > Hi Scott
> > 
> > 2016-06-09 1:47 GMT+02:00 Scott Wood <scott.wood@nxp.com>:
> > > On 06/07/2016 06:47 AM, Max Krummenacher wrote:
> > > > follow parameter name change (nand to mtd) to fix compiler
> > > > error.
> > > > 
> > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v2:
> > > > - Patch v1 1/1 went into master, but Scott's patch series
> > > > syncing
> > > >   with kernel v4.6 introduced an additional compile time error.
> > > > 
> > > >  drivers/mtd/nand/nand_util.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/nand_util.c
> > > > b/drivers/mtd/nand/nand_util.c
> > > > index 5bba66a..e8bcc34 100644
> > > > --- a/drivers/mtd/nand/nand_util.c
> > > > +++ b/drivers/mtd/nand/nand_util.c
> > > > @@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd,
> > > > loff_t
> > > > offset)
> > > >  {
> > > >       u_char patterns[] = {0xa5, 0x5a, 0x00};
> > > >       struct erase_info instr = {
> > > > -             .mtd = nand,
> > > > +             .mtd = mtd,
> > > >               .addr = offset,
> > > >               .len = mtd->erasesize,
> > > >       };
> > > 
> > > This got missed because no boards enable CONFIG_CMD_NAND_TORTURE.
> > >   If
> > > you use this option could you enable it in the relevant board?
> > 
> > I believe this makes option makes only sense if you do HW bringup
> > or
> > have issues with
> > your NAND driver. (Which I currently have with an i.MX 7 design)
> > So likely one would not want to enable this for production code.
> 
> That's why I suggested the alternative of adding one or more targets
> aimed at
> build coverage.
> 
> > On the other hand people switching on the option should be able to
> > fix
> > whatever issue
> > arises.
> > 
> > And, considering that it was broken since January 2013
> > (dfe64e2c89731a3f9950d7acd8681b68df2bae03) it can not be that
> > important.
> 
> This patch fixes a breakage that was merged within the past week. 
>  What was
> broken by the 2013 sync?

The 2013 sync did deprecate the outside use of the function pointers
in the mtd_info struct. To force this it did also rename the struct
members.
Since the torture code did not follow the rename it did no longer
compile.
Now fixed with
http://git.denx.de/?p=u-boot.git;a=commit;h=667067faa18334f1e28c01b4753
0b5cce1b6182f

Max

> 
> -Scott
> 

[U-Boot] [PATCH v2 1/2] nand: nand torture: follow sync with linux v4.6

[Scott Wood]

On Thu, 2016-06-09 at 23:13 +0200, Max Krummenacher wrote:
> Am Donnerstag, den 09.06.2016, 12:10 -0500 schrieb Scott Wood:
> > On Thu, 2016-06-09 at 10:35 +0200, Max Krummenacher wrote:
> > > Hi Scott
> > > 
> > > 2016-06-09 1:47 GMT+02:00 Scott Wood <scott.wood@nxp.com>:
> > > > On 06/07/2016 06:47 AM, Max Krummenacher wrote:
> > > > > follow parameter name change (nand to mtd) to fix compiler
> > > > > error.
> > > > > 
> > > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v2:
> > > > > - Patch v1 1/1 went into master, but Scott's patch series
> > > > > syncing
> > > > >   with kernel v4.6 introduced an additional compile time error.
> > > > > 
> > > > >  drivers/mtd/nand/nand_util.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/nand_util.c
> > > > > b/drivers/mtd/nand/nand_util.c
> > > > > index 5bba66a..e8bcc34 100644
> > > > > --- a/drivers/mtd/nand/nand_util.c
> > > > > +++ b/drivers/mtd/nand/nand_util.c
> > > > > @@ -820,7 +820,7 @@ int nand_torture(struct mtd_info *mtd,
> > > > > loff_t
> > > > > offset)
> > > > >  {
> > > > >       u_char patterns[] = {0xa5, 0x5a, 0x00};
> > > > >       struct erase_info instr = {
> > > > > -             .mtd = nand,
> > > > > +             .mtd = mtd,
> > > > >               .addr = offset,
> > > > >               .len = mtd->erasesize,
> > > > >       };
> > > > 
> > > > This got missed because no boards enable CONFIG_CMD_NAND_TORTURE.
> > > >   If
> > > > you use this option could you enable it in the relevant board?
> > > 
> > > I believe this makes option makes only sense if you do HW bringup
> > > or
> > > have issues with
> > > your NAND driver. (Which I currently have with an i.MX 7 design)
> > > So likely one would not want to enable this for production code.
> > 
> > That's why I suggested the alternative of adding one or more targets
> > aimed at
> > build coverage.
> > 
> > > On the other hand people switching on the option should be able to
> > > fix
> > > whatever issue
> > > arises.
> > > 
> > > And, considering that it was broken since January 2013
> > > (dfe64e2c89731a3f9950d7acd8681b68df2bae03) it can not be that
> > > important.
> > 
> > This patch fixes a breakage that was merged within the past week. 
> >  What was
> > broken by the 2013 sync?
> 
> The 2013 sync did deprecate the outside use of the function pointers
> in the mtd_info struct. To force this it did also rename the struct
> members.
> Since the torture code did not follow the rename it did no longer
> compile.
> Now fixed with
> http://git.denx.de/?p=u-boot.git;a=commit;h=667067faa18334f1e28c01b4753
> 0b5cce1b6182f

Oh, from the description I didn't realize that the function pointers were
actually not working.  Usually "deprecated" means marked for removal/change,
not actually removed/changed.

-Scott

[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@gmail.com> wrote:
> Hi Beno?t,
>
> 2016-06-09 1:41 GMT+02:00 Beno?t Th?baudeau <benoit.thebaudeau.dev@gmail.com>:
>> 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
[...]
>> +        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