[U-Boot] U-boot nand bug, read.part should fail

[U-Boot] U-boot nand bug, read.part should fail

[Harvey Chapman]

[ I started this conversation off-list before I joined the list. ]

The idea is to add .part as a valid command suffix to nand read/write so it would match nand erase.part. The code to implement it makes "nand read.part" act identically to "nand read".

On Feb 7, 2013, at 4:59 PM, Scott Wood <scottwood@freescale.com> wrote:

>> >> In fact, I think erase should be modified to deprecate erase.part and make erase work like read does now.
>> >
>> > Erase used to work like read does.  I deliberately changed it so that typos (e.g. "nand erase $partition $fliesize") don't end up erasing your entire partition or chip.
>> Ah, then maybe we should add .part to nand read for consistency? I'm ok either way.
> 
> That would get messy because it would be orthogonal to other suffixes.  Reading too much is not as harmful as

Nothing would change other than do_nand() would treat "nand read" and "nand read.part" identically.

> erasing too much.  Writing too much can be bad, though.  Perhaps we should just eliminate the ability to do reads/writes without explicit size (it already has problems with the size needing adjustment due to bad blocks).

I liked that I didn't have to specify the size. Converting from numbers to partition names led me to find the bug in the "nand read" code. Using partition names makes it much easier to work with u-boot since I don't have to count 0s every time I type an address or size.

[U-Boot] U-boot nand bug, read.part should fail

[Scott Wood]

On 02/07/2013 04:13:55 PM, Harvey Chapman wrote:
> [ I started this conversation off-list before I joined the list. ]
> 
> The idea is to add .part as a valid command suffix to nand read/write  
> so it would match nand erase.part. The code to implement it makes  
> "nand read.part" act identically to "nand read".
> 
> On Feb 7, 2013, at 4:59 PM, Scott Wood <scottwood@freescale.com>  
> wrote:
> 
> >> >> In fact, I think erase should be modified to deprecate  
> erase.part and make erase work like read does now.
> >> >
> >> > Erase used to work like read does.  I deliberately changed it so  
> that typos (e.g. "nand erase $partition $fliesize") don't end up  
> erasing your entire partition or chip.
> >> Ah, then maybe we should add .part to nand read for consistency?  
> I'm ok either way.
> >
> > That would get messy because it would be orthogonal to other  
> suffixes.  Reading too much is not as harmful as
> 
> Nothing would change other than do_nand() would treat "nand read" and  
> "nand read.part" identically.

The only reason to add .part/.chip is if the unsuffixed versions no  
longer operate on entire partitions/chips.

> > erasing too much.  Writing too much can be bad, though.  Perhaps we  
> should just eliminate the ability to do reads/writes without explicit  
> size (it already has problems with the size needing adjustment due to  
> bad blocks).
> 
> I liked that I didn't have to specify the size.

It's fine until you get a bad block in the partition, and you end up  
accessing the first block of the next partition (or getting "Attempt to  
read/write outside the flash area" if it's the last partition).

Of course, fixing partition/chip accesses to account for this when  
determining size would be even better. :-)

-Scott

[U-Boot] U-boot nand bug, read.part should fail

[Harvey Chapman]

On Feb 7, 2013, at 5:22 PM, Scott Wood <scottwood@freescale.com> wrote:

> It's fine until you get a bad block in the partition, and you end up accessing the first block of the next partition (or getting "Attempt to read/write outside the flash area" if it's the last partition).
> 
> Of course, fixing partition/chip accesses to account for this when determining size would be even better. :-)

Something like this?

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -621,60 +621,78 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
 
 		nand = &nand_info[dev];
 
 		s = strchr(cmd, '.');
 
 		if (s && !strcmp(s, ".raw")) {
 			raw = 1;
 
 			if (arg_off(argv[3], &dev, &off, &size))
 				return 1;
 
 			if (argc > 4 && !str2long(argv[4], &pagecount)) {
 				printf("'%s' is not a number\n", argv[4]);
 				return 1;
 			}
 
 			if (pagecount * nand->writesize > size) {
 				puts("Size exceeds partition or device limit\n");
 				return -1;
 			}
 
 			rwsize = pagecount * (nand->writesize + nand->oobsize);
 		} else {
 			if (arg_off_size(argc - 3, argv + 3, &dev,
 						&off, &size) != 0)
 				return 1;
 
 			rwsize = size;
 		}
 
