[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