[PATCH] [MTD-UTILS] Bad block handling in nandwrite when reading from standard input

[PATCH] [MTD-UTILS] Bad block handling in nandwrite when reading from standard input

[Jehan Bing]

Nandwrite tries to use lseek() when failing to write on a page. lseek() will fail when used on the standard input so nandwrite fails. This code replaces lseek with a buffer.

When the data is read, it is put in a buffer (filebuf). This buffer is reset at each block boundary. So a "seek" just means reading from the beginning of the buffer. writebuf and oobreadbuf are now just pointers to locations in filebuf.

With this change, reading from stdin or from a file now uses the same code path.


Signed-off-by: Jehan Bing <jehan@orb.com>

--- a/nandwrite.c	2009-06-08 13:33:32.000000000 -0700
+++ b/nandwrite.c	2009-06-08 14:46:59.000000000 -0700
@@ -45,13 +45,6 @@
 #define MAX_PAGE_SIZE	4096
 #define MAX_OOB_SIZE	128
 
-/*
- * Buffer array used for writing data
- */
-static unsigned char writebuf[MAX_PAGE_SIZE];
-static unsigned char oobbuf[MAX_OOB_SIZE];
-static unsigned char oobreadbuf[MAX_OOB_SIZE];
-
 // oob layouts to pass into the kernel as default
 static struct nand_oobinfo none_oobinfo = {
 	.useecc = MTD_NANDECC_OFF,
@@ -260,8 +253,16 @@ int main(int argc, char * const argv[])
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
-	int readcnt = 0;
 	bool failed = true;
+	// contains all the data read from the file so far for the current eraseblock
+	unsigned char *filebuf = NULL;
+	size_t filebuf_max = 0;
+	size_t filebuf_len = 0; 
+	// points to the current page inside filebuf
+	unsigned char *writebuf = NULL;
+	// points to the OOB for the current page in filebuf
+	unsigned char *oobreadbuf = NULL;
+	unsigned char oobbuf[MAX_OOB_SIZE];
 
 	process_options(argc, argv);
 
@@ -432,6 +433,16 @@ int main(int argc, char * const argv[])
 		goto closeall;
 	}
 
+	// Allocate a buffer big enough to contain all the data (OOB included) for one eraseblock
+	filebuf_max = pagelen * meminfo.erasesize / meminfo.writesize;
+	filebuf = (unsigned char*)malloc(filebuf_max);
+	if (!filebuf) {
+		fprintf(stderr, "Failed to allocate memory for file buffer (%d bytes)\n",
+				pagelen * meminfo.erasesize / meminfo.writesize);
+		goto closeall;
+	}
+	erase_buffer(filebuf, filebuf_max);
+
 	/*
 	 * Get data from input and write to the device while there is
 	 * still input to read and we are still within the device
@@ -439,7 +450,9 @@ int main(int argc, char * const argv[])
 	 * length is simply a quasi-boolean flag whose values are page
 	 * length or zero.
 	 */
