* [U-Boot] U-boot nand bug, read.part should fail [not found] <1360274360.27002.10@snotra> @ 2013-02-07 22:13 ` Harvey Chapman 2013-02-07 22:22 ` Scott Wood 0 siblings, 1 reply; 6+ messages in thread From: Harvey Chapman @ 2013-02-07 22:13 UTC (permalink / raw) To: u-boot [ 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 <scottwood@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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] U-boot nand bug, read.part should fail 2013-02-07 22:13 ` [U-Boot] U-boot nand bug, read.part should fail Harvey Chapman @ 2013-02-07 22:22 ` Scott Wood 2013-02-08 15:48 ` Harvey Chapman 0 siblings, 1 reply; 6+ messages in thread From: Scott Wood @ 2013-02-07 22:22 UTC (permalink / raw) To: u-boot 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 <scottwood@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] U-boot nand bug, read.part should fail 2013-02-07 22:22 ` Scott Wood @ 2013-02-08 15:48 ` Harvey Chapman 2013-02-08 16:44 ` Harvey Chapman 0 siblings, 1 reply; 6+ messages in thread From: Harvey Chapman @ 2013-02-08 15:48 UTC (permalink / raw) To: u-boot On Feb 7, 2013, at 5:22 PM, Scott Wood <scottwood@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 */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] U-boot nand bug, read.part should fail 2013-02-08 15:48 ` Harvey Chapman @ 2013-02-08 16:44 ` Harvey Chapman 2013-02-08 23:34 ` Scott Wood 0 siblings, 1 reply; 6+ messages in thread From: Harvey Chapman @ 2013-02-08 16:44 UTC (permalink / raw) To: u-boot 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 */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] U-boot nand bug, read.part should fail 2013-02-08 16:44 ` Harvey Chapman @ 2013-02-08 23:34 ` Scott Wood 2013-02-09 1:02 ` Harvey Chapman 0 siblings, 1 reply; 6+ messages in thread From: Scott Wood @ 2013-02-08 23:34 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] U-boot nand bug, read.part should fail 2013-02-08 23:34 ` Scott Wood @ 2013-02-09 1:02 ` Harvey Chapman 0 siblings, 0 replies; 6+ messages in thread From: Harvey Chapman @ 2013-02-09 1:02 UTC (permalink / raw) To: u-boot On Feb 8, 2013, at 6:34 PM, Scott Wood <scottwood@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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-09 1:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1360274360.27002.10@snotra>
2013-02-07 22:13 ` [U-Boot] U-boot nand bug, read.part should fail Harvey Chapman
2013-02-07 22:22 ` Scott Wood
2013-02-08 15:48 ` Harvey Chapman
2013-02-08 16:44 ` Harvey Chapman
2013-02-08 23:34 ` Scott Wood
2013-02-09 1:02 ` Harvey Chapman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.