[OpenWrt-Devel] mtd: skip bad blocks when writing

Message ID 20180518152530.2642-1-leventelist@gmail.com
State Changes Requested
Headers show
Series
  • [OpenWrt-Devel] mtd: skip bad blocks when writing
Related show

Commit Message

Lev May 18, 2018, 3:25 p.m.
---
 package/system/mtd/src/jffs2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Rafał Miłecki July 11, 2018, 4:36 a.m. | #1
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;
>
Mathias Kresin July 12, 2018, 4:52 p.m. | #2
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

Patch

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;