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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
