Re: [flashrom] [PATCH] Replace --mainboard with -p internal:mainboard

[Carl-Daniel Hailfinger]

Am 03.01.2012 09:52 schrieb Stefan Tauner:
> On Tue, 03 Jan 2012 01:04:37 +0100
> Carl-Daniel Hailfinger  wrote:
> > Index: flashrom-kill_cli_mainboard_parameter/layout.c
> >
> > --- flashrom-kill_cli_mainboard_parameter/layout.c	(Revision 1482)
> > +++ flashrom-kill_cli_mainboard_parameter/layout.c	(Arbeitskopie)
> > […]
> > @@ -126,14 +126,17 @@
> >    		       "seem to fit to this machine - forcing it.\n");
> >    	} else {
> >    		msg_pinfo("ERROR: Your firmware image (%s:%s) does not "
> > [...]
> > 		  "appear to\n"
> > 		  "       be correct for the detected "
> > 		  "mainboard (%s:%s)\n\n"
> > 		  "Override with -p internal:boardmismatch="
> > 		  "force if you are absolutely sure that\n"
> > 		  "you are using a correct image for this "
> > 		  "mainboard or override\n"
> > 		  "the detected values with -p internal:"
> > 		  "mainboard=<vendor>:<mainboard>.\n\n",
> > 		  mainboard_vendor, mainboard_part, lb_vendor,
> > 		  lb_part);
>
> imho this dual use is wrong. if we would not have the override switch,
> it would be somewhat ok, but since we do, the mainboard= values should
> be only hints. what does the code do if boardmismatch=force AND
> mainboard=... is given? just from the message above, i can't tell (not
> that i should be able to, but it suggests an inconsistency.

Message rewritten.
> > --- flashrom-kill_cli_mainboard_parameter/print_wiki.c	(Revision 1482)
> > +++ flashrom-kill_cli_mainboard_parameter/print_wiki.c	(Arbeitskopie)
> > @@ -167,7 +167,7 @@
> >    	       boards[i].url ? boards[i].url : "",
> >    	       boards[i].name,
> >    	       boards[i].url ? "]" : "",
> >        b[k].lb_vendor ? "-m " : "&mdash;",
> >        b[k].lb_vendor ? "-p internal:mainboard= " : "&mdash;",
>
> the space at the end is wrong for sure.

Fixed.
> > --- flashrom-kill_cli_mainboard_parameter/print.c	(Revision 1482)
> > +++ flashrom-kill_cli_mainboard_parameter/print.c	(Arbeitskopie)
> > @@ -389,7 +389,10 @@
> >    for (i = strlen("Board"); i < maxboardlen; i++)
> >    	msg_ginfo(" ");
> > msg_ginfo("Status  Required option\n\n");
> > msg_ginfo("Status  Required option for\n");
>
> for me the option is "mainboard" and the specific mainboard strings are
> the parameter for this option. hence i would write "Required parameter
> for" here. The format looks ok on my machine.

Used "value"
> > Index: flashrom-kill_cli_mainboard_parameter/board_enable.c
> >
> > --- flashrom-kill_cli_mainboard_parameter/board_enable.c	(Revision 1482)
> > +++ flashrom-kill_cli_mainboard_parameter/board_enable.c	(Arbeitskopie)
> > @@ -2070,7 +2070,8 @@
> > The coreboot ids are used two fold. When running with a coreboot firmware,
> > the ids uniquely matches the coreboot board identification string. When a
> > legacy bios is installed and when autodetection is not possible, these ids
> > can be used to identify the board through the -m command line argument.
> > can be used to identify the board through the -p internal:mainboard=
> > programmer parameter.
>
> here it is the other way round from above... imo: "option" instead of
> "parameter" because the sentence references the "mainboard=" part,
> which to me is the option.
> no idea if there is a textbook definition for this stuff... or if
> anyone besides me cares :)
> > When a board is identified through its coreboot ids (in both cases), the
> > main pci ids are still required to match, as a safeguard.
> >
> > @@ -2245,7 +2246,8 @@
> >    		msg_pinfo("AMBIGUOUS BOARD NAME: %s\n", part);
> >    		msg_pinfo("At least vendors '%s' and '%s' match.\n",
> >    			  partmatch->lb_vendor, board->lb_vendor);
> > 	msg_perr("Please use the full -m vendor:part syntax.\n");
> > 	msg_perr("Please use the full -p internal:mainboard="
> > 		 "vendor:part syntax.\n");
> > return NULL;
> > }
> > partmatch = board;
> >
> > @@ -2259,7 +2261,8 @@
> >    	 * coreboot table. If it was, the coreboot implementor is
> >    	 * expected to fix flashrom, too.
> >    	 */
> > msg_perr("\nUnknown vendor:board from -m option: %s:%s\n\n",
> > msg_perr("\nUnknown vendor:board from -p internal:mainboard="
> > 	 " programmer parameter:\n%s:%s\n\n",
>
> ditto

Inconsistency is known, and I don't know if fixing it will help us in
the future since option/parameter are somewhat stretchable definitions.
> Acked-by: Stefan Tauner

Thanks for your review! New patch follows for reference.
Committed in r1483.

--mainboard is a relic from a time before external programmers and makes
the CLI inconsistent.
Use a programmer parameter instead and free up the short option -m.

NOTE:
The --list-supported-wiki output changed to use -p internal:mainboard=
instead of -m
The --list-supported output changed the heading of the mainboard list from

Vendor Board   Status  Required option
to
Vendor Board   Status  Required value for
                       -p internal:mainboard=

Fix lb_vendor_dev_from_string() not to write to the supplied string.

Signed-off-by: Carl-Daniel Hailfinger 
Acked-by: Stefan Tauner

Index: flashrom-kill_cli_mainboard_parameter/cli_classic.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/cli_classic.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/cli_classic.c	(Arbeitskopie)
@@ -106,7 +106,7 @@
             "-z|"
 #endif
             "-E|-r <file>|-w <file>|-v <file>]\n"
-	       "       [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n"
+	       "       [-c <chipname>] [-l <file>]\n"
           "       [-i <image>] [-p <programmername>[:<parameters>]]\n\n");

printf("Please note that the command line interface for flashrom has "
@@ -128,11 +128,6 @@
           "   -V | --verbose                    more verbose output\n"
           "   -c | --chip <chipname>            probe only for specified "
             "flash chip\n"
-#if CONFIG_INTERNAL == 1
-	       /* FIXME: --mainboard should be a programmer parameter */
-	       "   -m | --mainboard <[vendor:]part>  override mainboard "
-	         "detection\n"
-#endif
           "   -f | --force                      force specific operations "
             "(see man page)\n"
           "   -n | --noverify                   don't auto-verify\n"
@@ -190,7 +185,6 @@
    	{"verify",		1, NULL, 'v'},
    	{"noverify",		0, NULL, 'n'},
    	{"chip",		1, NULL, 'c'},
-		{"mainboard",		1, NULL, 'm'},
    	{"verbose",		0, NULL, 'V'},
    	{"force",		0, NULL, 'f'},
    	{"layout",		1, NULL, 'l'},
@@ -275,17 +269,6 @@
    		}
    		erase_it = 1;
    		break;
-		case 'm':
-#if CONFIG_INTERNAL == 1
-			tempstr = strdup(optarg);
-			lb_vendor_dev_from_string(tempstr);
-#else
-			fprintf(stderr, "Error: Internal programmer support "
-				"was not compiled in and --mainboard only\n"
-				"applies to the internal programmer. Aborting.\n");
-			cli_classic_abort_usage();
-#endif
-			break;
    	case 'f':
    		force = 1;
    		break;
@@ -426,14 +409,6 @@
    if (prog == PROGRAMMER_INVALID)
    	prog = default_programmer;

-#if CONFIG_INTERNAL == 1
-	if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
-		fprintf(stderr, "Error: --mainboard requires the internal "
-				"programmer. Aborting.\n");
-		cli_classic_abort_usage();
-	}
-#endif
-
    /* FIXME: Delay calibration should happen in programmer code. */
    myusec_calibrate_delay();

