Re: [U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

On 02/26/2013 09:56:08 AM, Tom Rini wrote:

We make these two functions take a size_t pointer to how much space was used on NAND to read or write the buffer (when reads/writes happen) so that bad blocks can be accounted for. We also make them take an loff_t limit on how much data can be read or written. This means that we can now catch the case of when writing to a partition would exceed the partition size due to bad blocks. To do this we also need to make check_skip_len care about total actual size used rather than block_size chunks used. All callers of nand_(read|write)_skip_bad are adjusted to call these with the most sensible limits available.

The changes were started by Pantelis and finished by Tom.

Cc: Scott Wood scottwood@freescale.com Signed-off-by: Pantelis Antoniou panto@antoniou-consulting.com Signed-off-by: Tom Rini trini@ti.com

---

common/cmd_nand.c | 61 +++++++++++++++++++++-------------------- common/env_nand.c | 5 ++-- drivers/mtd/nand/nand_util.c | 62 +++++++++++++++++++++++++++++++----------- include/nand.h | 4 +-- 4 files changed, 82 insertions(+), 50 deletions(-)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c index 1568594..e091e02 100644 --- a/common/cmd_nand.c +++ b/common/cmd_nand.c @@ -137,7 +137,8 @@ static inline int str2long(const char *p, ulong *num) return *p != '\0' && *endptr == '\0'; }

-static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size) +static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size,

• loff_t *maxsize)
  

{ #ifdef CONFIG_CMD_MTDPARTS struct mtd_device *dev; @@ -160,6 +161,7 @@ static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size)

*off = part->offset; *size = part->size;

• *maxsize = part->offset + part->size; *idx = dev->id->num;

The name "maxsize" suggests that it's a size, not a position.

ret = set_dev(*idx); @@ -173,10 +175,11 @@ static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size) #endif }

-static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *maxsize) +static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *size,

• loff_t *maxsize)
  

{ if (!str2off(arg, off))

• return get_part(arg, idx, off, maxsize);
  

• return get_part(arg, idx, off, size, maxsize);
  
  

  if (*off >= nand_info[*idx].size) { puts("Offset exceeds device limit\n");

...and in the get_part case arg-off is still treating maxsize as a size.

@@ -605,7 +601,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }

if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) == 0) {

• size_t rwsize;
  

• size_t rwsize, actual;
  

  ulong pagecount = 1; int read; int raw;

@@ -625,7 +621,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (s && !strcmp(s, ".raw")) { raw = 1;

• 	if (arg_off(argv[3], &dev, &off, &size))
  

• 	if (arg_off(argv[3], &dev, &off, &size,  
  

&maxsize)) return 1;

	if (argc > 4 && !str2long(argv[4], &pagecount))  

{ @@ -641,7 +637,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) rwsize = pagecount * (nand->writesize + nand->oobsize); } else { if (arg_off_size(argc - 3, argv + 3, &dev,

• 				&off, &size) != 0)
  

• 				&off, &size, &maxsize)  
  

!= 0) return 1;

	rwsize = size;

@@ -651,9 +647,11 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) !strcmp(s, ".e") || !strcmp(s, ".i")) { if (read) ret = nand_read_skip_bad(nand, off, &rwsize,

• 					 &actual,  
  

maxsize, (u_char *)addr); else ret = nand_write_skip_bad(nand, off, &rwsize,

• 					  &actual,  
  

maxsize, (u_char *)addr, 0); #ifdef CONFIG_CMD_NAND_TRIMFFS } else if (!strcmp(s, ".trimffs")) { @@ -661,8 +659,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) printf("Unknown nand command suffix '%s'\n", s); return 1; }

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

• 				(u_char *)addr,
  

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

&actual,

• 				maxsize, (u_char *)addr,
  			WITH_DROP_FFS);
  

Do we care about actual here? Let the skip_bad functions accept NULL if the caller doesn't care.

diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c index 2ba0c5e..5ed5b1d 100644 --- a/drivers/mtd/nand/nand_util.c +++ b/drivers/mtd/nand/nand_util.c @@ -397,34 +397,38 @@ int nand_unlock(struct mtd_info *mtd, loff_t start, size_t length,

• blocks fits into device
• @param nand NAND device

• • @param offset offset in flash

• • @param offsetp offset in flash (on exit offset where it's ending)
  • @param length image length
  • @return 0 if the image fits and there are no bad blocks

  •     1 if the image fits, but there are bad blocks
    

  •    -1 if the image does not fit
    

  */

-static int check_skip_len(nand_info_t *nand, loff_t offset, size_t length) +static int check_skip_len(nand_info_t *nand, loff_t *offset, size_t length)

Comment changed "offset" to "offsetp" but code did not.

Can we use a different parameter to return the end offset (or actual size)? That way we don't need the tmp_offset stuff, and there should be fewer changes to this function.

{

• size_t len_excl_bad = 0; int ret = 0;

• while (len_excl_bad < length) {

• while (length > 0) { size_t block_len, block_off; loff_t block_start;

• if (offset >= nand->size)
  

• if (*offset >= nand->size)
  return -1;
  
  

• block_start = offset & ~(loff_t)(nand->erasesize - 1);
  

• block_off = offset & (nand->erasesize - 1);
  

• block_start = *offset & ~(loff_t)(nand->erasesize - 1);
  

• block_off = *offset & (nand->erasesize - 1);
  

  block_len = nand->erasesize - block_off;

• if (!nand_block_isbad(nand, block_start))
  

• 	len_excl_bad += block_len;
  

• else
  

• if (!nand_block_isbad(nand, block_start)) {
  

• 	if (block_len > length) {
  

• 		/* Final chunk is smaller than block. */
  

• 		*offset += length;
  

• 		return ret;
  

• 	} else
  

• 		length -= block_len;
  

• } else
  ret = 1;
  

Traditionally U-Boot style has been to use braces on both sides of an if/else if one side needs them.

• offset += block_len;
  

• *offset += block_len;
  

  }

  return ret;

@@ -459,22 +463,26 @@ static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,

• Write image to NAND flash.
• Blocks that are marked bad are skipped and the is written to the

next

• block instead as long as the image is short enough to fit even

after

• • skipping the bad blocks.

• • skipping the bad blocks. Note that the actual size needed may

exceed

• • both the length and available NAND due to bad blocks.

If that happens, then the function returns failure. Are the contents of "actual" well-defined when the function returns failure?

• @param nand NAND device
• @param offset offset in flash
• @param length buffer length

• • @param actual size required to write length worth of buffer
• • @param lim end location of where data in the

buffer may be written.

• @param buffer buffer to read from
• @param flags flags modifying the behaviour of the

write to NAND

• @return 0 in case of success

*/

Please note which pointer parameters are in/out versus out-only.

int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,

• 	u_char *buffer, int flags)
  

• 	size_t *actual, loff_t lim, u_char *buffer, int  
  

flags) { int rval = 0, blocksize; size_t left_to_write = *length; u_char *p_buffer = buffer; int need_skip;

• loff_t tmp_offset;

#ifdef CONFIG_CMD_NAND_YAFFS if (flags & WITH_YAFFS_OOB) { @@ -509,16 +517,25 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, if ((offset & (nand->writesize - 1)) != 0) { printf("Attempt to write non page-aligned data\n"); *length = 0;

• *actual = 0;
  

  return -EINVAL; }

• need_skip = check_skip_len(nand, offset, *length);

• tmp_offset = offset;
• need_skip = check_skip_len(nand, &tmp_offset, *length);
• *actual = tmp_offset;

More size/offset mismatch with actual. Docs above say it's a size.

-Scott