Re: [flashrom] [PATCH] Add deferred -i processing

[David Hendricks]

The patch is mostly ack-able, save for a few nits.

On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
[e-mail]> 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.

On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
[e-mail]> 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.

On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
[e-mail]> 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).

We can apply this fix in a follow-up patch if you'd prefer:

Index: layout.c
===================================================================
--- layout.c    (revision 1402)
+++ layout.c    (working copy)
@@ -159,7 +159,7 @@
                                 "file reached before end of layout
file.\n",
                                 MAX_ROMLAYOUT);
                        msg_gerr("Ignoring the rest of the layout file.\n");
-                       break;
+                       return 1;
                }
                if (2 != fscanf(romlayout, "%s %s\n", tempstr,
rom_entries[romimages].name))
                        continue;

On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
[e-mail]> 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.

Please run "flashrom --help" for usage info.
> Signed-off-by: Stefan Tauner
>
> 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.
> */
>
> +int process_include_args(void)
> +{
>   int i;
>   unsigned int found = 0;
>   for (i = 0; i < num_include_args && include_args[i] != NULL; i++) {
>           /* User has specified an area, but no layout file is
>
> loaded. */
>           if (!romimages) {
>                   msg_gerr("Region requested (with -i \"%s\"), "
>                            "but no layout data is available.\n",
>                            include_args[i]);
>                   return -1;

return 1

+               }
>           if (find_romentry(include_args[i]) < 0) {
>                   msg_gerr("Invalid region specified: \"%s\"\n",
>                            include_args[i]);
>                   return -1;

return 1
>           }
>           if (found == 0)
>                   msg_ginfo("Using region(s): \"%s\"",
>
> include_args[i]);
>           else
>                   msg_ginfo(", \"%s\"", include_args[i]);
>           found++;
>   }
>   msg_ginfo(".\n");
>   return 0;
>
> +}
>
> int find_next_included_romentry(unsigned int start)
>  {
>        int i;
> @@ -246,11 +295,12 @@ int handle_romentries(struct flashchip *flash,
> uint8_t *oldcontents, uint8_t *ne
>        int entry;
>        unsigned int size = flash->total_size * 1024;
>   /* If no layout file was specified or the layout file was empty,
>
> assume
>    * that the user wants to flash the complete new image.
>   /* If no regions were specified for inclusion, assume
>    * that the user wants to write the complete new image.
>   */
>   if (!romimages)
>   if (num_include_args == 0)
>          return 0;
>  /* Non-included romentries are ignored.
>   * The union of all included romentries is used from the new image.
>   */
>
> @@ -262,6 +312,8 @@ int handle_romentries(struct flashchip *flash, uint8_t
> *oldcontents, uint8_t *ne
>                               size - start);
>                        break;
>                }
>           /* For non-included region, copy from old content. */
>          if (rom_entries[entry].start > start)
>                  memcpy(newcontents + start, oldcontents + start,
>                         rom_entries[entry].start - start);