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

[Scott Wood <scottwood@freescale.com>]

On 02/28/2013 01:09:05 PM, 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 count not just complete blocks used but partial ones as
> well.  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.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
> Changes in v3:
> - Reworked skip_check_len changes to just add accounting for *used to
>   the logic.
> - Allow for actual to be NULL in nand_(read|write)_skip_bad, only DFU
>   calls this with a non-NULL parameter.  Make sure the comments for  
> both
>   functions explain the parameters and their behavior.
> - Other style changes requested by Scott.
> - As nand_(write|read)_skip_bad passes back just a used length now.
> 
> Changes in v2:
> - NAND skip_check_len changes reworked to allow
>   nand_(read|write)_skip_bad to return this information to the caller.
> 
>  common/cmd_nand.c            |   56  
> +++++++++++++++++++----------------
>  common/env_nand.c            |    3 +-
>  drivers/mtd/nand/nand_util.c |   67  
> +++++++++++++++++++++++++++++++++++++-----
>  include/nand.h               |    4 +--
>  4 files changed, 93 insertions(+), 37 deletions(-)

Looks mostly good, just some minor issues:

> -	if (*size > maxsize) {
> -		puts("Size exceeds partition or device limit\n");
> -		return -1;
> -	}
> -

I assume you're removing this because you rely on the read/write  
functions printing the error... what about other users of this such as  
erase, lock, etc?

> @@ -476,20 +483,30 @@ 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.  Due to bad blocks we may not be able to
> + * perform the requested write.  In the case where the write would
> + * extend beyond the end of the NAND device, both length and actual  
> (if
> + * not NULL) are set to 0.  In the case where the write would extend
> + * beyond the limit we are passed, length is set to 0 and actual is  
> set
> + * to the required length.
>   *
>   * @param nand  	NAND device
>   * @param offset	offset in flash
>   * @param length	buffer length
> + * @param actual	set to size required to write length worth of
> + *			buffer or 0, if not NULL

s/or 0/or 0 on error/
or
s/or 0/in case of success/

The read function doesn't have the "or 0" comment...

> + * @param lim		maximum size that length may be in  
> order to not
> + *			exceed the buffer

s/that length may be/that actual may be/

>   * @param buffer        buffer to read from
>   * @param flags		flags modifying the behaviour of the  
> write to NAND
>   * @return		0 in case of success
>   */
>  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;
> +	size_t used_for_write = 0;
>  	u_char *p_buffer = buffer;
>  	int need_skip;
> 
> @@ -526,16 +543,28 @@ 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;
> +		if (actual)
> +			*actual = 0;
>  		return -EINVAL;
>  	}

Again, what about the returns in the WITH_YAFFS_OOB section?  Or if we  
document that "actual" is undefined for error returns we can not worry  
about this.

-Scott