From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Thu, 28 Feb 2013 19:37:51 -0600 Subject: [U-Boot] [PATCH v3 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters In-Reply-To: <1362078548-24308-2-git-send-email-trini@ti.com> (from trini@ti.com on Thu Feb 28 13:09:05 2013) Message-ID: <1362101871.25315.20@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/28/2013 01:09:05 PM, 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 count not just complete blocks used but partial ones as > well. 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. > > Signed-off-by: Pantelis Antoniou > Signed-off-by: Tom Rini > --- > Changes in v3: > - Reworked skip_check_len changes to just add accounting for *used to > the logic. > - Allow for actual to be NULL in nand_(read|write)_skip_bad, only DFU > calls this with a non-NULL parameter. Make sure the comments for > both > functions explain the parameters and their behavior. > - Other style changes requested by Scott. > - As nand_(write|read)_skip_bad passes back just a used length now. > > Changes in v2: > - NAND skip_check_len changes reworked to allow > nand_(read|write)_skip_bad to return this information to the caller. > > common/cmd_nand.c | 56 > +++++++++++++++++++---------------- > common/env_nand.c | 3 +- > drivers/mtd/nand/nand_util.c | 67 > +++++++++++++++++++++++++++++++++++++----- > include/nand.h | 4 +-- > 4 files changed, 93 insertions(+), 37 deletions(-) Looks mostly good, just some minor issues: > - if (*size > maxsize) { > - puts("Size exceeds partition or device limit\n"); > - return -1; > - } > - I assume you're removing this because you rely on the read/write functions printing the error... what about other users of this such as erase, lock, etc? > @@ -476,20 +483,30 @@ 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. Due to bad blocks we may not be able to > + * perform the requested write. In the case where the write would > + * extend beyond the end of the NAND device, both length and actual > (if > + * not NULL) are set to 0. In the case where the write would extend > + * beyond the limit we are passed, length is set to 0 and actual is > set > + * to the required length. > * > * @param nand NAND device > * @param offset offset in flash > * @param length buffer length > + * @param actual set to size required to write length worth of > + * buffer or 0, if not NULL s/or 0/or 0 on error/ or s/or 0/in case of success/ The read function doesn't have the "or 0" comment... > + * @param lim maximum size that length may be in > order to not > + * exceed the buffer s/that length may be/that actual may be/ > * @param buffer buffer to read from > * @param flags flags modifying the behaviour of the > write to NAND > * @return 0 in case of success > */ > 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; > + size_t used_for_write = 0; > u_char *p_buffer = buffer; > int need_skip; > > @@ -526,16 +543,28 @@ 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; > + if (actual) > + *actual = 0; > return -EINVAL; > } Again, what about the returns in the WITH_YAFFS_OOB section? Or if we document that "actual" is undefined for error returns we can not worry about this. -Scott