Patchwork nandwrite: Add Support for Reading from Standard Input

login
register
mail settings
Submitter Grant Erickson
Date Sept. 8, 2008, 4:29 a.m.
Message ID <1220848159-22407-1-git-send-email-gerickson@nuovations.com>
Download mbox | patch
Permalink /patch/200/
State Accepted
Commit 2d0b9f58998c54dd114ff5c4476d10cc03a21864
Headers show

Comments

Grant Erickson - Sept. 8, 2008, 4:29 a.m.
Added suppport for reading in band data from standard input based on a
patch originally generated by Richard Titmuss <titmuss@slimdevices.com>
at <http://lists.slimdevices.com/pipermail/jive-checkins/2008-May/001918.html>.

Signed-off-by: Grant Erickson <gerickson@nuovations.com>
Ladislav Michl - Sept. 8, 2008, 4:14 p.m.
On Sun, Sep 07, 2008 at 09:29:19PM -0700, Grant Erickson wrote:
> Added suppport for reading in band data from standard input based on a
> patch originally generated by Richard Titmuss <titmuss@slimdevices.com>
> at <http://lists.slimdevices.com/pipermail/jive-checkins/2008-May/001918.html>.
> 
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
> 
> In addition to the above patch, further discussion of this feature was
> raised by Tommi Airikka <tommi.airikka@ericsson.com> at
> <http://lists.infradead.org/pipermail/linux-mtd/2008-September/022913.html>.

Hmm, so long I was defered with implementing this feature, that someone
else did it :-)

Anyway, let me raise few objections:
- code uses lseek. Obviously, you cannot seek on pipe.
- there is no need to use "invariant placeholder". What about simply
  read till file ends?
- there is no need to use special case for reading from stdin...
So what about:
while (not eof)
	read page (and oob if selected) data into buffer
next:
	if (write buffer fails)
		mark bad, select next page, goto next
Reading from stdin would imply padding. Care to implement that?

Best regards,
	ladis

PS: It is remotely possible that one day I do it myself, but I'm
notoriously slow on doing things I do not urgently need.
Grant Erickson - Sept. 8, 2008, 4:36 p.m.
On 9/8/08 9:14 AM, Ladislav Michl wrote:
> On Sun, Sep 07, 2008 at 09:29:19PM -0700, Grant Erickson wrote:
>> Added suppport for reading in band data from standard input based on a
>> patch originally generated by Richard Titmuss <titmuss@slimdevices.com>
>> at 
>> <http://lists.slimdevices.com/pipermail/jive-checkins/2008-May/001918.html>.
>> 
>> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
>> ---
>> 
>> In addition to the above patch, further discussion of this feature was
>> raised by Tommi Airikka <tommi.airikka@ericsson.com> at
>> <http://lists.infradead.org/pipermail/linux-mtd/2008-September/022913.html>.
> 
> Hmm, so long I was defered with implementing this feature, that someone
> else did it :-)
> 
> Anyway, let me raise few objections:

Fair enough.

However, considering that there is sufficient desire for this feature and it
has been implemented several times (a number of them off-list and never
submitted) let me propose getting this in place and in tree as-is and then
evolving and improving it.

> - code uses lseek. Obviously, you cannot seek on pipe.

True enough. The patch avoids the lseek to determine input file size;
however, did not address the 'markbad' recovery lseek case.

Short-term proposal: like 'writeoob', disable 'markbad' support when reading
from standard input and condition the whole 'markbad' logic on not reading
from standard input.

> - there is no need to use "invariant placeholder". What about simply
>   read till file ends?

Perhaps Richard can comment on his motivation here. I would suggest that the
loop termination condition is not quite that simple since the termination
cases are 1) there is no more input data to read (error or EOF) or 2) we
have moved outside of the output device bounds.

> - there is no need to use special case for reading from stdin...

The patch as-is from Richard did for whatever reasons he chose to go that
route. Perhaps he can comment on his motivations.

However, there may be an opportunity to unify the regular file versus
standard input cases along with adding support for '-l,--length=length'.

> So what about:
>
> while (not eof)
>   read page (and oob if selected) data into buffer

