[flashrom] [PATCH 1/1] Add support for reading the current flash contents from a file

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue Apr 19 22:18:56 CEST 2016


Hi Paul,

we're currently trying to transition to git, and thus we're still
working out the best workflow to continue development. Admittedly this
is a larger resource drain than anticipated.

I wish to apologize for the delayed review. Please find a few thoughts
of mine below.

On 19.04.2016 21:25, Paul Kocialkowski wrote:
> Hi,
> 
> Could we get the review moving on this patch? I first sent it over 4 months ago
> and got no feedback. I'd really like to have this feature merged, as I'm using
> it very often!
> 
> Thanks
> 
> Le lundi 29 février 2016 à 19:44 +0100, Paul Kocialkowski a écrit :
>> When developing software that has to be flashed to a flash chip to be
>> executed,
>> it often takes a long time to read the current flash contents (for flashrom to
>> know what pages to erase and reprogram) each time, when writing the new image.
>> However, when the flash was just reprogrammed, its current state is known to
>> be
>> the previous image that was flashed (assuming it was verified).
>>
>> Thus, it makes sense to provide that image as a file for the flash contents
>> instead of wasting valuable time read the whole flash each time.
>>
>> Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
>> ---
>>  cli_classic.c | 46 ++++++++++++++++++++++++++++------------------
>>  flash.h       |  2 +-
>>  flashrom.c    | 14 +++++++++++---
>>  3 files changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/cli_classic.c b/cli_classic.c
>> index a2c2014..dc0164d 100644
>> --- a/cli_classic.c
>> +++ b/cli_classic.c
>> @@ -44,24 +44,25 @@ static void cli_classic_usage(const char *name)
>>  	       "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...]
>> [-n] [-f]]\n"
>>  	       "[-V[V[V]]] [-o <logfile>]\n\n", name);
>>  
>> -	printf(" -h | --help                        print this help text\n"
>> -	       " -R | --version                     print version
>> (release)\n"
>> -	       " -r | --read <file>                 read flash and save to
>> <file>\n"
>> -	       " -w | --write <file>                write <file> to flash\n"
>> -	       " -v | --verify <file>               verify flash against
>> <file>\n"
>> -	       " -E | --erase                       erase flash memory\n"
>> -	       " -V | --verbose                     more verbose output\n"
>> -	       " -c | --chip <chipname>             probe only for specified
>> flash chip\n"
>> -	       " -f | --force                       force specific operations
>> (see man page)\n"
>> -	       " -n | --noverify                    don't auto-verify\n"
>> -	       " -l | --layout <layoutfile>         read ROM layout from
>> <layoutfile>\n"
>> -	       " -i | --image <name>                only flash image <name>
>> from flash layout\n"
>> -	       " -o | --output <logfile>            log output to
>> <logfile>\n"
>> -	       " -L | --list-supported              print supported
>> devices\n"
>> +	printf(" -h | --help                          print this help text\n"
>> +	       " -R | --version                       print version
>> (release)\n"
>> +	       " -r | --read <file>                   read flash and save to
>> <file>\n"
>> +	       " -w | --write <file>                  write <file> to
>> flash\n"
>> +	       " -v | --verify <file>                 verify flash against
>> <file>\n"
>> +	       " -E | --erase                         erase flash memory\n"
>> +	       " -V | --verbose                       more verbose output\n"
>> +	       " -c | --chip <chipname>               probe only for
>> specified flash chip\n"
>> +	       " -f | --force                         force specific
>> operations (see man page)\n"
>> +	       " -n | --noverify                      don't auto-verify\n"
>> +	       " -l | --layout <layoutfile>           read ROM layout from
>> <layoutfile>\n"
>> +	       " -i | --image <name>                  only flash image <name>
>> from flash layout\n"
>> +	       " -o | --output <logfile>              log output to
>> <logfile>\n"
>> +	       " -C | --flash-contents <contentsfile> assume flash contents
>> to be <contentsfile>\n"
>> +	       " -L | --list-supported                print supported
>> devices\n"
>>  #if CONFIG_PRINT_WIKI == 1
>> -	       " -z | --list-supported-wiki         print supported devices
>> in wiki syntax\n"
>> +	       " -z | --list-supported-wiki           print supported devices
>> in wiki syntax\n"
>>  #endif
>> -	       " -p | --programmer <name>[:<param>] specify the programmer
>> device. One of\n");
>> +	       " -p | --programmer <name>[:<param>]   specify the programmer
>> device. One of\n");
>>  	list_programmers_linebreak(4, 80, 0);
>>  	printf(".\n\nYou can specify one of -h, -R, -L, "
>>  #if CONFIG_PRINT_WIKI == 1
>> @@ -106,7 +107,7 @@ int main(int argc, char *argv[])
>>  	enum programmer prog = PROGRAMMER_INVALID;
>>  	int ret = 0;
>>  
>> -	static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:";
>> +	static const char optstring[] = "r:Rw:v:nVEfc:l:i:C:p:Lzho:";
>>  	static const struct option long_options[] = {
>>  		{"read",		1, NULL, 'r'},
>>  		{"write",		1, NULL, 'w'},
>> @@ -118,6 +119,7 @@ int main(int argc, char *argv[])
>>  		{"force",		0, NULL, 'f'},
>>  		{"layout",		1, NULL, 'l'},
>>  		{"image",		1, NULL, 'i'},
>> +		{"flash-contents",	1, NULL, 'C'},
>>  		{"list-supported",	0, NULL, 'L'},
>>  		{"list-supported-wiki",	0, NULL, 'z'},
>>  		{"programmer",		1, NULL, 'p'},
>> @@ -128,6 +130,7 @@ int main(int argc, char *argv[])
>>  	};
>>  
>>  	char *filename = NULL;
>> +	char *contentsfile = NULL;
>>  	char *layoutfile = NULL;
>>  #ifndef STANDALONE
>>  	char *logfile = NULL;
>> @@ -221,6 +224,9 @@ int main(int argc, char *argv[])
>>  				cli_classic_abort_usage();
>>  			}
>>  			break;
>> +		case 'C':
>> +			contentsfile = strdup(optarg);
>> +			break;
>>  		case 'L':
>>  			if (++operation_specified > 1) {
>>  				fprintf(stderr, "More than one operation "

You're introducing a new toplevel parameter for this, and I wonder
whether it would make more sense to handle it as a programmer option.
The dummy programmer supports reading in emulated flash chip content on
startup and writing out emulated flash chip content on shutdown, all
specified with -p dummy:emulate=M25P10.RES,image=foobar.bin

For consistency reasons, we should make sure that supplying assumed chip
contents works as designed both for hardware-based programmers as well
as the dummy programmer.
I don't have a strong preference for -C or -p foobar:image=some.bin, and
would like to hear what others think.

Regards,
Carl-Daniel




More information about the flashrom mailing list