[U-Boot] [PATCH] nand: adjust erase/read/write partition/chip size for bad blocks

[Scott Wood]

  On 02/25/2013 11:43:48 PM, Harvey Chapman wrote:
> Adjust the sizes calculated for whole partition/chip operations by
> removing the size of bad blocks so we don't try to erase/read/write
> past a partition/chip boundary.
> 
> Signed-off-by: Harvey Chapman <hchapman at 3gfp.com>
> ---
>  common/cmd_nand.c |   51  
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)

Looks OK except for style issues:

> 
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 495610c..657ea23 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -428,6 +428,32 @@ static int raw_access(nand_info_t *nand, ulong  
> addr, loff_t off, ulong count,
>  	return ret;
>  }
> 
> +static int adjust_size_for_badblocks(loff_t *size, loff_t offset,  
> int dev) {

Braces go on their own line for function definitions.

> +	/* We grab the nand info object here fresh because this is  
> usually
> +	 * called after arg_off_size() which can change the value of  
> dev.
> +	 */

/*
  * U-Boot multiline
  * brace style is like this
  */

...in general, U-Boot follows Linux code style.

> +	nand_info_t *nand = &nand_info[dev];
> +	loff_t original_size = *size;
> +	loff_t maxoffset = offset + *size;
> +	int badblocks = 0;
> +
> +	/* count badblocks in NAND from offset to offset + size */
> +	for (; offset < maxoffset; offset += nand->erasesize)
> +		if (nand_block_isbad(nand, offset)) {
> +			badblocks++;
> +		}

Need braces around multi-line statements.

> +	/* adjust size if any bad blocks found */
> +	if (badblocks) {
> +		*size -= badblocks * nand->erasesize;
> +		printf("size adjusted to 0x%llx (%d bad blocks)\n",
> +		       (unsigned long long)*size, badblocks);
> +	}
> +	/* return size adjusted as a positive value so callers
> +	 * can use the return code to determine if anything happened
> +	 */
> +	return (original_size - *size);

Unnecessary parens.

Do we have any callers that care about the return code?  If not, don't  
bother with it.  This is an internal static function, not a public  
API.  It's easy to change later if we need to.

> +		/* The size for erase.part and erase.chip has been  
> calculated
> +		 * for us as the remainder of the chip/partition from  
> offset.
> +		 * Adjust down for bad blocks, if necessary, so we don't
> +		 * erase past the end of the chip/partition by accident.
> +		 */
> +		if (adjust_size && !scrub) {
> +			adjust_size_for_badblocks(&size, off, dev);
> +		}
> +

No braces around single-line if-bodies.

>  		nand = &nand_info[dev];
> 
>  		memset(&opts, 0, sizeof(opts));
> @@ -644,6 +682,19 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,  
> int argc, char * const argv[])
>  						&off, &size) != 0)
>  				return 1;
> 
> +			/* If no size was given, it has been calculated  
> for us as
> +			 * the remainder of the chip/partition from  
> offset. Adjust
> +			 * down for bad blocks, if necessary, so we  
> don't
> +			 * read/write past the end of the partition by  
> accident.
> +			 *
> +			 * nand read addr part size     "size" is arg 5
> +			 */
> +			if (argc < 5) {
> +				/* Don't try to use rwsize here, it's  
> not the
> +				 * right type
> +				 */
> +				adjust_size_for_badblocks(&size, off,  
> dev);
> +			}

No need to be quite so verbose in the comments.  If someone tries to  
change "size" to "rwsize" the compiler will complain about the type  
mismatch.
As for the other comment, the function name "adjust_size_for_badblocks"  
explains what's going on well enough IMHO.  At most a single comment of  
/* size is unspecified */ to describe the if block.  At least put the  
explanation on the adjust_size_for_badblocks() function rather than  
repeat it for read/write and erase.

-Scott