--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.
A nice side effect is that there is one less corner case we have to care about in the logging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
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) @@ -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; 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/cbtable.c =================================================================== --- flashrom-kill_cli_mainboard_parameter/cbtable.c (Revision 1482) +++ flashrom-kill_cli_mainboard_parameter/cbtable.c (Arbeitskopie) @@ -33,17 +33,19 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +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_vendor = tempstr; lb_part = tempstr2; } else { lb_vendor = NULL; - lb_part = boardstring; + lb_part = tempstr; } }
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) @@ -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;
On Mon, 02 Jan 2012 03:48:40 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
--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.
A nice side effect is that there is one less corner case we have to care about in the logging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
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) @@ -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'},@@ -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;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;
the following hunk should get its own note in the changelog if it is merged imho.
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,17 +33,19 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +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_vendor = tempstr;
lb_part = boardstring;
lb_part = tempstr;
hm... we want something like free(tempstr); due to the strdup call but since we change the pointer in the process we do no longer know where the string started. and of course we would still want the extracted tokens to remain valid, so that would require another strdup... oh my.
hm but if the boardstring contains the wanted data only, the current version might be good enough. if we feed it with the return value of extract_programmer_param only (as we do atm) this is somewhat ok. not very obvious but at least not too leaky :)
and if (tempstr == NULL) is not handled (because empty arguments are checked for in the caller?). OTOH it is not really necessary because both strings are set to NULL then, but it is also not obvious that this is wanted.
please add a function comment explaining that stuff.
}
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) @@ -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;
what about this in cli_classic.c (seems to be completely useless now): #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
and this in board_enable.c: msg_perr("Please use the full -m vendor:part syntax.\n"); and msg_perr("\nUnknown vendor:board from -m option: %s:%s\n\n", vendor, part);
and this in print.c: msg_ginfo("-m %s:%s", e->lb_vendor, e->lb_part); and this in flashrom.8: [\fB-c\fR <chipname>] [\fB-m\fR [<vendor>:]<board>] \
aaaand... please use grep -- -m yourself, there are other bits referring to the old syntax.
the whole cb string handling is a bit messy. in internal_init we call extract_programmer_param("mainboard") to get the user input. then call lb_vendor_dev_from_string on the result, which internally sets a global variable. later in the same internal_init function we call board_flash_enable with the global variables as arguments.
the only other user is show_id in layout.c if we work around that and make lb_vendor_dev_from_string return the two strings, we could make lb_part and lb_vendor static.
id suggest splitting the patch and fix the lb_vendor_dev_from_string hunk together with the other cb table stuff. the -m removals are hard enough apparently to justify a patch on its own ;)
Am 02.01.2012 11:29 schrieb Stefan Tauner:
On Mon, 02 Jan 2012 03:48:40 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
--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.
A nice side effect is that there is one less corner case we have to care about in the logging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
the following hunk should get its own note in the changelog if it is merged imho.
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,17 +33,19 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +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_vendor = tempstr;
lb_part = boardstring;
lb_part = tempstr;hm... we want something like free(tempstr); due to the strdup call but since we change the pointer in the process we do no longer know where the string started. and of course we would still want the extracted tokens to remain valid, so that would require another strdup... oh my.
hm but if the boardstring contains the wanted data only, the current version might be good enough. if we feed it with the return value of extract_programmer_param only (as we do atm) this is somewhat ok. not very obvious but at least not too leaky :)
and if (tempstr == NULL) is not handled (because empty arguments are checked for in the caller?). OTOH it is not really necessary because both strings are set to NULL then, but it is also not obvious that this is wanted.
I tried to get the code right this time, but now it leaks lb_part and lb_vendor allocations (consistent with other places setting lb_part and lb_vendor) and we'd have to free them in some internal_shutdown handler. That said, such one-time leaks are all over the place and should be fixed once programmer_init()/programmer_shutdown() can be called multiple times pairwise in a row.
please add a function comment explaining that stuff.
Sure.
aaaand... please use grep -- -m yourself, there are other bits referring to the old syntax.
Sorry, fixed this time.
the whole cb string handling is a bit messy.
Understatement of the week.
in internal_init we call extract_programmer_param("mainboard") to get the user input. then call lb_vendor_dev_from_string on the result, which internally sets a global variable. later in the same internal_init function we call board_flash_enable with the global variables as arguments.
the only other user is show_id in layout.c
show_id() even has a comment which says it should be moved to cbtable.c.
if we work around that and make lb_vendor_dev_from_string return the two strings, we could make lb_part and lb_vendor static.
id suggest splitting the patch and fix the lb_vendor_dev_from_string hunk together with the other cb table stuff. the -m removals are hard enough apparently to justify a patch on its own ;)
The code calling lb_vendor_dev_from_string() changed too much and I don't really trust myself to remember fixing the bug if I don't do it now. Hopefully it is OK this time.
New patch.
--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 option for -p internal:mainboard=
I welcome comments on the --list-supported output change.
Fix lb_vendor_dev_from_string() not to write to the supplied string.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
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 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); 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 " : "—", + b[k].lb_vendor ? "-p internal:mainboard= " : "—", 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,21 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +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 option 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;
Do leaks really matter in this kind of program? Sure, you're going to leak; how much? Sometimes with this type of code you let it leak, and it ends up simpler otherwise. How much leakage are we talking about here?
I only ask because I was talking to someone a year ago who did not realize that the OS reclaimed all the storage a program leaked when the program exited :-) The poor guy was getting tangled up in knots over a 4k leak which in the long run really was of not importance ...
ron
On Tue, 03 Jan 2012 01:04:37 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 02.01.2012 11:29 schrieb Stefan Tauner:
On Mon, 02 Jan 2012 03:48:40 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
--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.
A nice side effect is that there is one less corner case we have to care about in the logging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
the following hunk should get its own note in the changelog if it is merged imho.
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,17 +33,19 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +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_vendor = tempstr;
lb_part = boardstring;
lb_part = tempstr;hm... we want something like free(tempstr); due to the strdup call but since we change the pointer in the process we do no longer know where the string started. and of course we would still want the extracted tokens to remain valid, so that would require another strdup... oh my.
hm but if the boardstring contains the wanted data only, the current version might be good enough. if we feed it with the return value of extract_programmer_param only (as we do atm) this is somewhat ok. not very obvious but at least not too leaky :)
and if (tempstr == NULL) is not handled (because empty arguments are checked for in the caller?). OTOH it is not really necessary because both strings are set to NULL then, but it is also not obvious that this is wanted.
I tried to get the code right this time, but now it leaks lb_part and lb_vendor allocations (consistent with other places setting lb_part and lb_vendor) and we'd have to free them in some internal_shutdown handler.
well that's not the only problem imo. you can't free tempstr like that, because it gets modified by strtok. i am not 100% sure what your version would do actually. the address tempstr points to, may be one that was not (the start of) an allocated area. what does calling free on that do according to the standard?
please look further below for my version which saves the original address of tempstr in a new variable and frees that one at the end.
That said, such one-time leaks are all over the place and should be fixed once programmer_init()/programmer_shutdown() can be called multiple times pairwise in a row.
please add a function comment explaining that stuff.
Sure.
missing in this iteration
the whole cb string handling is a bit messy.
Understatement of the week.
oh well, i rant so much so i have to be comforting and polite at least SOMETIMES :)
in internal_init we call extract_programmer_param("mainboard") to get the user input. then call lb_vendor_dev_from_string on the result, which internally sets a global variable. later in the same internal_init function we call board_flash_enable with the global variables as arguments.
the only other user is show_id in layout.c
show_id() even has a comment which says it should be moved to cbtable.c.
if we work around that and make lb_vendor_dev_from_string return the two strings, we could make lb_part and lb_vendor static.
id suggest splitting the patch and fix the lb_vendor_dev_from_string hunk together with the other cb table stuff. the -m removals are hard enough apparently to justify a patch on its own ;)
The code calling lb_vendor_dev_from_string() changed too much and I don't really trust myself to remember fixing the bug if I don't do it now. Hopefully it is OK this time.
New patch.
--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 option for -p internal:mainboard=
I welcome comments on the --list-supported output change.
Fix lb_vendor_dev_from_string() not to write to the supplied string.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
[…]
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\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 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.
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 " : "—",
b[k].lb_vendor ? "-p internal:mainboard= " : "—",
the space at the end is wrong for sure. i have looked at the output and it of course makes the column larger, a bit larger than the model name column (which is quite huge :) i think it is reasonable though. the whole table is split into two main columns so there is plenty of screen space before this patch, and still some after (i.e. there are wider tables in the output).
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,21 @@ char *lb_part = NULL, *lb_vendor = NULL; int partvendor_from_cbtable = 0;
-void lb_vendor_dev_from_string(char *boardstring) +void lb_vendor_dev_from_string(const char *boardstring) {
- /* strtok may modify the original string. */
- char *tempstr = strdup(boardstring);
+ char *freestr = tempstr;
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);
lb_part = boardstring;
lb_part = strdup(tempstr);- free(tempstr);
instead: + free(freestr);
}
static unsigned long compute_checksum(void *addr, unsigned long length) […] 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 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.
- for (i = 0; i < maxvendorlen + maxboardlen + strlen("Status "); i++)
msg_ginfo(" ");- msg_ginfo("-p internal:mainboard=\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.
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;@@ -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
apart from the free call in cbtable.c (+ the missing comment) and the additional space in the wiki output this is Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at NB: i have not verified row length limitations in code or output, or the resulting manpage layout. also, i have not rechecked for forgotten -m or --mainboard references this time. :)
On Tue, 3 Jan 2012 09:52:11 +0100 Stefan Tauner stefan.tauner@student.tuwien.ac.at wrote:
I tried to get the code right this time, but now it leaks lb_part and lb_vendor allocations (consistent with other places setting lb_part and lb_vendor) and we'd have to free them in some internal_shutdown handler.
well that's not the only problem imo. you can't free tempstr like that, because it gets modified by strtok. i am not 100% sure what your version would do actually. the address tempstr points to, may be one that was not (the start of) an allocated area. what does calling free on that do according to the standard?
uh... too early. strike all of that. of course the reference itself cant be changed out of the function itself just the referenced data/string is changed. so it points to the same address all of the time and your version is fine actually.
please look further below ...
no, please dont :)
Am 03.01.2012 09:52 schrieb Stefan Tauner:
On Tue, 03 Jan 2012 01:04:37 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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 " : "—",
b[k].lb_vendor ? "-p internal:mainboard= " : "—",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;@@ -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 stefan.tauner@student.tuwien.ac.at
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 c-d.hailfinger.devel.2006@gmx.net Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
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 " : "—", + b[k].lb_vendor ? "-p internal:mainboard=" : "—", 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;
-
Carl-Daniel Hailfinger -
ron minnich -
Stefan Tauner