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

[Scott Wood]

  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.

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

> @@ -605,7 +601,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  	}
> 
>  	if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) ==  
> 0) {
> -		size_t rwsize;
> +		size_t rwsize, actual;
>  		ulong pagecount = 1;
>  		int read;
>  		int raw;
> @@ -625,7 +621,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  		if (s && !strcmp(s, ".raw")) {
>  			raw = 1;
> 
> -			if (arg_off(argv[3], &dev, &off, &size))
> +			if (arg_off(argv[3], &dev, &off, &size,  
> &maxsize))
>  				return 1;
> 
>  			if (argc > 4 && !str2long(argv[4], &pagecount))  
> {
> @@ -641,7 +637,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  			rwsize = pagecount * (nand->writesize +  
> nand->oobsize);
>  		} else {
>  			if (arg_off_size(argc - 3, argv + 3, &dev,
> -						&off, &size) != 0)
> +						&off, &size, &maxsize)  
> != 0)
>  				return 1;
> 
>  			rwsize = size;
> @@ -651,9 +647,11 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  		    !strcmp(s, ".e") || !strcmp(s, ".i")) {
>  			if (read)
>  				ret = nand_read_skip_bad(nand, off,  
> &rwsize,
> +							 &actual,  
> maxsize,
>  							 (u_char  
> *)addr);
>  			else
>  				ret = nand_write_skip_bad(nand, off,  
> &rwsize,
> +							  &actual,  
> maxsize,
>  							  (u_char  
> *)addr, 0);
>  #ifdef CONFIG_CMD_NAND_TRIMFFS
>  		} else if (!strcmp(s, ".trimffs")) {
> @@ -661,8 +659,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  				printf("Unknown nand command suffix  
> '%s'\n", s);
>  				return 1;
>  			}
> -			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.

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

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.

>  {
> -	size_t len_excl_bad = 0;
>  	int ret = 0;
> 
> -	while (len_excl_bad < length) {
> +	while (length > 0) {
>  		size_t block_len, block_off;
>  		loff_t block_start;
> 
> -		if (offset >= nand->size)
> +		if (*offset >= nand->size)
>  			return -1;
> 
> -		block_start = offset & ~(loff_t)(nand->erasesize - 1);
> -		block_off = offset & (nand->erasesize - 1);
> +		block_start = *offset & ~(loff_t)(nand->erasesize - 1);
> +		block_off = *offset & (nand->erasesize - 1);
>  		block_len = nand->erasesize - block_off;
> 
> -		if (!nand_block_isbad(nand, block_start))
> -			len_excl_bad += block_len;
> -		else
> +		if (!nand_block_isbad(nand, block_start)) {
> +			if (block_len > length) {
> +				/* Final chunk is smaller than block. */
> +				*offset += length;
> +				return ret;
> +			} else
> +				length -= block_len;
> +		} else
>  			ret = 1;

Traditionally U-Boot style has been to use braces on both sides of an  
if/else if one side needs them.

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

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

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

-Scott