Message ID | 1220848159-22407-1-git-send-email-gerickson@nuovations.com |
---|---|
State | Accepted |
Commit | 2d0b9f58998c54dd114ff5c4476d10cc03a21864 |
Headers | show |
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.
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
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
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
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
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); }
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>