[U-Boot] [PATCH v2 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini <trini@ti.com>]

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