[U-Boot] [PATCH] Add +size syntax to nand commands to pad lengths to page sizes.

[U-Boot] [PATCH] Add +size syntax to nand commands to pad lengths to page sizes.

[Josh Karabin]

This change implements the suggestion from an earlier thread for
how to handle padding of non-page sized writes to NAND flashes.

See http://lists.denx.de/pipermail/u-boot/2009-February/047795.html
for the original discussion.  Note that validity of tail page's
memory is the reponsibility of the caller.

Signed-off-by: Josh Karabin <gkarabin@vocollect.com>
---
 common/cmd_nand.c |   41 +++++++++++++++++++++++++++++++----------
 1 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 1992531..f094101 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -102,9 +102,11 @@ static inline int str2long(char *p, ulong *num)
 }
 
 static int
-arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size)
+arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off,
+	      size_t *size, int *plussed)
 {
 	int idx = nand_curr_device;
+	char *ps = argv[1];
 #if defined(CONFIG_CMD_MTDPARTS)
 	struct mtd_device *dev;
 	struct part_info *part;
@@ -119,8 +121,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
 			}
 			*off = part->offset;
 			if (argc >= 2) {
-				if (!(str2long(argv[1], (ulong *)size))) {
-					printf("'%s' is not a number\n", argv[1]);
+				if (plussed && *ps == '+') {
+					*plussed = 1;
+					ps++;
+				}
+				if (!(str2long(ps, (ulong *)size))) {
+					printf("'%s' is not a number\n", ps);
 					return -1;
 				}
 				if (*size > part->size)
@@ -145,8 +151,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
 	}
 
 	if (argc >= 2) {
-		if (!(str2long(argv[1], (ulong *)size))) {
-			printf("'%s' is not a number\n", argv[1]);
+		if (plussed && *ps == '+') {
+			*plussed = 1;
+			ps++;
+		}
+		if (!(str2long(ps, (ulong *)size))) {
+			printf("'%s' is not a number\n", ps);
 			return -1;
 		}
 	} else {
@@ -317,7 +327,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 
 		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
 		/* skip first two or three arguments, look for offset and size */
-		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
+		if (arg_off_size(argc - o, argv + o, nand, &off, &size, NULL) != 0)
 			return 1;
 
 		memset(&opts, 0, sizeof(opts));
@@ -378,8 +388,18 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 
 		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
 		printf("\nNAND %s: ", read ? "read" : "write");
-		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
+		if (read && arg_off_size(argc - 3, argv + 3, nand, &off, &size, NULL) != 0)
 			return 1;
+		else if (!read) {
+			int plussed = 0;
+			if (arg_off_size(argc - 3, argv + 3, nand, &off, &size, &plussed) != 0)
+				return 1;
+			if (plussed) {
+				int tailsize = size & (nand->writesize - 1);
+				memset ((u_char *)addr + size, 0xff, nand->writesize - tailsize);
+				size += nand->writesize - tailsize;
+			}
+		}
 
 		s = strchr(cmd, '.');
 		if (!s || !strcmp(s, ".jffs2") ||
@@ -457,7 +477,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 	}
 
 	if (strcmp(cmd, "unlock") == 0) {
-		if (arg_off_size(argc - 2, argv + 2, nand, &off, &size) < 0)
+		if (arg_off_size(argc - 2, argv + 2, nand, &off, &size, NULL) < 0)
 			return 1;
 
 		if (!nand_unlock(nand, off, size)) {
@@ -481,9 +501,10 @@ U_BOOT_CMD(nand, 5, 1, do_nand,
 	   "info - show available NAND devices\n"
 	   "nand device [dev] - show or set current device\n"
 	   "nand read - addr off|partition size\n"
-	   "nand write - addr off|partition size\n"
+	   "nand write - addr off|partition [+]size\n"
 	   "    read/write 'size' bytes starting@offset 'off'\n"
-	   "    to/from memory address 'addr', skipping bad blocks.\n"
+	   "    to/from memory address 'addr', skipping bad blocks,\n"
+	   "    rounding up to a page size if '+' is specified.\n"
 	   "nand erase [clean] [off size] - erase 'size' bytes from\n"
 	   "    offset 'off' (entire device if not specified)\n"
 	   "nand bad - show bad blocks\n"
-- 
1.6.0.6

[U-Boot] [PATCH] Add +size syntax to nand commands to pad lengths to page sizes.

[Scott Wood]

On Fri, May 01, 2009 at 04:24:20PM -0400, Josh Karabin wrote:
> @@ -119,8 +121,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>  			}
>  			*off = part->offset;
>  			if (argc >= 2) {
> -				if (!(str2long(argv[1], (ulong *)size))) {
> -					printf("'%s' is not a number\n", argv[1]);
> +				if (plussed && *ps == '+') {
> +					*plussed = 1;
> +					ps++;
> +				}
> +				if (!(str2long(ps, (ulong *)size))) {
> +					printf("'%s' is not a number\n", ps);
>  					return -1;
>  				}
>  				if (*size > part->size)
> @@ -145,8 +151,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>  	}
>  
>  	if (argc >= 2) {
> -		if (!(str2long(argv[1], (ulong *)size))) {
> -			printf("'%s' is not a number\n", argv[1]);
> +		if (plussed && *ps == '+') {
> +			*plussed = 1;
> +			ps++;
> +		}
> +		if (!(str2long(ps, (ulong *)size))) {
> +			printf("'%s' is not a number\n", ps);
>  			return -1;

Hmm... would be nice to untangle the duplicated code path rather than add
more stuff to both branches.

> @@ -317,7 +327,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  
>  		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
>  		/* skip first two or three arguments, look for offset and size */
> -		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
> +		if (arg_off_size(argc - o, argv + o, nand, &off, &size, NULL) != 0)
>  			return 1;
>  
>  		memset(&opts, 0, sizeof(opts));
> @@ -378,8 +388,18 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>  
>  		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
>  		printf("\nNAND %s: ", read ? "read" : "write");
> -		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
> +		if (read && arg_off_size(argc - 3, argv + 3, nand, &off, &size, NULL) != 0)
>  			return 1;
> +		else if (!read) {
> +			int plussed = 0;
> +			if (arg_off_size(argc - 3, argv + 3, nand, &off, &size, &plussed) != 0)
> +				return 1;

Why not support plussed for read as well?

> +			if (plussed) {
> +				int tailsize = size & (nand->writesize - 1);
> +				memset ((u_char *)addr + size, 0xff, nand->writesize - tailsize);
> +				size += nand->writesize - tailsize;

NACK, you cannot write to arbitrary memory beyond the end of the range
specified.  Allocate a buffer to hold the partial page.

Plus, this will append an entire page of padding if the size does happen
to be page-aligned.

> +			}
> +		}

Please keep lines under 80 characters.

-Scott

[U-Boot] [PATCH] Add +size syntax to nand commands to pad lengths to page sizes.

[Josh Karabin]

Thanks for the review!  I have some questions below that'll help me get
rev 2 correct.

Scott Wood wrote:
> On Fri, May 01, 2009 at 04:24:20PM -0400, Josh Karabin wrote:
>> @@ -119,8 +121,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>>  			}
>>  			*off = part->offset;
>>  			if (argc >= 2) {
>> -				if (!(str2long(argv[1], (ulong *)size))) {
>> -					printf("'%s' is not a number\n", argv[1]);
>> +				if (plussed && *ps == '+') {
>> +					*plussed = 1;
>> +					ps++;
>> +				}
>> +				if (!(str2long(ps, (ulong *)size))) {
>> +					printf("'%s' is not a number\n", ps);
>>  					return -1;
>>  				}
>>  				if (*size > part->size)
>> @@ -145,8 +151,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>>  	}
>>  
>>  	if (argc >= 2) {
>> -		if (!(str2long(argv[1], (ulong *)size))) {
>> -			printf("'%s' is not a number\n", argv[1]);
>> +		if (plussed && *ps == '+') {
>> +			*plussed = 1;
>> +			ps++;
>> +		}
>> +		if (!(str2long(ps, (ulong *)size))) {
>> +			printf("'%s' is not a number\n", ps);
>>  			return -1;
> 
> Hmm... would be nice to untangle the duplicated code path rather than add
> more stuff to both branches.

No problem.  The two branches already duplicated the "str2long" check,
which was why I left it that way, but I can certainly clean it up.


>> @@ -317,7 +327,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>  
>>  		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
>>  		/* skip first two or three arguments, look for offset and size */
>> -		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
>> +		if (arg_off_size(argc - o, argv + o, nand, &off, &size, NULL) != 0)
>>  			return 1;
>>  
>>  		memset(&opts, 0, sizeof(opts));
>> @@ -378,8 +388,18 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>  
>>  		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
>>  		printf("\nNAND %s: ", read ? "read" : "write");
>> -		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
>> +		if (read && arg_off_size(argc - 3, argv + 3, nand, &off, &size, NULL) != 0)
>>  			return 1;
>> +		else if (!read) {
>> +			int plussed = 0;
>> +			if (arg_off_size(argc - 3, argv + 3, nand, &off, &size, &plussed) != 0)
>> +				return 1;
> 
> Why not support plussed for read as well?

"read" isn't strictly necessary, since the existing code permits lengths
that result in page-unaligned reads.

Other operations are a little obvious.  "erase" implicitly extends the
page size already if it needs to, while "unlock" requires page aligned
lengths, and "lock" is whole-chip only, so the concept doesn't apply there.

"write" is the only operation that would ever need to depend on
+{filesize} for which I could think of a non contrived example, so I
stopped there.  That said, would you prefer me to support plussed "read"
or any of the other operations (erase, unlock)?


> 
>> +			if (plussed) {
>> +				int tailsize = size & (nand->writesize - 1);
>> +				memset ((u_char *)addr + size, 0xff, nand->writesize - tailsize);
>> +				size += nand->writesize - tailsize;
> 
> NACK, you cannot write to arbitrary memory beyond the end of the range
> specified.  Allocate a buffer to hold the partial page.

OK.  I expected this, but I wanted to ask a question about it.

Are there actual callers for which this would cause a problem, or is
this intended to make the code future proof?  I couldn't find any, so I
assume that the objection is the latter?  Commands like "tftpboot" and
"loadb" already stomp on memory without allocating it, so I wasn't sure
why the nand commands should be handled differently.

That said, it's not a hard change, so I'll make it.


> Plus, this will append an entire page of padding if the size does happen
> to be page-aligned.

Thanks for the catch.


> 
>> +			}
>> +		}
> 
> Please keep lines under 80 characters.
> 

Will do.  I noticed a few other lines in the file that hadn't, so
figured it wasn't an enforced standard.  It will be corrected.


> -Scott

[U-Boot] [PATCH] Add +size syntax to nand commands to pad lengths to page sizes.

[Scott Wood]

Josh Karabin wrote:
>> Why not support plussed for read as well?
> 
> "read" isn't strictly necessary, since the existing code permits lengths
> that result in page-unaligned reads.

Would be nice to keep the syntax consistent and not error if the user 
does provide a plus, though.

> Other operations are a little obvious.  "erase" implicitly extends the
> page size already if it needs to,

That should probably be changed to only do so if plus is used -- it's 
just as dangerous as implicitly rounding up with write (more so, since 
the blocks are bigger).

> "write" is the only operation that would ever need to depend on
> +{filesize} for which I could think of a non contrived example, so I
> stopped there. 

Erase is also quite likely to use {filesize} -- and I could see it being 
used for load as well.

> That said, would you prefer me to support plussed "read"
> or any of the other operations (erase, unlock)?

Yes.

>>> +			if (plussed) {
>>> +				int tailsize = size & (nand->writesize - 1);
>>> +				memset ((u_char *)addr + size, 0xff, nand->writesize - tailsize);
>>> +				size += nand->writesize - tailsize;
>> NACK, you cannot write to arbitrary memory beyond the end of the range
>> specified.  Allocate a buffer to hold the partial page.
> 
> OK.  I expected this, but I wanted to ask a question about it.
> 
> Are there actual callers for which this would cause a problem, or is
> this intended to make the code future proof?  I couldn't find any, so I
> assume that the objection is the latter?

The caller is the user typing a command, and while most of the time it 
would be harmless, it's unexpected behavior.

> Commands like "tftpboot" and
> "loadb" already stomp on memory without allocating it, so I wasn't sure
> why the nand commands should be handled differently.

Stomping on memory that the user asks to be stomped on is one thing -- 
stomping on a few extra bytes beyond the end of that region is another 
(and if those other commands do that, I'd rather not emulate them).

>>> +			}
>>> +		}
>> Please keep lines under 80 characters.
>>
> 
> Will do.  I noticed a few other lines in the file that hadn't, so
> figured it wasn't an enforced standard.  It will be corrected.

Sometimes it's deliberately overlooked because the wrapped version would 
be too awkward, but most of the time those long lines got in there by 
accident.

-Scott