[flashrom] [PATCH] Add deferred -i processing

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Wed Aug 3 01:04:20 CEST 2011


On Tue, 2 Aug 2011 14:48:15 -0700
David Hendricks <dhendrix at google.com> wrote:

> The patch is mostly ack-able, save for a few nits.
> 
> On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
> stefan.tauner at student.tuwien.ac.at> wrote:
> 
> > My implementation does not defer the processing until doit(), but after the
> > argument parsing loop only (doit() should not contain argument checks).
> >
> 
> Just FYI -- The reason we deferred processing until doit() was because our
> usage case calls for reading the ROM content to find pre-existing mappings
> (e.g. fmap) which exist in the ROM image. It might be useful to eventually
> do the same thing for cbfs and Intel's flash descriptor.

jup i am aware of that. but i dont like to inflate doit() with even
more stuff, that does not belong there. we need to think about
refactoring the whole processing... we need something like pre-doit()
before doit() but after init and unlocking. a bit like the shutdown
process with the registration of some callbacks... i hope work on
libflashrom will enforce us to move forward with that.

> On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
> stefan.tauner at student.tuwien.ac.at> wrote:
> 
> > This allows to specify -i and -l parameters in any order.
> >
> > I don't like the output in error cases much, any ideas?
> > example of a good run:
> > flashrom -p dummy:emulate=SST25VF032B -i normal -w
> > ../testimages/4096kB.rand.img  -l test.layout -i gfxrom -i fallback
> > flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> > libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Using region(s): "normal", "gfxrom", "fallback".
> > Calibrating delay loop... OK.
> > Found SST flash chip "SST25VF032B" (4096 kB, SPI) on dummy.
> > Reading old flash chip contents... done.
> > Erasing and writing flash chip... Erase/write done.
> > Verifying flash... VERIFIED.
> >
> 
> Looks okay to me. And IIRC -V will list all ROM layout entries found even if
> they are not included, which is nice for debugging.

that was printed at default verbosity before, and is now spew-verbose
(which is now at -VVV level). but yes i wanted to keep something for
debugging.