What about a short read in which you only get part of a page or a whole page
and part of the OOB data? Bail out? Best effort?

> next:
> if (write buffer fails)
>   mark bad, select next page, goto next
>
> Reading from stdin would imply padding. Care to implement that?

For the time being, padding must be explicitly requested.

> PS: It is remotely possible that one day I do it myself, but I'm
> notoriously slow on doing things I do not urgently need.

Understood and agreed.

For the present, the patch satisfies my cited use case (clearly those of
Richard and hopefully those of Tommi) and given the value added, I'd
advocate its inclusion and incremental evolution through future patches.

Regards,

Grant
Ladislav Michl - Sept. 8, 2008, 9:02 p.m.
On Mon, Sep 08, 2008 at 09:36:52AM -0700, Grant Erickson wrote:
> On 9/8/08 9:14 AM, Ladislav Michl wrote:
> > So what about:
> >
> > while (not eof)
> >   read page (and oob if selected) data into buffer
> 
> What about a short read in which you only get part of a page or a whole page
> and part of the OOB data? Bail out? Best effort?

Simply loop reading (assuming blocking read) until
- read whole buffer (read returns something greater than zero) or
- read returns zero (end of file) and then pad buffer or
- read returns -1 and then bail out gracefully with strerror
  printed to user.

> > next:
> > if (write buffer fails)
> >   mark bad, select next page, goto next
> >
> > Reading from stdin would imply padding. Care to implement that?
> 
> For the time being, padding must be explicitly requested.

Yes, I'm just suggesting to enable it for stdin input as it IMHO makes
sense namely with buffering described above.

> > PS: It is remotely possible that one day I do it myself, but I'm
> > notoriously slow on doing things I do not urgently need.
> 
> Understood and agreed.
> 
> For the present, the patch satisfies my cited use case (clearly those of
> Richard and hopefully those of Tommi) and given the value added, I'd
> advocate its inclusion and incremental evolution through future patches.

Sure, I cannot raise single complain against patch as I do not offer
anything better. I just merely pointed to possible solution.

Best regards,
	ladis
Ladislav Michl - Nov. 20, 2008, 10:50 p.m.
On Mon, Sep 08, 2008 at 09:36:52AM -0700, Grant Erickson wrote:
> On 9/8/08 9:14 AM, Ladislav Michl wrote:
> > - code uses lseek. Obviously, you cannot seek on pipe.
> 
> True enough. The patch avoids the lseek to determine input file size;
> however, did not address the 'markbad' recovery lseek case.
> 
> Short-term proposal: like 'writeoob', disable 'markbad' support when reading
> from standard input and condition the whole 'markbad' logic on not reading
> from standard input.

It is not about marking bad blocks. Nandwrite will end with unrecoverable
error on first failed write and that is not acceptable in production
environment.

[snip]

> > PS: It is remotely possible that one day I do it myself, but I'm
> > notoriously slow on doing things I do not urgently need.

Well, it took less than three months. Not so bad :-)

And it turned out to be imposible to fix it properly without solving
following issue first.
MEMSETOOBSEL ioctl used to implement these options
  -a, --autoplace     Use auto oob layout
  -j, --jffs2         Force jffs2 oob layout (legacy support)
  -y, --yaffs         Force yaffs oob layout (legacy support)
  -f, --forcelegacy   Force legacy support on autoplacement-enabled mtd device
  -n, --noecc         Write without ecc
was removed more than two years ago and found its way into 2.6.18-rc1. See
http://lists.infradead.org/pipermail/linux-mtd-cvs/2006-May/005514.html

I hope legacy support can be safely removed (please raise your voice now)
which leaves only --noecc for implementation. Possible scenarios are:
1. Image does not contain OOB data
   Read whole eraseblock to buffer and write it to flash
2. Image contains OOB data (--oob is given)
   Set MTDFILEMODE to MTD_MODE_RAW, read whole eraseblock
   (data:oob:data:oob...) into buffer and write it to flash
3. Image does not contain OOB data and --noecc is given
   Set MTDFILEMODE to MTD_MODE_RAW, read whole eraseblock,
   insert 0xff into OOB after each writeblock, write into
   flash
