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

login
register
about
Submitter Paul Kocialkowski
Date 2016-02-29 18:44:33
Message ID <1456771473-9874-2-git-send-email-contact@paulk.fr>
Download mbox | patch
Permalink /patch/4414/
State New
Headers show

Comments

Paul Kocialkowski - 2016-02-29 18:44:33
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@paulk.fr>
---
 cli_classic.c | 46 ++++++++++++++++++++++++++++------------------
 flash.h       |  2 +-
 flashrom.c    | 14 +++++++++++---
 3 files changed, 40 insertions(+), 22 deletions(-)
Paul Kocialkowski - 2016-04-19 19:25:38
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@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 "
> @@ -332,6 +338,9 @@ int main(int argc, char *argv[])
>  	if (layoutfile && check_filename(layoutfile, "layout")) {
>  		cli_classic_abort_usage();
>  	}
> +	if (contentsfile && check_filename(contentsfile, "contents")) {
> +		cli_classic_abort_usage();
> +	}
>  
>  #ifndef STANDALONE
>  	if (logfile && check_filename(logfile, "log"))
> @@ -542,7 +551,7 @@ int main(int argc, char *argv[])
>  	 * Give the chip time to settle.
>  	 */
>  	programmer_delay(100000);
> -	ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it,
> verify_it);
> +	ret |= doit(fill_flash, force, filename, contentsfile, read_it,
> write_it, erase_it, verify_it);
>  
>  	unmap_flash(fill_flash);
>  out_shutdown:
> @@ -554,6 +563,7 @@ out:
>  	layout_cleanup();
>  	free(filename);
>  	free(layoutfile);
> +	free(contentsfile);
>  	free(pparam);
>  	/* clean up global variables */
>  	free((char *)chip_to_probe); /* Silence! Freeing is not modifying
> contents. */
> diff --git a/flash.h b/flash.h
> index dc5c140..2cf7ca6 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -284,7 +284,7 @@ void print_buildinfo(void);
>  void print_banner(void);
>  void list_programmers_linebreak(int startcol, int cols, int paren);
>  int selfcheck(void);
> -int doit(struct flashctx *flash, int force, const char *filename, int
> read_it, int write_it, int erase_it, int verify_it);
> +int doit(struct flashctx *flash, int force, const char *filename, const char
> *contentsfile, int read_it, int write_it, int erase_it, int verify_it);
>  int read_buf_from_file(unsigned char *buf, unsigned long size, const char
> *filename);
>  int write_buf_to_file(const unsigned char *buf, unsigned long size, const
> char *filename);
>  
> diff --git a/flashrom.c b/flashrom.c
> index d5c3238..d11f89d 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1978,8 +1978,9 @@ int chip_safety_check(const struct flashctx *flash, int
> force, int read_it, int
>   * but right now it allows us to split off the CLI code.
>   * Besides that, the function itself is a textbook example of abysmal code
> flow.
>   */
> -int doit(struct flashctx *flash, int force, const char *filename, int
> read_it,
> -	 int write_it, int erase_it, int verify_it)
> +int doit(struct flashctx *flash, int force, const char *filename,
> +	 const char *contentsfile, int read_it, int write_it, int erase_it,
> +	 int verify_it)
>  {
>  	uint8_t *oldcontents;
>  	uint8_t *newcontents;
> @@ -2068,7 +2069,14 @@ int doit(struct flashctx *flash, int force, const char
> *filename, int read_it,
>  	 * preserved, but in that case we might perform unneeded erase which
>  	 * takes time as well.
>  	 */
> -	if (read_all_first) {
> +	if (contentsfile) {
> +		msg_cinfo("Reading old flash chip contents from file... ");
> +		if (read_buf_from_file(oldcontents, size, contentsfile)) {
> +			ret = 1;
> +			msg_cinfo("FAILED.\n");
> +			goto out;
> +		}
> +	} else if (read_all_first) {
>  		msg_cinfo("Reading old flash chip contents... ");
>  		if (flash->chip->read(flash, oldcontents, 0, size)) {
>  			ret = 1;
Carl-Daniel Hailfinger - 2016-04-19 20:18:56
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@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
Michael Karcher - 2016-04-19 20:56:47
On 04/19/2016 10:18 PM, Carl-Daniel Hailfinger wrote:

> 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.
I strongly prefer a standalone toplevel option, as the feature is
programmer independent. Of course, this means that for the dummy
programmer we now have -C and -p image=some.bin. I don't consider it a
bad thing, but a good thing. An important use case for the dummy
programmer is to test flashrom under various conditions. This might be
flashrom getting "-C" with the contents of the chip, or flashrom getting
no "-C" and reading first. Both execution flows should be testable using
the dummy programmer.
On the other hand, separate options allow mismatch of "-C" and the image
file specified in "-p dummy:image=some.bin". Specifying wrong chip
contents is an undesireable situation one really wants to avoid in
practice, because it causes flashrom to do unpredictable things. In my
oppinion, we should nevertheless support getting into this situation
with the dummy programmer, as we are equally able to get into that
situation with real programmers.
Yet a way to avoid the mismatch seems a good thing to have, too. So I
suggest to have "-C" load the "actual contents" of the chip
independently from any programmer option or any programmer code, but
make it possible for the dummy programmer to initialize the emulated
flash chip from the loaded "actual contents". Something like -p
dummy:autoimage comes to my mind, although I understand that loading the
"-C" can well be performed after programmer initialization, but
initializing the emulated dummy flash chip is done during programmer
initialization. I still tend to work around that in code (i.e. having
the -C load before programmer init, even if it complicates the
read/erase/program/verify logic).

Regards,
  Michael Karcher
David Hendricks - 2016-04-20 23:54:34
Looks pretty good to me. FWIW, we implemented something nearly identical in
the chromiumos branch a few years ago:
https://chromium-review.googlesource.com/#/c/10180/ (we called it
"--diff"). It's been quite useful for development, especially when working
with large chips and slow external programmers.

On Tue, Apr 19, 2016 at 12:25 PM, Paul Kocialkowski <contact@paulk.fr>
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@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 "
> > @@ -332,6 +338,9 @@ int main(int argc, char *argv[])
> >       if (layoutfile && check_filename(layoutfile, "layout")) {
> >               cli_classic_abort_usage();
> >       }
> > +     if (contentsfile && check_filename(contentsfile, "contents")) {
> > +             cli_classic_abort_usage();
> > +     }
> >
> >  #ifndef STANDALONE
> >       if (logfile && check_filename(logfile, "log"))
> > @@ -542,7 +551,7 @@ int main(int argc, char *argv[])
> >        * Give the chip time to settle.
> >        */
> >       programmer_delay(100000);
> > -     ret |= doit(fill_flash, force, filename, read_it, write_it,
> erase_it,
> > verify_it);
> > +     ret |= doit(fill_flash, force, filename, contentsfile, read_it,
> > write_it, erase_it, verify_it);
> >
> >       unmap_flash(fill_flash);
> >  out_shutdown:
> > @@ -554,6 +563,7 @@ out:
> >       layout_cleanup();
> >       free(filename);
> >       free(layoutfile);
> > +     free(contentsfile);
> >       free(pparam);
> >       /* clean up global variables */
> >       free((char *)chip_to_probe); /* Silence! Freeing is not modifying
> > contents. */
> > diff --git a/flash.h b/flash.h
> > index dc5c140..2cf7ca6 100644
> > --- a/flash.h
> > +++ b/flash.h
> > @@ -284,7 +284,7 @@ void print_buildinfo(void);
> >  void print_banner(void);
> >  void list_programmers_linebreak(int startcol, int cols, int paren);
> >  int selfcheck(void);
> > -int doit(struct flashctx *flash, int force, const char *filename, int
> > read_it, int write_it, int erase_it, int verify_it);
> > +int doit(struct flashctx *flash, int force, const char *filename, const
> char
> > *contentsfile, int read_it, int write_it, int erase_it, int verify_it);
> >  int read_buf_from_file(unsigned char *buf, unsigned long size, const
> char
> > *filename);
> >  int write_buf_to_file(const unsigned char *buf, unsigned long size,
> const
> > char *filename);
> >
> > diff --git a/flashrom.c b/flashrom.c
> > index d5c3238..d11f89d 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1978,8 +1978,9 @@ int chip_safety_check(const struct flashctx
> *flash, int
> > force, int read_it, int
> >   * but right now it allows us to split off the CLI code.
> >   * Besides that, the function itself is a textbook example of abysmal
> code
> > flow.
> >   */
> > -int doit(struct flashctx *flash, int force, const char *filename, int
> > read_it,
> > -      int write_it, int erase_it, int verify_it)
> > +int doit(struct flashctx *flash, int force, const char *filename,
> > +      const char *contentsfile, int read_it, int write_it, int erase_it,
> > +      int verify_it)
> >  {
> >       uint8_t *oldcontents;
> >       uint8_t *newcontents;
> > @@ -2068,7 +2069,14 @@ int doit(struct flashctx *flash, int force, const
> char
> > *filename, int read_it,
> >        * preserved, but in that case we might perform unneeded erase
> which
> >        * takes time as well.
> >        */
> > -     if (read_all_first) {
> > +     if (contentsfile) {
> > +             msg_cinfo("Reading old flash chip contents from file... ");
> > +             if (read_buf_from_file(oldcontents, size, contentsfile)) {
> > +                     ret = 1;
> > +                     msg_cinfo("FAILED.\n");
> > +                     goto out;
> > +             }
> > +     } else if (read_all_first) {
> >               msg_cinfo("Reading old flash chip contents... ");
> >               if (flash->chip->read(flash, oldcontents, 0, size)) {
> >                       ret = 1;
>
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>
Paul Kocialkowski - 2016-04-23 17:30:04
Le mercredi 20 avril 2016 à 16:54 -0700, David Hendricks a écrit :
> Looks pretty good to me. FWIW, we implemented something nearly identical in
> the chromiumos branch a few years ago: https://chromium-review.googlesource.co
> m/#/c/10180/ (we called it "--diff"). It's been quite useful for development,
> especially when working with large chips and slow external programmers.

This has been my use case as well, in particular with the kb9012 embedded
controller flash. Nice to see that this use case is shared with others and that
we came up with a quite similar implementation independently!

> On Tue, Apr 19, 2016 at 12:25 PM, Paul Kocialkowski <contact@paulk.fr> 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@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 "
> > > @@ -332,6 +338,9 @@ int main(int argc, char *argv[])
> > >       if (layoutfile && check_filename(layoutfile, "layout")) {
> > >               cli_classic_abort_usage();
> > >       }
> > > +     if (contentsfile && check_filename(contentsfile, "contents")) {
> > > +             cli_classic_abort_usage();
> > > +     }
> > >  
> > >  #ifndef STANDALONE
> > >       if (logfile && check_filename(logfile, "log"))
> > > @@ -542,7 +551,7 @@ int main(int argc, char *argv[])
> > >        * Give the chip time to settle.
> > >        */
> > >       programmer_delay(100000);
> > > -     ret |= doit(fill_flash, force, filename, read_it, write_it,
> > erase_it,
> > > verify_it);
> > > +     ret |= doit(fill_flash, force, filename, contentsfile, read_it,
> > > write_it, erase_it, verify_it);
> > >  
> > >       unmap_flash(fill_flash);
> > >  out_shutdown:
> > > @@ -554,6 +563,7 @@ out:
> > >       layout_cleanup();
> > >       free(filename);
> > >       free(layoutfile);
> > > +     free(contentsfile);
> > >       free(pparam);
> > >       /* clean up global variables */
> > >       free((char *)chip_to_probe); /* Silence! Freeing is not modifying
> > > contents. */
> > > diff --git a/flash.h b/flash.h
> > > index dc5c140..2cf7ca6 100644
> > > --- a/flash.h
> > > +++ b/flash.h
> > > @@ -284,7 +284,7 @@ void print_buildinfo(void);
> > >  void print_banner(void);
> > >  void list_programmers_linebreak(int startcol, int cols, int paren);
> > >  int selfcheck(void);
> > > -int doit(struct flashctx *flash, int force, const char *filename, int
> > > read_it, int write_it, int erase_it, int verify_it);
> > > +int doit(struct flashctx *flash, int force, const char *filename, const
> > char
> > > *contentsfile, int read_it, int write_it, int erase_it, int verify_it);
> > >  int read_buf_from_file(unsigned char *buf, unsigned long size, const char
> > > *filename);
> > >  int write_buf_to_file(const unsigned char *buf, unsigned long size, const
> > > char *filename);
> > >  
> > > diff --git a/flashrom.c b/flashrom.c
> > > index d5c3238..d11f89d 100644
> > > --- a/flashrom.c
> > > +++ b/flashrom.c
> > > @@ -1978,8 +1978,9 @@ int chip_safety_check(const struct flashctx *flash,
> > int
> > > force, int read_it, int
> > >   * but right now it allows us to split off the CLI code.
> > >   * Besides that, the function itself is a textbook example of abysmal
> > code
> > > flow.
> > >   */
> > > -int doit(struct flashctx *flash, int force, const char *filename, int
> > > read_it,
> > > -      int write_it, int erase_it, int verify_it)
> > > +int doit(struct flashctx *flash, int force, const char *filename,
> > > +      const char *contentsfile, int read_it, int write_it, int erase_it,
> > > +      int verify_it)
> > >  {
> > >       uint8_t *oldcontents;
> > >       uint8_t *newcontents;
> > > @@ -2068,7 +2069,14 @@ int doit(struct flashctx *flash, int force, const
> > char
> > > *filename, int read_it,
> > >        * preserved, but in that case we might perform unneeded erase which
> > >        * takes time as well.
> > >        */
> > > -     if (read_all_first) {
> > > +     if (contentsfile) {
> > > +             msg_cinfo("Reading old flash chip contents from file... ");
> > > +             if (read_buf_from_file(oldcontents, size, contentsfile)) {
> > > +                     ret = 1;
> > > +                     msg_cinfo("FAILED.\n");
> > > +                     goto out;
> > > +             }
> > > +     } else if (read_all_first) {
> > >               msg_cinfo("Reading old flash chip contents... ");
> > >               if (flash->chip->read(flash, oldcontents, 0, size)) {
> > >                       ret = 1;
> > 
> > _______________________________________________
> > flashrom mailing list
> > flashrom@flashrom.org
> > https://www.flashrom.org/mailman/listinfo/flashrom
> 
> 
> 
> -- 
> David Hendricks (dhendrix)
> Systems Software Engineer, Google Inc.
Paul Kocialkowski - 2016-04-23 17:36:58
Le mardi 19 avril 2016 à 22:56 +0200, Michael Karcher a écrit :
> On 04/19/2016 10:18 PM, Carl-Daniel Hailfinger wrote:
> 
> > 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.
> I strongly prefer a standalone toplevel option, as the feature is
> programmer independent. Of course, this means that for the dummy
> programmer we now have -C and -p image=some.bin. I don't consider it a
> bad thing, but a good thing. An important use case for the dummy
> programmer is to test flashrom under various conditions. This might be
> flashrom getting "-C" with the contents of the chip, or flashrom getting
> no "-C" and reading first. Both execution flows should be testable using
> the dummy programmer.
> On the other hand, separate options allow mismatch of "-C" and the image
> file specified in "-p dummy:image=some.bin". Specifying wrong chip
> contents is an undesireable situation one really wants to avoid in
> practice, because it causes flashrom to do unpredictable things. In my
> oppinion, we should nevertheless support getting into this situation
> with the dummy programmer, as we are equally able to get into that
> situation with real programmers.
> Yet a way to avoid the mismatch seems a good thing to have, too. So I
> suggest to have "-C" load the "actual contents" of the chip
> independently from any programmer option or any programmer code,

I fully agree with your remarks, I'd stick with the toplevel approach too.

>  but make it possible for the dummy programmer to initialize the emulated
> flash chip from the loaded "actual contents". Something like -p
> dummy:autoimage comes to my mind, although I understand that loading the
> "-C" can well be performed after programmer initialization, but
> initializing the emulated dummy flash chip is done during programmer
> initialization.

Why not, but it seems that is would be just as easy to not specify "-C" in that
case. Or are you thinking purely about a test case here?

Cheers,

Patch

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 "
@@ -332,6 +338,9 @@  int main(int argc, char *argv[])
 	if (layoutfile && check_filename(layoutfile, "layout")) {
 		cli_classic_abort_usage();
 	}
+	if (contentsfile && check_filename(contentsfile, "contents")) {
+		cli_classic_abort_usage();
+	}
 
 #ifndef STANDALONE
 	if (logfile && check_filename(logfile, "log"))
@@ -542,7 +551,7 @@  int main(int argc, char *argv[])
 	 * Give the chip time to settle.
 	 */
 	programmer_delay(100000);
-	ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
+	ret |= doit(fill_flash, force, filename, contentsfile, read_it, write_it, erase_it, verify_it);
 
 	unmap_flash(fill_flash);
 out_shutdown:
@@ -554,6 +563,7 @@  out:
 	layout_cleanup();
 	free(filename);
 	free(layoutfile);
+	free(contentsfile);
 	free(pparam);
 	/* clean up global variables */
 	free((char *)chip_to_probe); /* Silence! Freeing is not modifying contents. */
diff --git a/flash.h b/flash.h
index dc5c140..2cf7ca6 100644
--- a/flash.h
+++ b/flash.h
@@ -284,7 +284,7 @@  void print_buildinfo(void);
 void print_banner(void);
 void list_programmers_linebreak(int startcol, int cols, int paren);
 int selfcheck(void);
-int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it);
+int doit(struct flashctx *flash, int force, const char *filename, const char *contentsfile, int read_it, int write_it, int erase_it, int verify_it);
 int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename);
 int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
 
diff --git a/flashrom.c b/flashrom.c
index d5c3238..d11f89d 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1978,8 +1978,9 @@  int chip_safety_check(const struct flashctx *flash, int force, int read_it, int
  * but right now it allows us to split off the CLI code.
  * Besides that, the function itself is a textbook example of abysmal code flow.
  */
-int doit(struct flashctx *flash, int force, const char *filename, int read_it,
-	 int write_it, int erase_it, int verify_it)
+int doit(struct flashctx *flash, int force, const char *filename,
+	 const char *contentsfile, int read_it, int write_it, int erase_it,
+	 int verify_it)
 {
 	uint8_t *oldcontents;
 	uint8_t *newcontents;
@@ -2068,7 +2069,14 @@  int doit(struct flashctx *flash, int force, const char *filename, int read_it,
 	 * preserved, but in that case we might perform unneeded erase which
 	 * takes time as well.
 	 */
-	if (read_all_first) {
+	if (contentsfile) {
+		msg_cinfo("Reading old flash chip contents from file... ");
+		if (read_buf_from_file(oldcontents, size, contentsfile)) {
+			ret = 1;
+			msg_cinfo("FAILED.\n");
+			goto out;
+		}
+	} else if (read_all_first) {
 		msg_cinfo("Reading old flash chip contents... ");
 		if (flash->chip->read(flash, oldcontents, 0, size)) {
 			ret = 1;