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

David Hendricks dhendrix at google.com
Thu Apr 21 01:54:34 CEST 2016


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 at 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 at paulk.fr>
> > ---
> >  cli_classic.c | 46 ++++++++++++++++++++++++++++------------------
> >  flash.h       |  2 +-
> >  flashrom.c    | 14 +++++++++++---
> >  3 files changed, 40 insertions(+), 22 deletions(-)
> >
> > diff --git a/cli_classic.c b/cli_classic.c
> > index a2c2014..dc0164d 100644
> > --- a/cli_classic.c
> > +++ b/cli_classic.c
> > @@ -44,24 +44,25 @@ static void cli_classic_usage(const char *name)
> >              "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i
> <imagename>]...]
> > [-n] [-f]]\n"
> >              "[-V[V[V]]] [-o <logfile>]\n\n", name);
> >
> > -     printf(" -h | --help                        print this help text\n"
> > -            " -R | --version                     print version
> > (release)\n"
> > -            " -r | --read <file>                 read flash and save to
> > <file>\n"
> > -            " -w | --write <file>                write <file> to
> flash\n"
> > -            " -v | --verify <file>               verify flash against
> > <file>\n"
> > -            " -E | --erase                       erase flash memory\n"
> > -            " -V | --verbose                     more verbose output\n"
> > -            " -c | --chip <chipname>             probe only for
> specified
> > flash chip\n"
> > -            " -f | --force                       force specific
> operations
> > (see man page)\n"
> > -            " -n | --noverify                    don't auto-verify\n"
> > -            " -l | --layout <layoutfile>         read ROM layout from
> > <layoutfile>\n"
> > -            " -i | --image <name>                only flash image <name>
> > from flash layout\n"
> > -            " -o | --output <logfile>            log output to
> > <logfile>\n"
> > -            " -L | --list-supported              print supported
> > devices\n"
> > +     printf(" -h | --help                          print this help
> text\n"
> > +            " -R | --version                       print version
> > (release)\n"
> > +            " -r | --read <file>                   read flash and save
> to
> > <file>\n"
> > +            " -w | --write <file>                  write <file> to
> > flash\n"
> > +            " -v | --verify <file>                 verify flash against
> > <file>\n"
> > +            " -E | --erase                         erase flash memory\n"
> > +            " -V | --verbose                       more verbose
> output\n"
> > +            " -c | --chip <chipname>               probe only for
> > specified flash chip\n"
> > +            " -f | --force                         force specific
> > operations (see man page)\n"
> > +            " -n | --noverify                      don't auto-verify\n"
> > +            " -l | --layout <layoutfile>           read ROM layout from
> > <layoutfile>\n"
> > +            " -i | --image <name>                  only flash image
> <name>
> > from flash layout\n"
> > +            " -o | --output <logfile>              log output to
> > <logfile>\n"
> > +            " -C | --flash-contents <contentsfile> assume flash contents
> > to be <contentsfile>\n"
> > +            " -L | --list-supported                print supported
> > devices\n"
> >  #if CONFIG_PRINT_WIKI == 1
> > -            " -z | --list-supported-wiki         print supported devices
> > in wiki syntax\n"
> > +            " -z | --list-supported-wiki           print supported
> devices
> > in wiki syntax\n"
> >  #endif
> > -            " -p | --programmer <name>[:<param>] specify the programmer
> > device. One of\n");
> > +            " -p | --programmer <name>[:<param>]   specify the
> programmer
> > device. One of\n");
> >       list_programmers_linebreak(4, 80, 0);
> >       printf(".\n\nYou can specify one of -h, -R, -L, "
> >  #if CONFIG_PRINT_WIKI == 1
> > @@ -106,7 +107,7 @@ int main(int argc, char *argv[])
> >       enum programmer prog = PROGRAMMER_INVALID;
> >       int ret = 0;
> >
> > -     static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:";
> > +     static const char optstring[] = "r:Rw:v:nVEfc:l:i:C:p:Lzho:";
> >       static const struct option long_options[] = {
> >               {"read",                1, NULL, 'r'},
> >               {"write",               1, NULL, 'w'},
> > @@ -118,6 +119,7 @@ int main(int argc, char *argv[])
> >               {"force",               0, NULL, 'f'},
> >               {"layout",              1, NULL, 'l'},
> >               {"image",               1, NULL, 'i'},
> > +             {"flash-contents",      1, NULL, 'C'},
> >               {"list-supported",      0, NULL, 'L'},
> >               {"list-supported-wiki", 0, NULL, 'z'},
> >               {"programmer",          1, NULL, 'p'},
> > @@ -128,6 +130,7 @@ int main(int argc, char *argv[])
> >       };
> >
> >       char *filename = NULL;
> > +     char *contentsfile = NULL;
> >       char *layoutfile = NULL;
> >  #ifndef STANDALONE
> >       char *logfile = NULL;
> > @@ -221,6 +224,9 @@ int main(int argc, char *argv[])
> >                               cli_classic_abort_usage();
> >                       }
> >                       break;
> > +             case 'C':
> > +                     contentsfile = strdup(optarg);
> > +                     break;
> >               case 'L':
> >                       if (++operation_specified > 1) {
> >                               fprintf(stderr, "More than one operation "
> > @@ -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 at flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>



-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20160420/0140adfb/attachment.html>


More information about the flashrom mailing list