-	while (imglen && (mtdoffset < meminfo.size)) {
+	while (((imglen > 0) || (writebuf < (filebuf + filebuf_len)))
+		&& (mtdoffset < meminfo.size))
+	{
 		// new eraseblock , check for bad block(s)
 		// Stay in the loop to be sure if the mtdoffset changes because
 		// of a bad block, that the next block that will be written to
@@ -449,6 +462,15 @@ int main(int argc, char * const argv[])
 		while (blockstart != (mtdoffset & (~meminfo.erasesize + 1))) {
 			blockstart = mtdoffset & (~meminfo.erasesize + 1);
 			offs = blockstart;
+
+			// if writebuf == filebuf, we are rewinding so we must not
+			// reset it but just replay it
+			if (writebuf != filebuf) {
+				erase_buffer(filebuf, filebuf_len);
+				filebuf_len = 0;
+				writebuf = filebuf;
+			}
+
 			baderaseblock = false;
 			if (!quiet)
 				fprintf (stdout, "Writing data to block %d at offset 0x%x\n",
@@ -476,71 +498,91 @@ int main(int argc, char * const argv[])
 
 		}
 
-		readlen = meminfo.writesize;
-
-		if (ifd != STDIN_FILENO) {
-			int tinycnt = 0;
-
-			if (pad && (imglen < readlen))
-			{
-				readlen = imglen;
-				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
-			}
-
-			/* Read Page Data from input file */
-			while(tinycnt < readlen) {
-				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
-					goto closeall;
-				}
-				tinycnt += cnt;
-			}
-		} else {
-			int tinycnt = 0;
-
+		// Read more data from the input if there isn't enough in the buffer
+		if ((writebuf + meminfo.writesize) > (filebuf + filebuf_len)) {
+			readlen = meminfo.writesize;
+
+			int alreadyread = (filebuf + filebuf_len) - writebuf;
+			int tinycnt = alreadyread;
+			
 			while(tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
 				if (cnt == 0) { // EOF
 					break;
 				} else if (cnt < 0) {
-					perror ("File I/O error on stdin");
+					perror ("File I/O error on input");
 					goto closeall;
 				}
 				tinycnt += cnt;
 			}
-
+			
 			/* No padding needed - we are done */
 			if (tinycnt == 0) {
-				imglen = 0;
+				// For standard input, set the imglen to 0 to signal
+				// the end of the "file". For non standard input, leave
+				// it as-is to detect an early EOF
+				if (ifd == STDIN_FILENO) {
+					imglen = 0;
+				}
 				break;
 			}
-
-			/* No more bytes - we are done after writing the remaining bytes */
-			if (cnt == 0) {
-				imglen = 0;
-			}
-
+			
 			/* Padding */
-			if (pad && (tinycnt < readlen)) {
+			if (tinycnt < readlen) {
+				if (!pad) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes. Use the padding option.\n",
+							readlen - tinycnt);
+					goto closeall;
+				}
 				erase_buffer(writebuf + tinycnt, meminfo.writesize - tinycnt);
 			}
+
+			filebuf_len += readlen - alreadyread;
+			if (ifd != STDIN_FILENO) {
+				imglen -= tinycnt - alreadyread;
+			}
+			else if (cnt == 0) {
+				/* No more bytes - we are done after writing the remaining bytes */
+				imglen = 0;
+			}
 		}
 
 		if (writeoob) {
-			int tinycnt = 0;
+			oobreadbuf = writebuf + meminfo.writesize;
 
-			while(tinycnt < meminfo.oobsize) {
-				cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
+			// Read more data for the OOB from the input if there isn't enough in the buffer
+			if ((oobreadbuf + meminfo.oobsize) > (filebuf + filebuf_len)) {
+				readlen = meminfo.oobsize;
+
+				int alreadyread = (filebuf + filebuf_len) - oobreadbuf;
+				int tinycnt = alreadyread;
+
+				while(tinycnt < readlen) {
+					cnt = read(ifd, oobreadbuf + tinycnt, readlen - tinycnt);
+					if (cnt == 0) { // EOF
+						break;
+					} else if (cnt < 0) {
+						perror ("File I/O error on input");
+						goto closeall;
+					}
+					tinycnt += cnt;
+				}
+
+				if (tinycnt < readlen) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes for OOB\n", readlen - tinycnt);
 					goto closeall;
 				}
-				tinycnt += cnt;
+
+				filebuf_len += readlen - alreadyread;
+				if (ifd != STDIN_FILENO) {
+					imglen -= tinycnt - alreadyread;
+				}
+				else if (cnt == 0) {
+					/* No more bytes - we are done after writing the remaining bytes */
+					imglen = 0;
+				}
 			}
 
 			if (!noecc) {
@@ -570,19 +612,20 @@ int main(int argc, char * const argv[])
 							len);
 				}
 			}
+			else {
+				oob.ptr = oobreadbuf;				
+			}
+
 			/* Write OOB data first, as ecc will be placed in there*/
 			oob.start = mtdoffset;
 			if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
 				perror ("ioctl(MEMWRITEOOB)");
 				goto closeall;
 			}
-			imglen -= meminfo.oobsize;
 		}
 
 		/* Write out the Page data */
 		if (pwrite(fd, writebuf, meminfo.writesize, mtdoffset) != meminfo.writesize) {
-			int rewind_blocks;
-			off_t rewind_bytes;
 			erase_info_t erase;
 
 			perror ("pwrite");
@@ -591,15 +634,8 @@ int main(int argc, char * const argv[])
 			}
 
 			/* Must rewind to blockstart if we can */
