[PATCH 00/10] nandwrite: cleanup, support ioctl(MEMWRITE)

[PATCH 00/10] nandwrite: cleanup, support ioctl(MEMWRITE)

[Brian Norris]

This patch series cleans up old features in nandwrite and supports new
features with ioctl(MEMWRITE).

Byproducts of this include:
(1) kill --raw
(2) re-implement --autoplace
(3) support OOB writes for MLC NAND (only write once per page)

(1) is done because it's difficult to really tell the reason for --raw
in the first place. Thus, in restructuring mtd_write and nandwrite, I
cannot support it. I don't think anyone will miss it.
(2) is done just because we can. Previously, support for the autoplace
OOB layout was spotty, depended on the obsolete ioctl(MEMGETOOBSEL), and
utilized user-space code to imitate an internal kernel interface. Old
code can be supported with little impact by pushing it to a "legacy"
function and supporting autoplacement primarily by way of the new
ioctl(MEMWRITE).
(3) was kind of the end goal for introducing ioctl(MEMWRITE). We needed
a user interface that could write data+OOB in a single operation.

Brian

Brian Norris (10):
  nandwrite: trivial variable move
  mtd-utils: update mtd-abi.h
  nandwrite: consolidate buffer usage
  libmtd: modify `mtd_write' to cover OOB writes
  libmtd: support MEMWRITE ioctl
  nandwrite: merge `mtd_write_oob' and `mtd_write' calls
  mtdutils: move OOB auto-layout into libmtd's mtd_write
  nandwrite: kill `--raw' option
  nandwrite: re-implement `--autoplace' option
  nandwrite: use common.h "errmsg" functions

 include/libmtd.h      |   15 ++++--
 include/mtd/mtd-abi.h |  122 ++++++++++++++++++++++++++++++++++++------
 lib/libmtd.c          |   86 ++++++++++++++++++++++++++----
 nanddump.c            |    2 +-
 nandwrite.c           |  143 +++++++++++++++++--------------------------------
 ubi-utils/ubiformat.c |    6 ++-
 6 files changed, 245 insertions(+), 129 deletions(-)

-- 
1.7.5.4

[PATCH 01/10] nandwrite: trivial variable move

[Brian Norris]

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

