Submitter | Carl-Daniel Hailfinger |
---|---|
Date | 2011-09-04 23:15:35 |
Message ID | <4E640697.5090303@gmx.net> |
Download | mbox | patch |
Permalink | /patch/3401/ |
State | Accepted |
Commit | r1433 |
Headers | show |
Comments
On Mon, 05 Sep 2011 01:15:35 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > 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. why? when you set the enabled programmers with a script you could also set the default programmer to one of them programmatically. will look at the second revision later.
On Mon, 05 Sep 2011 01:15:35 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > 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)) { just curious: why was this needed/wanted before? > […] > Index: flashrom-programmer_selection_fix/flashrom.c > =================================================================== > --- flashrom-programmer_selection_fix/flashrom.c (Revision 1427) > +++ flashrom-programmer_selection_fix/flashrom.c (Arbeitskopie) > […] > @@ -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) { should we also check against < 0? enums are based on int. the default starting point is 0 (if the first entry does not have a specific value assigned with =), but i guess one could cast (enum programmer)-1 or so? untested and maybe stupid... :) > + msg_perr("Invalid programmer specified!\n"); > + return -1; why so negative? ;) > […] apart from that and our default programmer dispute it looks good to me. so please think of it as Acked-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Am 05.09.2011 23:57 schrieb Stefan Tauner: > On Mon, 05 Sep 2011 01:15:35 +0200 > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > >> 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)) { >> > just curious: why was this needed/wanted before? > This was a hunk I forgot to remove when I killed the separate it87spi programmer. That piece of code was the only location which needed a global programmer variable. >> […] >> Index: flashrom-programmer_selection_fix/flashrom.c >> =================================================================== >> --- flashrom-programmer_selection_fix/flashrom.c (Revision 1427) >> +++ flashrom-programmer_selection_fix/flashrom.c (Arbeitskopie) >> […] >> @@ -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) { >> > should we also check against < 0? enums are based on int. Not sure about that: http://software.intel.com/en-us/articles/strict-ansi-switch-in-linux-and-mac/ Are they really guaranteed to be signed int? If not, some compilers will warn about "comparison always yields false". > the default > starting point is 0 (if the first entry does not have a specific value > assigned with =), but i guess one could cast (enum programmer)-1 or so? > untested and maybe stupid... :) > I thought about it, and decided to only check for >= PROGRAMMER_INVALID. >> + msg_perr("Invalid programmer specified!\n"); >> + return -1; >> > why so negative? ;) > > >> […] >> > apart from that and our default programmer dispute it looks good to me. > so please think of it as > Acked-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> > Thanks, I'll commit in the next ~24 hours. If you're unhappy about my enum answer, please tell me and I'll dig into the standard again. Regards, Carl-Daniel
On Tue, 06 Sep 2011 02:20:31 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > should we also check against < 0? enums are based on int. > > Not sure about that: > http://software.intel.com/en-us/articles/strict-ansi-switch-in-linux-and-mac/ > Are they really guaranteed to be signed int? If not, some compilers will > warn about "comparison always yields false". 6.7.2.2 Enumeration specifiers paragraph 2: "The expression that defines the value of an enumeration constant shall be an integer constant expression that has a value representable as an int." but that does only restrict the "input" type of the "initializers". the really important part is paragraph 4: "Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined, but shall be capable of representing the values of all the members of the enumeration. The enumerated type is incomplete until after the } that terminates the list of enumerator declarations." so eventually this is another implementation-defined behavior, hurray. gcc does this: http://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Structures-unions-enumerations-and-bit_002dfields-implementation.html define a precondition for programmer_init that parameter prog must be in the range defined by the enum and else the behavior is undefined, and think of the > part of the check as a user-friendly bonus :) a detailed description of what the functions exported by libflashrom do is required anyway... i am fine with the current implementation.
On Mon, Sep 05, 2011 at 01:15:35AM +0200, Carl-Daniel Hailfinger 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. > 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> Acked-by: Uwe Hermann <uwe@hermann-uwe.de> +1 for making 'programmer' static. Uwe.
Am 05.09.2011 23:57 schrieb Stefan Tauner:
> Acked-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Thanks, committed in r1433.
Regards,
Carl-Daniel
Patch
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 {