flash_{un,}lock upgrades
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
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
@@ -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
PATCH 2/11 flash_{un,}lock: switch to getopt library
In reply to Brian NorrisWe 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
@@ -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
PATCH 3/11 flash_{un,}lock: support --version flag
In reply to Brian NorrisJust use the common helper macro.
--- flash_unlock.c | 4 ++++ 1 file changed, 4 insertions(+)
flash_unlock.c +4 -0
@@ -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
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
@@ -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
PATCH 5/11 flash_{un, }lock: abstract the argument positions
In reply to Brian NorrisPreviously, 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
@@ -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
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
@@ -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
PATCH 7/11 flash_{un, }lock: support both lock/unlock in the same binary
In reply to Brian NorrisAdd 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
@@ -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
PATCH 8/11 flash_{un,}lock: add MEMISLOCKED support
In reply to Brian NorrisWith 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
@@ -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
PATCH 9/11 flash_{un, }lock: improve strtol() error handling
In reply to Brian NorrisUse 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
@@ -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
PATCH 10/11 flash_{un,}lock: don't allow "last byte + 1"
In reply to Brian NorrisA 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
@@ -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
PATCH 11/11 flash_{un,}lock: document block count == -1
In reply to Brian NorrisThese 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
@@ -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
Brian Norris
AppliedQuoted 25 lines · Brian Norris
Hi,
I've done a bit of work to make the
flash_locktool more useful, in order to help test some new SPI NOR block protection features. Of note:* added getopt support
* further unifiedflash_unlockandflash_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 usingflash_lock--info, so both seem worth having IMOUnsolved issues:
* the MEM{LOCK,UNLOCK,ISLOCKED} ioctls all still use the out-dated 32-bit
erase_info_userstruct, 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 removeflash_unlockeventually (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