From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Tue, 26 Feb 2013 20:08:46 -0600 Subject: [U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters In-Reply-To: <1361894171-3379-2-git-send-email-trini@ti.com> (from trini@ti.com on Tue Feb 26 09:56:08 2013) Message-ID: <1361930926.12570.19@snotra> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > Signed-off-by: Pantelis Antoniou > Signed-off-by: Tom Rini > --- > 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