Am 04.09.2011 13:03 schrieb Stefan Tauner:
On Sun, 04 Sep 2011 02:33:38 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote
Change programmer selection in cli and generic code
Bugfix: Do not accept multiple conflicting --programmer selections. Restriction: Do not accept multiple --programmer selections even if there is no conflict. Do not rely on exported programmer variable anymore. programmer_init requires the programmer as first parameter.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-programmer_selection_fix/cli_classic.c
--- flashrom-programmer_selection_fix/cli_classic.c (Revision 1427) +++ flashrom-programmer_selection_fix/cli_classic.c (Arbeitskopie) @@ -258,8 +259,16 @@ #endif break; case 'p':
for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {name = programmer_table[programmer].name;
if (prog != PROGRAMMER_INVALID) {fprintf(stderr, "Error: --programmer specified ""more than once. You can specify ""multiple progammer parameters with ""\",\". Please see the man page for "
- You can specify multiple progammer parameters with ",".
- You can specify multiple parameters for a progammer separated with ",".
or similar... the above one sounds like multiple programmers are supported somehow imo.
Yes, I saw that problem shortly after submission, but I couldn't see a way to phrase it in a better way. Your suggestion looks good.
"details.\n");cli_classic_abort_usage();}for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {name = programmer_table[prog].name; namelen = strlen(name); if (strncmp(optarg, name, namelen) == 0) { switch (optarg[namelen]) {Index: flashrom-programmer_selection_fix/flashrom.c
--- flashrom-programmer_selection_fix/flashrom.c (Revision 1427) +++ flashrom-programmer_selection_fix/flashrom.c (Arbeitskopie) @@ -42,10 +42,12 @@ char *chip_to_probe = NULL; int verbose = 0;
+enum programmer programmer = PROGRAMMER_INVALID;
everything below (i.e. the declaration and definition of default_programmer) should be moved to the cli file(s). selecting the default programmer is in the scope of the library user. and it slashes away one global.
Right.
this may make chromiumos cry a bit though.
They duplicate the CLI anyway, so I don't see this as a big problem for them.
if it is moved, these ifdefs could go inline into main()... i do not suggest this, just mentioning. a static variable is probably better.
I prefer static.
#if CONFIG_INTERNAL == 1 -enum programmer programmer = PROGRAMMER_INTERNAL; +enum programmer default_programmer = PROGRAMMER_INTERNAL; #elif CONFIG_DUMMY == 1 -enum programmer programmer = PROGRAMMER_DUMMY; +enum programmer default_programmer = PROGRAMMER_DUMMY; #else /* If neither internal nor dummy are selected, we must pick a sensible default.
- Since there is no reason to prefer a particular external programmer, we fail
@@ -55,7 +57,7 @@ #if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1
way too much convenience for a (non-existent?) theoretical package maintainer at the cost of really existing library maintainers.
Really? I use it constantly during development for compile/run tests and I'm not a package maintainer. It also serves as sanity check once a new programmer is added.
i would remove the monster if and check for a compiler flag DEFAULT_PROGRAMMER or so first. if that's not defined, look for internal and dummy as fall-back then and just bail out with an error similar to the one below (but with mentioning DEFAULT_PROGRAMMER).
That would make test compiles/runs with various programmers enabled/disabled a real nightmare for me. And yes, I test all compilation/run of all 2048 possible CONFIG_* combinations from time to time. After all, I don't want to ship a potentially broken flashrom.
this would also have benefits for the packager: he could select a default programmer no matter what other programmers are enabled.
Hm. Not so sure about those claimed benefits...
#error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one. #endif -enum programmer programmer = +enum programmer default_programmer = #if CONFIG_NIC3COM == 1 PROGRAMMER_NIC3COM #endif
New patch.
Change programmer selection in cli and generic code
Bugfix: Do not accept multiple conflicting --programmer selections. Restriction: Do not accept multiple --programmer selections even if there is no conflict. Unexport the programmer variable. programmer_init requires the programmer as first parameter. The default programmer selection is now part of cli_classic.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-programmer_selection_fix/it87spi.c =================================================================== --- flashrom-programmer_selection_fix/it87spi.c (Revision 1427) +++ flashrom-programmer_selection_fix/it87spi.c (Arbeitskopie) @@ -129,10 +129,8 @@ enter_conf_mode_ite(port); /* NOLDN, reg 0x24, mask out lowest bit (suspend) */ tmp = sio_read(port, 0x24) & 0xFE; - /* If IT87SPI was not explicitly selected, we want to check - * quickly if LPC->SPI translation is active. - */ - if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) { + /* Check if LPC->SPI translation is active. */ + if (!(tmp & 0x0e)) { msg_pdbg("No IT87* serial flash segment enabled.\n"); exit_conf_mode_ite(port); /* Nothing to do. */ Index: flashrom-programmer_selection_fix/cli_classic.c =================================================================== --- flashrom-programmer_selection_fix/cli_classic.c (Revision 1427) +++ flashrom-programmer_selection_fix/cli_classic.c (Arbeitskopie) @@ -31,6 +31,74 @@ #include "flashchips.h" #include "programmer.h"
+#if CONFIG_INTERNAL == 1 +static enum programmer default_programmer = PROGRAMMER_INTERNAL; +#elif CONFIG_DUMMY == 1 +static enum programmer default_programmer = PROGRAMMER_DUMMY; +#else +/* If neither internal nor dummy are selected, we must pick a sensible default. + * Since there is no reason to prefer a particular external programmer, we fail + * if more than one of them is selected. If only one is selected, it is clear + * that the user wants that one to become the default. + */ +#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1 +#error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one. +#endif +static enum programmer default_programmer = +#if CONFIG_NIC3COM == 1 + PROGRAMMER_NIC3COM +#endif +#if CONFIG_NICREALTEK == 1 + PROGRAMMER_NICREALTEK +#endif +#if CONFIG_NICNATSEMI == 1 + PROGRAMMER_NICNATSEMI +#endif +#if CONFIG_GFXNVIDIA == 1 + PROGRAMMER_GFXNVIDIA +#endif +#if CONFIG_DRKAISER == 1 + PROGRAMMER_DRKAISER +#endif +#if CONFIG_SATASII == 1 + PROGRAMMER_SATASII +#endif +#if CONFIG_ATAHPT == 1 + PROGRAMMER_ATAHPT +#endif +#if CONFIG_FT2232_SPI == 1 + PROGRAMMER_FT2232_SPI +#endif +#if CONFIG_SERPROG == 1 + PROGRAMMER_SERPROG +#endif +#if CONFIG_BUSPIRATE_SPI == 1 + PROGRAMMER_BUSPIRATE_SPI +#endif +#if CONFIG_DEDIPROG == 1 + PROGRAMMER_DEDIPROG +#endif +#if CONFIG_RAYER_SPI == 1 + PROGRAMMER_RAYER_SPI +#endif +#if CONFIG_NICINTEL == 1 + PROGRAMMER_NICINTEL +#endif +#if CONFIG_NICINTEL_SPI == 1 + PROGRAMMER_NICINTEL_SPI +#endif +#if CONFIG_OGP_SPI == 1 + PROGRAMMER_OGP_SPI +#endif +#if CONFIG_SATAMV == 1 + PROGRAMMER_SATAMV +#endif +#if CONFIG_LINUX_SPI == 1 + PROGRAMMER_LINUX_SPI +#endif +; +#endif + static void cli_classic_usage(const char *name) { printf("Usage: flashrom [-n] [-V] [-f] [-h|-R|-L|" @@ -111,6 +179,7 @@ #endif int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int dont_verify_it = 0, list_supported = 0, operation_specified = 0; + enum programmer prog = PROGRAMMER_INVALID; int ret = 0;
static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; @@ -258,8 +327,16 @@ #endif break; case 'p': - for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) { - name = programmer_table[programmer].name; + if (prog != PROGRAMMER_INVALID) { + fprintf(stderr, "Error: --programmer specified " + "more than once. You can separate " + "multiple\nparameters for a programmer " + "with ",". Please see the man page " + "for details.\n"); + cli_classic_abort_usage(); + } + for (prog = 0; prog < PROGRAMMER_INVALID; prog++) { + name = programmer_table[prog].name; namelen = strlen(name); if (strncmp(optarg, name, namelen) == 0) { switch (optarg[namelen]) { @@ -283,7 +360,7 @@ break; } } - if (programmer == PROGRAMMER_INVALID) { + if (prog == PROGRAMMER_INVALID) { fprintf(stderr, "Error: Unknown programmer " "%s.\n", optarg); cli_classic_abort_usage(); @@ -332,14 +409,6 @@ } #endif
-#if CONFIG_INTERNAL == 1 - if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) { - fprintf(stderr, "Error: --mainboard requires the internal " - "programmer. Aborting.\n"); - cli_classic_abort_usage(); - } -#endif - if (chip_to_probe) { for (flash = flashchips; flash && flash->name; flash++) if (!strcmp(flash->name, chip_to_probe)) @@ -355,10 +424,21 @@ flash = NULL; }
+ 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();
- if (programmer_init(pparam)) { + if (programmer_init(prog, pparam)) { fprintf(stderr, "Error: Programmer initialization failed.\n"); ret = 1; goto out_shutdown; Index: flashrom-programmer_selection_fix/flashrom.c =================================================================== --- flashrom-programmer_selection_fix/flashrom.c (Revision 1427) +++ flashrom-programmer_selection_fix/flashrom.c (Arbeitskopie) @@ -42,73 +42,7 @@ char *chip_to_probe = NULL; int verbose = 0;
-#if CONFIG_INTERNAL == 1 -enum programmer programmer = PROGRAMMER_INTERNAL; -#elif CONFIG_DUMMY == 1 -enum programmer programmer = PROGRAMMER_DUMMY; -#else -/* If neither internal nor dummy are selected, we must pick a sensible default. - * Since there is no reason to prefer a particular external programmer, we fail - * if more than one of them is selected. If only one is selected, it is clear - * that the user wants that one to become the default. - */ -#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1 -#error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one. -#endif -enum programmer programmer = -#if CONFIG_NIC3COM == 1 - PROGRAMMER_NIC3COM -#endif -#if CONFIG_NICREALTEK == 1 - PROGRAMMER_NICREALTEK -#endif -#if CONFIG_NICNATSEMI == 1 - PROGRAMMER_NICNATSEMI -#endif -#if CONFIG_GFXNVIDIA == 1 - PROGRAMMER_GFXNVIDIA -#endif -#if CONFIG_DRKAISER == 1 - PROGRAMMER_DRKAISER -#endif -#if CONFIG_SATASII == 1 - PROGRAMMER_SATASII -#endif -#if CONFIG_ATAHPT == 1 - PROGRAMMER_ATAHPT -#endif -#if CONFIG_FT2232_SPI == 1 - PROGRAMMER_FT2232_SPI -#endif -#if CONFIG_SERPROG == 1 - PROGRAMMER_SERPROG -#endif -#if CONFIG_BUSPIRATE_SPI == 1 - PROGRAMMER_BUSPIRATE_SPI -#endif -#if CONFIG_DEDIPROG == 1 - PROGRAMMER_DEDIPROG -#endif -#if CONFIG_RAYER_SPI == 1 - PROGRAMMER_RAYER_SPI -#endif -#if CONFIG_NICINTEL == 1 - PROGRAMMER_NICINTEL -#endif -#if CONFIG_NICINTEL_SPI == 1 - PROGRAMMER_NICINTEL_SPI -#endif -#if CONFIG_OGP_SPI == 1 - PROGRAMMER_OGP_SPI -#endif -#if CONFIG_SATAMV == 1 - PROGRAMMER_SATAMV -#endif -#if CONFIG_LINUX_SPI == 1 - PROGRAMMER_LINUX_SPI -#endif -; -#endif +static enum programmer programmer = PROGRAMMER_INVALID;
static char *programmer_param = NULL;
@@ -515,9 +449,15 @@ return 0; }
-int programmer_init(char *param) +int programmer_init(enum programmer prog, char *param) { int ret; + + if (prog >= PROGRAMMER_INVALID) { + msg_perr("Invalid programmer specified!\n"); + return -1; + } + programmer = prog; /* Initialize all programmer specific data. */ /* Default to unlimited decode sizes. */ max_rom_decode = (const struct decode_sizes) { Index: flashrom-programmer_selection_fix/programmer.h =================================================================== --- flashrom-programmer_selection_fix/programmer.h (Revision 1427) +++ flashrom-programmer_selection_fix/programmer.h (Arbeitskopie) @@ -85,8 +85,6 @@ PROGRAMMER_INVALID /* This must always be the last entry. */ };
-extern enum programmer programmer; - struct programmer_entry { const char *vendor; const char *name; @@ -110,7 +108,7 @@
extern const struct programmer_entry programmer_table[];
-int programmer_init(char *param); +int programmer_init(enum programmer prog, char *param); int programmer_shutdown(void);
enum bitbang_spi_master_type {