Re: [U-Boot] U-boot nand bug, read.part should fail
On Feb 7, 2013, at 5:22 PM, Scott Wood scottw...@freescale.com wrote:
It's fine until you get a bad block in the partition, and you end up
accessing the first block of the next partition (or getting Attempt to
read/write outside the flash area if it's the last partition).
Of course, fixing partition/chip accesses to account for this when
determining size would be even better. :-)
Something like 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,78 @@ 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.
+ */
+if (argc 4) {
+nand_info_t *nand = nand_info[dev];
+int maxsize = rwsize;
+int offset = off;
+for (; offset maxsize; offset += nand-erasesize)
+if (nand_block_isbad(nand, offset))
+rwsize -= nand-erasesize;
+
+if (rwsize != maxsize)
+printf(\nsize adjusted to %d (%d bad
blocks)\n,
+ rwsize,
+ (maxsize - rwsize) / nand-erasesize);
+}
+
if (!s || !strcmp(s, .jffs2) ||
!strcmp(s, .e) || !strcmp(s, .i) || !strcmp(s,
.part)) {
if (read)
ret = nand_read_skip_bad(nand, off, rwsize,
(u_char *)addr);
else
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr, 0);
#ifdef CONFIG_CMD_NAND_TRIMFFS
} else if (!strcmp(s, .trimffs)) {
if (read) {
printf(Unknown nand command suffix '%s'\n, s);
return 1;
}
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr,
WITH_DROP_FFS);
#endif
#ifdef CONFIG_CMD_NAND_YAFFS
} else if (!strcmp(s, .yaffs)) {
if (read) {
printf(Unknown nand command suffix '%s'.\n,
s);
return 1;
}
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr,
WITH_INLINE_OOB);
#endif
} else if (!strcmp(s, .oob)) {
/* out-of-band data */
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-boot nand bug, read.part should fail
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.
+ */
+if (argc 5) {
+nand_info_t *nand = nand_info[dev];
+int size = rwsize;
+int maxoffset = off + rwsize;
+int offset = off;
+int badblocks = 0;
+for (offset = off; offset maxoffset; offset +=
nand-erasesize)
+if (nand_block_isbad(nand, offset))
+badblocks++;
+if (badblocks) {
+rwsize -= badblocks * nand-erasesize;
+printf(size adjusted to 0x%llx (%d bad
blocks)\n,
+ (unsigned long long)rwsize, badblocks);
+}
+}
+
if (!s || !strcmp(s, .jffs2) ||
!strcmp(s, .e) || !strcmp(s, .i) || !strcmp(s,
.part)) {
if (read)
ret = nand_read_skip_bad(nand, off, rwsize,
(u_char *)addr);
else
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr, 0);
#ifdef CONFIG_CMD_NAND_TRIMFFS
} else if (!strcmp(s, .trimffs)) {
if (read) {
printf(Unknown nand command suffix '%s'\n, s);
return 1;
}
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr,
WITH_DROP_FFS);
#endif
#ifdef CONFIG_CMD_NAND_YAFFS
} else if (!strcmp(s, .yaffs)) {
if (read) {
printf(Unknown nand command suffix '%s'.\n,
s);
return 1;
}
ret = nand_write_skip_bad(nand, off, rwsize,
(u_char *)addr,
WITH_INLINE_OOB);
#endif
} else if (!strcmp(s, .oob)) {
/* out-of-band data */
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-boot nand bug, read.part should fail
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-boot nand bug, read.part should fail
On Feb 8, 2013, at 6:34 PM, Scott Wood scottw...@freescale.com wrote: On 02/08/2013 10:44:30 AM, Harvey Chapman wrote: This should go inside the not raw path of the previous if statement. Please use tabs to indent. We already have nand in this context. We already have size -- and you don't even seem to use it. Offsets do not fit in int. Use loff_t. 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. Thanks for the feedback. This was just a proof of concept. I'll make the changes and submit the patch more formally. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-boot nand bug, read.part should fail
On 02/07/2013 04:13:55 PM, Harvey Chapman wrote: [ I started this conversation off-list before I joined the list. ] The idea is to add .part as a valid command suffix to nand read/write so it would match nand erase.part. The code to implement it makes nand read.part act identically to nand read. On Feb 7, 2013, at 4:59 PM, Scott Wood scottw...@freescale.com wrote: In fact, I think erase should be modified to deprecate erase.part and make erase work like read does now. Erase used to work like read does. I deliberately changed it so that typos (e.g. nand erase $partition $fliesize) don't end up erasing your entire partition or chip. Ah, then maybe we should add .part to nand read for consistency? I'm ok either way. That would get messy because it would be orthogonal to other suffixes. Reading too much is not as harmful as Nothing would change other than do_nand() would treat nand read and nand read.part identically. The only reason to add .part/.chip is if the unsuffixed versions no longer operate on entire partitions/chips. erasing too much. Writing too much can be bad, though. Perhaps we should just eliminate the ability to do reads/writes without explicit size (it already has problems with the size needing adjustment due to bad blocks). I liked that I didn't have to specify the size. It's fine until you get a bad block in the partition, and you end up accessing the first block of the next partition (or getting Attempt to read/write outside the flash area if it's the last partition). Of course, fixing partition/chip accesses to account for this when determining size would be even better. :-) -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-boot nand bug, read.part should fail
[ I started this conversation off-list before I joined the list. ] The idea is to add .part as a valid command suffix to nand read/write so it would match nand erase.part. The code to implement it makes nand read.part act identically to nand read. On Feb 7, 2013, at 4:59 PM, Scott Wood scottw...@freescale.com wrote: In fact, I think erase should be modified to deprecate erase.part and make erase work like read does now. Erase used to work like read does. I deliberately changed it so that typos (e.g. nand erase $partition $fliesize) don't end up erasing your entire partition or chip. Ah, then maybe we should add .part to nand read for consistency? I'm ok either way. That would get messy because it would be orthogonal to other suffixes. Reading too much is not as harmful as Nothing would change other than do_nand() would treat nand read and nand read.part identically. erasing too much. Writing too much can be bad, though. Perhaps we should just eliminate the ability to do reads/writes without explicit size (it already has problems with the size needing adjustment due to bad blocks). I liked that I didn't have to specify the size. Converting from numbers to partition names led me to find the bug in the nand read code. Using partition names makes it much easier to work with u-boot since I don't have to count 0s every time I type an address or size. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
