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

[Josh Karabin <gkarabin@vocollect.com>]

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