Index: flashrom-kill_cli_mainboard_parameter/internal.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/internal.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/internal.c	(Arbeitskopie)
@@ -213,6 +213,16 @@
    }
    free(arg);

+	arg = extract_programmer_param("mainboard");
+	if (arg && strlen(arg)) {
+		lb_vendor_dev_from_string(arg);
+	} else if (arg && !strlen(arg)) {
+		msg_perr("Missing argument for mainboard.\n");
+		free(arg);
+		return 1;
+	}
+	free(arg);
+
    get_io_perms();
    if (register_shutdown(internal_shutdown, NULL))
    	return 1;
Index: flashrom-kill_cli_mainboard_parameter/layout.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/layout.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/layout.c	(Arbeitskopie)
@@ -106,11 +106,11 @@

/*
     * If lb_vendor is not set, the coreboot table was
-	 * not found. Nor was -m VENDOR:PART specified.
+	 * not found. Nor was -p internal:mainboard=VENDOR:PART specified.
     */
    if (!lb_vendor || !lb_part) {
-		msg_pinfo("Note: If the following flash access fails, "
-		       "try -m <vendor>:<mainboard>.\n");
+		msg_pinfo("Note: If the following flash access fails, try "
+			  "-p internal:mainboard=<vendor>:<mainboard>.\n");
    	return 0;
    }

