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

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

[Harvey Chapman]

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@3gfp.com>
---
 common/cmd_nand.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

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) {
+	/* We grab the nand info object here fresh because this is usually
+	 * called after arg_off_size() which can change the value of dev.
+	 */
+	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++;
+		}
+	/* 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);
+}
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;
@@ -524,6 +550,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		int scrub = !strncmp(cmd, "scrub", 5);
 		int spread = 0;
 		int args = 2;
+		int adjust_size = 0;
 		const char *scrub_warn =
 			"Warning: "
 			"scrub option will erase all factory set bad blocks!\n"
@@ -540,8 +567,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				spread = 1;
 			} else if (!strcmp(&cmd[5], ".part")) {
 				args = 1;
+				adjust_size = 1;
 			} else if (!strcmp(&cmd[5], ".chip")) {
 				args = 0;
+				adjust_size = 1;
 			} else {
 				goto usage;
 			}
@@ -560,6 +589,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
 			return 1;
 
+		/* 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);
+		}
+
 		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);
+			}
 			rwsize = size;
 		}
 
-- 
1.7.10.4

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

[Harvey Chapman]

[Slightly off-topic, but I can't find the answer with google]
I changed the From: line in this patch e-mail to the address I use for this list rather than the address I committed with. Will this affect the author line once accepted upstream? That is, should I have left the From: address as it was created by format-patch?

Thanks,
Harvey

On Feb 25, 2013, at 11:40 PM, Harvey Chapman <hchapman-uboot@3gfp.com> 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@3gfp.com>
> ---
> common/cmd_nand.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
> 
> 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) {
> +	/* We grab the nand info object here fresh because this is usually
> +	 * called after arg_off_size() which can change the value of dev.
> +	 */
> +	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++;
> +		}
> +	/* 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);
> +}
> +
> static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> {
> 	int i, ret = 0;
> @@ -524,6 +550,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 		int scrub = !strncmp(cmd, "scrub", 5);
> 		int spread = 0;
> 		int args = 2;
> +		int adjust_size = 0;
> 		const char *scrub_warn =
> 			"Warning: "
> 			"scrub option will erase all factory set bad blocks!\n"
> @@ -540,8 +567,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 				spread = 1;
> 			} else if (!strcmp(&cmd[5], ".part")) {
> 				args = 1;
> +				adjust_size = 1;
> 			} else if (!strcmp(&cmd[5], ".chip")) {
> 				args = 0;
> +				adjust_size = 1;
> 			} else {
> 				goto usage;
> 			}
> @@ -560,6 +589,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
> 			return 1;
> 
> +		/* 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);
> +		}
> +
> 		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);
> +			}
> 			rwsize = size;
> 		}
> 
> -- 
> 1.7.10.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

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

[Harvey Chapman]

On Feb 26, 2013, at 12:22 AM, Harvey Chapman <hchapman-uboot@3gfp.com> wrote:

> [Slightly off-topic, but I can't find the answer with google]
> I changed the From: line in this patch e-mail to the address I use for this list rather than the address I committed with. Will this affect the author line once accepted upstream? That is, should I have left the From: address as it was created by format-patch?

I got an answer via StackOverflow. Re-submitting the patch as a reply (hopefully).

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

[Harvey Chapman]

From: Harvey Chapman <hchapman@3gfp.com>

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@3gfp.com>
---
 common/cmd_nand.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

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) {
+	/* We grab the nand info object here fresh because this is usually
+	 * called after arg_off_size() which can change the value of dev.
+	 */
+	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++;
+		}
+	/* 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);
+}
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;
@@ -524,6 +550,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		int scrub = !strncmp(cmd, "scrub", 5);
 		int spread = 0;
 		int args = 2;
+		int adjust_size = 0;
 		const char *scrub_warn =
 			"Warning: "
 			"scrub option will erase all factory set bad blocks!\n"
@@ -540,8 +567,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				spread = 1;
 			} else if (!strcmp(&cmd[5], ".part")) {
 				args = 1;
+				adjust_size = 1;
 			} else if (!strcmp(&cmd[5], ".chip")) {
 				args = 0;
+				adjust_size = 1;
 			} else {
 				goto usage;
 			}
@@ -560,6 +589,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
 			return 1;
 
+		/* 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);
+		}
+
 		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);
+			}
 			rwsize = size;
 		}
 
-- 
1.7.10.4

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

[Harvey Chapman]

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@3gfp.com>
---
 common/cmd_nand.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

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) {
+	/* We grab the nand info object here fresh because this is usually
+	 * called after arg_off_size() which can change the value of dev.
+	 */
+	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++;
+		}
+	/* 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);
+}
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;
@@ -524,6 +550,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		int scrub = !strncmp(cmd, "scrub", 5);
 		int spread = 0;
 		int args = 2;
+		int adjust_size = 0;
 		const char *scrub_warn =
 			"Warning: "
 			"scrub option will erase all factory set bad blocks!\n"
@@ -540,8 +567,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				spread = 1;
 			} else if (!strcmp(&cmd[5], ".part")) {
 				args = 1;
+				adjust_size = 1;
 			} else if (!strcmp(&cmd[5], ".chip")) {
 				args = 0;
+				adjust_size = 1;
 			} else {
 				goto usage;
 			}
@@ -560,6 +589,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
 			return 1;
 
+		/* 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);
+		}
+
 		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);
+			}
 			rwsize = size;
 		}
 
-- 
1.7.10.4

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

[Harvey Chapman]

Sorry for all of the e-mails while I fumble with git send-email.

On Feb 26, 2013, at 12:43 AM, Harvey Chapman <hchapman@3gfp.com> 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@3gfp.com>
> ---
> common/cmd_nand.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
> 
> 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) {
> +	/* We grab the nand info object here fresh because this is usually
> +	 * called after arg_off_size() which can change the value of dev.
> +	 */
> +	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++;
> +		}
> +	/* 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);
> +}
> +
> static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> {
> 	int i, ret = 0;
> @@ -524,6 +550,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 		int scrub = !strncmp(cmd, "scrub", 5);
> 		int spread = 0;
> 		int args = 2;
> +		int adjust_size = 0;
> 		const char *scrub_warn =
> 			"Warning: "
> 			"scrub option will erase all factory set bad blocks!\n"
> @@ -540,8 +567,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 				spread = 1;
> 			} else if (!strcmp(&cmd[5], ".part")) {
> 				args = 1;
> +				adjust_size = 1;
> 			} else if (!strcmp(&cmd[5], ".chip")) {
> 				args = 0;
> +				adjust_size = 1;
> 			} else {
> 				goto usage;
> 			}
> @@ -560,6 +589,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
> 			return 1;
> 
> +		/* 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);
> +		}
> +
> 		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);
> +			}
> 			rwsize = size;
> 		}
> 
> -- 
> 1.7.10.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[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@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

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

[Harvey Chapman]

On Feb 26, 2013, at 8:42 PM, Scott Wood <scottwood@freescale.com> wrote:

> On 02/25/2013 11:43:48 PM, Harvey Chapman wrote:
> 
> Looks OK except for style issues:

Will do.

>> +	/* 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.

I was trying to match the file. Every other multi-line comment in that file has the asterisks aligned.

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

I misunderstood you before. I thought you wanted them around the if body. Got it.

>> +	/* 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.

Ok.

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

Actually, it does give a warning that flies by and is buried inside the compile log. Then, it'll let you run and walk on memory. Ask me how I know. :)

I don't have a special love for that comment; I'm ok to remove it.

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

will do.

Thanks,
Harvey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1089 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130226/2b592bfa/attachment.bin>

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

[Harvey Chapman]

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@3gfp.com>
---
 common/cmd_nand.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 495610c..67d9b59 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -428,6 +428,31 @@ static int raw_access(nand_info_t *nand, ulong addr, loff_t off, ulong count,
 	return ret;
 }
 
+/* Adjust a chip/partition size down for bad blocks so we don't
+ * read/write/erase past the end of a chip/partition by accident.
+ */
+static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev)
+{
+	/* We grab the nand info object here fresh because this is usually
+	 * called after arg_off_size() which can change the value of dev.
+	 */
+	nand_info_t *nand = &nand_info[dev];
+	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++;
+	}
+	/* 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);
+	}
+}
+
 static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i, ret = 0;
@@ -524,6 +549,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		int scrub = !strncmp(cmd, "scrub", 5);
 		int spread = 0;
 		int args = 2;
+		int adjust_size = 0;
 		const char *scrub_warn =
 			"Warning: "
 			"scrub option will erase all factory set bad blocks!\n"
@@ -540,8 +566,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 				spread = 1;
 			} else if (!strcmp(&cmd[5], ".part")) {
 				args = 1;
+				adjust_size = 1;
 			} else if (!strcmp(&cmd[5], ".chip")) {
 				args = 0;
+				adjust_size = 1;
 			} else {
 				goto usage;
 			}
@@ -560,6 +588,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (arg_off_size(argc - o, argv + o, &dev, &off, &size) != 0)
 			return 1;
 
+		/* size is unspecified */
+		if (adjust_size && !scrub)
+			adjust_size_for_badblocks(&size, off, dev);
+
 		nand = &nand_info[dev];
 
 		memset(&opts, 0, sizeof(opts));