4. Image contains OOB data and --noecc is given
   ???

As I have never need writing images with OOB I'd like to ask for help
how should be case 3 and 4 handled.

Thanks,
	ladis
Schlaegl Manfred jun. - Nov. 28, 2008, 10:40 p.m.
Am Donnerstag, den 20.11.2008, 23:50 +0100 schrieb Ladislav Michl: 
> On Mon, Sep 08, 2008 at 09:36:52AM -0700, Grant Erickson wrote:
> > On 9/8/08 9:14 AM, Ladislav Michl wrote:
> > > - code uses lseek. Obviously, you cannot seek on pipe.
> > 
> > True enough. The patch avoids the lseek to determine input file size;
> > however, did not address the 'markbad' recovery lseek case.
> > 
> > Short-term proposal: like 'writeoob', disable 'markbad' support when reading
> > from standard input and condition the whole 'markbad' logic on not reading
> > from standard input.
> 
> It is not about marking bad blocks. Nandwrite will end with unrecoverable
> error on first failed write and that is not acceptable in production
> environment.

On our productive Systems(automatic system-update) we wrote own code
with special security options:
The Image is written whole, without marking blocks bad, also if
write-errors occure, the number of errors will counted and as result we
deliver a "failed", with the number of write errors. Based on the number
of errors, we're able to react in diffent ways. For example: 
 * to many bad-blocks: heavy error -> abort, because of hardware error
 * to many bad-blocks: image doesn't fit -> abort
 * "block got bad" error -> rewrite with marking blocks bad (if new
errors occure -> repeat procedure)
But this is only reliable, if we there is an file-image.

So I think, one option is to do this by an command-line option:
"--makebad", or something. (see below)

> [snip]
> 
> > > PS: It is remotely possible that one day I do it myself, but I'm
> > > notoriously slow on doing things I do not urgently need.
> 
> Well, it took less than three months. Not so bad :-)
all day work sucks... *g*
> 
> And it turned out to be imposible to fix it properly without solving
> following issue first.
> MEMSETOOBSEL ioctl used to implement these options
>   -a, --autoplace     Use auto oob layout
>   -j, --jffs2         Force jffs2 oob layout (legacy support)
>   -y, --yaffs         Force yaffs oob layout (legacy support)
>   -f, --forcelegacy   Force legacy support on autoplacement-enabled mtd device
>   -n, --noecc         Write without ecc
> was removed more than two years ago and found its way into 2.6.18-rc1. See
> http://lists.infradead.org/pipermail/linux-mtd-cvs/2006-May/005514.html
> 
> I hope legacy support can be safely removed (please raise your voice now)
> which leaves only --noecc for implementation. Possible scenarios are:
> 1. Image does not contain OOB data
>    Read whole eraseblock to buffer and write it to flash
> 2. Image contains OOB data (--oob is given)
>    Set MTDFILEMODE to MTD_MODE_RAW, read whole eraseblock
>    (data:oob:data:oob...) into buffer and write it to flash
> 3. Image does not contain OOB data and --noecc is given
>    Set MTDFILEMODE to MTD_MODE_RAW, read whole eraseblock,
>    insert 0xff into OOB after each writeblock, write into
>    flash
> 4. Image contains OOB data and --noecc is given
>    ???

I think, the --noecc option only makes sense, if --oob ist not given. 
The oob-option is, logically seen, more than a simply change in
handling, because we practically write to an abstract device.

> 
> As I have never need writing images with OOB I'd like to ask for help
> how should be case 3 and 4 handled.
I used --oob for testing:
* to write a raw-image from physical(target) to nandsim(host) for
analysis (powerdown, etc.)
* generate bad-blocks on nand-devices for testing 

Ideas:
 --oob or --raw: writes unmodified data+oob (data:oob:data:oob, ...)
(raw-mode), if not given, write data, with autogenerated
ecc(abstract-mode). 
 --markbad: 
   * given: mark blocks with write errros as bad and perform write on
next block (caution: image-size)
   * not given: 
     * write image at whole and count errors -> result
     * or, abort on first error
 
