-----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 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.
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