Skip to contentSkip to conversation

flash_{un,}lock upgrades

PATCH Applied Brian Norris 1 participant 13 messages Aug 31, 2015 – Nov 11, 2015 Memory technology devices (MTD)

lore
Share & tools
lore
#0 lore raw

cover flash_{un,}lock upgrades

Hi,

I've done a bit of work to make the flash_lock tool more useful, in order to help test some new SPI NOR block protection features. Of note:

* added getopt support
* further unified flash_unlock and flash_lock -- the only difference now is their default feature, and both binaries contain support for all features
* add support for MEMISLOCKED, so we can query arbitrary regions for protection status
* mtdinfo -M already can dump some block lock information, but you get a little more flexibility using flash_lock --info, so both seem worth having IMO

Unsolved issues:

* the MEM{LOCK,UNLOCK,ISLOCKED} ioctls all still use the out-dated 32-bit erase_info_user struct, which means they'll have problems supporting >=4GB flash. This isn't a mtd-utils problem, per se, and at the moment, Linux NAND (the only (?) candidate for >=4GB flash) support doesn't implement any of the locking APIs. But just an observation I ran across.
* Deprecation plan: it would make sense to deprecate and remove flash_unlock eventually (in favor of 'flash_lock --unlock'), but this isn't really pressing, so I don't see a good reason to spend much effort on it.

Enjoy,
Brian

Brian Norris (11):
flash_{un,}lock: nest optional parameters in help message flash_{un,}lock: switch to getopt library flash_{un,}lock: support --version flag flash_{un,}lock: document option flags flash_{un,}lock: abstract the argument positions flash_{un,}lock: move args processing to its own function flash_{un,}lock: support both lock/unlock in the same binary flash_{un,}lock: add MEMISLOCKED support flash_{un,}lock: improve strtol() error handling flash_{un,}lock: don't allow "last byte + 1" flash_{un,}lock: document block count == -1

 flash_unlock.c | 182 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 156 insertions(+), 26 deletions(-)
Signature · 1 line
2.5.0.457.gab17608
#1 lore raw

PATCH 1/11 flash_{un, }lock: nest optional parameters in help message

block count should be nested within the optional offset listing. That is, we require offset before we accept a block count.

---
 flash_unlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
flash_unlock.c +1 -1
index 1cc8c2ff2d9b..ee8ac890f61c 100644
@@ -28,7 +28,7 @@
 static void usage(int status)
 {
 	fprintf(status ? stderr : stdout,
-		"Usage: %s <mtd device> [offset] [block count]\n\n"
+		"Usage: %s <mtd device> [offset [block count]]\n\n"
 		"If offset is not specified, it defaults to 0.\n"
 		"If block count is not specified, it defaults to all blocks.\n",
 		PROGRAM_NAME);
Signature · 1 line
2.5.0.457.gab17608
#2 lore raw

PATCH 2/11 flash_{un,}lock: switch to getopt library

In reply to Brian Norris

We will be adding some more flags, so the getopt library can help.

---
 flash_unlock.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
flash_unlock.c +24 -2
index ee8ac890f61c..777e4375a44d 100644
@@ -13,6 +13,7 @@
 #define FLASH_UNLOCK 0
 #endif
 
+#include <getopt.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -35,6 +36,12 @@ static void usage(int status)
 	exit(status);
 }
 
