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

[Scott Wood]

On 02/27/2013 08:20:19 AM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/26/2013 09:08 PM, Scott Wood wrote:
> > 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).

Wouldn't it be less intrusive to just make it actually be the size  
instead of an offset?

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

The comments could use expanding (it doesn't even explain what happens  
to length in the non-error case), but also it looks like there are some  
error paths where actual doesn't get cleared, in the  
CONFIG_CMD_NAND_YAFFS section.

I was also wondering about the case where check_skip_bad() says it  
doesn't fit.  It doesn't return the actual in that case -- it returns  
offset as of when it stopped.  So a caller of  
nand_read|write_skip_bad() would see the same "actual" as if it just  
barely fit.  I'm not sure that this function ever would return an  
actual size that exceeds the available NAND.

-Scott