flashrom does not honor argument ordering for operations. Not only does this violate the principle of least surprise, it also caused one bug where -Ewv was specified and the flash ended up being empty.
Support only one operation at a time. As a side benefit, this allows us to clean up main() quite a bit.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-one_commandline_operation_only/flashrom.8 =================================================================== --- flashrom-one_commandline_operation_only/flashrom.8 (Revision 583) +++ flashrom-one_commandline_operation_only/flashrom.8 (Arbeitskopie) @@ -2,7 +2,7 @@ .SH NAME flashrom - detect, read, write, verify and erase flash chips .SH SYNOPSIS -.B flashrom \fR[\fB-EVfLhR\fR] [\fB-r\fR file] [\fB-w\fR file] [\fB-v\fR file] +.B flashrom \fR[\fB-VfLhR\fR] [\fB-E\fR|\fB-r\fR file|\fB-w\fR file|\fB-v\fR file] [\fB-c\fR chipname] [\fB-s\fR addr] [\fB-e\fR addr] [\fB-m\fR [vendor:]part] [\fB-l\fR file] [\fB-i\fR image] [\fB-p\fR programmer] [file] .SH DESCRIPTION @@ -25,7 +25,8 @@ flashrom 1.0. Do not use flashrom in scripts or other automated tools without checking that your flashrom version won't interpret options in a different way. .PP -If no file is specified, then all that happens +You can specify one of -E, -r, -w, -v or no operation. +If no operation is specified, then all that happens is that flash info is dumped and the flash chip is set to writable. .TP .B "-r, --read <file>" Index: flashrom-one_commandline_operation_only/flashrom.c =================================================================== --- flashrom-one_commandline_operation_only/flashrom.c (Revision 583) +++ flashrom-one_commandline_operation_only/flashrom.c (Arbeitskopie) @@ -471,7 +471,7 @@
void usage(const char *name) { - printf("usage: %s [-EVfLhR] [-r file] [-w file] [-v file] [-c chipname] [-s addr]\n" + printf("usage: %s [-VfLhR] [-E|-r file|-w file|-v file] [-c chipname] [-s addr]\n" " [-e addr] [-m [vendor:]part] [-l file] [-i image] [-p programmer] [file]\n\n", name);
@@ -498,7 +498,8 @@ " (internal, dummy, nic3com, satasii, it87spi)\n" " -h | --help: print this help text\n" " -R | --version: print the version (release)\n" - "\nIf no file is specified, then all that happens" + "\nYou can specify one of -E, -r, -w, -v or no operation.\n" + "If no operation is specified, then all that happens" " is that flash info is dumped.\n\n"); exit(1); } @@ -520,6 +521,7 @@ int force = 0; int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int list_supported = 0; + int operation_specified = 0; int ret = 0, i;
static struct option long_options[] = { @@ -562,12 +564,27 @@ long_options, &option_index)) != EOF) { switch (opt) { case 'r': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation " + "specified. Aborting.\n"); + exit(1); + } read_it = 1; break; case 'w': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation " + "specified. Aborting.\n"); + exit(1); + } write_it = 1; break; case 'v': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation " + "specified. Aborting.\n"); + exit(1); + } verify_it = 1; break; case 'c': @@ -577,6 +594,11 @@ verbose = 1; break; case 'E': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation " + "specified. Aborting.\n"); + exit(1); + } erase_it = 1; break; case 's':
On Fri, Jun 12, 2009 at 01:12:17PM +0200, Carl-Daniel Hailfinger wrote:
flashrom does not honor argument ordering for operations. Not only does this violate the principle of least surprise, it also caused one bug where -Ewv was specified and the flash ended up being empty.
Support only one operation at a time. As a side benefit, this allows us to clean up main() quite a bit.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Uwe Hermann uwe@hermann-uwe.de
I agree with Ron that we should not combine commands.
But we _should_ make write/erase autoverify!
Uwe.
On 12.06.2009 15:57, Uwe Hermann wrote:
On Fri, Jun 12, 2009 at 01:12:17PM +0200, Carl-Daniel Hailfinger wrote:
Support only one operation at a time.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Thanks, committed in r585.
I agree with Ron that we should not combine commands.
But we _should_ make write/erase autoverify!
True. I just sent a patch which converts half of the chip to auto-verify of erase. The code is sprinkled all over the place because some write routines have implicit erase and that needs to be checked as well.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
I agree with Ron that we should not combine commands.
But we _should_ make write/erase autoverify!
If flashrom is going to make a serious effort to branch out into supporting external programmers then I think its time to refactor the core code into libflashrom and provide separate UI's for the different needs.
The workflow of flashrom to flash coreboot ont the chip on your motherboard is different from the workflow/feature needs of a general purpose external programmer.
An attempt to find a single UI that works for both is going to result in one that only does a half-ass job at either one.
On 12.06.2009 18:06, Richard Smith wrote:
If flashrom is going to make a serious effort to branch out into supporting external programmers then I think its time to refactor the core code into libflashrom and provide separate UI's for the different needs.
We already support three external programmers, one of them SPI. I think that counts as a serious effort. With the FT2232, we'll have four.
The libflashrom idea has been mentioned repeatedly in the last few years. Those who proposed it either didn't have the time or skills to transform the core code into flashrom and still have a working flashrom binary (on top of libflashrom).
The workflow of flashrom to flash coreboot ont the chip on your motherboard is different from the workflow/feature needs of a general purpose external programmer.
An attempt to find a single UI that works for both is going to result in one that only does a half-ass job at either one.
What's wrong with the one-operation-at-a-time UI we have now? It is definitely wrappable in a bash script for any advanced stuff. You seem to be unhappy with it and I'd like to understand why.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 12.06.2009 18:06, Richard Smith wrote:
If flashrom is going to make a serious effort to branch out into supporting external programmers then I think its time to refactor the core code into libflashrom and provide separate UI's for the different needs.
We already support three external programmers, one of them SPI. I think that counts as a serious effort. With the FT2232, we'll have four.
Nice. Guess that road is started.
The libflashrom idea has been mentioned repeatedly in the last few years. Those who proposed it either didn't have the time or skills to transform the core code into flashrom and still have a working flashrom binary (on top of libflashrom).
Isn't a lot of that already on its way with the refactoring you have been doing? Perhaps you could do it in stages?
What's wrong with the one-operation-at-a-time UI we have now? It is definitely wrappable in a bash script for any advanced stuff. You seem to be unhappy with it and I'd like to understand why.
Perhaps nothing but I haven't had a chance to use flashrom in my daily routine yet because I use the cheetah and 4232H support only just started working. After I use it I can report back.
(is the cheetah stuff at a testable level yet?)
I'm more thinking about the future where the feature set would grow based on the features of a specific programmer or something that a non-coreboot user would need.
On 12.06.2009 19:16, Richard Smith wrote:
Carl-Daniel Hailfinger wrote:
We already support three external programmers [...]
Nice. Guess that road is started.
Indeed.
The libflashrom idea [...]
Isn't a lot of that already on its way with the refactoring you have been doing? Perhaps you could do it in stages?
Well, since 1. OFW uses its own flasher, so it won't benefit from libflashrom 2. any emergency flasher in coreboot needs to be small (a one-chip-only solution) and flashrom is 114k stripped I don't see any benefit in libflashrom.
[...] I haven't had a chance to use flashrom in my daily routine yet because I use the cheetah and 4232H support only just started working. After I use it I can report back.
Please do.
(is the cheetah stuff at a testable level yet?)
I concentrated on FT4232H support for now. Both Cheetah and FT4232H have one big problem: Compilation and linking. Do we link against libftdi or not? Will symbol resolution be done by the linker automatically or do we use dlopen() and dlsym()? Should we use compile time flags instead? How should we modify our makefiles for that?
I'm more thinking about the future where the feature set would grow based on the features of a specific programmer or something that a non-coreboot user would need.
I'd say we wait for such feature set extensions before trying to design an API. Most attempts in the past to create future-proof APIs were thwarted either by new features or crappy hardware...
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
Well, since
- OFW uses its own flasher, so it won't benefit from libflashrom
- any emergency flasher in coreboot needs to be small (a one-chip-only
solution) and flashrom is 114k stripped I don't see any benefit in libflashrom.
Its only a benefit if you see value in having multiple front ends if flashrom is flexable enough that its the the only user interface then libflashrom dosen't help you much.
The goal of a libflashrom would be so that commercial manufacturers/hobbyist makers of device programmers have a nice clean way of using the underlying flashrom plumbing for whatever user interface they want to provide.
(is the cheetah stuff at a testable level yet?)
I concentrated on FT4232H support for now. Both Cheetah and FT4232H have one big problem: Compilation and linking. Do we link against libftdi or not?W ill symbol resolution be done by the linker automatically or do we use dlopen() and dlsym()? Should we use compile time flags instead? How should we modify our makefiles for that?
My vote would be for a compile time option including the ability to link static. The cheetah appears to make that difficult since they only provide an .so. I don't think its too much to to make users who want to use flashrom with external support recompile with that support enabled.
I'm more thinking about the future where the feature set would grow based on the features of a specific programmer or something that a non-coreboot user would need.
I'd say we wait for such feature set extensions before trying to design an API. Most attempts in the past to create future-proof APIs were thwarted either by new features or crappy hardware...
This is what I mean about being serious about supporting external programmers. Setting up the stage to become the "the" plumbing for open source programmers. Perhaps thats too far outside of the coreboot scope but if thats the case then external programmer support will just be something that a few of us with specific needs are driving rather than something that can develop into a life of its own.
Richard Smith wrote:
Setting up the stage to become the "the" plumbing for open source programmers.
This is at least a long term goal of mine. But the flashrom code is quite far away from this, and will stay that way for some time.
//Peter
-
Carl-Daniel Hailfinger -
Peter Stuge -
Richard Smith -
Uwe Hermann