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

[Paul Kocialkowski]

Here is an example script that I'm using to read the flash contents to a file
once and update it as I flash newer versions of the software:

#!/bin/sh

flashrom="flashrom"
programmer="ft2232_spi:type=openmoko,divisor=8"

contents="/home/paulk/embedded-freedom/Devices/G505s/kb9012/flashrom/flash-contents.img"
image="$1"

if [ -z $image ]
then
    echo "Reading initial flash contents"

timeb=$( date +%s )
    $flashrom -p $programmer -r $contents
    timea=$( date +%s )

echo "Time: "$(( $timea - $timeb ))" seconds"
else
    echo "Writing new flash contents"

timeb=$( date +%s )
    $flashrom -p $programmer -w $image -C $contents --noverify
    rc=$?
    timea=$( date +%s )

if [ "$rc" -eq "0" ]
    then
    	echo "Copying new flash contents"
    	cp $image $contents
    fi

echo "Time: "$(( $timea - $timeb ))" seconds"
fi

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

[Paul Kocialkowski]

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 "
@@ -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;

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

[Paul Kocialkowski]

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 "
> @@ -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;

[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

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

[Michael Karcher]

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

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

[Paul Kocialkowski]

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 1/1] Add support for reading the current flash contents from a file

[David Hendricks]

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 
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 "
> > @@ -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
> [e-mail]
> https://www.flashrom.org/mailman/listinfo/flashrom