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; }

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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

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