[PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[Brian Norris]

We should allow the dump length to be 64-bit, especially since the value
was read in as a "long long" by strtoll().

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nanddump.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/nanddump.c b/nanddump.c
index 86a71c9..fe29596 100644
--- a/nanddump.c
+++ b/nanddump.c
@@ -83,7 +83,7 @@ static bool			noecc = false;		// don't error correct
 static bool			noskipbad = false;	// don't skip bad blocks
 static bool			omitoob = false;	// omit oob data
 static unsigned long long	start_addr;		// start address
-static unsigned long		length;			// dump length
+static unsigned long long	length;			// dump length
 static const char		*mtddev;		// mtd device name
 static const char		*dumpfile;		// dump file name
 static bool			omitbad = false;
-- 
1.7.0.4

[PATCH 02/10] mtd-utils: nandwrite: Comment, style fixups

[Brian Norris]

Comment on "blockalign" default value is incorrect; it only defaults
to a 1x multiplier of the actual block size. Perhaps this is a relic
of early NAND where all block sizes were 16KB?

Reformatted a multi-line comment.

Changed separete "if" statements to a combined "if-else-if" since they
were logically combinable. Should have no effect on results with minor
effect on efficiency.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index b0c4366..3fbfca9 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -120,7 +120,7 @@ static bool		forcelegacy = false;
 static bool		noecc = false;
 static bool		noskipbad = false;
 static bool		pad = false;
-static int		blockalign = 1; /* default to using 16K block size */
+static int		blockalign = 1; /* default to using actual block size */
 
 static void process_options(int argc, char * const argv[])
 {
@@ -483,8 +483,7 @@ int main(int argc, char * const argv[])
 				if ((ret = ioctl(fd, MEMGETBADBLOCK, &offs)) < 0) {
 					perror("ioctl(MEMGETBADBLOCK)");
 					goto closeall;
-				}
-				if (ret == 1) {
+				} else if (ret == 1) {
 					baderaseblock = true;
 					if (!quiet)
 						fprintf(stderr, "Bad block at %x, %u block(s) "
@@ -520,9 +519,11 @@ int main(int argc, char * const argv[])
 
 			/* No padding needed - we are done */
 			if (tinycnt == 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
+				/*
+				 * For standard input, set imglen to 0 to signal
+				 * the end of the "file". For nonstandard input,
+				 * leave it as-is to detect an early EOF.
+				 */
 				if (ifd == STDIN_FILENO) {
 					imglen = 0;
 				}
-- 
1.7.0.4

[PATCH 03/10] mtd-utils: nandwrite: Clarify usage of aligned "erasesize"

[Brian Norris]

Due to the presence of the "--block-align" flag, nandwrite uses a
blocksize throughout that, depeding on the execution parameters, may
not be the actual erasesize of the NAND flash. In order to clarify
this situation for the untrained viewer of nandwrite's code, we should
not change the value of "meminfo.erasesize" itself; rather, we can
utilize a separate, calculated "ebsize_aligned". Then, when a user
actually wants to refer to the physical erasesize, it's straightforward.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   30 +++++++++++++++++-------------
 1 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 3fbfca9..66a9ef7 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -275,6 +275,7 @@ int main(int argc, char * const argv[])
 	// points to the OOB for the current page in filebuf
 	unsigned char *oobreadbuf = NULL;
 	unsigned char *oobbuf = NULL;
+	int ebsize_aligned;
 
 	process_options(argc, argv);
 
@@ -296,9 +297,12 @@ int main(int argc, char * const argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	/* Set erasesize to specified number of blocks - to match jffs2
-	 * (virtual) block size */
-	meminfo.erasesize *= blockalign;
+	/*
+	 * Pretend erasesize is specified number of blocks - to match jffs2
+	 *   (virtual) block size
+	 * Use this value throughout unless otherwise necessary
+	 */
+	ebsize_aligned = meminfo.erasesize * blockalign;
 
 	if (mtdoffset & (meminfo.writesize - 1)) {
 		fprintf(stderr, "The start address is not page-aligned !\n"
@@ -435,7 +439,7 @@ int main(int argc, char * const argv[])
 	}
 
 	// Allocate a buffer big enough to contain all the data (OOB included) for one eraseblock
-	filebuf_max = pagelen * meminfo.erasesize / meminfo.writesize;
+	filebuf_max = pagelen * ebsize_aligned / meminfo.writesize;
 	filebuf = xmalloc(filebuf_max);
 	erase_buffer(filebuf, filebuf_max);
 
@@ -459,8 +463,8 @@ int main(int argc, char * const argv[])
 		 * skipped block(s) is also bad (number of blocks depending on
 		 * the blockalign).
 		 */
-		while (blockstart != (mtdoffset & (~meminfo.erasesize + 1))) {
-			blockstart = mtdoffset & (~meminfo.erasesize + 1);
+		while (blockstart != (mtdoffset & (~ebsize_aligned + 1))) {
+			blockstart = mtdoffset & (~ebsize_aligned + 1);
 			offs = blockstart;
 
 			// if writebuf == filebuf, we are rewinding so we must not
@@ -474,7 +478,7 @@ int main(int argc, char * const argv[])
 			baderaseblock = false;
 			if (!quiet)
 				fprintf(stdout, "Writing data to block %d at offset 0x%x\n",
-						 blockstart / meminfo.erasesize, blockstart);
+						 blockstart / ebsize_aligned, blockstart);
 
 			/* Check all the blocks in an erase block for bad blocks */
 			if (noskipbad)
@@ -492,10 +496,10 @@ int main(int argc, char * const argv[])
 				}
 
 				if (baderaseblock) {
-					mtdoffset = blockstart + meminfo.erasesize;
+					mtdoffset = blockstart + ebsize_aligned;
 				}
-				offs +=  meminfo.erasesize / blockalign;
-			} while (offs < blockstart + meminfo.erasesize);
+				offs +=  ebsize_aligned / blockalign;
+			} while (offs < blockstart + ebsize_aligned);
 
 		}
 
@@ -642,7 +646,7 @@ int main(int argc, char * const argv[])
 			writebuf = filebuf;
 
 			erase.start = blockstart;
-			erase.length = meminfo.erasesize;
+			erase.length = ebsize_aligned;
 			fprintf(stderr, "Erasing failed write from %08lx-%08lx\n",
 				(long)erase.start, (long)erase.start+erase.length-1);
 			if (ioctl(fd, MEMERASE, &erase) != 0) {
@@ -654,14 +658,14 @@ int main(int argc, char * const argv[])
 			}
 
 			if (markbad) {
-				loff_t bad_addr = mtdoffset & (~(meminfo.erasesize / blockalign) + 1);
+				loff_t bad_addr = mtdoffset & (~meminfo.erasesize + 1);
 				fprintf(stderr, "Marking block at %08lx bad\n", (long)bad_addr);
 				if (ioctl(fd, MEMSETBADBLOCK, &bad_addr)) {
 					perror("MEMSETBADBLOCK");
 					goto closeall;
 				}
 			}
-			mtdoffset = blockstart + meminfo.erasesize;
+			mtdoffset = blockstart + ebsize_aligned;
 
 			continue;
 		}
-- 
1.7.0.4

[PATCH 04/10] mtd-utils: nandwrite: switch "oobsize" for "writesize"

[Brian Norris]

The text of a printf() states that we're printing OOB area, but the
corresponding argument passes writesize. That probably wasn't the intent.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 66a9ef7..a3ac968 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -433,7 +433,7 @@ int main(int argc, char * const argv[])
 	// Check, if length fits into device
 	if (((imglen / pagelen) * meminfo.writesize) > (meminfo.size - mtdoffset)) {
 		fprintf(stderr, "Image %d bytes, NAND page %d bytes, OOB area %u bytes, device size %u bytes\n",
-				imglen, pagelen, meminfo.writesize, meminfo.size);
+				imglen, pagelen, meminfo.oobsize, meminfo.size);
 		perror("Input file does not fit into device");
 		goto closeall;
 	}
-- 
1.7.0.4

[PATCH 05/10] mtd-utils: nandwrite: Use libmtd to get correct mtd parameters

[Brian Norris]

Begin utilizing libmtd for MTD operations: use mtd_get_dev_info() to return
a more detailed set of information about our MTD. Most importantly, libmtd
will yield a 64-bit "size" parameter. This is necessary to properly detect
devices larer than 4GB.

printf() arguments needed reformatted for the new mtd_dev_info data types.
In addition, the printf() was restructured to keep lines shorter.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   60 +++++++++++++++++++++++++++++++---------------------------
 1 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index a3ac968..d05d257 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -42,6 +42,7 @@
 #include <asm/types.h>
 #include "mtd/mtd-user.h"
 #include "common.h"
+#include <libmtd.h>
 
 // oob layouts to pass into the kernel as default
 static struct nand_oobinfo none_oobinfo = {
@@ -259,7 +260,7 @@ int main(int argc, char * const argv[])
 	int imglen = 0, pagelen;
 	bool baderaseblock = false;
 	int blockstart = -1;
-	struct mtd_info_user meminfo;
+	struct mtd_dev_info mtd;
 	struct mtd_oob_buf oob;
 	loff_t offs;
 	int ret;
@@ -275,6 +276,7 @@ int main(int argc, char * const argv[])
 	// points to the OOB for the current page in filebuf
 	unsigned char *oobreadbuf = NULL;
 	unsigned char *oobbuf = NULL;
+	libmtd_t mtd_desc;
 	int ebsize_aligned;
 
 	process_options(argc, argv);
@@ -290,24 +292,24 @@ int main(int argc, char * const argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	mtd_desc = libmtd_open();
+	if (!mtd_desc)
+		return errmsg("can't initialize libmtd");
 	/* Fill in MTD device capability structure */
-	if (ioctl(fd, MEMGETINFO, &meminfo) != 0) {
-		perror("MEMGETINFO");
-		close(fd);
-		exit(EXIT_FAILURE);
-	}
+	if (mtd_get_dev_info(mtd_desc, mtd_device, &mtd) < 0)
+		return errmsg("mtd_get_dev_info failed");
 
 	/*
 	 * Pretend erasesize is specified number of blocks - to match jffs2
 	 *   (virtual) block size
 	 * Use this value throughout unless otherwise necessary
 	 */
-	ebsize_aligned = meminfo.erasesize * blockalign;
+	ebsize_aligned = mtd.eb_size * blockalign;
 
-	if (mtdoffset & (meminfo.writesize - 1)) {
+	if (mtdoffset & (mtd.min_io_size - 1)) {
 		fprintf(stderr, "The start address is not page-aligned !\n"
 				"The pagesize of this NAND Flash is 0x%x.\n",
-				meminfo.writesize);
+				mtd.min_io_size);
 		close(fd);
 		exit(EXIT_FAILURE);
 	}
@@ -373,7 +375,7 @@ int main(int argc, char * const argv[])
 			fprintf(stderr, "Use -f option to enforce legacy placement on autoplacement enabled mtd device\n");
 			goto restoreoob;
 		}
-		if (meminfo.oobsize == 8) {
+		if (mtd.oob_size == 8) {
 			if (forceyaffs) {
 				fprintf(stderr, "YAFSS cannot operate on 256 Byte page size");
 				goto restoreoob;
@@ -388,7 +390,7 @@ int main(int argc, char * const argv[])
 		}
 	}
 
-	oob.length = meminfo.oobsize;
+	oob.length = mtd.oob_size;
 	oob.ptr = noecc ? oobreadbuf : oobbuf;
 
 	/* Determine if we are reading from standard input or from a file. */
@@ -403,7 +405,7 @@ int main(int argc, char * const argv[])
 		goto restoreoob;
 	}
 
-	pagelen = meminfo.writesize + ((writeoob) ? meminfo.oobsize : 0);
+	pagelen = mtd.min_io_size + ((writeoob) ? mtd.oob_size : 0);
 
 	/*
 	 * For the standard input case, the input size is merely an
@@ -431,20 +433,21 @@ int main(int argc, char * const argv[])
 	}
 
 	// Check, if length fits into device
-	if (((imglen / pagelen) * meminfo.writesize) > (meminfo.size - mtdoffset)) {
-		fprintf(stderr, "Image %d bytes, NAND page %d bytes, OOB area %u bytes, device size %u bytes\n",
-				imglen, pagelen, meminfo.oobsize, meminfo.size);
+	if (((imglen / pagelen) * mtd.min_io_size) > (mtd.size - mtdoffset)) {
+		fprintf(stderr, "Image %d bytes, NAND page %d bytes, OOB area %d"
+				" bytes, device size %lld bytes\n",
+				imglen, pagelen, mtd.oob_size, mtd.size);
 		perror("Input file does not fit into device");
 		goto closeall;
 	}
 
 	// Allocate a buffer big enough to contain all the data (OOB included) for one eraseblock
-	filebuf_max = pagelen * ebsize_aligned / meminfo.writesize;
+	filebuf_max = pagelen * ebsize_aligned / mtd.min_io_size;
 	filebuf = xmalloc(filebuf_max);
 	erase_buffer(filebuf, filebuf_max);
 
-	oobbuf = xmalloc(meminfo.oobsize);
-	erase_buffer(oobbuf, meminfo.oobsize);
+	oobbuf = xmalloc(mtd.oob_size);
+	erase_buffer(oobbuf, mtd.oob_size);
 
 	/*
 	 * Get data from input and write to the device while there is
@@ -454,7 +457,7 @@ int main(int argc, char * const argv[])
 	 * length or zero.
 	 */
 	while (((imglen > 0) || (writebuf < (filebuf + filebuf_len)))
-		&& (mtdoffset < meminfo.size)) {
+		&& (mtdoffset < mtd.size)) {
 		/*
 		 * New eraseblock, check for bad block(s)
 		 * Stay in the loop to be sure that, if mtdoffset changes because
@@ -504,8 +507,8 @@ int main(int argc, char * const argv[])
 		}
 
 		// 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;
+		if ((writebuf + mtd.min_io_size) > (filebuf + filebuf_len)) {
+			int readlen = mtd.min_io_size;
 
 			int alreadyread = (filebuf + filebuf_len) - writebuf;
 			int tinycnt = alreadyread;
@@ -556,11 +559,11 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
-			oobreadbuf = writebuf + meminfo.writesize;
+			oobreadbuf = writebuf + mtd.min_io_size;
 
 			// 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;
+			if ((oobreadbuf + mtd.oob_size) > (filebuf + filebuf_len)) {
+				int readlen = mtd.oob_size;
 				int alreadyread = (filebuf + filebuf_len) - oobreadbuf;
 				int tinycnt = alreadyread;
 
@@ -619,7 +622,7 @@ int main(int argc, char * const argv[])
 				} else {
 					/* Set at least the ecc byte positions to 0xff */
 					start = old_oobinfo.eccbytes;
-					len = meminfo.oobsize - start;
+					len = mtd.oob_size - start;
 					memcpy(oobbuf + start,
 							oobreadbuf + start,
 							len);
@@ -634,7 +637,7 @@ int main(int argc, char * const argv[])
 		}
 
 		/* Write out the Page data */
-		if (pwrite(fd, writebuf, meminfo.writesize, mtdoffset) != meminfo.writesize) {
+		if (pwrite(fd, writebuf, mtd.min_io_size, mtdoffset) != mtd.min_io_size) {
 			erase_info_t erase;
 
 			if (errno != EIO) {
@@ -658,7 +661,7 @@ int main(int argc, char * const argv[])
 			}
 
 			if (markbad) {
-				loff_t bad_addr = mtdoffset & (~meminfo.erasesize + 1);
+				loff_t bad_addr = mtdoffset & (~mtd.eb_size + 1);
 				fprintf(stderr, "Marking block at %08lx bad\n", (long)bad_addr);
 				if (ioctl(fd, MEMSETBADBLOCK, &bad_addr)) {
 					perror("MEMSETBADBLOCK");
@@ -669,7 +672,7 @@ int main(int argc, char * const argv[])
 
 			continue;
 		}
-		mtdoffset += meminfo.writesize;
+		mtdoffset += mtd.min_io_size;
 		writebuf += pagelen;
 	}
 
@@ -679,6 +682,7 @@ closeall:
 	close(ifd);
 
 restoreoob:
+	libmtd_close(mtd_desc);
 	free(filebuf);
 	free(oobbuf);
 
-- 
1.7.0.4

[PATCH 06/10] mtd-utils: nandwrite: Use 64-bit offset

[Brian Norris]

To support large NAND devices, we need 64-bit data types for
write offsets. This patch makes data type changes along with
their corresponding printf() formats and the input conversion
(i.e., use "strtoll()" instead of "strol()").

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index d05d257..b362c29 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -109,7 +109,7 @@ static void display_version(void)
 
 static const char	*standard_input = "-";
 static const char	*mtd_device, *img;
-static int		mtdoffset = 0;
+static long long	mtdoffset = 0;
 static bool		quiet = false;
 static bool		writeoob = false;
 static bool		rawoob = false;
@@ -201,7 +201,7 @@ static void process_options(int argc, char * const argv[])
 				writeoob = true;
 				break;
 			case 's':
-				mtdoffset = strtol(optarg, NULL, 0);
+				mtdoffset = strtoll(optarg, NULL, 0);
 				break;
 			case 'b':
 				blockalign = atoi(optarg);
@@ -213,7 +213,7 @@ static void process_options(int argc, char * const argv[])
 	}
 
 	if (mtdoffset < 0) {
-		fprintf(stderr, "Can't specify a negative device offset `%d'\n",
+		fprintf(stderr, "Can't specify a negative device offset `%lld'\n",
 				mtdoffset);
 		exit(EXIT_FAILURE);
 	}
@@ -259,7 +259,7 @@ int main(int argc, char * const argv[])
 	int ifd = -1;
 	int imglen = 0, pagelen;
 	bool baderaseblock = false;
-	int blockstart = -1;
+	long long blockstart = -1;
 	struct mtd_dev_info mtd;
 	struct mtd_oob_buf oob;
 	loff_t offs;
@@ -480,7 +480,7 @@ int main(int argc, char * const argv[])
 
 			baderaseblock = false;
 			if (!quiet)
-				fprintf(stdout, "Writing data to block %d at offset 0x%x\n",
+				fprintf(stdout, "Writing data to block %lld at offset 0x%llx\n",
 						 blockstart / ebsize_aligned, blockstart);
 
 			/* Check all the blocks in an erase block for bad blocks */
@@ -494,7 +494,7 @@ int main(int argc, char * const argv[])
 					baderaseblock = true;
 					if (!quiet)
 						fprintf(stderr, "Bad block at %x, %u block(s) "
-								"from %x will be skipped\n",
+								"from %llx will be skipped\n",
 								(int) offs, blockalign, blockstart);
 				}
 
-- 
1.7.0.4

[PATCH 07/10] mtd-utils: nandwrite: avoid NULL buffer pointers

[Brian Norris]

Commit 07005d915d6a79dbdee14b0c4360df5058c3a98b made changes to the
buffer allocation in nandwrite and did not handle all affected code
areas properly. In particular, we were assigning:
	oob.ptr = noecc ? oobreadbuf : oobbuf;
However, since oobreadbuf and oobbuf are declared dynamically, they
are NULL at this point. If they aren't properly assigned later, we
unwittingly are passing a NULL pointer as oob buffer.

This assignment line is best moved after the buffer allocations and
pointer assignment.

Effects of this problem can be seen when writing oob data with the "-o"
flag and without the "-n" flag:
	$ ./nandwrite -o /dev/mtd0 img.bin
	Writing data to block 0 at offset 0x0
	ioctl(MEMWRITEOOB): Bad address
	Data was only partially written due to error
	: Bad address

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index b362c29..8ec5afe 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -391,7 +391,6 @@ int main(int argc, char * const argv[])
 	}
 
 	oob.length = mtd.oob_size;
-	oob.ptr = noecc ? oobreadbuf : oobbuf;
 
 	/* Determine if we are reading from standard input or from a file. */
 	if (strcmp(img, standard_input) == 0) {
@@ -594,9 +593,7 @@ int main(int argc, char * const argv[])
 				}
 			}
 
-			if (noecc) {
-				oob.ptr = oobreadbuf;
-			} else {
+			if (!noecc) {
 				int i, start, len;
 				int tags_pos = 0;
 				/*
@@ -630,6 +627,7 @@ int main(int argc, char * const argv[])
 			}
 			/* Write OOB data first, as ecc will be placed in there */
 			oob.start = mtdoffset;
+			oob.ptr = noecc ? oobreadbuf : oobbuf;
 			if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
 				perror("ioctl(MEMWRITEOOB)");
 				goto closeall;
-- 
1.7.0.4

[PATCH 08/10] mtd-utils: nandwrite: prevent 32-bit overflow

[Brian Norris]

For large block- and page-sizes, the multiplication of ebsize_aligned
and pagelen can overflow a 32-bit integer.  This overflow can be
prevented by a simple change in order of operations (i.e., do division
first).

Since ebsize_aligned is always a multiple of mtd.min_io_size, this
produces no change in results.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 8ec5afe..364acdf 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -441,7 +441,7 @@ int main(int argc, char * const argv[])
 	}
 
 	// Allocate a buffer big enough to contain all the data (OOB included) for one eraseblock
-	filebuf_max = pagelen * ebsize_aligned / mtd.min_io_size;
+	filebuf_max = ebsize_aligned / mtd.min_io_size * pagelen;
 	filebuf = xmalloc(filebuf_max);
 	erase_buffer(filebuf, filebuf_max);
 
-- 
1.7.0.4

[PATCH 09/10] mtd-utils: nanddump: type consistency

[Brian Norris]

Change type off "offs" for type consistency of 64-bit data types.
Also change an accompanying printf().

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 364acdf..a28f7fb 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -262,7 +262,7 @@ int main(int argc, char * const argv[])
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
 	struct mtd_oob_buf oob;
-	loff_t offs;
+	long long offs;
 	int ret;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
@@ -492,9 +492,9 @@ int main(int argc, char * const argv[])
 				} else if (ret == 1) {
 					baderaseblock = true;
 					if (!quiet)
-						fprintf(stderr, "Bad block at %x, %u block(s) "
+						fprintf(stderr, "Bad block at %llx, %u block(s) "
 								"from %llx will be skipped\n",
-								(int) offs, blockalign, blockstart);
+								offs, blockalign, blockstart);
 				}
 
 				if (baderaseblock) {
-- 
1.7.0.4

[PATCH 10/10] mtd-utils: nandwrite: full 64-bit support w/ libmtd

[Brian Norris]

Several ioctls are replaced with libmtd calls which should give us 64-bit
support for large devices. libmtd mostly provides drop-in replacements
for the functionality we need. However, when we require erasure of a
badly-written block, mtd_erase() only erases a single block, whereas
MEMERASE could erase a larger region. In nandwrite, we may have a "virtual
blocksize" of more than one (when blockalign > 1). Thus, I added a loop
for this case.

The mtd_oob_buf struct is no longer needed, nor is "erase_info_t".

Error messages for the new libmtd calls reflect the style found in
flash_erase.

Tested with nandsim and with NAND chips up to 4GB in size (I don't have
a device that truly requires 64-bit addressing yet).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   49 +++++++++++++++++++++++--------------------------
 1 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index a28f7fb..37d2204 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -261,7 +261,6 @@ int main(int argc, char * const argv[])
 	bool baderaseblock = false;
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
-	struct mtd_oob_buf oob;
 	long long offs;
 	int ret;
 	int oobinfochanged = 0;
@@ -390,8 +389,6 @@ int main(int argc, char * const argv[])
 		}
 	}
 
-	oob.length = mtd.oob_size;
-
 	/* Determine if we are reading from standard input or from a file. */
 	if (strcmp(img, standard_input) == 0) {
 		ifd = STDIN_FILENO;
@@ -486,8 +483,8 @@ int main(int argc, char * const argv[])
 			if (noskipbad)
 				continue;
 			do {
-				if ((ret = ioctl(fd, MEMGETBADBLOCK, &offs)) < 0) {
-					perror("ioctl(MEMGETBADBLOCK)");
+				if ((ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned)) < 0) {
+					sys_errmsg("%s: MTD get bad block failed", mtd_device);
 					goto closeall;
 				} else if (ret == 1) {
 					baderaseblock = true;
@@ -626,43 +623,43 @@ int main(int argc, char * const argv[])
 				}
 			}
 			/* Write OOB data first, as ecc will be placed in there */
-			oob.start = mtdoffset;
-			oob.ptr = noecc ? oobreadbuf : oobbuf;
-			if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
-				perror("ioctl(MEMWRITEOOB)");
+			if (mtd_write_oob(mtd_desc, &mtd, fd, mtdoffset,
+						mtd.oob_size,
+						noecc ? oobreadbuf : oobbuf)) {
+				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				goto closeall;
 			}
 		}
 
 		/* Write out the Page data */
-		if (pwrite(fd, writebuf, mtd.min_io_size, mtdoffset) != mtd.min_io_size) {
-			erase_info_t erase;
-
+		if (mtd_write(&mtd, fd, mtdoffset / mtd.eb_size, mtdoffset % mtd.eb_size,
+					writebuf, mtd.min_io_size)) {
+			int i;
 			if (errno != EIO) {
-				perror("pwrite");
+				sys_errmsg("%s: MTD write failure", mtd_device);
 				goto closeall;
 			}
 
 			/* Must rewind to blockstart if we can */
 			writebuf = filebuf;
 
-			erase.start = blockstart;
-			erase.length = ebsize_aligned;
-			fprintf(stderr, "Erasing failed write from %08lx-%08lx\n",
-				(long)erase.start, (long)erase.start+erase.length-1);
-			if (ioctl(fd, MEMERASE, &erase) != 0) {
-				int errno_tmp = errno;
-				perror("MEMERASE");
-				if (errno_tmp != EIO) {
-					goto closeall;
+			fprintf(stderr, "Erasing failed write from %#08llx to %#08llx\n",
+				blockstart, blockstart + ebsize_aligned - 1);
+			for (i = blockstart; i < blockstart + ebsize_aligned; i += mtd.eb_size) {
+				if (mtd_erase(mtd_desc, &mtd, fd, mtd.eb_size)) {
+					int errno_tmp = errno;
+					sys_errmsg("%s: MTD Erase failure", mtd_device);
+					if (errno_tmp != EIO) {
+						goto closeall;
+					}
 				}
 			}
 
 			if (markbad) {
-				loff_t bad_addr = mtdoffset & (~mtd.eb_size + 1);
-				fprintf(stderr, "Marking block at %08lx bad\n", (long)bad_addr);
-				if (ioctl(fd, MEMSETBADBLOCK, &bad_addr)) {
-					perror("MEMSETBADBLOCK");
+				fprintf(stderr, "Marking block at %08llx bad\n",
+						mtdoffset & (~mtd.eb_size + 1));
+				if (mtd_mark_bad(&mtd, fd, mtdoffset / mtd.eb_size)) {
+					sys_errmsg("%s: MTD Mark bad block failure", mtd_device);
 					goto closeall;
 				}
 			}
-- 
1.7.0.4

Re: [PATCH 08/10] mtd-utils: nandwrite: prevent 32-bit overflow

[Mike Frysinger]

On Wed, Nov 3, 2010 at 04:27, Brian Norris wrote:
> For large block- and page-sizes, the multiplication of ebsize_aligned
> and pagelen can overflow a 32-bit integer.  This overflow can be
> prevented by a simple change in order of operations (i.e., do division
> first).
>
> -       filebuf_max = pagelen * ebsize_aligned / mtd.min_io_size;
> +       filebuf_max = ebsize_aligned / mtd.min_io_size * pagelen;

i'd insert a comment noting that order of operations matters to avoid
32bit overflow so someone doesnt switch it back by accident in the
future
-mike

Re: [PATCH 09/10] mtd-utils: nanddump: type consistency

[Mike Frysinger]

On Wed, Nov 3, 2010 at 04:27, Brian Norris wrote:
>  nandwrite.c |    6 +++---

the subject says "nanddump" ...

> --- a/nandwrite.c
> +++ b/nandwrite.c
> @@ -262,7 +262,7 @@ int main(int argc, char * const argv[])
>        long long blockstart = -1;
>        struct mtd_dev_info mtd;
>        struct mtd_oob_buf oob;
> -       loff_t offs;
> +       long long offs;
>        int ret;
>        int oobinfochanged = 0;
>        struct nand_oobinfo old_oobinfo;

i dont think this is quite right.  "offs" is given to an ioctl which
takes __kernel_loff_t ... we shouldnt assume "long long" and
"__kernel_loff_t" are always the same.
-mike

Re: [PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[Mike Frysinger]

most of these changes look sane ... i commented on the ones that i
think need tweaking, but for the rest, Acked-by: Mike Frysinger
<vapier@gentoo.org>
-mike

Re: [PATCH 08/10] mtd-utils: nandwrite: prevent 32-bit overflow

[Artem Bityutskiy]

On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
> For large block- and page-sizes, the multiplication of ebsize_aligned
> and pagelen can overflow a 32-bit integer.  This overflow can be
> prevented by a simple change in order of operations (i.e., do division
> first).
> 
> Since ebsize_aligned is always a multiple of mtd.min_io_size, this
> produces no change in results.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Brian, increasingly have less and less time for mtd, but I remember
about your patches - I'll pick them as soon as I have some time. You can
meanwhile address Mikes comments.

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

Re: [PATCH 09/10] mtd-utils: nanddump: type consistency

[Brian Norris]

On 11/9/2010 1:51 AM, Mike Frysinger wrote:
> On Wed, Nov 3, 2010 at 04:27, Brian Norris wrote:
>>  nandwrite.c |    6 +++---
> 
> the subject says "nanddump" ...

Oops, I'll change that if I send a "v2".

>> --- a/nandwrite.c
>> +++ b/nandwrite.c
>> @@ -262,7 +262,7 @@ int main(int argc, char * const argv[])
>>        long long blockstart = -1;
>>        struct mtd_dev_info mtd;
>>        struct mtd_oob_buf oob;
>> -       loff_t offs;
>> +       long long offs;
>>        int ret;
>>        int oobinfochanged = 0;
>>        struct nand_oobinfo old_oobinfo;
> 
> i dont think this is quite right.  "offs" is given to an ioctl which
> takes __kernel_loff_t ... we shouldnt assume "long long" and
> "__kernel_loff_t" are always the same.

This problem is sort of a "which comes first" question for me: I put
this patch before patch 10, where in patch 10, the ioctl in question is
replaced with an entirely different call to "mtd_is_bad".

In the new usage (patch 10), "mtd_is_bad" needs the eraseblock number as
an int, so there's some arithmetic going on ("offs" divided by ebsize)
that passes an int to "mtd_is_bad".

I was just trying to unify the usage of different 64-bit data types, and
after patch 10, there's no need to be using the "loff_t" type. However,
if it makes more sense, I can rearrange patch 9 and 10 such that the
type change comes after the ioctl is no longer called directly.

Thanks,
Brian

Re: [PATCH 09/10] mtd-utils: nanddump: type consistency

[Mike Frysinger]

On Tue, Nov 9, 2010 at 13:19, Brian Norris wrote:
> On 11/9/2010 1:51 AM, Mike Frysinger wrote:
>> On Wed, Nov 3, 2010 at 04:27, Brian Norris wrote:
>>> --- a/nandwrite.c
>>> +++ b/nandwrite.c
>>> @@ -262,7 +262,7 @@ int main(int argc, char * const argv[])
>>>        long long blockstart = -1;
>>>        struct mtd_dev_info mtd;
>>>        struct mtd_oob_buf oob;
>>> -       loff_t offs;
>>> +       long long offs;
>>>        int ret;
>>>        int oobinfochanged = 0;
>>>        struct nand_oobinfo old_oobinfo;
>>
>> i dont think this is quite right.  "offs" is given to an ioctl which
>> takes __kernel_loff_t ... we shouldnt assume "long long" and
>> "__kernel_loff_t" are always the same.
>
> This problem is sort of a "which comes first" question for me: I put
> this patch before patch 10, where in patch 10, the ioctl in question is
> replaced with an entirely different call to "mtd_is_bad".
>
> In the new usage (patch 10), "mtd_is_bad" needs the eraseblock number as
> an int, so there's some arithmetic going on ("offs" divided by ebsize)
> that passes an int to "mtd_is_bad".
>
> I was just trying to unify the usage of different 64-bit data types, and
> after patch 10, there's no need to be using the "loff_t" type. However,
> if it makes more sense, I can rearrange patch 9 and 10 such that the
> type change comes after the ioctl is no longer called directly.

i'd leave the type unchanged when using the ioctl() and change it only
when moving to a different API
-mike

[PATCH v2 08/10] mtd-utils: nandwrite: prevent 32-bit overflow

[Brian Norris]

For large block- and page-sizes, the multiplication of ebsize_aligned
and pagelen can overflow a 32-bit integer.  This overflow can be
prevented by a simple change in order of operations (i.e., do division
first).

Since ebsize_aligned is always a multiple of mtd.min_io_size, this
produces no change in results.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 8ec5afe..aea7572 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -440,8 +440,13 @@ 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 * ebsize_aligned / mtd.min_io_size;
+	/*
+	 * Allocate a buffer big enough to contain all the data (OOB included)
+	 * for one eraseblock. The order of operations here matters; if ebsize
+	 * and pagelen are large enough, then "ebsize_aligned * pagelen" could
+	 * overflow a 32-bit data type.
+	 */
+	filebuf_max = ebsize_aligned / mtd.min_io_size * pagelen;
 	filebuf = xmalloc(filebuf_max);
 	erase_buffer(filebuf, filebuf_max);
 
-- 
1.7.0.4

[PATCH v2 09/10] mtd-utils: nandwrite: full 64-bit support w/ libmtd

[Brian Norris]

Several ioctls are replaced with libmtd calls which should give us 64-bit
support for large devices. libmtd mostly provides drop-in replacements
for the functionality we need. However, when we require erasure of a
badly-written block, mtd_erase() only erases a single block, whereas
MEMERASE could erase a larger region. In nandwrite, we may have a "virtual
blocksize" of more than one (when blockalign > 1). Thus, I added a loop
for this case.

The mtd_oob_buf struct is no longer needed, nor is "erase_info_t".

Error messages for the new libmtd calls reflect the style found in
flash_erase.

Tested with nandsim and with NAND chips up to 4GB in size (I don't have
a device that truly requires 64-bit addressing yet).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |   49 +++++++++++++++++++++++--------------------------
 1 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index aea7572..00e7c28 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -261,7 +261,6 @@ int main(int argc, char * const argv[])
 	bool baderaseblock = false;
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
-	struct mtd_oob_buf oob;
 	loff_t offs;
 	int ret;
 	int oobinfochanged = 0;
@@ -390,8 +389,6 @@ int main(int argc, char * const argv[])
 		}
 	}
 
-	oob.length = mtd.oob_size;
-
 	/* Determine if we are reading from standard input or from a file. */
 	if (strcmp(img, standard_input) == 0) {
 		ifd = STDIN_FILENO;
@@ -491,8 +488,8 @@ int main(int argc, char * const argv[])
 			if (noskipbad)
 				continue;
 			do {
-				if ((ret = ioctl(fd, MEMGETBADBLOCK, &offs)) < 0) {
-					perror("ioctl(MEMGETBADBLOCK)");
+				if ((ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned)) < 0) {
+					sys_errmsg("%s: MTD get bad block failed", mtd_device);
 					goto closeall;
 				} else if (ret == 1) {
 					baderaseblock = true;
@@ -631,43 +628,43 @@ int main(int argc, char * const argv[])
 				}
 			}
 			/* Write OOB data first, as ecc will be placed in there */
-			oob.start = mtdoffset;
-			oob.ptr = noecc ? oobreadbuf : oobbuf;
-			if (ioctl(fd, MEMWRITEOOB, &oob) != 0) {
-				perror("ioctl(MEMWRITEOOB)");
+			if (mtd_write_oob(mtd_desc, &mtd, fd, mtdoffset,
+						mtd.oob_size,
+						noecc ? oobreadbuf : oobbuf)) {
+				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				goto closeall;
 			}
 		}
 
 		/* Write out the Page data */
-		if (pwrite(fd, writebuf, mtd.min_io_size, mtdoffset) != mtd.min_io_size) {
-			erase_info_t erase;
-
+		if (mtd_write(&mtd, fd, mtdoffset / mtd.eb_size, mtdoffset % mtd.eb_size,
+					writebuf, mtd.min_io_size)) {
+			int i;
 			if (errno != EIO) {
-				perror("pwrite");
+				sys_errmsg("%s: MTD write failure", mtd_device);
 				goto closeall;
 			}
 
 			/* Must rewind to blockstart if we can */
 			writebuf = filebuf;
 
-			erase.start = blockstart;
-			erase.length = ebsize_aligned;
-			fprintf(stderr, "Erasing failed write from %08lx-%08lx\n",
-				(long)erase.start, (long)erase.start+erase.length-1);
-			if (ioctl(fd, MEMERASE, &erase) != 0) {
-				int errno_tmp = errno;
-				perror("MEMERASE");
-				if (errno_tmp != EIO) {
-					goto closeall;
+			fprintf(stderr, "Erasing failed write from %#08llx to %#08llx\n",
+				blockstart, blockstart + ebsize_aligned - 1);
+			for (i = blockstart; i < blockstart + ebsize_aligned; i += mtd.eb_size) {
+				if (mtd_erase(mtd_desc, &mtd, fd, mtd.eb_size)) {
+					int errno_tmp = errno;
+					sys_errmsg("%s: MTD Erase failure", mtd_device);
+					if (errno_tmp != EIO) {
+						goto closeall;
+					}
 				}
 			}
 
 			if (markbad) {
-				loff_t bad_addr = mtdoffset & (~mtd.eb_size + 1);
-				fprintf(stderr, "Marking block at %08lx bad\n", (long)bad_addr);
-				if (ioctl(fd, MEMSETBADBLOCK, &bad_addr)) {
-					perror("MEMSETBADBLOCK");
+				fprintf(stderr, "Marking block at %08llx bad\n",
+						mtdoffset & (~mtd.eb_size + 1));
+				if (mtd_mark_bad(&mtd, fd, mtdoffset / mtd.eb_size)) {
+					sys_errmsg("%s: MTD Mark bad block failure", mtd_device);
 					goto closeall;
 				}
 			}
-- 
1.7.0.4

[PATCH v2 10/10] mtd-utils: nandwrite: type consistency

[Brian Norris]

Change type off "offs" for type consistency of 64-bit data types. The
"loff_t" type is no longer needed for the MEMGETBADBLOCK ioctl since
it isn't called dirently anymore - this is handled by mtd_is_bad().

Also change an accompanying printf().

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index 00e7c28..bbe69b0 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -261,7 +261,7 @@ int main(int argc, char * const argv[])
 	bool baderaseblock = false;
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
-	loff_t offs;
+	long long offs;
 	int ret;
 	int oobinfochanged = 0;
 	struct nand_oobinfo old_oobinfo;
@@ -494,9 +494,9 @@ int main(int argc, char * const argv[])
 				} else if (ret == 1) {
 					baderaseblock = true;
 					if (!quiet)
-						fprintf(stderr, "Bad block at %x, %u block(s) "
+						fprintf(stderr, "Bad block at %llx, %u block(s) "
 								"from %llx will be skipped\n",
-								(int) offs, blockalign, blockstart);
+								offs, blockalign, blockstart);
 				}
 
 				if (baderaseblock) {
-- 
1.7.0.4

Re: [PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[Artem Bityutskiy]

On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
> We should allow the dump length to be 64-bit, especially since the value
> was read in as a "long long" by strtoll().
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  nanddump.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/nanddump.c b/nanddump.c
> index 86a71c9..fe29596 100644
> --- a/nanddump.c
> +++ b/nanddump.c
> @@ -83,7 +83,7 @@ static bool			noecc = false;		// don't error correct
>  static bool			noskipbad = false;	// don't skip bad blocks
>  static bool			omitoob = false;	// omit oob data
>  static unsigned long long	start_addr;		// start address
> -static unsigned long		length;			// dump length
> +static unsigned long long	length;			// dump length
>  static const char		*mtddev;		// mtd device name
>  static const char		*dumpfile;		// dump file name
>  static bool			omitbad = false;

This patch does not apply. Which version of MTD utils do you use? This
change was done long time ago by the following commit:

commit b16c1b630491a461b3ebb55d714d7bb0cd122737
Author: Grant Erickson <gerickson@nuovations.com>
Date:   Sun Sep 7 20:45:21 2008 +0000

    nanddump: Qualifier Clean-up
    
    Static-qualified all globals except 'main' because they have no use
    beyond file scope.
    Constant-qualified MTD device and input positional parameter globals.
    Constant-qualified argv array.
    
    Signed-off-by: Grant Erickson <gerickson@nuovations.com>
    Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
    Signed-off-by: Josh Boyer <jwboyer@gmail.com>

Forgot to git-pull ? :-))))

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

Re: [PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[Artem Bityutskiy]

On Sat, 2010-11-13 at 13:31 +0200, Artem Bityutskiy wrote:
> Forgot to git-pull ? :-))))

Brian, ignore this e-mail please :-) I've just pushed this patch,
thanks!

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

Re: [PATCH 06/10] mtd-utils: nandwrite: Use 64-bit offset

[Artem Bityutskiy]

On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
> -				mtdoffset = strtol(optarg, NULL, 0);
> +				mtdoffset = strtoll(optarg, NULL, 0);

Ideally this should check for strtoll failures, but this just a side
note. Not sure if this is the best way, but I'd do it like this:

char *endp;

mtdoffset = strtoul(optarg, &endp, 0);
if (*endp != '\0' || endp == optarg)
	return errmsg("bad volume alignment: \"%s\"", optarg);

or something like this. The check for negative mtdoffset could be there
as well.

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

Re: [PATCH v2 09/10] mtd-utils: nandwrite: full 64-bit support w/ libmtd

[Artem Bityutskiy]

On Wed, 2010-11-10 at 22:39 -0800, Brian Norris wrote:
> Several ioctls are replaced with libmtd calls which should give us 64-bit
> support for large devices. libmtd mostly provides drop-in replacements
> for the functionality we need. However, when we require erasure of a
> badly-written block, mtd_erase() only erases a single block, whereas
> MEMERASE could erase a larger region. In nandwrite, we may have a "virtual
> blocksize" of more than one (when blockalign > 1). Thus, I added a loop
> for this case.

You could as well just introduce another libmtd helper - would be
cleaner, IMO.

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

Re: [PATCH 01/10] mtd-utils: nanddump: Allow 64-bit lengths

[Artem Bityutskiy]

On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
> We should allow the dump length to be 64-bit, especially since the value
> was read in as a "long long" by strtoll().
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I've pushed whole series, thanks a lot!

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

Re: [PATCH 06/10] mtd-utils: nandwrite: Use 64-bit offset

[Mike Frysinger]

On Sat, Nov 13, 2010 at 06:48, Artem Bityutskiy wrote:
> On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
>> -                             mtdoffset = strtol(optarg, NULL, 0);
>> +                             mtdoffset = strtoll(optarg, NULL, 0);
>
> Ideally this should check for strtoll failures, but this just a side
> note. Not sure if this is the best way, but I'd do it like this:
>
> char *endp;
>
> mtdoffset = strtoul(optarg, &endp, 0);
> if (*endp != '\0' || endp == optarg)
>        return errmsg("bad volume alignment: \"%s\"", optarg);
>
> or something like this. The check for negative mtdoffset could be there
> as well.

or use simple_strtoll() from common.h as that includes error handling ...
-mike

Re: [PATCH 06/10] mtd-utils: nandwrite: Use 64-bit offset

[Artem Bityutskiy]

On Sat, 2010-11-13 at 17:45 -0500, Mike Frysinger wrote:
> On Sat, Nov 13, 2010 at 06:48, Artem Bityutskiy wrote:
> > On Wed, 2010-11-03 at 01:27 -0700, Brian Norris wrote:
> >> -                             mtdoffset = strtol(optarg, NULL, 0);
> >> +                             mtdoffset = strtoll(optarg, NULL, 0);
> >
> > Ideally this should check for strtoll failures, but this just a side
> > note. Not sure if this is the best way, but I'd do it like this:
> >
> > char *endp;
> >
> > mtdoffset = strtoul(optarg, &endp, 0);
> > if (*endp != '\0' || endp == optarg)
> >        return errmsg("bad volume alignment: \"%s\"", optarg);
> >
> > or something like this. The check for negative mtdoffset could be there
> > as well.
> 
> or use simple_strtoll() from common.h as that includes error handling ...

Oh, even better.

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

Re: [PATCH v2 09/10] mtd-utils: nandwrite: full 64-bit support w/ libmtd

[Brian Norris]

On 11/13/2010 3:53 AM, Artem Bityutskiy wrote:
> You could as well just introduce another libmtd helper - would be
> cleaner, IMO.

OK, I'll look at this and the "simple_strtoll()" suggestion sometime. I
should probably examine "common.h" more before patching mtd-utils next time!

And thanks for the push.

Brian

Re: [PATCH v2 09/10] mtd-utils: nandwrite: full 64-bit support w/ libmtd

[Mike Frysinger]

On Tue, Nov 16, 2010 at 12:06, Brian Norris wrote:
> On 11/13/2010 3:53 AM, Artem Bityutskiy wrote:
>> You could as well just introduce another libmtd helper - would be
>> cleaner, IMO.
>
> OK, I'll look at this and the "simple_strtoll()" suggestion sometime. I
> should probably examine "common.h" more before patching mtd-utils next time!

it's an evolving helper, so if you come across generally useful funcs
that are in an app or two, feel free to propose moving them to the
header for all to benefit
-mike