+static const char short_opts[] = "h";
+static const struct option long_opts[] = {
+	{ "help",	no_argument,	0, 'h' },
+	{ NULL,		0,		0, 0 },
+};
+
 int main(int argc, char *argv[])
 {
 	int fd, request;
@@ -43,11 +50,26 @@ int main(int argc, char *argv[])
 	int count;
 	const char *dev;
 
+	for (;;) {
+		int c;
+
+		c = getopt_long(argc, argv, short_opts, long_opts, NULL);
+		if (c == EOF)
+			break;
+
+		switch (c) {
+		case 'h':
+			usage(0);
+			break;
+		default:
+			usage(1);
+			break;
+		}
+	}
+
 	/* Parse command line options */
 	if (argc < 2 || argc > 4)
 		usage(1);
-	if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
-		usage(0);
 
 	dev = argv[1];
 
Signature · 1 line
2.5.0.457.gab17608
#3 lore raw

PATCH 3/11 flash_{un,}lock: support --version flag

In reply to Brian Norris

Just use the common helper macro.

---
 flash_unlock.c | 4 ++++
 1 file changed, 4 insertions(+)
flash_unlock.c +4 -0
index 777e4375a44d..9a24b5f796e0 100644
@@ -39,6 +39,7 @@ static void usage(int status)
 static const char short_opts[] = "h";
 static const struct option long_opts[] = {
 	{ "help",	no_argument,	0, 'h' },
+	{ "version",	no_argument,	0, 'v' },
 	{ NULL,		0,		0, 0 },
 };
 
@@ -61,6 +62,9 @@ int main(int argc, char *argv[])
 		case 'h':
 			usage(0);
 			break;
+		case 'v':
+			common_print_version();
+			exit(0);
 		default:
 			usage(1);
 			break;
Signature · 1 line
2.5.0.457.gab17608
#4 lore raw

PATCH 4/11 flash_{un,}lock: document option flags

In reply to Brian Norris
---
 flash_unlock.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
flash_unlock.c +6 -1
index 9a24b5f796e0..27e7df1eac3f 100644
@@ -29,7 +29,12 @@
 static void usage(int status)
 {
 	fprintf(status ? stderr : stdout,
-		"Usage: %s <mtd device> [offset [block count]]\n\n"
+		"Usage: %s [options] [--] <mtd device> [offset [block count]]\n"
+		"\n"
+		"Options:\n"
+		" -h         --help              Display this help and exit\n"
+		"            --version           Display version information and exit\n"
+		"\n"
 		"If offset is not specified, it defaults to 0.\n"
 		"If block count is not specified, it defaults to all blocks.\n",
 		PROGRAM_NAME);
Signature · 1 line
2.5.0.457.gab17608
#5 lore raw

PATCH 5/11 flash_{un, }lock: abstract the argument positions

In reply to Brian Norris

Previously, there were no options (besides stand-alone --help and --version), so we just used fixed-position argv indexes. Let's change that.

Also clean up the sanity checks a bit to make them more verbose and specific.

---
 flash_unlock.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)
flash_unlock.c +29 -8
index 27e7df1eac3f..ce72e49d4ba6 100644
@@ -54,7 +54,8 @@ int main(int argc, char *argv[])
 	struct mtd_info_user mtdInfo;
 	struct erase_info_user mtdLockInfo;
 	int count;
-	const char *dev;
+	const char *dev, *offs_s, *count_s;
+	int arg_idx;
 
 	for (;;) {
 		int c;
@@ -76,11 +77,31 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	/* Parse command line options */
-	if (argc < 2 || argc > 4)
+	arg_idx = optind;
+
+	/* Sanity checks */
+	if (argc - arg_idx < 1) {
+		errmsg("too few arguments");
+		usage(1);
+	} else if (argc - arg_idx > 3) {
+		errmsg("too many arguments");
 		usage(1);
+	}
+
+	/* First non-option argument */
+	dev = argv[arg_idx++];
 
-	dev = argv[1];
+	/* Second non-option argument */
+	if (arg_idx < argc)
+		offs_s = argv[arg_idx++];
+	else
+		offs_s = NULL;
+
+	/* Third non-option argument */
+	if (arg_idx < argc)
+		count_s = argv[arg_idx++];
+	else
+		count_s = NULL;
 
 	/* Get the device info to compare to command line sizes */
 	fd = open(dev, O_RDWR);
@@ -91,16 +112,16 @@ int main(int argc, char *argv[])
 		sys_errmsg_die("could not get mtd info: %s", dev);
 
 	/* Make sure user options are valid */
-	if (argc > 2)
-		mtdLockInfo.start = strtol(argv[2], NULL, 0);
+	if (offs_s)
+		mtdLockInfo.start = strtol(offs_s, NULL, 0);
 	else
 		mtdLockInfo.start = 0;
 	if (mtdLockInfo.start > mtdInfo.size)
 		errmsg_die("%#x is beyond device size %#x",
 			mtdLockInfo.start, mtdInfo.size);
 
-	if (argc > 3) {
-		count = strtol(argv[3], NULL, 0);
+	if (count_s) {
+		count = strtol(count_s, NULL, 0);
 		if (count == -1)
 			mtdLockInfo.length = mtdInfo.size;
 		else
Signature · 1 line
2.5.0.457.gab17608
#6 lore raw

PATCH 6/11 flash_{un, }lock: move args processing to its own function

In reply to Brian Norris
---
 flash_unlock.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
flash_unlock.c +15 -6
index ce72e49d4ba6..67277493d4b3 100644
@@ -48,13 +48,11 @@ static const struct option long_opts[] = {
 	{ NULL,		0,		0, 0 },
 };
 
-int main(int argc, char *argv[])
+/* Program arguments */
+static const char *dev, *offs_s, *count_s;
+
+static void process_args(int argc, char *argv[])
 {
-	int fd, request;
-	struct mtd_info_user mtdInfo;
-	struct erase_info_user mtdLockInfo;
-	int count;
-	const char *dev, *offs_s, *count_s;
 	int arg_idx;
 
 	for (;;) {
@@ -103,6 +101,17 @@ int main(int argc, char *argv[])
 	else
 		count_s = NULL;
 
+}
+
+int main(int argc, char *argv[])
+{
+	int fd, request;
+	struct mtd_info_user mtdInfo;
+	struct erase_info_user mtdLockInfo;
+	int count;
+
+	process_args(argc, argv);
+
 	/* Get the device info to compare to command line sizes */
 	fd = open(dev, O_RDWR);
 	if (fd < 0)
Signature · 1 line
2.5.0.457.gab17608
#7 lore raw

PATCH 7/11 flash_{un, }lock: support both lock/unlock in the same binary

In reply to Brian Norris

Add new --lock/--unlock flags, so we can do either with the same binary. This will prepare for the addition of other features, so we don't have to keep duplicating the same binary via #include "flash_unlock.c".

The defaults still work as expected: flash_unlock will default to REQUEST_UNLOCK, and flash_lock will default to REQUEST_LOCK.

Eventually, we might deprecate one of the two (flash_unlock, probably), so we only have to ship one flash_{un,}lock binary.

---
 flash_unlock.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 8 deletions(-)
flash_unlock.c +48 -8
index 67277493d4b3..be09347faa92 100644
@@ -4,13 +4,16 @@
  * utilities for locking/unlocking sectors of flash devices
  */
 
+enum flash_lock_request {
+	REQUEST_LOCK,
+	REQUEST_UNLOCK,
+};
+
 #ifndef PROGRAM_NAME
-#define PROGRAM_NAME "flash_unlock"
-#define FLASH_MSG    "unlock"
-#define FLASH_UNLOCK 1
+#define PROGRAM_NAME		"flash_unlock"
+#define DEFAULT_REQUEST		REQUEST_UNLOCK
 #else
-#define FLASH_MSG    "lock"
-#define FLASH_UNLOCK 0
+#define DEFAULT_REQUEST		REQUEST_LOCK
 #endif
 
 #include <getopt.h>
@@ -26,34 +29,48 @@
 #include "common.h"
 #include <mtd/mtd-user.h>
 
+static const char *flash_msg[] = {
+	[ REQUEST_LOCK ]	= "lock",
+	[ REQUEST_UNLOCK ]	= "unlock",
+};
+
 static void usage(int status)
 {
 	fprintf(status ? stderr : stdout,
+		"Utility to lock or unlock the flash. Default action: %s\n"
+		"\n"
 		"Usage: %s [options] [--] <mtd device> [offset [block count]]\n"
 		"\n"
 		"Options:\n"
 		" -h         --help              Display this help and exit\n"
 		"            --version           Display version information and exit\n"
+		" -l         --lock              Lock a region of flash\n"
+		" -u         --unlock            Unlock a region of flash\n"
 		"\n"
 		"If offset is not specified, it defaults to 0.\n"
 		"If block count is not specified, it defaults to all blocks.\n",
+		flash_msg[DEFAULT_REQUEST],
 		PROGRAM_NAME);
 	exit(status);
 }
 
-static const char short_opts[] = "h";
+static const char short_opts[] = "hlu";
 static const struct option long_opts[] = {
 	{ "help",	no_argument,	0, 'h' },
+	{ "lock",	no_argument,	0, 'l' },
+	{ "unlock",	no_argument,	0, 'u' },
 	{ "version",	no_argument,	0, 'v' },
 	{ NULL,		0,		0, 0 },
 };
 
 /* Program arguments */
 static const char *dev, *offs_s, *count_s;
+static enum flash_lock_request req = DEFAULT_REQUEST;
 
 static void process_args(int argc, char *argv[])
 {
 	int arg_idx;
+	int req_set = 0;
 
 	for (;;) {
 		int c;
@@ -66,6 +83,14 @@ static void process_args(int argc, char *argv[])
 		case 'h':
 			usage(0);
 			break;
+		case 'l':
+			req = REQUEST_LOCK;
+			req_set++;
+			break;
+		case 'u':
+			req = REQUEST_UNLOCK;
+			req_set++;
+			break;
 		case 'v':
 			common_print_version();
 			exit(0);
@@ -75,6 +100,11 @@ static void process_args(int argc, char *argv[])
 		}
 	}
 
+	if (req_set > 1) {
+		errmsg("cannot specify more than one lock/unlock option");
+		usage(1);
+	}
+
 	arg_idx = optind;
 
 	/* Sanity checks */
@@ -142,10 +172,20 @@ int main(int argc, char *argv[])
 			mtdLockInfo.start, mtdLockInfo.length, mtdInfo.size);
 
 	/* Finally do the operation */
-	request = FLASH_UNLOCK ? MEMUNLOCK : MEMLOCK;
+	switch (req) {
+	case REQUEST_LOCK:
+		request = MEMLOCK;
+		break;
+	case REQUEST_UNLOCK:
+		request = MEMUNLOCK;
+		break;
+	default:
+		errmsg_die("unknown request type: %d", req);
+		break;
+	}
 	if (ioctl(fd, request, &mtdLockInfo))
 		sys_errmsg_die("could not %s device: %s\n",
-			FLASH_MSG, dev);
+				flash_msg[req], dev);
 
 	return 0;
 }
Signature · 1 line
2.5.0.457.gab17608
#8 lore raw

PATCH 8/11 flash_{un,}lock: add MEMISLOCKED support

In reply to Brian Norris

With the -i / --islocked flags.

Sample output:

  # flash_lock --islocked /dev/mtd0
  Device: /dev/mtd0
  Start: 0
  Len: 0x400000
  Lock status: unlocked
  Return code: 0
---
 flash_unlock.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)
flash_unlock.c +26 -4
index be09347faa92..644360548601 100644
@@ -7,6 +7,7 @@
 enum flash_lock_request {
 	REQUEST_LOCK,
 	REQUEST_UNLOCK,
+	REQUEST_ISLOCKED,
 };
 
 #ifndef PROGRAM_NAME
@@ -32,18 +33,21 @@ enum flash_lock_request {
 static const char *flash_msg[] = {
 	[ REQUEST_LOCK ]	= "lock",
 	[ REQUEST_UNLOCK ]	= "unlock",
+	[ REQUEST_ISLOCKED ]	= "check lock status",
 };
 
 static void usage(int status)
 {
 	fprintf(status ? stderr : stdout,
-		"Utility to lock or unlock the flash. Default action: %s\n"
+		"Utility to lock, unlock, or check the lock status of the flash.\n"
+		"Default action: %s\n"
 		"\n"
 		"Usage: %s [options] [--] <mtd device> [offset [block count]]\n"
 		"\n"
 		"Options:\n"
 		" -h         --help              Display this help and exit\n"
 		"            --version           Display version information and exit\n"
+		" -i         --islocked          Check if flash region is locked\n"
 		" -l         --lock              Lock a region of flash\n"
 		" -u         --unlock            Unlock a region of flash\n"
 		"\n"
@@ -54,9 +58,10 @@ static void usage(int status)
 	exit(status);
 }
 
-static const char short_opts[] = "hlu";
+static const char short_opts[] = "hilu";
 static const struct option long_opts[] = {
 	{ "help",	no_argument,	0, 'h' },
+	{ "islocked",	no_argument,	0, 'i' },
 	{ "lock",	no_argument,	0, 'l' },
 	{ "unlock",	no_argument,	0, 'u' },
 	{ "version",	no_argument,	0, 'v' },
@@ -83,6 +88,10 @@ static void process_args(int argc, char *argv[])
 		case 'h':
 			usage(0);
 			break;
+		case 'i':
+			req = REQUEST_ISLOCKED;
+			req_set++;
+			break;
 		case 'l':
 			req = REQUEST_LOCK;
 			req_set++;
@@ -101,7 +110,7 @@ static void process_args(int argc, char *argv[])
 	}
 
 	if (req_set > 1) {
-		errmsg("cannot specify more than one lock/unlock option");
+		errmsg("cannot specify more than one lock/unlock/islocked option");
 		usage(1);
 	}
 
@@ -139,6 +148,7 @@ int main(int argc, char *argv[])
 	struct mtd_info_user mtdInfo;
 	struct erase_info_user mtdLockInfo;
 	int count;
+	int ret;
 
 	process_args(argc, argv);
 
@@ -179,13 +189,25 @@ int main(int argc, char *argv[])
 	case REQUEST_UNLOCK:
 		request = MEMUNLOCK;
 		break;
+	case REQUEST_ISLOCKED:
+		request = MEMISLOCKED;
+		break;
 	default:
 		errmsg_die("unknown request type: %d", req);
 		break;
 	}
-	if (ioctl(fd, request, &mtdLockInfo))
+	ret = ioctl(fd, request, &mtdLockInfo);
+	if (ret < 0)
 		sys_errmsg_die("could not %s device: %s\n",
 				flash_msg[req], dev);
 
+	if (req == REQUEST_ISLOCKED) {
+		printf("Device: %s\n", dev);
+		printf("Start: %#0x\n", mtdLockInfo.start);
+		printf("Len: %#0x\n", mtdLockInfo.length);
+		printf("Lock status: %s\n", ret ? "locked" : "unlocked");
+		printf("Return code: %d\n", ret);
+	}
+
 	return 0;
 }
Signature · 1 line
2.5.0.457.gab17608
#9 lore raw

PATCH 9/11 flash_{un, }lock: improve strtol() error handling

In reply to Brian Norris

Use the simple_* helpers to improve error checking.

Also fixup brace style at the same time.

---
 flash_unlock.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
flash_unlock.c +13 -7
index 644360548601..7a7a773d1d76 100644
@@ -147,8 +147,8 @@ int main(int argc, char *argv[])
 	int fd, request;
 	struct mtd_info_user mtdInfo;
 	struct erase_info_user mtdLockInfo;
-	int count;
-	int ret;
+	long count;
+	int ret = 0;
 
 	process_args(argc, argv);
 
@@ -161,22 +161,28 @@ int main(int argc, char *argv[])
 		sys_errmsg_die("could not get mtd info: %s", dev);
 
 	/* Make sure user options are valid */
-	if (offs_s)
-		mtdLockInfo.start = strtol(offs_s, NULL, 0);
-	else
+	if (offs_s) {
+		mtdLockInfo.start = simple_strtol(offs_s, &ret);
+		if (ret)
+			errmsg_die("bad offset");
+	} else {
 		mtdLockInfo.start = 0;
+	}
 	if (mtdLockInfo.start > mtdInfo.size)
 		errmsg_die("%#x is beyond device size %#x",
 			mtdLockInfo.start, mtdInfo.size);
 
 	if (count_s) {
-		count = strtol(count_s, NULL, 0);
+		count = simple_strtol(count_s, &ret);
+		if (ret)
+			errmsg_die("bad count");
 		if (count == -1)
 			mtdLockInfo.length = mtdInfo.size;
 		else
 			mtdLockInfo.length = mtdInfo.erasesize * count;
-	} else
+	} else {
 		mtdLockInfo.length = mtdInfo.size;
+	}
 	if (mtdLockInfo.start + mtdLockInfo.length > mtdInfo.size)
 		errmsg_die("range is more than device supports: %#x + %#x > %#x",
 			mtdLockInfo.start, mtdLockInfo.length, mtdInfo.size);
Signature · 1 line
2.5.0.457.gab17608
#10 lore raw

PATCH 10/11 flash_{un,}lock: don't allow "last byte + 1"

In reply to Brian Norris

A lock/unlock/islocked ioctl() should be prevented from anything past the last byte, inclusive. But we were doing an exclusive check.

This isn't a big deal, as the kernel MTD APIs would be guarding this anyway, but let's do this for completeness.

---
 flash_unlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
flash_unlock.c +1 -1
index 7a7a773d1d76..d775c0b01950 100644
@@ -168,7 +168,7 @@ int main(int argc, char *argv[])
 	} else {
 		mtdLockInfo.start = 0;
 	}
