[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters
Tom Rini
trini at ti.com
Wed Feb 27 15:20:19 CET 2013
-----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 at freescale.com> Signed-off-by: Pantelis
>> Antoniou <panto at antoniou-consulting.com> Signed-off-by: Tom Rini
>> <trini at 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
-----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-----
More information about the U-Boot
mailing list