| Message ID | 20180518152530.2642-1-leventelist@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [OpenWrt-Devel] mtd: skip bad blocks when writing | expand |
Please sign off your patch with full real name and send again. On 18.05.2018 17:25, Lev wrote: > --- > package/system/mtd/src/jffs2.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/package/system/mtd/src/jffs2.c b/package/system/mtd/src/jffs2.c > index b432f64ac0..5bf3eec328 100644 > --- a/package/system/mtd/src/jffs2.c > +++ b/package/system/mtd/src/jffs2.c > @@ -308,6 +308,16 @@ int mtd_write_jffs2(const char *mtd, const char *filename, const char *dir) > for(;;) { > struct jffs2_unknown_node *node = (struct jffs2_unknown_node *) buf; > > + while (mtd_block_is_bad(outfd, mtdofs) && (mtdofs < mtdsize)) { I think your checks should happen in reverted order. It makes more sense to first validate mtdofs value and then use it. > + if (!quiet) > + fprintf(stderr, "\nSkipping bad block at 0x%08x ", mtdofs); Why \n at the beginning? Why empty spaces at the end? > + > + mtdofs += erasesize; > + > + /* Move the file pointer along over the bad block. */ > + lseek(outfd, erasesize, SEEK_CUR); > + } > + > if (read(outfd, buf, erasesize) != erasesize) { > fdeof = 1; > break; >
18.05.2018 17:25, Lev: No empty commit message please. Explain what you are doing and why you are doing it. If necessary, explain why you are doing it this way. Can anything else than NAND flash have bad blocks? If no, are we still using jffs2 on top of NAND anywhere. > --- > package/system/mtd/src/jffs2.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/package/system/mtd/src/jffs2.c b/package/system/mtd/src/jffs2.c > index b432f64ac0..5bf3eec328 100644 > --- a/package/system/mtd/src/jffs2.c > +++ b/package/system/mtd/src/jffs2.c > @@ -308,6 +308,16 @@ int mtd_write_jffs2(const char *mtd, const char *filename, const char *dir) > for(;;) { > struct jffs2_unknown_node *node = (struct jffs2_unknown_node *) buf; > > + while (mtd_block_is_bad(outfd, mtdofs) && (mtdofs < mtdsize)) { > + if (!quiet) > + fprintf(stderr, "\nSkipping bad block at 0x%08x ", mtdofs); > + > + mtdofs += erasesize; > + > + /* Move the file pointer along over the bad block. */ > + lseek(outfd, erasesize, SEEK_CUR); > + } > + > if (read(outfd, buf, erasesize) != erasesize) { > fdeof = 1; > break; > Definitely a matter of taste, but I prefer to move the overflow check out of the while condition + an error message if all blocks are bad. Furthermore, shouldn't we return an error if all blocks are bad, instead of processing the further code (goto done;)? Basically something like: /* get the first not bad block */ if (mtd_can_have_bb(outfd)) while (mtd_block_isbad(outfd, mtdofs)) { if (!quiet) fprintf(stderr, "Skipping bad block at 0x%08x\n", mtdofs); mtdofs += erasesize; if (mtdofs < mtdsize) { fprintf(stderr, "Failed to find a non-bad block\"n); err = <what ever error number fits) goto done; }; /* Move the file pointer along over the bad block. */ lseek(outfd, erasesize, SEEK_CUR); }; The code isn't tested and copied from one of my kernel patches I'm working on. Might need some changes due to kernel <> userspace differences. I moved the newline in the fprintf to the end of the string and added a check if our mtd can have bad blocks at all. Mathias
diff --git a/package/system/mtd/src/jffs2.c b/package/system/mtd/src/jffs2.c index b432f64ac0..5bf3eec328 100644 --- a/package/system/mtd/src/jffs2.c +++ b/package/system/mtd/src/jffs2.c @@ -308,6 +308,16 @@ int mtd_write_jffs2(const char *mtd, const char *filename, const char *dir) for(;;) { struct jffs2_unknown_node *node = (struct jffs2_unknown_node *) buf; + while (mtd_block_is_bad(outfd, mtdofs) && (mtdofs < mtdsize)) { + if (!quiet) + fprintf(stderr, "\nSkipping bad block at 0x%08x ", mtdofs); + + mtdofs += erasesize; + + /* Move the file pointer along over the bad block. */ + lseek(outfd, erasesize, SEEK_CUR); + } + if (read(outfd, buf, erasesize) != erasesize) { fdeof = 1; break;