-	if (mtdLockInfo.start > mtdInfo.size)
+	if (mtdLockInfo.start >= mtdInfo.size)
 		errmsg_die("%#x is beyond device size %#x",
 			mtdLockInfo.start, mtdInfo.size);
 
Signature · 1 line
2.5.0.457.gab17608
#11 lore raw

PATCH 11/11 flash_{un,}lock: document block count == -1

In reply to Brian Norris

These utilities have accepted -1 as a block count to mean "all blocks." Let's document that.

---
 flash_unlock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
flash_unlock.c +2 -1
index d775c0b01950..f51870aeb69f 100644
@@ -52,7 +52,8 @@ static void usage(int status)
 		" -u         --unlock            Unlock a region of flash\n"
 		"\n"
 		"If offset is not specified, it defaults to 0.\n"
-		"If block count is not specified, it defaults to all blocks.\n",
+		"If block count is not specified, it defaults to all blocks.\n"
+		"A block count of -1 means all blocks.\n",
 		flash_msg[DEFAULT_REQUEST],
 		PROGRAM_NAME);
 	exit(status);
Signature · 1 line
2.5.0.457.gab17608
author Applied
#12 lore raw
In reply to Brian Norris
Quoted 25 lines · Brian Norris

Hi,

I've done a bit of work to make the flash_lock tool more useful, in order to help test some new SPI NOR block protection features. Of note:

* added getopt support
* further unified flash_unlock and flash_lock -- the only difference now is their default feature, and both binaries contain support for all features
* add support for MEMISLOCKED, so we can query arbitrary regions for protection status
* mtdinfo -M already can dump some block lock information, but you get a little more flexibility using flash_lock --info, so both seem worth having IMO

Unsolved issues:

* the MEM{LOCK,UNLOCK,ISLOCKED} ioctls all still use the out-dated 32-bit erase_info_user struct, which means they'll have problems supporting >=4GB flash. This isn't a mtd-utils problem, per se, and at the moment, Linux NAND (the only (?) candidate for >=4GB flash) support doesn't implement any of the locking APIs. But just an observation I ran across.
* Deprecation plan: it would make sense to deprecate and remove flash_unlock eventually (in favor of 'flash_lock --unlock'), but this isn't really pressing, so I don't see a good reason to spend much effort on it.

Quoted 2 lines

Enjoy,
Brian

Pushed to mtd-utils.git