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

[Carl-Daniel Hailfinger]

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
> >
> > 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