Re: [U-Boot] [PATCH] Add a + prefix form to nand commands.

On Mon, May 18, 2009 at 11:58:45PM -0400, Josh Karabin wrote:

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, size_t *headsize, size_t *tailsize)

{ int idx = nand_curr_device;

• char *ps = argv[1];
• size_t max_tailsize;

#if defined(CONFIG_CMD_MTDPARTS) struct mtd_device *dev; struct part_info *part; u8 pnum; +#endif

• max_tailsize = *tailsize;

Should allow tailsize to be NULL if the caller doesn't want to support +.

• if (ps != argv[1]) {

• *tailsize = *size & (max_tailsize - 1);
  

• *headsize = *size - *tailsize;
  

• if (*tailsize)
  

• 	*size = *headsize + max_tailsize;
  

If max_tailsize is zero (doesn't happen currently, but...), this will assign everything to tailsize rather than headsize.

• size_t tailsize = 0;
• size_t headsize = 0;

Does GCC complain if you remove those initializations? They don't seem necessary, and suggest that something is actually using those default values.

@@ -346,7 +369,18 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) return -1; } }

• ret = nand_erase_opts(nand, &opts);
  

• if (headsize)
  

• 	ret = nand_erase_opts(nand, &opts);
  

• else
  

• 	ret = 0;
  

• if (!ret && tailsize) {
  

• 	opts.offset = off + headsize;
  

• 	opts.length = nand->erasesize;
  

• 	ret = nand_erase_opts(nand, &opts);
  

• }
  

Why not just set opts.length = size, rounded up if the plus is there? I'm not sure the headsize/tailsize split makes sense for erase.

I assume that having offset be unaligned is still an error.

if (!s || !strcmp(s, ".jffs2") ||
    !strcmp(s, ".e") || !strcmp(s, ".i")) {
	if (read)

• 		ret = nand_read_skip_bad(nand, off, &size,
  

• 					 (u_char *)addr);
  

• 	else
  

• 		ret = nand_write_skip_bad(nand, off, &size,
  

• 					  (u_char *)addr);
  

• 		ret = nand_read_skip_bad(nand, off, &headsize,
  

• 					(u_char *)addr);
  

• 	else {
  

Looks like you don't handle the tail when reading -- only when writing.

• 		if (headsize)
  

• 			ret = nand_write_skip_bad(nand, off, 
  

• 						&headsize,
  

• 						(u_char *)addr);
  

• 		else
  

• 			ret = 0;
  

• 		if (!ret && tailsize) {
  

• 			size_t pagesize = nand->writesize;
  

• 			u_char *tailpage;
  

• 			tailpage = malloc(pagesize);
  

• 			if (tailpage) {
  

• 				memcpy(tailpage, 
  

• 					(u_char *)addr + 
  

• 					headsize, tailsize);
  

• 				memset(tailpage + tailsize,
  

• 					  0xff,
  

• 					  pagesize - tailsize);
  

• 				ret = nand_write_skip_bad(
  

• 					nand, 
  

• 					  off + headsize,
  

• 					&pagesize,
  

• 					tailpage);
  

Please factor things into their own functions when indentation and if/else complexity reaches this level.

• 			}
  

• 			else
  

• 				ret = 1;
  

If one half of the if/else has braces (which should be on the same line as the "else"), the other half should have them too.

If malloc fails, we should let the user know that rather than opaquely fail.

• 		}
  

• 	}
  

  } else if (!strcmp(s, ".oob")) { /* out-of-band data */ mtd_oob_ops_t ops = { .oobbuf = (u8 *)addr,

• 		.ooblen = size,
  

• 		.ooblen = size + tailsize,
  

Doesn't "size" include "tailsize"? Why would there be any rounding involved with OOB (other than perhaps 16-bit alignment for such chips)?

• if (!nand_unlock(nand, off, size)) {
  

• if (headsize)
  

• 	ret = nand_unlock(nand, off, headsize);
  

• else
  

• 	ret = 0;
  

• if (!ret && tailsize)
  

• 	ret = nand_unlock(nand, off+headsize, nand->erasesize);
  

Same comment as erase.

-Scott