The main issue I see, is the size of the device. Nobody is able to
predict the size of the destination device.

> 
> Thanks,
> 	ladis
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
best regards
	Manfred

Patch

diff --git a/nandwrite.c b/nandwrite.c
index b7cd72e..fc23e85 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -24,6 +24,8 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -74,7 +76,7 @@  static struct nand_oobinfo autoplace_oobinfo = {
 static void display_help (void)
 {
 	printf(
-"Usage: nandwrite [OPTION] MTD_DEVICE INPUTFILE\n"
+"Usage: nandwrite [OPTION] MTD_DEVICE [INPUTFILE|-]\n"
 "Writes to the specified MTD device.\n"
 "\n"
 "  -a, --autoplace         Use auto oob layout\n"
@@ -110,6 +112,7 @@  static void display_version (void)
 	exit (EXIT_SUCCESS);
 }
 
+static const char	*standard_input = "-";
 static const char	*mtd_device, *img;
 static int		mtdoffset = 0;
 static bool		quiet = false;
@@ -198,16 +201,46 @@  static void process_options (int argc, char * const argv[])
 				blockalign = atoi (optarg);
 				break;
 			case '?':
-				error = 1;
+				error++;
 				break;
 		}
 	}
 
-	if ((argc - optind) != 2 || error)
+	if (mtdoffset < 0) {
+		fprintf(stderr, "Can't specify a negative device offset `%d'\n",
+				mtdoffset);
+		exit (EXIT_FAILURE);
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	/*
+	 * There must be at least the MTD device node positional
+	 * argument remaining and, optionally, the input file.
+	 */
+
+	if (argc < 1 || argc > 2 || error)
 		display_help ();
 
-	mtd_device = argv[optind++];
-	img = argv[optind];
+	mtd_device = argv[0];
+
+	/*
+	 * Standard input may be specified either explictly as "-" or
+	 * implicity by simply omitting the second of the two
+	 * positional arguments.
+	 */
+
+	img = ((argc == 2) ? argv[1] : standard_input);
+}
+
+static void erase_buffer(void *buffer, size_t size)
+{
+	const uint8_t kEraseByte = 0xff;
+
+	if (buffer != NULL && size > 0) {
+		memset(buffer, kEraseByte, size);
+	}
 }
 
 /*
@@ -215,8 +248,12 @@  static void process_options (int argc, char * const argv[])
  */
 int main(int argc, char * const argv[])
 {
-	int cnt, fd, ifd, imglen = 0, pagelen, blockstart = -1;
+	int cnt = 0;
+	int fd = -1;
+	int ifd = -1;
+	int imglen = 0, pagelen;
 	bool baderaseblock = false;
+	int blockstart = -1;
 	struct mtd_info_user meminfo;
 	struct mtd_oob_buf oob;
 	loff_t offs;
@@ -226,7 +263,7 @@  int main(int argc, char * const argv[])
 
 	process_options(argc, argv);
 
-	memset(oobbuf, 0xff, sizeof(oobbuf));
+	erase_buffer(oobbuf, sizeof(oobbuf));
 
 	if (pad && writeoob) {
 		fprintf(stderr, "Can't pad when oob data is present.\n");
@@ -340,21 +377,48 @@  int main(int argc, char * const argv[])
 	oob.length = meminfo.oobsize;
 	oob.ptr = noecc ? oobreadbuf : oobbuf;
 
-	/* Open the input file */
-	if ((ifd = open(img, O_RDONLY)) == -1) {
+	/* Determine if we are reading from standard input or from a file. */
+	if (strcmp(img, standard_input) == 0) {
+		ifd = STDIN_FILENO;
+	} else {
+		ifd = open(img, O_RDONLY);
+	}
+
+	if (ifd == -1) {
 		perror(img);
 		goto restoreoob;
 	}
 
-	// get image length
-	imglen = lseek(ifd, 0, SEEK_END);
-	lseek (ifd, 0, SEEK_SET);
+	/* For now, don't allow writing oob when reading from standard input. */
+	if (ifd == STDIN_FILENO && writeoob) {
+		fprintf(stderr, "Can't write oob when reading from standard input.\n");
+		goto closeall;
+	}
 
 	pagelen = meminfo.writesize + ((writeoob) ? meminfo.oobsize : 0);
 
-	// Check, if file is pagealigned
+	/*
+	 * For the standard input case, the input size is merely an
+	 * invariant placeholder and is set to the write page
+	 * size. Otherwise, just use the input file size.
+	 *
+	 * TODO: Add support for the -l,--length=length option (see
+	 * previous discussion by Tommi Airikka <tommi.airikka@ericsson.com> at
+	 * <http://lists.infradead.org/pipermail/linux-mtd/2008-September/
+	 * 022913.html>
+	 */
+
+	if (ifd == STDIN_FILENO) {
+	    imglen = pagelen;
+	} else {
+	    imglen = lseek(ifd, 0, SEEK_END);
+	    lseek (ifd, 0, SEEK_SET);
+	}
+
+	// Check, if file is page-aligned
 	if ((!pad) && ((imglen % pagelen) != 0)) {
-		fprintf (stderr, "Input file is not page aligned\n");
+		fprintf (stderr, "Input file is not page-aligned. Use the padding "
+				 "option.\n");
 		goto closeall;
 	}
 
@@ -366,7 +430,13 @@  int main(int argc, char * const argv[])
 		goto closeall;
 	}
 
-	/* Get data from input and write to the device */
+	/*
+	 * Get data from input and write to the device while there is
+	 * still input to read and we are still within the device
+	 * bounds. Note that in the case of standard input, the input
+	 * length is simply a quasi-boolean flag whose values are page
+	 * length or zero.
+	 */
 	while (imglen && (mtdoffset < meminfo.size)) {
 		// new eraseblock , check for bad block(s)
 		// Stay in the loop to be sure if the mtdoffset changes because
@@ -379,7 +449,8 @@  int main(int argc, char * const argv[])
 			offs = blockstart;
 			baderaseblock = false;
 			if (!quiet)
-				fprintf (stdout, "Writing data to block %x\n", blockstart);
+				fprintf (stdout, "Writing data to block %d at offset 0x%x\n",
+						 blockstart / meminfo.erasesize, blockstart);
 
 			/* Check all the blocks in an erase block for bad blocks */
 			do {
@@ -404,18 +475,50 @@  int main(int argc, char * const argv[])
 		}
 
 		readlen = meminfo.writesize;
-		if (pad && (imglen < readlen))
-		{
-			readlen = imglen;
-			memset(writebuf + readlen, 0xff, meminfo.writesize - readlen);
-		}
 
-		/* Read Page Data from input file */
-		if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
-			if (cnt == 0)	// EOF
+		if (ifd != STDIN_FILENO) {
+			if (pad && (imglen < readlen))
+			{
+				readlen = imglen;
+				erase_buffer(writebuf + readlen, meminfo.writesize - readlen);
+			}
+
+			/* Read Page Data from input file */
+			if ((cnt = read(ifd, writebuf, readlen)) != readlen) {
+				if (cnt == 0)	// EOF
+					break;
+				perror ("File I/O error on input file");
+				goto closeall;
+			}
+		} 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");
+					goto closeall;
+				}
+				tinycnt += cnt;
+			}
+
+			/* No padding needed - we are done */
+			if (tinycnt == 0) {
+				imglen = 0;
 				break;
-			perror ("File I/O error on input file");
-			goto closeall;
+			}
+
+			/* 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 (writeoob) {
@@ -499,7 +602,9 @@  int main(int argc, char * const argv[])
 
 			continue;
 		}
-		imglen -= readlen;
+		if (ifd != STDIN_FILENO) {
+			imglen -= readlen;
+		}
 		mtdoffset += meminfo.writesize;
 	}
 
@@ -517,7 +622,7 @@  restoreoob:
 
 	close(fd);
 
-	if (imglen > 0) {
+	if ((ifd != STDIN_FILENO) && (imglen > 0)) {
 		perror ("Data was only partially written due to error\n");
 		exit (EXIT_FAILURE);
 	}