-			rewind_blocks = (mtdoffset - blockstart) / meminfo.writesize; /* Not including the one we just attempted */
-			rewind_bytes = (rewind_blocks * meminfo.writesize) + readlen;
-			if (writeoob)
-				rewind_bytes += (rewind_blocks + 1) * meminfo.oobsize;
-			if (lseek(ifd, -rewind_bytes, SEEK_CUR) == -1) {
-				perror("lseek");
-				fprintf(stderr, "Failed to seek backwards to recover from write error\n");
-				goto closeall;
-			}
+			writebuf = filebuf;
+
 			erase.start = blockstart;
 			erase.length = meminfo.erasesize;
 			fprintf(stderr, "Erasing failed write from %08lx-%08lx\n",
@@ -620,19 +656,20 @@ int main(int argc, char * const argv[])
 				}
 			}
 			mtdoffset = blockstart + meminfo.erasesize;
-			imglen += rewind_blocks * meminfo.writesize;
 
 			continue;
 		}
-		if (ifd != STDIN_FILENO) {
-			imglen -= readlen;
-		}
 		mtdoffset += meminfo.writesize;
+		writebuf += pagelen;
 	}
 
 	failed = false;
 
 closeall:
+	if (filebuf) {
+		free(filebuf);
+	}
+
 	close(ifd);
 
 restoreoob:
@@ -646,7 +683,10 @@ restoreoob:
 
 	close(fd);
 
-	if (failed || ((ifd != STDIN_FILENO) && (imglen > 0))) {
+	if (failed
+		|| ((ifd != STDIN_FILENO) && (imglen > 0))
+		|| (writebuf < (filebuf + filebuf_len)))
+	{
 		perror ("Data was only partially written due to error\n");
 		exit (EXIT_FAILURE);
 	}

Re: [PATCH] [MTD-UTILS] Bad block handling in nandwrite when reading from standard input

[Artem Bityutskiy]

On Mon, 2009-06-08 at 15:32 -0700, Jehan Bing wrote:
> Nandwrite tries to use lseek() when failing to write on a page. lseek() will fail when used on the standard input so nandwrite fails. This code replaces lseek with a buffer.
> 
> When the data is read, it is put in a buffer (filebuf). This buffer is reset at each block boundary. So a "seek" just means reading from the beginning of the buffer. writebuf and oobreadbuf are now just pointers to locations in filebuf.
> 
> With this change, reading from stdin or from a file now uses the same code path.
> 
> 
> Signed-off-by: Jehan Bing <jehan@orb.com>

Too large patch for me to review. Could you split it on few smaller
ones please?

Also, please, do not send e-mails with
 looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong
lines. Please, wrap them to 78 characters. You'll make it then
easier for other people to deal with you. Let's be nice.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] [MTD-UTILS] Bad block handling in nandwrite when reading from standard input

[Jehan Bing]

Artem Bityutskiy wrote:
> On Mon, 2009-06-08 at 15:32 -0700, Jehan Bing wrote:
>   
>> Nandwrite tries to use lseek() when failing to write on a page. lseek() will fail when used on the standard input so nandwrite fails. This code replaces lseek with a buffer.
>>
>> When the data is read, it is put in a buffer (filebuf). This buffer is reset at each block boundary. So a "seek" just means reading from the beginning of the buffer. writebuf and oobreadbuf are now just pointers to locations in filebuf.
>>
>> With this change, reading from stdin or from a file now uses the same code path.
>>
>>
>> Signed-off-by: Jehan Bing <jehan@orb.com>
>
> Too large patch for me to review. Could you split it on few smaller
> ones please?
>   

Ok, I'll see what I can do.


> Also, please, do not send e-mails with
>  looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong
> lines. Please, wrap them to 78 characters. You'll make it then
> easier for other people to deal with you. Let's be nice.
>   

Sorry, I'm still trying to find the correct configuration for my mailer. 
I followed the instruction in the email-clients documentation but that 
broke things for me.
Hopefully this one will work better.

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Jehan Bing]

Use same code path for reading data (not the OOB) from either the 
standard input or a regular file.

Signed-off-by: Jehan Bing <jehan@orb.com>

--- a/nandwrite.c	2009-06-08 13:33:32.000000000 -0700
+++ b/nandwrite.c	2009-06-09 13:15:17.000000000 -0700
@@ -260,7 +260,6 @@ int main(int argc, char * const argv[])
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
-	int readcnt = 0;
 	bool failed = true;
 
 	process_options(argc, argv);