@@ -126,14 +126,17 @@
    		       "seem to fit to this machine - forcing it.\n");
    	} else {
    		msg_pinfo("ERROR: Your firmware image (%s:%s) does not "
-			       "appear to\n       be correct for the detected "
-			       "mainboard (%s:%s)\n\nOverride with -p internal:"
-			       "boardmismatch=force if you are absolutely sure "
-			       "that\nyou are using a correct "
-			       "image for this mainboard or override\nthe detected "
-			       "values with --mainboard <vendor>:<mainboard>.\n\n",
-			       mainboard_vendor, mainboard_part, lb_vendor,
-			       lb_part);
+				  "appear to\n"
+				  "       be correct for the detected "
+				  "mainboard (%s:%s)\n\n"
+				  "Override with -p internal:boardmismatch="
+				  "force to ignore the board name in the\n"
+				  "firmware image or override the detected "
+				  "mainboard with\n"
+				  "-p internal:mainboard=<vendor>:<mainboard>."
+				  "\n\n",
+				  mainboard_vendor, mainboard_part, lb_vendor,
+				  lb_part);
    		exit(1);
    	}
    }
Index: flashrom-kill_cli_mainboard_parameter/print_wiki.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/print_wiki.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/print_wiki.c	(Arbeitskopie)
@@ -167,7 +167,7 @@
    	       boards[i].url ? boards[i].url : "",
    	       boards[i].name,
    	       boards[i].url ? "]" : "",
-		       b[k].lb_vendor ? "-m " : "&mdash;",
+		       b[k].lb_vendor ? "-p internal:mainboard=" : "&mdash;",
    	       b[k].lb_vendor ? b[k].lb_vendor : "",
    	       b[k].lb_vendor ? ":" : "",
    	       b[k].lb_vendor ? b[k].lb_part : "",
Index: flashrom-kill_cli_mainboard_parameter/cbtable.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/cbtable.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/cbtable.c	(Arbeitskopie)
@@ -33,18 +33,27 @@
 char *lb_part = NULL, *lb_vendor = NULL;
 int partvendor_from_cbtable = 0;

-void lb_vendor_dev_from_string(char *boardstring)
+/* Parse the [<vendor>:]<board> string specified by the user as part of
+ * -p internal:mainboard=[<vendor>:]<board> and set lb_vendor and lb_part
+ * to the extracted values.
+ * Note: strtok modifies the original string, so we work on a copy and allocate
+ * memory for lb_vendor and lb_part with strdup.
+ */
+void lb_vendor_dev_from_string(const char *boardstring)
 {
+	/* strtok may modify the original string. */
+	char *tempstr = strdup(boardstring);
    char *tempstr2 = NULL;
-	strtok(boardstring, ":");
+	strtok(tempstr, ":");
    tempstr2 = strtok(NULL, ":");
    if (tempstr2) {
-		lb_vendor = boardstring;
-		lb_part = tempstr2;
+		lb_vendor = strdup(tempstr);
+		lb_part = strdup(tempstr2);
    } else {
    	lb_vendor = NULL;
-		lb_part = boardstring;
+		lb_part = strdup(tempstr);
    }
+	free(tempstr);
 }

static unsigned long compute_checksum(void *addr, unsigned long length)
Index: flashrom-kill_cli_mainboard_parameter/flashrom.8
===================================================================
--- flashrom-kill_cli_mainboard_parameter/flashrom.8	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/flashrom.8	(Arbeitskopie)
@@ -5,7 +5,7 @@
 .B flashrom \fR[\fB-n\fR] [\fB-V\fR] [\fB-f\fR] [\fB-h\fR|\fB-R\fR|\
 \fB-L\fR|\fB-z\fR|\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\
 \fB-v\fR <file>]
-         [\fB-c\fR <chipname>] [\fB-m\fR [<vendor>:]<board>] \
+         [\fB-c\fR <chipname>] \
 [\fB-l\fR <file>]
          [\fB-i\fR <image>] [\fB-p\fR <programmername>[:<parameters>]]
 .SH DESCRIPTION
@@ -88,19 +88,6 @@
 without the vendor name as parameter. Please note that the chip name is
 case sensitive.
 .TP