> 
> On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
> stefan.tauner at student.tuwien.ac.at> wrote:
> 
> > example of a semi-good run:
> > flashrom -p dummy:emulate=SST25VF032B -i normal -w
> > ../testimages/4096kB.rand.img  -l test.layout -i gfxrom -i fallback
> > flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> > libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Maximum number of ROM images (4) in layout file reached before end of
> > layout file.
> > Ignoring the rest of the layout file.
> >
> 
> I think that if the user accidentally specifies an invalid layout or number
> of included regions, then Flashrom should quit. Otherwise Flashrom might not
> flash everything the user intends, leaving the content in an inconsistent
> (probably bricked) state. This has become problematic due to complicated ROM
> layouts in systems where multiple masters use a shared SPI flash (e.g. an EC
> or Intel's ME insanity).

yes, that's certainly a well argued proposal imo. carl-daniel?

> On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
> stefan.tauner at student.tuwien.ac.at> wrote:
> 
> > example of a faulty run:
> > flashrom -p dummy:emulate=SST25VF032B -i normal -w
> > ../testimages/4096kB.rand.img  -l test.layout -i gfxrom -i fallback -i bios
> > flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> > libpci 3.1.7, GCC 4.4.5, little endian
> > flashrom is free software, get the source code at http://www.flashrom.org
> >
> > Maximum number of ROM images (4) in layout file reached before end of
> > layout file.
> > Ignoring the rest of the layout file.
> > Using region(s): "normal", "gfxrom", "fallback"Invalid region specified:
> > "bios"
> >
> 
> If you split that last line so that the "Invalid region" part shows up on a
> newline, it looks okay.

that is one option yes. but i tend to think we may defer the output of
the used regions and push it into its own loop at the end of
process_include_args.
this was there would be no "using region..." output in case of errors
at all. and it may even be more readable(?)


> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> > ---
> >  cli_classic.c |   11 ++++-----
> >  flash.h       |    2 +
> >  layout.c      |   68
> > ++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 67 insertions(+), 14 deletions(-)
> >
> > diff --git a/cli_classic.c b/cli_classic.c
> > index 36fe9ad..e44ea86 100644
> > --- a/cli_classic.c
> > +++ b/cli_classic.c
> > @@ -233,14 +233,9 @@ int cli_classic(int argc, char *argv[])
> >                                cli_classic_abort_usage();
> >                        break;
> >                case 'i':
> > -                       /* FIXME: -l has to be specified before -i. */
> >                        tempstr = strdup(optarg);
> > -                       if (find_romentry(tempstr) < 0) {
> > -                               fprintf(stderr, "Error: image %s not found
> > in "
> > -                                       "layout file or -i specified before
> > "
> > -                                       "-l\n", tempstr);
> > +                       if (register_include_arg(tempstr) < 0)
> >                                cli_classic_abort_usage();
> > -                       }
> >                        break;
> >                case 'L':
> >                        if (++operation_specified > 1) {
> > @@ -325,6 +320,10 @@ int cli_classic(int argc, char *argv[])
> >                cli_classic_abort_usage();
> >        }
> >
> > +       if (process_include_args() < 0) {
> > +               cli_classic_abort_usage();
> > +       }
> > +
> >        /* FIXME: Print the actions flashrom will take. */
> >
> >        if (list_supported) {
> > diff --git a/flash.h b/flash.h
> > index 5b49e9d..0848255 100644
> > --- a/flash.h
> > +++ b/flash.h
> > @@ -251,6 +251,8 @@ int print(int type, const char *fmt, ...)
> > __attribute__((format(printf, 2, 3)));
> >  int cli_classic(int argc, char *argv[]);
> >
> >  /* layout.c */
> > +int register_include_arg(char *name);
> > +int process_include_args(void);
> >  int read_romlayout(char *name);
> >  int find_romentry(char *name);
> >  int handle_romentries(struct flashchip *flash, uint8_t *oldcontents,
> > uint8_t *newcontents);
> > diff --git a/layout.c b/layout.c
> > index d719a05..936e316 100644
> > --- a/layout.c
> > +++ b/layout.c
> > @@ -41,6 +41,12 @@ typedef struct {
> >        char name[256];
> >  } romlayout_t;
> >
> > +/* include_args lists arguments specified at the command line with -i.
> > They
> > + * must be processed at some point so that desired regions are marked as
> > + * "included" in the rom_entries list.
> > + */
> > +static char *include_args[MAX_ROMLAYOUT];
> > +static int num_include_args = 0; /* the number of valid entries. */
> >  static romlayout_t rom_entries[MAX_ROMLAYOUT];
> >
> >  #if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */
> > @@ -194,6 +200,20 @@ int read_romlayout(char *name)
> >  }
> >  #endif
> >
> > +/* register an include argument (-i) for later processing */
> > +int register_include_arg(char *name)
> > +{
> > +       if (num_include_args >= MAX_ROMLAYOUT) {
> > +               msg_gerr("Too many regions included (%i).\n",
> > num_include_args);
> > +               return -1;
> > +       }
> > +
> > +       include_args[num_include_args] = name;
> > +       num_include_args++;
> > +       return num_include_args;
> > +}
> > +
> > +/* returns the index of the entry (or a negative value if it is not found)
> > */
> >  int find_romentry(char *name)
> >  {
> >        int i;
> > @@ -201,20 +221,49 @@ int find_romentry(char *name)
> >        if (!romimages)
> >                return -1;
> >
> > -       msg_ginfo("Looking for \"%s\"... ", name);
> > -
> > +       msg_gspew("Looking for region \"%s\"... ", name);
> >        for (i = 0; i < romimages; i++) {
> >                if (!strcmp(rom_entries[i].name, name)) {
> >                        rom_entries[i].included = 1;
> > -                       msg_ginfo("found.\n");
> > +                       msg_gspew("found.\n");
> >                        return i;
> >                }
> >        }
> > -       msg_ginfo("not found.\n");      // Not found. Error.
> > -
> > +       msg_gspew("not found.\n");
> >        return -1;
> >  }
> >
> > +/* process -i arguments
> > + * returns 0 to indicate success, <0 to indicate failure
> >
> 
> I think the usual Flashrom convention is to return 1 to indicate failure. I
> used <0 out of habit. Let's go ahead and fix that coding convention for
> upstream if desired, and update the call sites to do error checking
> correctly.

well -1 is also used very often, but that's no argument to introduce
more inconsistencies :) i'll fix this, thanks.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list