@@ -644,6 +676,9 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 						&off, &size) != 0)
 				return 1;
 
+			/* size is unspecified */
+			if (argc < 5)
+				adjust_size_for_badblocks(&rwsize, off, dev);
 			rwsize = size;
 		}
 
-- 
1.7.10.4

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

[Scott Wood]

On 02/26/2013 09:04:22 PM, Harvey Chapman wrote:
> On Feb 26, 2013, at 8:42 PM, Scott Wood <scottwood@freescale.com>  
> wrote:
> > 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.
> 
> Actually, it does give a warning that flies by and is buried inside  
> the compile log. Then, it'll let you run and walk on memory. Ask me  
> how I know. :)

Warnings are easier to spot if you pass "-s" to make (or use MAKEALL  
which does this).

-Scott

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

[Scott Wood]

On Tue, Feb 26, 2013 at 05:21:27PM -0000, 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@3gfp.com>
> 
> ---
> common/cmd_nand.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)

Applied to u-boot-nand-flash...

> +			/* size is unspecified */
> +			if (argc < 5)
> +				adjust_size_for_badblocks(&rwsize, off, dev);

...after fixing this to be &size rather than &rwsize.

-Scott

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

[Scott Wood]

On 05/22/2013 04:36:41 PM, Scott Wood wrote:
> On Tue, Feb 26, 2013 at 05:21:27PM -0000, 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@3gfp.com>
> >
> > ---
> > common/cmd_nand.c |   35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> 
> Applied to u-boot-nand-flash...
> 
> > +			/* size is unspecified */
> > +			if (argc < 5)
> > +				adjust_size_for_badblocks(&rwsize, off,  
> dev);
> 
> ...after fixing this to be &size rather than &rwsize.

...and then noticed that the next patch in my queue was a resent  
version of this with that fixed. :-P

-Scott