-.B "-m, --mainboard" [<vendor>:]<board>
-Override mainboard settings.
-.sp
-flashrom reads the coreboot table to determine the current mainboard. If no
-coreboot table could be read or if you want to override these values, you can
-specify -m, e.g.:
-.sp
-.B "  flashrom --mainboard AGAMI:ARUMA -w agami_aruma.rom"
-.sp
-See the 'Known boards' or 'Known laptops' section in the output
-of 'flashrom -L' for a list of boards which require the specification of
-the board name, if no coreboot table is found.
-.TP
 .B "-f, --force"
 Force one or more of the following actions:
 .sp
@@ -245,11 +232,16 @@
 running coreboot, the mainboard type is determined from the coreboot table.
 Otherwise, the mainboard is detected by examining the onboard PCI devices
 and possibly DMI info. If PCI and DMI do not contain information to uniquely
-identify the mainboard (which is the exception), it might be necessary to
-specify the mainboard using the
-.B -m
-switch (see above).
+identify the mainboard (which is the exception), or if you want to override
+the detected mainboard model, you can specify the mainboard using the
 .sp
+.B "  flashrom -p internal:mainboard=[<vendor>:]<board>"
+syntax.
+.sp
+See the 'Known boards' or 'Known laptops' section in the output
+of 'flashrom -L' for a list of boards which require the specification of
+the board name, if no coreboot table is found.
+.sp
 Some of these board-specific flash enabling functions (called
 .BR "board enables" )
 in flashrom have not yet been tested. If your mainboard is detected needing
Index: flashrom-kill_cli_mainboard_parameter/programmer.h
===================================================================
--- flashrom-kill_cli_mainboard_parameter/programmer.h	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/programmer.h	(Arbeitskopie)
@@ -264,7 +264,7 @@
 void cleanup_cpu_msr(void);

/* cbtable.c */
-void lb_vendor_dev_from_string(char *boardstring);
+void lb_vendor_dev_from_string(const char *boardstring);
 int coreboot_init(void);
 extern char *lb_part, *lb_vendor;
 extern int partvendor_from_cbtable;
Index: flashrom-kill_cli_mainboard_parameter/print.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/print.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/print.c	(Arbeitskopie)
@@ -389,7 +389,10 @@
    for (i = strlen("Board"); i < maxboardlen; i++)
    	msg_ginfo(" ");

-	msg_ginfo("Status  Required option\n\n");
+	msg_ginfo("Status  Required value for\n");
+	for (i = 0; i < maxvendorlen + maxboardlen + strlen("Status  "); i++)
+		msg_ginfo(" ");
+	msg_ginfo("-p internal:mainboard=\n");

for (b = boards; b->vendor != NULL; b++) {
    	msg_ginfo("%s", b->vendor);
@@ -407,7 +410,7 @@
    		if (e->lb_vendor == NULL)
    			msg_ginfo("(autodetected)");
    		else
-				msg_ginfo("-m %s:%s", e->lb_vendor,
+				msg_ginfo("%s:%s", e->lb_vendor,
    					   e->lb_part);
    	}
    	msg_ginfo("\n");
Index: flashrom-kill_cli_mainboard_parameter/board_enable.c
===================================================================
--- flashrom-kill_cli_mainboard_parameter/board_enable.c	(Revision 1482)
+++ flashrom-kill_cli_mainboard_parameter/board_enable.c	(Arbeitskopie)
@@ -2070,7 +2070,8 @@
  * The coreboot ids are used two fold. When running with a coreboot firmware,
  * the ids uniquely matches the coreboot board identification string. When a
  * legacy bios is installed and when autodetection is not possible, these ids
- * can be used to identify the board through the -m command line argument.
+ * can be used to identify the board through the -p internal:mainboard=
+ * programmer parameter.
  *
  * When a board is identified through its coreboot ids (in both cases), the
  * main pci ids are still required to match, as a safeguard.
@@ -2245,7 +2246,8 @@
    		msg_pinfo("AMBIGUOUS BOARD NAME: %s\n", part);
    		msg_pinfo("At least vendors '%s' and '%s' match.\n",
    			  partmatch->lb_vendor, board->lb_vendor);
-			msg_perr("Please use the full -m vendor:part syntax.\n");
+			msg_perr("Please use the full -p internal:mainboard="
+				 "vendor:part syntax.\n");
    		return NULL;
    	}
    	partmatch = board;
@@ -2259,7 +2261,8 @@
    	 * coreboot table. If it was, the coreboot implementor is
    	 * expected to fix flashrom, too.
    	 */
-		msg_perr("\nUnknown vendor:board from -m option: %s:%s\n\n",
+		msg_perr("\nUnknown vendor:board from -p internal:mainboard="
+			 " programmer parameter:\n%s:%s\n\n",
    		 vendor, part);
    }
    return NULL;