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

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