@@ -476,37 +475,18 @@ int main(int argc, char * const argv[])
 
 		}
 
-		readlen = meminfo.writesize;
 
-		if (ifd != STDIN_FILENO) {
-			int tinycnt = 0;
-
-			if (pad && (imglen < readlen))
-			{
-				readlen = imglen;
-				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
-			}
+		{
+			readlen = meminfo.writesize;
 
-			/* Read Page Data from input file */
-			while(tinycnt < readlen) {
-				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
-					goto closeall;
-				}
-				tinycnt += cnt;
-			}
-		} else {
 			int tinycnt = 0;
 
-			while(tinycnt < readlen) {
+			while (tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
 				if (cnt == 0) { // EOF
 					break;
 				} else if (cnt < 0) {
-					perror ("File I/O error on stdin");
+					perror ("File I/O error on input");
 					goto closeall;
 				}
 				tinycnt += cnt;
@@ -514,18 +494,30 @@ int main(int argc, char * const argv[])
 
 			/* No padding needed - we are done */
 			if (tinycnt == 0) {
-				imglen = 0;
+				// For standard input, set the imglen to 0 to signal
+				// the end of the "file". For non standard input, leave
+				// it as-is to detect an early EOF
+				if (ifd == STDIN_FILENO) {
+					imglen = 0;
+				}
 				break;
 			}
 
-			/* No more bytes - we are done after writing the remaining bytes */
-			if (cnt == 0) {
-				imglen = 0;
-			}
 
 			/* Padding */
-			if (pad && (tinycnt < readlen)) {
-				erase_buffer(writebuf + tinycnt, meminfo.writesize - tinycnt);
+			if (tinycnt < readlen) {
+				if (!pad) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes. Use the padding option.\n",
+							readlen - tinycnt);
+					goto closeall;
+				}
+				erase_buffer(writebuf + tinycnt, readlen - tinycnt);
+			}
+
+			/* No more bytes - we are done after writing the remaining bytes */
+			if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+				imglen = 0;
 			}
 		}
 

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Artem Bityutskiy]

