From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Fri, 8 Feb 2013 17:34:17 -0600
Subject: [U-Boot] U-boot nand bug, read.part should fail
In-Reply-To: <AE2124FA-64A9-49A4-A02A-ECCAB85BBA6F@3gfp.com> (from
	hchapman-uboot@3gfp.com on Fri Feb  8 10:44:30 2013)
Message-ID: <1360366457.22064.12@snotra>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 02/08/2013 10:44:30 AM, Harvey Chapman wrote:
> Eh, I shouldn't post code that quickly? Try this:
> 
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -621,60 +621,80 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
> 
>  		nand = &nand_info[dev];
> 
>  		s = strchr(cmd, '.');
> 
>  		if (s && !strcmp(s, ".raw")) {
>  			raw = 1;
> 
>  			if (arg_off(argv[3], &dev, &off, &size))
>  				return 1;
> 
>  			if (argc > 4 && !str2long(argv[4], &pagecount))  
> {
>  				printf("'%s' is not a number\n",  
> argv[4]);
>  				return 1;
>  			}
> 
>  			if (pagecount * nand->writesize > size) {
>  				puts("Size exceeds partition or device  
> limit\n");
>  				return -1;
>  			}
> 
>  			rwsize = pagecount * (nand->writesize +  
> nand->oobsize);
>  		} else {
>  			if (arg_off_size(argc - 3, argv + 3, &dev,
>  						&off, &size) != 0)
>  				return 1;
> 
>  			rwsize = size;
>  		}
> 
> +                /* If no size was given, it has been calculated for  
> us as
> +                 * the remainder of the chip or partition from  
> offset. Adjust
> +                 * down for bad blocks, if necessary.
> +                 */

This should go inside the "not raw" path of the previous "if" statement.

Please use tabs to indent.

> +                if (argc < 5) {
> +                        nand_info_t *nand = &nand_info[dev];

We already have "nand" in this context.

> +                        int size = rwsize;

We already have "size" -- and you don't even seem to use it.

> +                        int maxoffset = off + rwsize;
> +                        int offset = off;

Offsets do not fit in "int".  Use loff_t.

> +                        int badblocks = 0;
> +                        for (offset = off; offset < maxoffset;  
> offset += nand->erasesize)
> +                                if (nand_block_isbad(nand, offset))
> +                                        badblocks++;

Braces around multi-line "if" bodies (even if a single statement).

Please leave a blank line after the variable declaration block.

Maybe move this to its own function (having both "offset" and "off" is  
unpleasant, plus this function is way too big already).

We need this adjustment to be made to erase.part/chip as well.

-Scott