From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Wed, 27 Feb 2013 09:20:19 -0500 Subject: [U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters In-Reply-To: <1361930926.12570.19@snotra> References: <1361930926.12570.19@snotra> Message-ID: <512E1623.5080509@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/26/2013 09:08 PM, Scott Wood wrote: > 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. OK, I'll call it maxoff (because it's the max offset within the NAND for a given partition, or end of the NAND). >> 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. OK, bug here then I missed. Making sure both *size and *maxoff are set when get_part doesn't set them. [snip] >> - 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. Will make it so. >> 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. Oops. > 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. First, the big changes to the function are so that we track (and report back) the correct amount of a partial block we would use for the request. I'll see if I can't do something like loff_t offset, *something else. [snip] > Traditionally U-Boot style has been to use braces on both sides of > an if/else if one side needs them. OK, fixing. > >> - 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? They are as well defined as what happens with length. If we say we can't write, we set both to 0 and return an error. I'll take this as a request to expand the comment and do so. >> * * @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. I think I follow, I'll re-word. >> 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. I guess I sacrificed continence for clarity here. The "issue" is that we walk blocks from *offset until we've fit in length. Another way of doing this and not muddying the type-waters is starting to stare me in the face now, so I'll go see about re-working things. Thanks! - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRLhYiAAoJENk4IS6UOR1WxMcP/jQKxJlm6/f24eF/RQ3Q2l/B RfCu67cdIg3MKX/B9RHyXRIRRCsDwWq/PKFRWXdOMKeirhposKqfDxc8r/s/MpiP GL+x4hiunH6q+Ct4ZkVg9tBY90+F+UQc7cHsxjAQmHK3gjR+0ODdmk7iHg0MzNEB 4MhASBYfSnfLY7yk0/+Lq4UktULzyalq1aR653DvFhX1Gn9bso4eUlXRjEen2+22 nEfIGHtKQV1CNK6rGWFgyNIjvjFZqtHtz/gjFDEqr/xPynCROXm+dF7LNnSlVF+4 7SzxRdEHqTsBj/1H6sEZsw3NzA56aFLlrsUUiv5cbeaHDsvOCDvqxoDfqXg9U1t3 +5Jw8ctdiDk1uh4ARFKow3DmGKW/7pTDmQNz/zLNUusXOY6KhsPEraDSZeS62hnM RpM5fWnMivFLOGcif5ELN6MHPGntTJ+J39L8ABRNPouFV6BFHNqHqC4+7PrJ4BsG ALRNdcrk1IpglehcxJTBd/GliKkT8GbMQv9chlZOAO+t/ND20SX1HTDElHH1reEf cXYRjt/UUW43HQa5CNvM1pq4uSvBkWS/2aGR0Wzrs/ZoLcbssW3xIBdTcTgRWSMG NbM69czKVRUS4gDt451akASONXbpnm91CKLsg31jYIGphpw4WBr+IT+VZGsl1pgC Bz73jBNxHA4VYK3vJJ9W =gAJe -----END PGP SIGNATURE-----