On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
> -		readlen = meminfo.writesize;
>  
> -		if (ifd != STDIN_FILENO) {
> -			int tinycnt = 0;
> -
> -			if (pad && (imglen < readlen))
> -			{
> -				readlen = imglen;
> -				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
> -			}
> +		{
> +			readlen = meminfo.writesize;
>  

Err, why do you need these spare { } ?

> -			/* Read Page Data from input file */
> -			while(tinycnt < readlen) {
> -				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
> -				if (cnt == 0) { // EOF
> -					break;
> -				} else if (cnt < 0) {
> -					perror ("File I/O error on input file");
> -					goto closeall;
> -				}
> -				tinycnt += cnt;
> -			}
> -		} else {
>  			int tinycnt = 0;

Err, is it normal C do do stuff like
{
	readlen = meminfo.writesize;
	int tinycnt += cnt;

? I think this is C++.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Artem Bityutskiy]

On Wed, 2009-06-10 at 19:03 +0300, Artem Bityutskiy wrote:
> Err, is it normal C do do stuff like
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt += cnt;

Sorry, I meant
{
	readlen = meminfo.writesize;
	int tinycnt = 0;

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Jamie Lokier]

Artem Bityutskiy wrote:
> On Wed, 2009-06-10 at 19:03 +0300, Artem Bityutskiy wrote:
> > Err, is it normal C do do stuff like
> > {
> > 	readlen = meminfo.writesize;
> > 	int tinycnt += cnt;
> 
> Sorry, I meant
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt = 0;

It's C99, and GCC has accepted that syntax since GCC 3.0.

If you want to disable it, use -Werror=declaration-after-statement in
current GCC, or -pedantic (but that has other effects).

-- Jamie

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Jehan Bing]

Artem Bityutskiy wrote:
> On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
>   
>> -		readlen = meminfo.writesize;
>>  
>> -		if (ifd != STDIN_FILENO) {
>> -			int tinycnt = 0;
>> -
>> -			if (pad && (imglen < readlen))
>> -			{
>> -				readlen = imglen;
>> -				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
>> -			}
>> +		{
>> +			readlen = meminfo.writesize;
>>  
>>     
>
> Err, why do you need these spare { } ?
>   

Right, I wanted to comment on that but forgot by the time I started the 
email.
My idea was to make the patches clearer. Patch 3/3 puts that code inside 
a condition. So to avoid extra changes just because of the indentation, 
I added the extra { } in this patch instead since I was already heavily 
modifying it anyway.
Do you prefer me to remove them?


>> -			/* Read Page Data from input file */
>> -			while(tinycnt < readlen) {
>> -				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
>> -				if (cnt == 0) { // EOF
>> -					break;
>> -				} else if (cnt < 0) {
>> -					perror ("File I/O error on input file");
>> -					goto closeall;
>> -				}
>> -				tinycnt += cnt;
>> -			}
>> -		} else {
>>  			int tinycnt = 0;
>>     
>
> Err, is it normal C do do stuff like
> {
> 	readlen = meminfo.writesize;
> 	int tinycnt += cnt;
>
> ? I think this is C++.
>   

gcc didn't complain. Easy to fix.

Re: [PATCH 1/3] [MTD-UTILS] Unified reading from standard input and from file

[Artem Bityutskiy]

On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
> Use same code path for reading data (not the OOB) from either the 
> standard input or a regular file.
> 
> Signed-off-by: Jehan Bing <jehan@orb.com>

The patches look OK to me, but I do not have time to review them very
really well. So would it please be possible to describe how you tested
them to convince me they are ok? Then I'd push them to mtd-utils.git
tree. Did you test writing with/without oob, from file/stdin, etc?

Thanks!

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/3 v2] [MTD-UTILS] Unified reading from standard input and from file

[Jehan Bing]

Artem Bityutskiy wrote:
> On Tue, 2009-06-09 at 16:04 -0700, Jehan Bing wrote:
>   
>> Use same code path for reading data (not the OOB) from either the 
>> standard input or a regular file.
>>
>> Signed-off-by: Jehan Bing <jehan@orb.com>
>>     
>
> The patches look OK to me, but I do not have time to review them very
> really well. So would it please be possible to describe how you tested
> them to convince me they are ok? Then I'd push them to mtd-utils.git
> tree. Did you test writing with/without oob, from file/stdin, etc?
>
> Thanks

For the tests, I used the nandsim driver and 
"eraseall-nandwrite-nanddump-md5sum". I did  a first one with the 
official nandwrite from Ubuntu as a reference, then tried different 
options with my changes.

And here the same patch with a couple of blank lines removed and a 
comment moved. I forgot the regenerate it with the others after cleaning 
up the code. The third patch won't apply without it.


Signed-off-by: Jehan Bing <jehan@orb.com>

--- mtd-utils.orig/nandwrite.c	2009-06-08 13:33:32.000000000 -0700
+++ mtd-utils.unified/nandwrite.c	2009-06-09 13:20:56.000000000 -0700
@@ -260,7 +260,6 @@ int main(int argc, char * const argv[])
 	int ret, readlen;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
-	int readcnt = 0;
 	bool failed = true;
 
 	process_options(argc, argv);
@@ -476,37 +475,17 @@ int main(int argc, char * const argv[])
 
 		}
 
-		readlen = meminfo.writesize;
+		{
+			readlen = meminfo.writesize;
 
-		if (ifd != STDIN_FILENO) {
 			int tinycnt = 0;
 
-			if (pad && (imglen < readlen))
-			{
-				readlen = imglen;
-				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
-			}
-
-			/* Read Page Data from input file */
-			while(tinycnt < readlen) {
+			while (tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
 				if (cnt == 0) { // EOF
 					break;
 				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
-					goto closeall;
-				}
-				tinycnt += cnt;
-			}
-		} else {
-			int tinycnt = 0;
-
-			while(tinycnt < readlen) {
-				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on stdin");
+					perror ("File I/O error on input");
 					goto closeall;
 				}
 				tinycnt += cnt;
@@ -514,18 +493,29 @@ int main(int argc, char * const argv[])
 
 			/* No padding needed - we are done */
 			if (tinycnt == 0) {
-				imglen = 0;
+				// For standard input, set the imglen to 0 to signal
+				// the end of the "file". For non standard input, leave
+				// it as-is to detect an early EOF
+				if (ifd == STDIN_FILENO) {
+					imglen = 0;
+				}
 				break;
 			}
 
-			/* No more bytes - we are done after writing the remaining bytes */
-			if (cnt == 0) {
-				imglen = 0;
+			/* Padding */
+			if (tinycnt < readlen) {
+				if (!pad) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes. Use the padding option.\n",
+							readlen - tinycnt);
+					goto closeall;
+				}
+				erase_buffer(writebuf + tinycnt, readlen - tinycnt);
 			}
 
-			/* Padding */
-			if (pad && (tinycnt < readlen)) {
-				erase_buffer(writebuf + tinycnt, meminfo.writesize - tinycnt);
+			if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+				/* No more bytes - we are done after writing the remaining bytes */
+				imglen = 0;
 			}
 		}
 

Re: [PATCH 1/3 v2] [MTD-UTILS] Unified reading from standard input and from file

[Artem Bityutskiy]

On Thu, 2009-06-11 at 10:43 -0700, Jehan Bing wrote:
> > The patches look OK to me, but I do not have time to review them very
> > really well. So would it please be possible to describe how you tested
> > them to convince me they are ok? Then I'd push them to mtd-utils.git
> > tree. Did you test writing with/without oob, from file/stdin, etc?
> >
> > Thanks
> 
> For the tests, I used the nandsim driver and 
> "eraseall-nandwrite-nanddump-md5sum". I did  a first one with the 
> official nandwrite from Ubuntu as a reference, then tried different 
> options with my changes.
> 
> And here the same patch with a couple of blank lines removed and a 
> comment moved. I forgot the regenerate it with the others after cleaning 
> up the code. The third patch won't apply without it.

Sorry, I still feel a little bit unsure. Could you please test it
like this:

1. Compile nandwrite1 which excludes these 4 changes;
2. Compile nandwrite2 which includes these 4 changes;
3. Make sure things which work with nandwrite1 also work with
   nandwrite2. Namely:
   a. writing an image
   b. writing an image from stdin
   c. writing an image with oob
   d. writing an image with oob with stdin
4. Then test how nandwrite2 reacts on badblocks - use the 'weakblocks'
   parameter of nandsim for this (see modinfo nandsim).
5. Test now nandwrite2 reacts on write failures - use 'weakpages'
   nandsim argument

Could this please be done to make sure nandwrite does not have
regressions but only has improvement? Sorry for being PITA.

Thanks!

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/3] [MTD-UTILS] Use same kind of code for reading OOB than for regular data

[Nahor]

Use the same code structure when reading the OOB than when reading the 
regular data.

Signed-off-by: Jehan Bing <jehan@orb.com>

--- a/nandwrite.c	2009-06-09 13:20:56.000000000 -0700
+++ b/nandwrite.c	2009-06-09 13:20:44.000000000 -0700
@@ -520,17 +520,32 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
-			int tinycnt = 0;
+			{
+				int readlen = meminfo.oobsize;
 
-			while(tinycnt < meminfo.oobsize) {
-				cnt = read(ifd, oobreadbuf + tinycnt, meminfo.oobsize - tinycnt);
-				if (cnt == 0) { // EOF
-					break;
-				} else if (cnt < 0) {
-					perror ("File I/O error on input file");
+				int tinycnt = 0;
+
+				while (tinycnt < readlen) {
+					cnt = read(ifd, oobreadbuf + tinycnt, readlen - tinycnt);
+					if (cnt == 0) { // EOF
+						break;
+					} else if (cnt < 0) {
+						perror ("File I/O error on input");
+						goto closeall;
+					}
+					tinycnt += cnt;
+				}
+
+				if (tinycnt < readlen) {
+					fprintf(stderr, "Unexpected EOF. Expecting at least "
+							"%d more bytes for OOB\n", readlen - tinycnt);
 					goto closeall;
 				}
-				tinycnt += cnt;
+
+				if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+					/* No more bytes - we are done after writing the remaining bytes */
+					imglen = 0;
+				}
 			}
 
 			if (!noecc) {

Re: [PATCH 3/3] [MTD-UTILS] Handle bad block when reading from standard input

[Jehan Bing]

It's still a fairly big patch but I don't see how I can cut more and 
keep a compilable tree. I can make "sub-patch" (init+free, buffer reset, 
buffer filling, bad block handling) if it's okay that they break the 
compilation.


Nandwrite tries to use lseek() when failing to write on a page. lseek() 
will fail when used on the standard input so nandwrite fails. This code 
replaces lseek with a buffer.

When the data is read, it is put in a buffer (filebuf). This buffer is 
reset at each block boundary. So a "seek" just means reading from the 
beginning of the buffer. writebuf and oobreadbuf are now just pointers 
to locations in filebuf.

Signed-off-by: Jehan Bing <jehan@orb.com>

--- a/nandwrite.c	2009-06-09 13:20:44.000000000 -0700
+++ b/nandwrite.c	2009-06-09 15:41:39.000000000 -0700
@@ -45,13 +45,6 @@
 #define MAX_PAGE_SIZE	4096
 #define MAX_OOB_SIZE	128
 
-/*
- * Buffer array used for writing data
- */
-static unsigned char writebuf[MAX_PAGE_SIZE];
-static unsigned char oobbuf[MAX_OOB_SIZE];
-static unsigned char oobreadbuf[MAX_OOB_SIZE];
-
 // oob layouts to pass into the kernel as default
 static struct nand_oobinfo none_oobinfo = {
 	.useecc = MTD_NANDECC_OFF,
@@ -257,10 +250,19 @@ int main(int argc, char * const argv[])
 	struct mtd_info_user meminfo;
 	struct mtd_oob_buf oob;
 	loff_t offs;
-	int ret, readlen;
+	int ret;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
 	bool failed = true;
+	// contains all the data read from the file so far for the current eraseblock
+	unsigned char *filebuf = NULL;
+	size_t filebuf_max = 0;
+	size_t filebuf_len = 0; 
+	// points to the current page inside filebuf
+	unsigned char *writebuf = NULL;
+	// points to the OOB for the current page in filebuf
+	unsigned char *oobreadbuf = NULL;
+	unsigned char oobbuf[MAX_OOB_SIZE];
 
 	process_options(argc, argv);
 
@@ -431,6 +433,16 @@ int main(int argc, char * const argv[])
 		goto closeall;
 	}
 
+	// Allocate a buffer big enough to contain all the data (OOB included) for one eraseblock
+	filebuf_max = pagelen * meminfo.erasesize / meminfo.writesize;
+	filebuf = (unsigned char*)malloc(filebuf_max);
+	if (!filebuf) {
+		fprintf(stderr, "Failed to allocate memory for file buffer (%d bytes)\n",
+				pagelen * meminfo.erasesize / meminfo.writesize);
+		goto closeall;
+	}
+	erase_buffer(filebuf, filebuf_max);
+
 	/*
 	 * Get data from input and write to the device while there is
 	 * still input to read and we are still within the device
@@ -438,7 +450,9 @@ int main(int argc, char * const argv[])
 	 * length is simply a quasi-boolean flag whose values are page
 	 * length or zero.
 	 */
-	while (imglen && (mtdoffset < meminfo.size)) {
+	while (((imglen > 0) || (writebuf < (filebuf + filebuf_len)))
+		&& (mtdoffset < meminfo.size))
+	{
 		// new eraseblock , check for bad block(s)
 		// Stay in the loop to be sure if the mtdoffset changes because
 		// of a bad block, that the next block that will be written to
@@ -448,6 +462,15 @@ int main(int argc, char * const argv[])
 		while (blockstart != (mtdoffset & (~meminfo.erasesize + 1))) {
 			blockstart = mtdoffset & (~meminfo.erasesize + 1);
 			offs = blockstart;
+
+			// if writebuf == filebuf, we are rewinding so we must not
+			// reset the buffer but just replay it
+			if (writebuf != filebuf) {
+				erase_buffer(filebuf, filebuf_len);
+				filebuf_len = 0;
+				writebuf = filebuf;
+			}
+
 			baderaseblock = false;
 			if (!quiet)
 				fprintf (stdout, "Writing data to block %d at offset 0x%x\n",
@@ -475,10 +498,12 @@ int main(int argc, char * const argv[])
 
 		}
 
-		{
-			readlen = meminfo.writesize;
+		// Read more data from the input if there isn't enough in the buffer
+		if ((writebuf + meminfo.writesize) > (filebuf + filebuf_len)) {
+			int readlen = meminfo.writesize;
 
-			int tinycnt = 0;
+			int alreadyread = (filebuf + filebuf_len) - writebuf;
+			int tinycnt = alreadyread;
 
 			while (tinycnt < readlen) {
 				cnt = read(ifd, writebuf + tinycnt, readlen - tinycnt);
@@ -513,17 +538,25 @@ int main(int argc, char * const argv[])
 				erase_buffer(writebuf + tinycnt, readlen - tinycnt);
 			}
 
-			if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+			filebuf_len += readlen - alreadyread;
+			if (ifd != STDIN_FILENO) {
+				imglen -= tinycnt - alreadyread;
+			}
+			else if (cnt == 0) {
 				/* No more bytes - we are done after writing the remaining bytes */
 				imglen = 0;
 			}
 		}
 
 		if (writeoob) {
-			{
+			oobreadbuf = writebuf + meminfo.writesize;
+
+			// Read more data for the OOB from the input if there isn't enough in the buffer
+			if ((oobreadbuf + meminfo.oobsize) > (filebuf + filebuf_len)) {
 				int readlen = meminfo.oobsize;
 
-				int tinycnt = 0;
+				int alreadyread = (filebuf + filebuf_len) - oobreadbuf;
+				int tinycnt = alreadyread;
 
 				while (tinycnt < readlen) {
 					cnt = read(ifd, oobreadbuf + tinycnt, readlen - tinycnt);
@@ -542,13 +575,19 @@ int main(int argc, char * const argv[])
 					goto closeall;
 				}
 
-				if ((ifd == STDIN_FILENO) && (cnt == 0)) {
+				filebuf_len += readlen - alreadyread;
+				if (ifd != STDIN_FILENO) {
+					imglen -= tinycnt - alreadyread;
+				}
+				else if (cnt == 0) {
 					/* No more bytes - we are done after writing the remaining bytes */
 					imglen = 0;
 				}
 			}
 
-			if (!noecc) {
+			if (noecc) {
+				oob.ptr = oobreadbuf;
+			} else {
 				int i, start, len;
 				/*
 				 *  We use autoplacement and have the oobinfo with the autoplacement
@@ -581,13 +620,10 @@ int main(int argc, char * const argv[])
 				perror ("ioctl(MEMWRITEOOB)");
 				goto closeall;
 			}
-			imglen -= meminfo.oobsize;
 		}
 
 		/* Write out the Page data */
 		if (pwrite(fd, writebuf, meminfo.writesize, mtdoffset) != meminfo.writesize) {
-			int rewind_blocks;
-			off_t rewind_bytes;
 			erase_info_t erase;
 
 			perror ("pwrite");
@@ -596,15 +632,8 @@ int main(int argc, char * const argv[])
 			}
 
 			/* Must rewind to blockstart if we can */
-			rewind_blocks = (mtdoffset - blockstart) / meminfo.writesize; /* Not including the one we just attempted */
-			rewind_bytes = (rewind_blocks * meminfo.writesize) + readlen;
-			if (writeoob)
-				rewind_bytes += (rewind_blocks + 1) * meminfo.oobsize;
-			if (lseek(ifd, -rewind_bytes, SEEK_CUR) == -1) {
-				perror("lseek");
-				fprintf(stderr, "Failed to seek backwards to recover from write error\n");
-				goto closeall;
-			}
+			writebuf = filebuf;
+
 			erase.start = blockstart;
 			erase.length = meminfo.erasesize;
 			fprintf(stderr, "Erasing failed write from %08lx-%08lx\n",
@@ -625,19 +654,20 @@ int main(int argc, char * const argv[])
 				}
 			}
 			mtdoffset = blockstart + meminfo.erasesize;
-			imglen += rewind_blocks * meminfo.writesize;
 
 			continue;
 		}
-		if (ifd != STDIN_FILENO) {
-			imglen -= readlen;
-		}
 		mtdoffset += meminfo.writesize;
+		writebuf += pagelen;
 	}
 
 	failed = false;
 
 closeall:
+	if (filebuf) {
+		free(filebuf);
+	}
+
 	close(ifd);
 
 restoreoob:
@@ -651,7 +681,10 @@ restoreoob:
 
 	close(fd);
 
-	if (failed || ((ifd != STDIN_FILENO) && (imglen > 0))) {
+	if (failed
+		|| ((ifd != STDIN_FILENO) && (imglen > 0))
+		|| (writebuf < (filebuf + filebuf_len)))
+	{
 		perror ("Data was only partially written due to error\n");
 		exit (EXIT_FAILURE);
 	}