+                /* If no size was given, it has been calculated for us as
+                 * the remainder of the chip or partition from offset. Adjust
+                 * down for bad blocks, if necessary.
+                 */
+                if (argc < 4) {
+                        nand_info_t *nand = &nand_info[dev];
+                        int maxsize = rwsize;
+                        int offset = off;
+                        for (; offset < maxsize; offset += nand->erasesize)
+                                if (nand_block_isbad(nand, offset))
+                                        rwsize -= nand->erasesize;
+
+                        if (rwsize != maxsize)
+                                printf("\nsize adjusted to %d (%d bad blocks)\n",
+                                       rwsize,
+                                       (maxsize - rwsize) / nand->erasesize);
+                }
+
 		if (!s || !strcmp(s, ".jffs2") ||
 		    !strcmp(s, ".e") || !strcmp(s, ".i") || !strcmp(s, ".part")) {
 			if (read)
 				ret = nand_read_skip_bad(nand, off, &rwsize,
 							 (u_char *)addr);
 			else
 				ret = nand_write_skip_bad(nand, off, &rwsize,
 							  (u_char *)addr, 0);
 #ifdef CONFIG_CMD_NAND_TRIMFFS
 		} else if (!strcmp(s, ".trimffs")) {
 			if (read) {
 				printf("Unknown nand command suffix '%s'\n", s);
 				return 1;
 			}
 			ret = nand_write_skip_bad(nand, off, &rwsize,
 						(u_char *)addr,
 						WITH_DROP_FFS);
 #endif
 #ifdef CONFIG_CMD_NAND_YAFFS
 		} else if (!strcmp(s, ".yaffs")) {
 			if (read) {
 				printf("Unknown nand command suffix '%s'.\n", s);
 				return 1;
 			}
 			ret = nand_write_skip_bad(nand, off, &rwsize,
 						(u_char *)addr,
 						WITH_INLINE_OOB);
 #endif
 		} else if (!strcmp(s, ".oob")) {
 			/* out-of-band data */

[U-Boot] U-boot nand bug, read.part should fail

[Harvey Chapman]

Eh, I shouldn't post code that quickly? Try this:

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -621,60 +621,80 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
 
 		nand = &nand_info[dev];
 
 		s = strchr(cmd, '.');
 
 		if (s && !strcmp(s, ".raw")) {
 			raw = 1;
 
 			if (arg_off(argv[3], &dev, &off, &size))
 				return 1;
 
 			if (argc > 4 && !str2long(argv[4], &pagecount)) {
 				printf("'%s' is not a number\n", argv[4]);
 				return 1;
 			}
 
 			if (pagecount * nand->writesize > size) {
 				puts("Size exceeds partition or device limit\n");
 				return -1;
 			}
 
 			rwsize = pagecount * (nand->writesize + nand->oobsize);
 		} else {
 			if (arg_off_size(argc - 3, argv + 3, &dev,
 						&off, &size) != 0)
 				return 1;
 
 			rwsize = size;
 		}
 
+                /* If no size was given, it has been calculated for us as
+                 * the remainder of the chip or partition from offset. Adjust
+                 * down for bad blocks, if necessary.
+                 */
+                if (argc < 5) {
+                        nand_info_t *nand = &nand_info[dev];
+                        int size = rwsize;
+                        int maxoffset = off + rwsize;
+                        int offset = off;
+                        int badblocks = 0;
+                        for (offset = off; offset < maxoffset; offset += nand->erasesize)
+                                if (nand_block_isbad(nand, offset))
+                                        badblocks++;
+                        if (badblocks) {
+                                rwsize -= badblocks * nand->erasesize;
+                                printf("size adjusted to 0x%llx (%d bad blocks)\n",
+                                       (unsigned long long)rwsize, badblocks);
+                        }
+                }
+
 		if (!s || !strcmp(s, ".jffs2") ||
 		    !strcmp(s, ".e") || !strcmp(s, ".i") || !strcmp(s, ".part")) {
 			if (read)
 				ret = nand_read_skip_bad(nand, off, &rwsize,
 							 (u_char *)addr);
 			else
 				ret = nand_write_skip_bad(nand, off, &rwsize,
 							  (u_char *)addr, 0);
 #ifdef CONFIG_CMD_NAND_TRIMFFS
 		} else if (!strcmp(s, ".trimffs")) {
 			if (read) {
 				printf("Unknown nand command suffix '%s'\n", s);
 				return 1;
 			}
 			ret = nand_write_skip_bad(nand, off, &rwsize,
 						(u_char *)addr,
 						WITH_DROP_FFS);
 #endif
 #ifdef CONFIG_CMD_NAND_YAFFS
 		} else if (!strcmp(s, ".yaffs")) {
 			if (read) {
 				printf("Unknown nand command suffix '%s'.\n", s);
 				return 1;
 			}
 			ret = nand_write_skip_bad(nand, off, &rwsize,
 						(u_char *)addr,
 						WITH_INLINE_OOB);
 #endif
 		} else if (!strcmp(s, ".oob")) {
 			/* out-of-band data */

[U-Boot] U-boot nand bug, read.part should fail

[Scott Wood]

On 02/08/2013 10:44:30 AM, Harvey Chapman wrote:
> Eh, I shouldn't post code that quickly? Try this:
> 
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -621,60 +621,80 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
> 
>  		nand = &nand_info[dev];
> 
>  		s = strchr(cmd, '.');
> 
>  		if (s && !strcmp(s, ".raw")) {
>  			raw = 1;
> 
>  			if (arg_off(argv[3], &dev, &off, &size))
>  				return 1;
> 
>  			if (argc > 4 && !str2long(argv[4], &pagecount))  
> {
>  				printf("'%s' is not a number\n",  
> argv[4]);
>  				return 1;
>  			}
> 
>  			if (pagecount * nand->writesize > size) {
>  				puts("Size exceeds partition or device  
> limit\n");
>  				return -1;
>  			}
> 
>  			rwsize = pagecount * (nand->writesize +  
> nand->oobsize);
>  		} else {
>  			if (arg_off_size(argc - 3, argv + 3, &dev,
>  						&off, &size) != 0)
>  				return 1;
> 
>  			rwsize = size;
>  		}
> 
> +                /* If no size was given, it has been calculated for  
> us as
> +                 * the remainder of the chip or partition from  
> offset. Adjust
> +                 * down for bad blocks, if necessary.
> +                 */

This should go inside the "not raw" path of the previous "if" statement.

Please use tabs to indent.

> +                if (argc < 5) {
> +                        nand_info_t *nand = &nand_info[dev];

We already have "nand" in this context.

> +                        int size = rwsize;

We already have "size" -- and you don't even seem to use it.

> +                        int maxoffset = off + rwsize;
> +                        int offset = off;

Offsets do not fit in "int".  Use loff_t.

> +                        int badblocks = 0;
> +                        for (offset = off; offset < maxoffset;  
> offset += nand->erasesize)
> +                                if (nand_block_isbad(nand, offset))
> +                                        badblocks++;

Braces around multi-line "if" bodies (even if a single statement).

Please leave a blank line after the variable declaration block.

Maybe move this to its own function (having both "offset" and "off" is  
unpleasant, plus this function is way too big already).

We need this adjustment to be made to erase.part/chip as well.

-Scott

[U-Boot] U-boot nand bug, read.part should fail

[Harvey Chapman]

On Feb 8, 2013, at 6:34 PM, Scott Wood <scottwood@freescale.com> wrote:

> On 02/08/2013 10:44:30 AM, Harvey Chapman wrote:
> This should go inside the "not raw" path of the previous "if" statement.
> Please use tabs to indent.
> We already have "nand" in this context.
> We already have "size" -- and you don't even seem to use it.
> Offsets do not fit in "int".  Use loff_t.
> Braces around multi-line "if" bodies (even if a single statement).
> Please leave a blank line after the variable declaration block.
> Maybe move this to its own function (having both "offset" and "off" is unpleasant, plus this function is way too big already).
> We need this adjustment to be made to erase.part/chip as well.

Thanks for the feedback. This was just a proof of concept. I'll make the changes and submit the patch more formally.