diff --git a/nandwrite.c b/nandwrite.c
index 95baa38..17a717c 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -496,7 +496,7 @@ int main(int argc, char * const argv[])
 			}
 
 			if (!noecc) {
-				int i, start, len;
+				int start, len;
 				struct nand_oobinfo old_oobinfo;
 
 				/* Read the current oob info */
@@ -514,7 +514,7 @@ int main(int argc, char * const argv[])
 				 * such as the layout used by diskonchip.c
 				 */
 				if (old_oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
-					int tags_pos = 0, tmp_ofs;
+					int i, tags_pos = 0, tmp_ofs;
 					for (i = 0; old_oobinfo.oobfree[i][1]; i++) {
 						/* Set the reserved bytes to 0xff */
 						start = old_oobinfo.oobfree[i][0];
-- 
1.7.5.4

[PATCH 02/10] mtd-utils: update mtd-abi.h

[Brian Norris]

Kernel ABI header added a new ioctl, killed an old one, and exposed OOB
modes to user-space. Plus, it added a lot of documentation.

We have some trivial name changes for some MTD mode constants as well.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/mtd/mtd-abi.h |  122 ++++++++++++++++++++++++++++++++++++++++++-------
 nanddump.c            |    2 +-
 nandwrite.c           |    2 +-
 3 files changed, 107 insertions(+), 19 deletions(-)

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index a86364a..66be707 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -45,6 +45,51 @@ struct mtd_oob_buf64 {
 	__u64 usr_ptr;
 };
 
+/**
+ * MTD operation modes
+ *
+ * @MTD_OPS_PLACE_OOB:	OOB data are placed at the given offset (default)
+ * @MTD_OPS_AUTO_OOB:	OOB data are automatically placed at the free areas
+ *			which are defined by the internal ecclayout
+ * @MTD_OPS_RAW:	data are transferred as-is, with no error correction;
+ *			this mode implies %MTD_OPS_PLACE_OOB
+ *
+ * These modes can be passed to ioctl(MEMWRITE) and are also used internally.
+ * See notes on "MTD file modes" for discussion on %MTD_OPS_RAW vs.
+ * %MTD_FILE_MODE_RAW.
+ */
+enum {
+	MTD_OPS_PLACE_OOB = 0,
+	MTD_OPS_AUTO_OOB = 1,
+	MTD_OPS_RAW = 2,
+};
+
+/**
+ * struct mtd_write_req - data structure for requesting a write operation
+ *
+ * @start:	start address
+ * @len:	length of data buffer
+ * @ooblen:	length of OOB buffer
+ * @usr_data:	user-provided data buffer
+ * @usr_oob:	user-provided OOB buffer
+ * @mode:	MTD mode (see "MTD operation modes")
+ * @padding:	reserved, must be set to 0
+ *
+ * This structure supports ioctl(MEMWRITE) operations, allowing data and/or OOB
+ * writes in various modes. To write to OOB-only, set @usr_data == NULL, and to
+ * write data-only, set @usr_oob == NULL. However, setting both @usr_data and
+ * @usr_oob to NULL is not allowed.
+ */
+struct mtd_write_req {
+	__u64 start;
+	__u32 len;
+	__u32 ooblen;
+	__u64 usr_data;
+	__u64 usr_oob;
+	__u8 mode;
+	__u8 padding[7];
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -59,13 +104,13 @@ struct mtd_oob_buf64 {
 #define MTD_NO_ERASE		0x1000	/* No erase necessary */
 #define MTD_POWERUP_LOCK	0x2000	/* Always locked after reset */
 
-// Some common devices / combinations of capabilities
+/* Some common devices / combinations of capabilities */
 #define MTD_CAP_ROM		0
 #define MTD_CAP_RAM		(MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_NO_ERASE)
 #define MTD_CAP_NORFLASH	(MTD_WRITEABLE | MTD_BIT_WRITEABLE)
 #define MTD_CAP_NANDFLASH	(MTD_WRITEABLE)
 
-/* ECC byte placement */
+/* Obsolete ECC byte placement modes (used with obsolete MEMGETOOBSEL) */
 #define MTD_NANDECC_OFF		0	// Switch off ECC (Not recommended)
 #define MTD_NANDECC_PLACE	1	// Use the given placement in the structure (YAFFS1 legacy mode)
 #define MTD_NANDECC_AUTOPLACE	2	// Use the default placement scheme
@@ -80,21 +125,18 @@ struct mtd_oob_buf64 {
 struct mtd_info_user {
 	__u8 type;
 	__u32 flags;
-	__u32 size;	 // Total size of the MTD
+	__u32 size;	/* Total size of the MTD */
 	__u32 erasesize;
 	__u32 writesize;
-	__u32 oobsize;   // Amount of OOB data per block (e.g. 16)
-	/* The below two fields are obsolete and broken, do not use them
-	 * (TODO: remove at some point) */
-	__u32 ecctype;
-	__u32 eccsize;
+	__u32 oobsize;	/* Amount of OOB data per block (e.g. 16) */
+	__u64 padding;	/* Old obsolete field; do not use */
 };
 
 struct region_info_user {
 	__u32 offset;		/* At which this region starts,
-					 * from the beginning of the MTD */
-	__u32 erasesize;		/* For this region */
-	__u32 numblocks;		/* Number of blocks in this region */
+				 * from the beginning of the MTD */
+	__u32 erasesize;	/* For this region */
+	__u32 numblocks;	/* Number of blocks in this region */
 	__u32 regionindex;
 };
 
@@ -104,29 +146,61 @@ struct otp_info {
 	__u32 locked;
 };
 
+/*
+ * Note, the following ioctl existed in the past and was removed:
+ * #define MEMSETOOBSEL           _IOW('M', 9, struct nand_oobinfo)
+ * Try to avoid adding a new ioctl with the same ioctl number.
+ */
+
+/* Get basic MTD characteristics info (better to use sysfs) */
 #define MEMGETINFO		_IOR('M', 1, struct mtd_info_user)
+/* Erase segment of MTD */
 #define MEMERASE		_IOW('M', 2, struct erase_info_user)
+/* Write out-of-band data from MTD */
 #define MEMWRITEOOB		_IOWR('M', 3, struct mtd_oob_buf)
+/* Read out-of-band data from MTD */
 #define MEMREADOOB		_IOWR('M', 4, struct mtd_oob_buf)
+/* Lock a chip (for MTD that supports it) */
 #define MEMLOCK			_IOW('M', 5, struct erase_info_user)
+/* Unlock a chip (for MTD that supports it) */
 #define MEMUNLOCK		_IOW('M', 6, struct erase_info_user)
+/* Get the number of different erase regions */
 #define MEMGETREGIONCOUNT	_IOR('M', 7, int)
+/* Get information about the erase region for a specific index */
 #define MEMGETREGIONINFO	_IOWR('M', 8, struct region_info_user)
-#define MEMSETOOBSEL		_IOW('M', 9, struct nand_oobinfo)
+/* Get info about OOB modes (e.g., RAW, PLACE, AUTO) - legacy interface */
 #define MEMGETOOBSEL		_IOR('M', 10, struct nand_oobinfo)
+/* Check if an eraseblock is bad */
 #define MEMGETBADBLOCK		_IOW('M', 11, __kernel_loff_t)
+/* Mark an eraseblock as bad */
 #define MEMSETBADBLOCK		_IOW('M', 12, __kernel_loff_t)
+/* Set OTP (One-Time Programmable) mode (factory vs. user) */
 #define OTPSELECT		_IOR('M', 13, int)
+/* Get number of OTP (One-Time Programmable) regions */
 #define OTPGETREGIONCOUNT	_IOW('M', 14, int)
+/* Get all OTP (One-Time Programmable) info about MTD */
 #define OTPGETREGIONINFO	_IOW('M', 15, struct otp_info)
+/* Lock a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
 #define OTPLOCK			_IOR('M', 16, struct otp_info)
+/* Get ECC layout (deprecated) */
 #define ECCGETLAYOUT		_IOR('M', 17, struct nand_ecclayout_user)
+/* Get statistics about corrected/uncorrected errors */
 #define ECCGETSTATS		_IOR('M', 18, struct mtd_ecc_stats)
+/* Set MTD mode on a per-file-descriptor basis (see "MTD file modes") */
 #define MTDFILEMODE		_IO('M', 19)
+/* Erase segment of MTD (supports 64-bit address) */
 #define MEMERASE64		_IOW('M', 20, struct erase_info_user64)
+/* Write data to OOB (64-bit version) */
 #define MEMWRITEOOB64		_IOWR('M', 21, struct mtd_oob_buf64)
+/* Read data from OOB (64-bit version) */
 #define MEMREADOOB64		_IOWR('M', 22, struct mtd_oob_buf64)
+/* Check if chip is locked (for MTD that supports it) */
 #define MEMISLOCKED		_IOR('M', 23, struct erase_info_user)
+/*
+ * Most generic write interface; can write in-band and/or out-of-band in various
+ * modes (see "struct mtd_write_req")
+ */
+#define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
@@ -177,13 +251,27 @@ struct mtd_ecc_stats {
 };
 
 /*
- * Read/write file modes for access to MTD
+ * MTD file modes - for read/write access to MTD
+ *
+ * @MTD_FILE_MODE_NORMAL:	OTP disabled, ECC enabled
+ * @MTD_FILE_MODE_OTP_FACTORY:	OTP enabled in factory mode
+ * @MTD_FILE_MODE_OTP_USER:	OTP enabled in user mode
+ * @MTD_FILE_MODE_RAW:		OTP disabled, ECC disabled
+ *
+ * These modes can be set via ioctl(MTDFILEMODE). The mode mode will be retained
+ * separately for each open file descriptor.
+ *
+ * Note: %MTD_FILE_MODE_RAW provides the same functionality as %MTD_OPS_RAW -
+ * raw access to the flash, without error correction or autoplacement schemes.
+ * Wherever possible, the MTD_OPS_* mode will override the MTD_FILE_MODE_* mode
+ * (e.g., when using ioctl(MEMWRITE)), but in some cases, the MTD_FILE_MODE is
+ * used out of necessity (e.g., `write()', ioctl(MEMWRITEOOB64)).
  */
 enum mtd_file_modes {
-	MTD_MODE_NORMAL = MTD_OTP_OFF,
-	MTD_MODE_OTP_FACTORY = MTD_OTP_FACTORY,
-	MTD_MODE_OTP_USER = MTD_OTP_USER,
-	MTD_MODE_RAW,
+	MTD_FILE_MODE_NORMAL = MTD_OTP_OFF,
+	MTD_FILE_MODE_OTP_FACTORY = MTD_OTP_FACTORY,
+	MTD_FILE_MODE_OTP_USER = MTD_OTP_USER,
+	MTD_FILE_MODE_RAW,
 };
 
 #endif /* __MTD_ABI_H__ */
diff --git a/nanddump.c b/nanddump.c
index 7a24c0d..be458c6 100644
--- a/nanddump.c
+++ b/nanddump.c
@@ -336,7 +336,7 @@ int main(int argc, char * const argv[])
 	readbuf = xmalloc(sizeof(readbuf) * mtd.min_io_size);
 
 	if (noecc)  {
-		if (ioctl(fd, MTDFILEMODE, MTD_MODE_RAW) != 0) {
+		if (ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW) != 0) {
 				perror("MTDFILEMODE");
 				goto closeall;
 		}
diff --git a/nandwrite.c b/nandwrite.c
index 17a717c..3f70cb3 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -274,7 +274,7 @@ int main(int argc, char * const argv[])
 	}
 
 	if (noecc)  {
-		ret = ioctl(fd, MTDFILEMODE, MTD_MODE_RAW);
+		ret = ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW);
 		if (ret) {
 			switch (errno) {
 			case ENOTTY:
-- 
1.7.5.4

[PATCH 03/10] nandwrite: consolidate buffer usage

[Brian Norris]

Instead of using two different output buffers for OOB data, let's just
use the same one for all output. This adds an extra memcpy, but it
simplifies some future work, so it's worth it.

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

diff --git a/nandwrite.c b/nandwrite.c
index 3f70cb3..3eea6e2 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -531,11 +531,12 @@ int main(int argc, char * const argv[])
 							oobreadbuf + start,
 							len);
 				}
+			} else {
+				memcpy(oobbuf, oobreadbuf, mtd.oob_size);
 			}
 			/* Write OOB data first, as ecc will be placed in there */
 			if (mtd_write_oob(mtd_desc, &mtd, fd, mtdoffset,
-						mtd.oob_size,
-						noecc ? oobreadbuf : oobbuf)) {
+						mtd.oob_size, oobbuf)) {
 				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				goto closeall;
 			}
-- 
1.7.5.4

[PATCH 04/10] libmtd: modify `mtd_write' to cover OOB writes

[Brian Norris]

To support the MEMWRITE ioctl, we will need a different sort of libmtd
interface for writing to flash. We will expand mtd_write to include more
functionality; for now, we just change the function definition and
description as we begin to add the actual functionality.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/libmtd.h      |   15 +++++++++++----
 lib/libmtd.c          |   10 ++++++----
 nandwrite.c           |    4 ++--
 ubi-utils/ubiformat.c |    6 ++++--
 4 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/include/libmtd.h b/include/libmtd.h
index 9efccbc..07c304a 100644
--- a/include/libmtd.h
+++ b/include/libmtd.h
@@ -255,19 +255,26 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
 
 /**
  * mtd_write - write data to an MTD device.
+ * @desc: MTD library descriptor
  * @mtd: MTD device description object
  * @fd: MTD device node file descriptor
  * @eb: eraseblock to write to
  * @offs: offset withing the eraseblock to write to
- * @buf: buffer to write
- * @len: how many bytes to write
+ * @data: data buffer to write
+ * @len: how many data bytes to write
+ * @oob: OOB buffer to write
+ * @ooblen: how many OOB bytes to write
+ * @mode: write mode (e.g., %MTD_OOB_PLACE, %MTD_OOB_RAW)
  *
  * This function writes @len bytes of data to eraseblock @eb and offset @offs
  * of the MTD device defined by @mtd. Returns %0 in case of success and %-1 in
  * case of failure.
+ *
+ * Can only write to a single page at a time if writing to OOB.
  */
-int mtd_write(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
-	      void *buf, int len);
+int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
+	      int offs, void *data, int len, void *oob, int ooblen,
+	      uint8_t mode);
 
 /**
  * mtd_read_oob - read out-of-band area.
diff --git a/lib/libmtd.c b/lib/libmtd.c
index c34874e..746ea69 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -970,7 +970,8 @@ int mtd_torture(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb)
 
 		/* Write a pattern and check it */
 		memset(buf, patterns[i], mtd->eb_size);
-		err = mtd_write(mtd, fd, eb, 0, buf, mtd->eb_size);
+		err = mtd_write(desc, mtd, fd, eb, 0, buf, mtd->eb_size, NULL,
+				0, 0);
 		if (err)
 			goto out;
 
@@ -1070,8 +1071,9 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
 	return 0;
 }
 
-int mtd_write(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
-	      void *buf, int len)
+int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
+	      int offs, void *data, int len, void *oob, int ooblen,
+	      uint8_t mode)
 {
 	int ret;
 	off_t seek;
@@ -1105,7 +1107,7 @@ int mtd_write(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
 		return sys_errmsg("cannot seek mtd%d to offset %llu",
 				  mtd->mtd_num, (unsigned long long)seek);
 
-	ret = write(fd, buf, len);
+	ret = write(fd, data, len);
 	if (ret != len)
 		return sys_errmsg("cannot write %d bytes to mtd%d (eraseblock %d, offset %d)",
 				  len, mtd->mtd_num, eb, offs);
diff --git a/nandwrite.c b/nandwrite.c
index 3eea6e2..33a3b8f 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -543,8 +543,8 @@ int main(int argc, char * const argv[])
 		}
 
 		/* Write out the Page data */
-		if (!onlyoob && mtd_write(&mtd, fd, mtdoffset / mtd.eb_size, mtdoffset % mtd.eb_size,
-					writebuf, mtd.min_io_size)) {
+		if (!onlyoob && mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size, mtdoffset % mtd.eb_size,
+					writebuf, mtd.min_io_size, NULL, 0, 0)) {
 			int i;
 			if (errno != EIO) {
 				sys_errmsg("%s: MTD write failure", mtd_device);
diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
index bfa1730..ed2b8d0 100644
--- a/ubi-utils/ubiformat.c
+++ b/ubi-utils/ubiformat.c
@@ -534,7 +534,8 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 
 		new_len = drop_ffs(mtd, buf, mtd->eb_size);
 
-		err = mtd_write(mtd, args.node_fd, eb, 0, buf, new_len);
+		err = mtd_write(libmtd, mtd, args.node_fd, eb, 0, buf, new_len,
+				NULL, 0, 0);
 		if (err) {
 			sys_errmsg("cannot write eraseblock %d", eb);
 
@@ -637,7 +638,8 @@ static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 			fflush(stdout);
 		}
 
-		err = mtd_write(mtd, args.node_fd, eb, 0, hdr, write_size);
+		err = mtd_write(libmtd, mtd, args.node_fd, eb, 0, hdr,
+				write_size, NULL, 0, 0);
 		if (err) {
 			if (!args.quiet && !args.verbose)
 				printf("\n");
-- 
1.7.5.4

[PATCH 05/10] libmtd: support MEMWRITE ioctl

[Brian Norris]

`mtd_write()' now will first attempt to use MEMWRITE. Then, if that
doesn't exist, it will attempt to fall back to old methods for writing
OOB and/or page data.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 lib/libmtd.c |   39 +++++++++++++++++++++++++++++++--------
 1 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/lib/libmtd.c b/lib/libmtd.c
index 746ea69..d47b307 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -1077,6 +1077,7 @@ int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
 {
 	int ret;
 	off_t seek;
+	struct mtd_write_req ops;
 
 	ret = mtd_valid_erase_block(mtd, eb);
 	if (ret)
@@ -1101,16 +1102,38 @@ int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
 		return -1;
 	}
 
-	/* Seek to the beginning of the eraseblock */
+	/* Calculate seek address */
 	seek = (off_t)eb * mtd->eb_size + offs;
-	if (lseek(fd, seek, SEEK_SET) != seek)
-		return sys_errmsg("cannot seek mtd%d to offset %llu",
-				  mtd->mtd_num, (unsigned long long)seek);
 
-	ret = write(fd, data, len);
-	if (ret != len)
-		return sys_errmsg("cannot write %d bytes to mtd%d (eraseblock %d, offset %d)",
-				  len, mtd->mtd_num, eb, offs);
+	ops.start = seek;
+	ops.len = len;
+	ops.ooblen = ooblen;
+	ops.usr_data = (uint64_t)(unsigned long)data;
+	ops.usr_oob = (uint64_t)(unsigned long)oob;
+	ops.mode = mode;
+
+	ret = ioctl(fd, MEMWRITE, &ops);
+	if (ret == 0)
+		return 0;
+	else if (errno != ENOTTY)
+		return mtd_ioctl_error(mtd, eb, "MEMWRITE");
+
+	/* Fall back to old methods if necessary */
+	if (oob) {
+		if (mtd_write_oob(desc, mtd, fd, seek, ooblen, oob) < 0)
+			return sys_errmsg("cannot write to OOB");
+	}
+	if (data) {
+		/* Seek to the beginning of the eraseblock */
+		if (lseek(fd, seek, SEEK_SET) != seek)
+			return sys_errmsg("cannot seek mtd%d to offset %llu",
+					mtd->mtd_num, (unsigned long long)seek);
+		ret = write(fd, data, len);
+		if (ret != len)
+			return sys_errmsg("cannot write %d bytes to mtd%d "
+					  "(eraseblock %d, offset %d)",
+					  len, mtd->mtd_num, eb, offs);
+	}
 
 	return 0;
 }
-- 
1.7.5.4

[PATCH 06/10] nandwrite: merge `mtd_write_oob' and `mtd_write' calls

[Brian Norris]

Now that `mtd_write' can write to both OOB and data regions, we need to
perform our `mtd_write' call only once.

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

diff --git a/nandwrite.c b/nandwrite.c
index 33a3b8f..a78b0b6 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -534,17 +534,17 @@ int main(int argc, char * const argv[])
 			} else {
 				memcpy(oobbuf, oobreadbuf, mtd.oob_size);
 			}
-			/* Write OOB data first, as ecc will be placed in there */
-			if (mtd_write_oob(mtd_desc, &mtd, fd, mtdoffset,
-						mtd.oob_size, oobbuf)) {
-				sys_errmsg("%s: MTD writeoob failure", mtd_device);
-				goto closeall;
-			}
 		}
 
-		/* Write out the Page data */
-		if (!onlyoob && mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size, mtdoffset % mtd.eb_size,
-					writebuf, mtd.min_io_size, NULL, 0, 0)) {
+		/* Write out data */
+		ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size,
+				mtdoffset % mtd.eb_size,
+				onlyoob ? NULL : writebuf,
+				onlyoob ? 0 : mtd.min_io_size,
+				writeoob ? oobbuf : NULL,
+				writeoob ? mtd.oob_size : 0,
+				noecc ? MTD_OPS_RAW : MTD_OPS_PLACE_OOB);
+		if (ret) {
 			int i;
 			if (errno != EIO) {
 				sys_errmsg("%s: MTD write failure", mtd_device);
-- 
1.7.5.4

[PATCH 07/10] mtdutils: move OOB auto-layout into libmtd's mtd_write

[Brian Norris]

With the addition of the the new ioctl(MEMWRITE), we can use the
kernel's internal OOB autoplacement option. It's a cleaner interface and
avoids too much duplication of coding effort.

This patch moves any legacy code (using MEMGETOOBSEL) into a legacy
function in libmtd.c. It's not exactly a "pre-2.6.30" feature, so I'm not
moving it to libmtd_legacy.c.

Now, autoplacement features are only activated if we call mtd_write with
mode == MTD_OPS_AUTO_OOB. This should fix some discrepancies for
nandwrite, where we weren't handling OOB consistently (i.e., we had
different functionality when the kernel did/didn't support MEMWRITE).
But that also means that we now default to using MTD_OPS_PLACE_OOB
instead of AUTO layout. To re-enable autoplacement, we can re-implement
the `--autoplace' option that had previously rotted.

This patch also cleans up a need for an extra OOB buffer in nandwrite.

This has been tested a little in nandsim as well as on SLC NAND flash.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 lib/libmtd.c |   39 +++++++++++++++++++++++++++++++++++++++
 nandwrite.c  |   53 ++++-------------------------------------------------
 2 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/lib/libmtd.c b/lib/libmtd.c
index d47b307..1b16de5 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -1071,6 +1071,42 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs,
 	return 0;
 }
 
+static int legacy_auto_oob_layout(const struct mtd_dev_info *mtd, int fd,
+				  int ooblen, void *oob) {
+	struct nand_oobinfo old_oobinfo;
+	int start, len;
+	uint8_t *tmp_buf;
+
+	/* Read the current oob info */
+	if (ioctl(fd, MEMGETOOBSEL, &old_oobinfo))
+		return sys_errmsg("MEMGETOOBSEL failed");
+
+	tmp_buf = malloc(ooblen);
+	memcpy(tmp_buf, oob, ooblen);
+
+	/*
+	 * We use autoplacement and have the oobinfo with the autoplacement
+	 * information from the kernel available
+	 */
+	if (old_oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
+		int i, tags_pos = 0;
+		for (i = 0; old_oobinfo.oobfree[i][1]; i++) {
+			/* Set the reserved bytes to 0xff */
+			start = old_oobinfo.oobfree[i][0];
+			len = old_oobinfo.oobfree[i][1];
+			memcpy(oob + start, tmp_buf + tags_pos, len);
+			tags_pos += len;
+		}
+	} else {
+		/* Set at least the ecc byte positions to 0xff */
+		start = old_oobinfo.eccbytes;
+		len = mtd->oob_size - start;
+		memcpy(oob + start, tmp_buf + start, len);
+	}
+
+	return 0;
+}
+
 int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
 	      int offs, void *data, int len, void *oob, int ooblen,
 	      uint8_t mode)
@@ -1120,6 +1156,9 @@ int mtd_write(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb,
 
 	/* Fall back to old methods if necessary */
 	if (oob) {
+		if (mode == MTD_OPS_AUTO_OOB)
+			if (legacy_auto_oob_layout(mtd, fd, ooblen, oob))
+				return -1;
 		if (mtd_write_oob(desc, mtd, fd, seek, ooblen, oob) < 0)
 			return sys_errmsg("cannot write to OOB");
 	}
diff --git a/nandwrite.c b/nandwrite.c
index a78b0b6..920863f 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -233,7 +233,6 @@ int main(int argc, char * const argv[])
 	/* 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 = NULL;
 	libmtd_t mtd_desc;
 	int ebsize_aligned;
@@ -345,9 +344,6 @@ int main(int argc, char * const argv[])
 	filebuf = xmalloc(filebuf_max);
 	erase_buffer(filebuf, filebuf_max);
 
-	oobbuf = xmalloc(mtd.oob_size);
-	erase_buffer(oobbuf, mtd.oob_size);
-
 	/*
 	 * Get data from input and write to the device while there is
 	 * still input to read and we are still within the device
@@ -460,16 +456,16 @@ int main(int argc, char * const argv[])
 		}
 
 		if (writeoob) {
-			oobreadbuf = writebuf + mtd.min_io_size;
+			oobbuf = writebuf + mtd.min_io_size;
 
 			/* Read more data for the OOB from the input if there isn't enough in the buffer */
-			if ((oobreadbuf + mtd.oob_size) > (filebuf + filebuf_len)) {
+			if ((oobbuf + mtd.oob_size) > (filebuf + filebuf_len)) {
 				int readlen = mtd.oob_size;
-				int alreadyread = (filebuf + filebuf_len) - oobreadbuf;
+				int alreadyread = (filebuf + filebuf_len) - oobbuf;
 				int tinycnt = alreadyread;
 
 				while (tinycnt < readlen) {
-					cnt = read(ifd, oobreadbuf + tinycnt, readlen - tinycnt);
+					cnt = read(ifd, oobbuf + tinycnt, readlen - tinycnt);
 					if (cnt == 0) { /* EOF */
 						break;
 					} else if (cnt < 0) {
@@ -494,46 +490,6 @@ int main(int argc, char * const argv[])
 					imglen = 0;
 				}
 			}
-
-			if (!noecc) {
-				int start, len;
-				struct nand_oobinfo old_oobinfo;
-
-				/* Read the current oob info */
-				if (ioctl(fd, MEMGETOOBSEL, &old_oobinfo) != 0) {
-					perror("MEMGETOOBSEL");
-					close(fd);
-					exit(EXIT_FAILURE);
-				}
-
-				/*
-				 * We use autoplacement and have the oobinfo with the autoplacement
-				 * information from the kernel available
-				 *
-				 * Modified to support out of order oobfree segments,
-				 * such as the layout used by diskonchip.c
-				 */
-				if (old_oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
-					int i, tags_pos = 0, tmp_ofs;
-					for (i = 0; old_oobinfo.oobfree[i][1]; i++) {
-						/* Set the reserved bytes to 0xff */
-						start = old_oobinfo.oobfree[i][0];
-						len = old_oobinfo.oobfree[i][1];
-						tmp_ofs = rawoob ? start : tags_pos;
-						memcpy(oobbuf + start, oobreadbuf + tmp_ofs, len);
-						tags_pos += len;
-					}
-				} else {
-					/* Set at least the ecc byte positions to 0xff */
-					start = old_oobinfo.eccbytes;
-					len = mtd.oob_size - start;
-					memcpy(oobbuf + start,
-							oobreadbuf + start,
-							len);
-				}
-			} else {
-				memcpy(oobbuf, oobreadbuf, mtd.oob_size);
-			}
 		}
 
 		/* Write out data */
@@ -588,7 +544,6 @@ closeall:
 	close(ifd);
 	libmtd_close(mtd_desc);
 	free(filebuf);
-	free(oobbuf);
 	close(fd);
 
 	if (failed
-- 
1.7.5.4

[PATCH 08/10] nandwrite: kill `--raw' option

[Brian Norris]

The `--raw' option has lost all usefulness as it overlapped with several
other OOB modes. I cannot even figure out what it was actually intended
to do, but I'm sure its functionality fits somewhere in the
MTD_OPS_{AUTO_OOB,PLACE_OOB,RAW} options, which are mostly implemented
in libmtd's mtd_write().

I don't think users need a warning for this one, unless someone can tell
me what it actually was supposed to have done in the first place.

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

diff --git a/nandwrite.c b/nandwrite.c
index 920863f..45782c7 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -54,7 +54,6 @@ static void display_help(void)
 "  -N, --noskipbad         Write without bad block skipping\n"
 "  -o, --oob               Image contains oob data\n"
 "  -O, --onlyoob           Image contains oob data and only write the oob part\n"
-"  -r, --raw               Image contains the raw oob data dumped by nanddump\n"
 "  -s addr, --start=addr   Set start address (default is 0)\n"
 "  -p, --pad               Pad to page size\n"
 "  -b, --blockalign=1|2|4  Set multiple of eraseblocks to align to\n"
@@ -86,7 +85,6 @@ static const char	*mtd_device, *img;
 static long long	mtdoffset = 0;
 static bool		quiet = false;
 static bool		writeoob = false;
-static bool		rawoob = false;
 static bool		onlyoob = false;
 static bool		markbad = false;
 static bool		noecc = false;
@@ -100,7 +98,7 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "b:mnNoOpqrs:";
+		static const char *short_options = "b:mnNoOpqs:";
 		static const struct option long_options[] = {
 			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
@@ -112,7 +110,6 @@ static void process_options(int argc, char * const argv[])
 			{"onlyoob", no_argument, 0, 'O'},
 			{"pad", no_argument, 0, 'p'},
 			{"quiet", no_argument, 0, 'q'},
-			{"raw", no_argument, 0, 'r'},
 			{"start", required_argument, 0, 's'},
 			{0, 0, 0, 0},
 		};
@@ -156,10 +153,6 @@ static void process_options(int argc, char * const argv[])
 			case 'p':
 				pad = true;
 				break;
-			case 'r':
-				rawoob = true;
-				writeoob = true;
-				break;
 			case 's':
 				mtdoffset = simple_strtoll(optarg, &error);
 				break;
-- 
1.7.5.4

[PATCH 09/10] nandwrite: re-implement `--autoplace' option

[Brian Norris]

The restructuring of mtd_write() has allowed us to use `--autoplace'
somewhat successfully; it is supported by the new ioctl(MEMWRITE) as
well as some legacy code utilizing the deprecated ioctl(MEMGETOOBSEL).

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

diff --git a/nandwrite.c b/nandwrite.c
index 45782c7..ca72f16 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -49,6 +49,7 @@ static void display_help(void)
 "Usage: nandwrite [OPTION] MTD_DEVICE [INPUTFILE|-]\n"
 "Writes to the specified MTD device.\n"
 "\n"
+"  -a, --autoplace         Use auto OOB layout\n"
 "  -m, --markbad           Mark blocks bad if write fails\n"
 "  -n, --noecc             Write without ecc\n"
 "  -N, --noskipbad         Write without bad block skipping\n"
@@ -88,6 +89,7 @@ static bool		writeoob = false;
 static bool		onlyoob = false;
 static bool		markbad = false;
 static bool		noecc = false;
+static bool		autoplace = false;
 static bool		noskipbad = false;
 static bool		pad = false;
 static int		blockalign = 1; /* default to using actual block size */
@@ -98,7 +100,7 @@ static void process_options(int argc, char * const argv[])
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "b:mnNoOpqs:";
+		static const char *short_options = "b:mnNoOpqs:a";
 		static const struct option long_options[] = {
 			{"help", no_argument, 0, 0},
 			{"version", no_argument, 0, 0},
@@ -111,6 +113,7 @@ static void process_options(int argc, char * const argv[])
 			{"pad", no_argument, 0, 'p'},
 			{"quiet", no_argument, 0, 'q'},
 			{"start", required_argument, 0, 's'},
+			{"autoplace", no_argument, 0, 'a'},
 			{0, 0, 0, 0},
 		};
 
@@ -159,6 +162,9 @@ static void process_options(int argc, char * const argv[])
 			case 'b':
 				blockalign = atoi(optarg);
 				break;
+			case 'a':
+				autoplace = true;
+				break;
 			case '?':
 				error++;
 				break;
@@ -173,6 +179,9 @@ static void process_options(int argc, char * const argv[])
 		errmsg_die("Can't specify negative blockalign with option -b:"
 				" %d", blockalign);
 
+	if (autoplace && noecc)
+		errmsg_die("Autoplacement and no-ECC are mutually exclusive");
+
 	argc -= optind;
 	argv += optind;
 
@@ -229,6 +238,7 @@ int main(int argc, char * const argv[])
 	unsigned char *oobbuf = NULL;
 	libmtd_t mtd_desc;
 	int ebsize_aligned;
+	uint8_t write_mode;
 
 	process_options(argc, argv);
 
@@ -265,6 +275,14 @@ int main(int argc, char * const argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	/* Select OOB write mode */
+	if (noecc)
+		write_mode = MTD_OPS_RAW;
+	else if (autoplace)
+		write_mode = MTD_OPS_AUTO_OOB;
+	else
+		write_mode = MTD_OPS_PLACE_OOB;
+
 	if (noecc)  {
 		ret = ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW);
 		if (ret) {
@@ -492,7 +510,7 @@ int main(int argc, char * const argv[])
 				onlyoob ? 0 : mtd.min_io_size,
 				writeoob ? oobbuf : NULL,
 				writeoob ? mtd.oob_size : 0,
-				noecc ? MTD_OPS_RAW : MTD_OPS_PLACE_OOB);
+				write_mode);
 		if (ret) {
 			int i;
 			if (errno != EIO) {
-- 
1.7.5.4

[PATCH 10/10] nandwrite: use common.h "errmsg" functions

[Brian Norris]

Convert several forms of exit(), perror(), etc. to use common.h helper
functions.

Also, move one of our parameter checks inside the process_options()
function for consistency.

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

diff --git a/nandwrite.c b/nandwrite.c
index ca72f16..a42f7c9 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -182,6 +182,9 @@ static void process_options(int argc, char * const argv[])
 	if (autoplace && noecc)
 		errmsg_die("Autoplacement and no-ECC are mutually exclusive");
 
+	if (!onlyoob && (pad && writeoob))
+		errmsg_die("Can't pad when oob data is present");
+
 	argc -= optind;
 	argv += optind;
 
@@ -242,23 +245,16 @@ int main(int argc, char * const argv[])
 
 	process_options(argc, argv);
 
-	if (!onlyoob && (pad && writeoob)) {
-		fprintf(stderr, "Can't pad when oob data is present.\n");
-		exit(EXIT_FAILURE);
-	}
-
 	/* Open the device */
-	if ((fd = open(mtd_device, O_RDWR)) == -1) {
-		perror(mtd_device);
-		exit(EXIT_FAILURE);
-	}
+	if ((fd = open(mtd_device, O_RDWR)) == -1)
+		sys_errmsg_die("%s", mtd_device);
 
 	mtd_desc = libmtd_open();
 	if (!mtd_desc)
-		return errmsg("can't initialize libmtd");
+		errmsg_die("can't initialize libmtd");
 	/* Fill in MTD device capability structure */
 	if (mtd_get_dev_info(mtd_desc, mtd_device, &mtd) < 0)
-		return errmsg("mtd_get_dev_info failed");
+		errmsg_die("mtd_get_dev_info failed");
 
 	/*
 	 * Pretend erasesize is specified number of blocks - to match jffs2
@@ -267,13 +263,10 @@ int main(int argc, char * const argv[])
 	 */
 	ebsize_aligned = mtd.eb_size * blockalign;
 
-	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",
-				mtd.min_io_size);
-		close(fd);
-		exit(EXIT_FAILURE);
-	}
+	if (mtdoffset & (mtd.min_io_size - 1))
+		errmsg_die("The start address is not page-aligned !\n"
+			   "The pagesize of this NAND Flash is 0x%x.\n",
+			   mtd.min_io_size);
 
 	/* Select OOB write mode */
 	if (noecc)
@@ -290,9 +283,7 @@ int main(int argc, char * const argv[])
 			case ENOTTY:
 				errmsg_die("ioctl MTDFILEMODE is missing");
 			default:
-				perror("MTDFILEMODE");
-				close(fd);
-				exit(EXIT_FAILURE);
+				sys_errmsg_die("MTDFILEMODE");
 			}
 		}
 	}
@@ -341,7 +332,7 @@ int main(int argc, char * const argv[])
 		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");
+		sys_errmsg("Input file does not fit into device");
 		goto closeall;
 	}
 
@@ -557,12 +548,9 @@ closeall:
 	free(filebuf);
 	close(fd);
 
-	if (failed
-		|| ((ifd != STDIN_FILENO) && (imglen > 0))
-		|| (writebuf < (filebuf + filebuf_len))) {
-		perror("Data was only partially written due to error\n");
-		exit(EXIT_FAILURE);
-	}
+	if (failed || ((ifd != STDIN_FILENO) && (imglen > 0))
+		   || (writebuf < (filebuf + filebuf_len)))
+		sys_errmsg_die("Data was only partially written due to error");
 
 	/* Return happy */
 	return EXIT_SUCCESS;
-- 
1.7.5.4

[PATCH v2 02/10] mtd-utils: update mtd-abi.h

[Brian Norris]

Kernel ABI header added a new ioctl, killed an old one, and exposed OOB
modes to user-space. Plus, it added a lot of documentation.

We have some trivial name changes for some MTD mode constants as well.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: change __u32 to __u64 in mtd_write_req for future-proofing; this
    depends on the v2 patch for ioctl(MEMWRITE); see
    http://lists.infradead.org/pipermail/linux-mtd/2011-September/037838.html

 include/mtd/mtd-abi.h |  122 ++++++++++++++++++++++++++++++++++++++++++-------
 nanddump.c            |    2 +-
 nandwrite.c           |    2 +-
 3 files changed, 107 insertions(+), 19 deletions(-)

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index a86364a..4de167b 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -45,6 +45,51 @@ struct mtd_oob_buf64 {
 	__u64 usr_ptr;
 };
 
+/**
+ * MTD operation modes
+ *
+ * @MTD_OPS_PLACE_OOB:	OOB data are placed at the given offset (default)
+ * @MTD_OPS_AUTO_OOB:	OOB data are automatically placed at the free areas
+ *			which are defined by the internal ecclayout
+ * @MTD_OPS_RAW:	data are transferred as-is, with no error correction;
+ *			this mode implies %MTD_OPS_PLACE_OOB
+ *
+ * These modes can be passed to ioctl(MEMWRITE) and are also used internally.
+ * See notes on "MTD file modes" for discussion on %MTD_OPS_RAW vs.
+ * %MTD_FILE_MODE_RAW.
+ */
+enum {
+	MTD_OPS_PLACE_OOB = 0,
+	MTD_OPS_AUTO_OOB = 1,
+	MTD_OPS_RAW = 2,
+};
+
+/**
+ * struct mtd_write_req - data structure for requesting a write operation
+ *
+ * @start:	start address
+ * @len:	length of data buffer
+ * @ooblen:	length of OOB buffer
+ * @usr_data:	user-provided data buffer
+ * @usr_oob:	user-provided OOB buffer
+ * @mode:	MTD mode (see "MTD operation modes")
+ * @padding:	reserved, must be set to 0
+ *
+ * This structure supports ioctl(MEMWRITE) operations, allowing data and/or OOB
+ * writes in various modes. To write to OOB-only, set @usr_data == NULL, and to
+ * write data-only, set @usr_oob == NULL. However, setting both @usr_data and
+ * @usr_oob to NULL is not allowed.
+ */
+struct mtd_write_req {
+	__u64 start;
+	__u64 len;
+	__u64 ooblen;
+	__u64 usr_data;
+	__u64 usr_oob;
+	__u8 mode;
+	__u8 padding[7];
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -59,13 +104,13 @@ struct mtd_oob_buf64 {
 #define MTD_NO_ERASE		0x1000	/* No erase necessary */
 #define MTD_POWERUP_LOCK	0x2000	/* Always locked after reset */
 
-// Some common devices / combinations of capabilities
+/* Some common devices / combinations of capabilities */
 #define MTD_CAP_ROM		0
 #define MTD_CAP_RAM		(MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_NO_ERASE)
 #define MTD_CAP_NORFLASH	(MTD_WRITEABLE | MTD_BIT_WRITEABLE)
 #define MTD_CAP_NANDFLASH	(MTD_WRITEABLE)
 
-/* ECC byte placement */
+/* Obsolete ECC byte placement modes (used with obsolete MEMGETOOBSEL) */
 #define MTD_NANDECC_OFF		0	// Switch off ECC (Not recommended)
 #define MTD_NANDECC_PLACE	1	// Use the given placement in the structure (YAFFS1 legacy mode)
 #define MTD_NANDECC_AUTOPLACE	2	// Use the default placement scheme
@@ -80,21 +125,18 @@ struct mtd_oob_buf64 {
 struct mtd_info_user {
 	__u8 type;
 	__u32 flags;
-	__u32 size;	 // Total size of the MTD
+	__u32 size;	/* Total size of the MTD */
 	__u32 erasesize;
 	__u32 writesize;
-	__u32 oobsize;   // Amount of OOB data per block (e.g. 16)
-	/* The below two fields are obsolete and broken, do not use them
-	 * (TODO: remove at some point) */
-	__u32 ecctype;
-	__u32 eccsize;
+	__u32 oobsize;	/* Amount of OOB data per block (e.g. 16) */
+	__u64 padding;	/* Old obsolete field; do not use */
 };
 
 struct region_info_user {
 	__u32 offset;		/* At which this region starts,
-					 * from the beginning of the MTD */
-	__u32 erasesize;		/* For this region */
-	__u32 numblocks;		/* Number of blocks in this region */
+				 * from the beginning of the MTD */
+	__u32 erasesize;	/* For this region */
+	__u32 numblocks;	/* Number of blocks in this region */
 	__u32 regionindex;
 };
 
@@ -104,29 +146,61 @@ struct otp_info {
 	__u32 locked;
 };
 
+/*
+ * Note, the following ioctl existed in the past and was removed:
+ * #define MEMSETOOBSEL           _IOW('M', 9, struct nand_oobinfo)
+ * Try to avoid adding a new ioctl with the same ioctl number.
+ */
+
+/* Get basic MTD characteristics info (better to use sysfs) */
 #define MEMGETINFO		_IOR('M', 1, struct mtd_info_user)
+/* Erase segment of MTD */
 #define MEMERASE		_IOW('M', 2, struct erase_info_user)
+/* Write out-of-band data from MTD */
 #define MEMWRITEOOB		_IOWR('M', 3, struct mtd_oob_buf)
+/* Read out-of-band data from MTD */
 #define MEMREADOOB		_IOWR('M', 4, struct mtd_oob_buf)
+/* Lock a chip (for MTD that supports it) */
 #define MEMLOCK			_IOW('M', 5, struct erase_info_user)
+/* Unlock a chip (for MTD that supports it) */
 #define MEMUNLOCK		_IOW('M', 6, struct erase_info_user)
+/* Get the number of different erase regions */
 #define MEMGETREGIONCOUNT	_IOR('M', 7, int)
+/* Get information about the erase region for a specific index */
 #define MEMGETREGIONINFO	_IOWR('M', 8, struct region_info_user)
-#define MEMSETOOBSEL		_IOW('M', 9, struct nand_oobinfo)
+/* Get info about OOB modes (e.g., RAW, PLACE, AUTO) - legacy interface */
 #define MEMGETOOBSEL		_IOR('M', 10, struct nand_oobinfo)
+/* Check if an eraseblock is bad */
 #define MEMGETBADBLOCK		_IOW('M', 11, __kernel_loff_t)
+/* Mark an eraseblock as bad */
 #define MEMSETBADBLOCK		_IOW('M', 12, __kernel_loff_t)
+/* Set OTP (One-Time Programmable) mode (factory vs. user) */
 #define OTPSELECT		_IOR('M', 13, int)
+/* Get number of OTP (One-Time Programmable) regions */
 #define OTPGETREGIONCOUNT	_IOW('M', 14, int)
+/* Get all OTP (One-Time Programmable) info about MTD */
 #define OTPGETREGIONINFO	_IOW('M', 15, struct otp_info)
+/* Lock a given range of user data (must be in mode %MTD_FILE_MODE_OTP_USER) */
 #define OTPLOCK			_IOR('M', 16, struct otp_info)
+/* Get ECC layout (deprecated) */
 #define ECCGETLAYOUT		_IOR('M', 17, struct nand_ecclayout_user)
+/* Get statistics about corrected/uncorrected errors */
 #define ECCGETSTATS		_IOR('M', 18, struct mtd_ecc_stats)
+/* Set MTD mode on a per-file-descriptor basis (see "MTD file modes") */
 #define MTDFILEMODE		_IO('M', 19)
+/* Erase segment of MTD (supports 64-bit address) */
 #define MEMERASE64		_IOW('M', 20, struct erase_info_user64)
+/* Write data to OOB (64-bit version) */
 #define MEMWRITEOOB64		_IOWR('M', 21, struct mtd_oob_buf64)
+/* Read data from OOB (64-bit version) */
 #define MEMREADOOB64		_IOWR('M', 22, struct mtd_oob_buf64)
+/* Check if chip is locked (for MTD that supports it) */
 #define MEMISLOCKED		_IOR('M', 23, struct erase_info_user)
+/*
+ * Most generic write interface; can write in-band and/or out-of-band in various
+ * modes (see "struct mtd_write_req")
+ */
+#define MEMWRITE		_IOWR('M', 24, struct mtd_write_req)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
@@ -177,13 +251,27 @@ struct mtd_ecc_stats {
 };
 
 /*
- * Read/write file modes for access to MTD
+ * MTD file modes - for read/write access to MTD
+ *
+ * @MTD_FILE_MODE_NORMAL:	OTP disabled, ECC enabled
+ * @MTD_FILE_MODE_OTP_FACTORY:	OTP enabled in factory mode
+ * @MTD_FILE_MODE_OTP_USER:	OTP enabled in user mode
+ * @MTD_FILE_MODE_RAW:		OTP disabled, ECC disabled
+ *
+ * These modes can be set via ioctl(MTDFILEMODE). The mode mode will be retained
+ * separately for each open file descriptor.
+ *
+ * Note: %MTD_FILE_MODE_RAW provides the same functionality as %MTD_OPS_RAW -
+ * raw access to the flash, without error correction or autoplacement schemes.
+ * Wherever possible, the MTD_OPS_* mode will override the MTD_FILE_MODE_* mode
+ * (e.g., when using ioctl(MEMWRITE)), but in some cases, the MTD_FILE_MODE is
+ * used out of necessity (e.g., `write()', ioctl(MEMWRITEOOB64)).
  */
 enum mtd_file_modes {
-	MTD_MODE_NORMAL = MTD_OTP_OFF,
-	MTD_MODE_OTP_FACTORY = MTD_OTP_FACTORY,
-	MTD_MODE_OTP_USER = MTD_OTP_USER,
-	MTD_MODE_RAW,
+	MTD_FILE_MODE_NORMAL = MTD_OTP_OFF,
+	MTD_FILE_MODE_OTP_FACTORY = MTD_OTP_FACTORY,
+	MTD_FILE_MODE_OTP_USER = MTD_OTP_USER,
+	MTD_FILE_MODE_RAW,
 };
 
 #endif /* __MTD_ABI_H__ */
diff --git a/nanddump.c b/nanddump.c
index 7a24c0d..be458c6 100644
--- a/nanddump.c
+++ b/nanddump.c
@@ -336,7 +336,7 @@ int main(int argc, char * const argv[])
 	readbuf = xmalloc(sizeof(readbuf) * mtd.min_io_size);
 
 	if (noecc)  {
-		if (ioctl(fd, MTDFILEMODE, MTD_MODE_RAW) != 0) {
+		if (ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW) != 0) {
 				perror("MTDFILEMODE");
 				goto closeall;
 		}
diff --git a/nandwrite.c b/nandwrite.c
index 17a717c..3f70cb3 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -274,7 +274,7 @@ int main(int argc, char * const argv[])
 	}
 
 	if (noecc)  {
-		ret = ioctl(fd, MTDFILEMODE, MTD_MODE_RAW);
+		ret = ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW);
 		if (ret) {
 			switch (errno) {
 			case ENOTTY:
-- 
1.7.5.4

Re: [PATCH 01/10] nandwrite: trivial variable move

[Artem Bityutskiy]

On Wed, 2011-08-31 at 13:00 -0700, Brian Norris wrote:
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  nandwrite.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Pushed this patch, thanks.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 00/10] nandwrite: cleanup, support ioctl(MEMWRITE)

[Artem Bityutskiy]

On Wed, 2011-08-31 at 13:00 -0700, Brian Norris wrote:
> This patch series cleans up old features in nandwrite and supports new
> features with ioctl(MEMWRITE).
> 
> Byproducts of this include:
> (1) kill --raw
> (2) re-implement --autoplace
> (3) support OOB writes for MLC NAND (only write once per page)

I do not have time to thoroughly review the patches, but they look good,
and you make a great job splitting making then easy to follow by
splitting nicely.

I am not very comfortable with "no warning" --raw removal, but we can
try it this way, I guess, it is unlikely someone will be hurt, but there
is some probability.

And last but not least, I think we should not push your changes to the
master branch of mtd-utils _befor_ the corresponding kernel-space
changes have reached Linuses tree (e.g., dwmw2 might dislike something).

So I pushed 2 patches which look independent to the master branch, and
the rest are in the 'brian' branch so far.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 03/10] nandwrite: consolidate buffer usage

[Artem Bityutskiy]

On Wed, 2011-08-31 at 13:00 -0700, Brian Norris wrote:
> Instead of using two different output buffers for OOB data, let's just
> use the same one for all output. This adds an extra memcpy, but it
> simplifies some future work, so it's worth it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Also pushed this one, thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 03/10] nandwrite: consolidate buffer usage

[Mike Frysinger]

On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
> Instead of using two different output buffers for OOB data, let's just
> use the same one for all output. This adds an extra memcpy, but it
> simplifies some future work, so it's worth it.

could it be done by pulling out the pointer ?  make oobbuf a "char *",
rename existing oobbuf to like "char _oobbuf[]", and then assign
oobbuf to the relevant buffer and assume it's always set ...
-mike

Re: [PATCH 00/10] nandwrite: cleanup, support ioctl(MEMWRITE)

[Mike Frysinger]

On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
> This patch series cleans up old features in nandwrite and supports new
> features with ioctl(MEMWRITE).

in general things look OK to me

(sorry for the delay, but i'm working on MTD stuff only in my random
spare time now unlike before)
-mike

Re: [PATCH 03/10] nandwrite: consolidate buffer usage

[Brian Norris]

On Wed, Sep 14, 2011 at 1:15 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
>> Instead of using two different output buffers for OOB data, let's just
>> use the same one for all output. This adds an extra memcpy, but it
>> simplifies some future work, so it's worth it.
>
> could it be done by pulling out the pointer ?  make oobbuf a "char *",
> rename existing oobbuf to like "char _oobbuf[]", and then assign
> oobbuf to the relevant buffer and assume it's always set ...

It could be done this way, but actually, this patch was not intended
to stand alone; it was a precursor to removing one of the buffers from
nandwrite.c, as done in patch 07/10:
   mtdutils: move OOB auto-layout into libmtd's mtd_write
>From the comment in patch 07:
   "This patch also cleans up a need for an extra OOB buffer in nandwrite."

So I'd actually recommend that Artem push this patch (patch 03) out of
'master' and into the 'brian' branch, then we can review the final
product there. In the end, I intended nandwrite to only have a single
buffer for OOB, instead of having a read buffer and an oob buffer. We
would simply pass our unmodified OOB data to mtd_write() and then to
the new MEMWRITE ioctl, where the kernel would do AUTO or PLACE
layouts for us depending on the mode we chose.

Of course, this is just the ideal approach; many kernels will not
support MEMWRITE yet, so they would fall back to the old code. That's
what patch 07 did; it cut out one buffer and moved the old layout code
into its own libmtd function, legacy_auto_oob_layout().
legacy_auto_oob_layout allocates its own buffer when needed; this part
is very inefficient (and actually has a memory leak now that I look at
it...) Perhaps the "legacy" handling should be reviewed a little more
to prevent too many trivial buffers and memcpy's.

If nothing else, I will at least need to send a v2 for patch 07 so we
free `tmp_buf' in `legacy_auto_oob_layout()'. But it'd be better just
to completely rework that function's buffers...

Brian

Re: [PATCH 03/10] nandwrite: consolidate buffer usage

[Mike Frysinger]

On Wed, Sep 14, 2011 at 14:22, Brian Norris wrote:
> On Wed, Sep 14, 2011 at 1:15 AM, Mike Frysinger wrote:
>> On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
>>> Instead of using two different output buffers for OOB data, let's just
>>> use the same one for all output. This adds an extra memcpy, but it
>>> simplifies some future work, so it's worth it.
>>
>> could it be done by pulling out the pointer ?  make oobbuf a "char *",
>> rename existing oobbuf to like "char _oobbuf[]", and then assign
>> oobbuf to the relevant buffer and assume it's always set ...
>
> It could be done this way, but actually, this patch was not intended
> to stand alone; it was a precursor to removing one of the buffers from
> nandwrite.c

if the useless memcpy is removed by way of future patches, then this is fine
-mike

Re: [PATCH 03/10] nandwrite: consolidate buffer usage

[Brian Norris]

On Sat, Sep 17, 2011 at 7:53 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> if the useless memcpy is removed by way of future patches, then this is fine

It's not completely removed. Please see patch 07.

I'll summarize: the memcpy is moved out of the main code path so that
if ioctl(MEMWRITE) is supported in your kernel, you have no extra
memcpy's. However, I didn't optimize the original code, which uses the
standard write+ioctl(MEMWRITEOOB[64]); it still has the useless memcpy
that we are discussing.

I'm not sure if/how your solution can work embedded at that level of
libmtd. Perhaps some functions I moved around need to be restructured
a bit.

Brian