Skip to contentSkip to conversation

nandwrite: cleanup, support ioctl(MEMWRITE)

PATCH In review Brian Norris 3 participants 20 messages Aug 31, 2011 – Sep 19, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover nandwrite: cleanup, support ioctl(MEMWRITE)

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(-)
Signature · 1 line
1.7.5.4
#1 lore raw

PATCH 1/10 nandwrite: trivial variable move

---
 nandwrite.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
nandwrite.c +2 -2
index 95baa38..17a717c 100644
@@ -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];
Signature · 1 line
1.7.5.4
#2 lore raw

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

Quoted 4 lines · Brian Norris
---
 nandwrite.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Pushed this patch, thanks.

Signature · 2 lines
Best Regards,
Artem Bityutskiy
#3 lore raw

PATCH 2/10 mtd-utils: update mtd-abi.h

In reply to 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.

---
 include/mtd/mtd-abi.h |  122 ++++++++++++++++++++++++++++++++++++++++++-------
 nanddump.c            |    2 +-
 nandwrite.c           |    2 +-
 3 files changed, 107 insertions(+), 19 deletions(-)
include/mtd/mtd-abi.h +105 -17
index a86364a..66be707 100644
@@ -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__ */
nanddump.c +1 -1
index 7a24c0d..be458c6 100644
@@ -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;
 		}
nandwrite.c +1 -1
index 17a717c..3f70cb3 100644
@@ -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:
Signature · 1 line
1.7.5.4
#4 lore raw

PATCH v2 2/10 mtd-utils: update mtd-abi.h

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.

---
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(-)
include/mtd/mtd-abi.h +105 -17
index a86364a..4de167b 100644
@@ -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__ */
nanddump.c +1 -1
index 7a24c0d..be458c6 100644
@@ -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;
 		}
nandwrite.c +1 -1
index 17a717c..3f70cb3 100644
@@ -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:
Signature · 1 line
1.7.5.4
#5 lore raw

PATCH 3/10 nandwrite: consolidate buffer usage

In reply to 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.

---
 nandwrite.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
nandwrite.c +3 -2
index 3f70cb3..3eea6e2 100644
@@ -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;
 			}
Signature · 1 line
1.7.5.4
#6 lore raw

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

Quoted 5 lines · 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.

Also pushed this one, thanks!

Signature · 2 lines
Best Regards,
Artem Bityutskiy
#7 lore raw

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

In reply to Brian Norris
Quoted 3 lines · 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.

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

#8 lore raw

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

Quoted 8 lines · Mike Frysinger
Quoted 3 lines · 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.

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

Quoted 1 line

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

#9 lore raw

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

Quoted 6 lines · Brian Norris
Quoted 5 lines · Mike Frysinger
Quoted 3 lines · 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.

Quoted 7 lines
Quoted 3 lines

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

#10 lore raw

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

Quoted 1 line · Mike Frysinger

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

#11 lore raw

PATCH 4/10 libmtd: modify `mtd_write' to cover OOB writes

In reply to 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.

---
 include/libmtd.h      |   15 +++++++++++----
 lib/libmtd.c          |   10 ++++++----
 nandwrite.c           |    4 ++--
 ubi-utils/ubiformat.c |    6 ++++--
 4 files changed, 23 insertions(+), 12 deletions(-)
include/libmtd.h +11 -4
index 9efccbc..07c304a 100644
@@ -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.
lib/libmtd.c +6 -4
index c34874e..746ea69 100644
@@ -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);
nandwrite.c +2 -2
index 3eea6e2..33a3b8f 100644
@@ -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);
ubi-utils/ubiformat.c +4 -2
index bfa1730..ed2b8d0 100644
@@ -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");
Signature · 1 line
1.7.5.4
#12 lore raw

PATCH 5/10 libmtd: support MEMWRITE ioctl

In reply to 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.

---
 lib/libmtd.c |   39 +++++++++++++++++++++++++++++++--------
 1 files changed, 31 insertions(+), 8 deletions(-)
lib/libmtd.c +31 -8
index 746ea69..d47b307 100644
@@ -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;
 }
Signature · 1 line
1.7.5.4
#13 lore raw

PATCH 6/10 nandwrite: merge `mtd_write_oob' and `mtd_write' calls

In reply to Brian Norris

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

---
 nandwrite.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
nandwrite.c +9 -9
index 33a3b8f..a78b0b6 100644
@@ -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);
Signature · 1 line
1.7.5.4
#14 lore raw

PATCH 7/10 mtdutils: move OOB auto-layout into libmtd's mtd_write

In reply to 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.

---
 lib/libmtd.c |   39 +++++++++++++++++++++++++++++++++++++++
 nandwrite.c  |   53 ++++-------------------------------------------------
 2 files changed, 43 insertions(+), 49 deletions(-)
lib/libmtd.c +39 -0
index d47b307..1b16de5 100644
@@ -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");
 	}
nandwrite.c +4 -49

89 lines not on this page · raw message

Signature · 1 line
1.7.5.4
#15 lore raw

PATCH 8/10 nandwrite: kill `--raw' option

In reply to 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.

---
 nandwrite.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)
nandwrite.c +1 -8
index 920863f..45782c7 100644
@@ -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;
Signature · 1 line
1.7.5.4
#16 lore raw

PATCH 9/10 nandwrite: re-implement `--autoplace' option

In reply to 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).

---
 nandwrite.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)
nandwrite.c +20 -2
index 45782c7..ca72f16 100644
@@ -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) {
Signature · 1 line
1.7.5.4
#17 lore raw

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

In reply to 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.

---
 nandwrite.c |   44 ++++++++++++++++----------------------------
 1 files changed, 16 insertions(+), 28 deletions(-)
nandwrite.c +16 -28
index ca72f16..a42f7c9 100644
@@ -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;
Signature · 1 line
1.7.5.4
#18 lore raw
In reply to Brian Norris
Quoted 7 lines · 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)

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!

Signature · 2 lines
Best Regards,
Artem Bityutskiy
#19 lore raw
In reply to Brian Norris
Quoted 2 lines · Brian Norris

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