Condense 'read_it', 'write_it', 'erase_it', 'verify_it', and 'force' into a single 'flags' parameter. This makes the function signatures much more sane, while also making it much easier to add new features/flags in the future.
Signed-off-by: Nate Case ncase@xes-inc.com --- cli_classic.c | 34 +++++++++++++++++----------------- flash.h | 13 ++++++++++++- flashrom.c | 44 +++++++++++++++++++------------------------- 3 files changed, 48 insertions(+), 43 deletions(-)
Index: flashrom.c =================================================================== --- flashrom.c (revision 1832) +++ flashrom.c (working copy) @@ -1850,30 +1850,26 @@ } }
-/* FIXME: This function signature needs to be improved once doit() has a better - * function signature. - */ -int chip_safety_check(const struct flashctx *flash, int force, int read_it, int write_it, int erase_it, - int verify_it) +int chip_safety_check(const struct flashctx *flash, int flags) { const struct flashchip *chip = flash->chip;
- if (!programmer_may_write && (write_it || erase_it)) { + if (!programmer_may_write && (flags & (ACTION_FLAG_WRITE | ACTION_FLAG_ERASE))) { msg_perr("Write/erase is not working yet on your programmer in " "its current configuration.\n"); /* --force is the wrong approach, but it's the best we can do * until the generic programmer parameter parser is merged. */ - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); }
- if (read_it || erase_it || write_it || verify_it) { + if (flags & ACTION_FLAGS_RWEV) { /* Everything needs read. */ if (chip->tested.read == BAD) { msg_cerr("Read is not working on this chip. "); - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1883,7 +1879,7 @@ return 1; } } - if (erase_it || write_it) { + if (flags & (ACTION_FLAG_ERASE | ACTION_FLAG_WRITE)) { /* Write needs erase. */ if (chip->tested.erase == NA) { msg_cerr("Erase is not possible on this chip.\n"); @@ -1891,7 +1887,7 @@ } if (chip->tested.erase == BAD) { msg_cerr("Erase is not working on this chip. "); - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1901,14 +1897,14 @@ return 1; } } - if (write_it) { + if (flags & ACTION_FLAG_WRITE) { if (chip->tested.write == NA) { msg_cerr("Write is not possible on this chip.\n"); return 1; } if (chip->tested.write == BAD) { msg_cerr("Write is not working on this chip. "); - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1921,19 +1917,17 @@ return 0; }
-/* This function signature is horrible. We need to design a better interface, - * but right now it allows us to split off the CLI code. - * Besides that, the function itself is a textbook example of abysmal code flow. +/* + * FIXME: Abysmal code flow */ -int doit(struct flashctx *flash, int force, const char *filename, int read_it, - int write_it, int erase_it, int verify_it) +int doit(struct flashctx *flash, const char *filename, int flags) { uint8_t *oldcontents; uint8_t *newcontents; int ret = 0; unsigned long size = flash->chip->total_size * 1024;
- if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { + if (chip_safety_check(flash, flags)) { msg_cerr("Aborting.\n"); return 1; } @@ -1949,7 +1943,7 @@ if (flash->chip->unlock) flash->chip->unlock(flash);
- if (read_it) { + if (flags & ACTION_FLAG_READ) { return read_flash_to_file(flash, filename); }
@@ -1973,7 +1967,7 @@ * before we can write. */
- if (erase_it) { + if (flags & ACTION_FLAG_ERASE) { /* FIXME: Do we really want the scary warning if erase failed? * After all, after erase the chip is either blank or partially * blank or it has the old contents. A blank chip won't boot, @@ -1987,7 +1981,7 @@ goto out; }
- if (write_it || verify_it) { + if (flags & (ACTION_FLAG_WRITE | ACTION_FLAG_VERIFY)) { if (read_buf_from_file(newcontents, size, filename)) { ret = 1; goto out; @@ -2026,7 +2020,7 @@
// ////////////////////////////////////////////////////////////
- if (write_it) { + if (flags & ACTION_FLAG_WRITE) { if (erase_and_write_flash(flash, oldcontents, newcontents)) { msg_cerr("Uh oh. Erase/write failed. Checking if anything has changed.\n"); msg_cinfo("Reading current flash chip contents... "); @@ -2047,10 +2041,10 @@ }
/* Verify only if we either did not try to write (verify operation) or actually changed something. */ - if (verify_it && (!write_it || !all_skipped)) { + if ((flags & ACTION_FLAG_VERIFY) && (!(flags & ACTION_FLAG_WRITE) || !all_skipped)) { msg_cinfo("Verifying flash... ");
- if (write_it) { + if (flags & ACTION_FLAG_WRITE) { /* Work around chips which need some time to calm down. */ programmer_delay(1000*1000); ret = verify_range(flash, newcontents, 0, size); Index: cli_classic.c =================================================================== --- cli_classic.c (revision 1832) +++ cli_classic.c (working copy) @@ -98,11 +98,11 @@ struct flashctx *fill_flash; const char *name; int namelen, opt, i, j; - int startchip = -1, chipcount = 0, option_index = 0, force = 0; + int startchip = -1, chipcount = 0, option_index = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + int flags = 0; int dont_verify_it = 0, list_supported = 0, operation_specified = 0; enum programmer prog = PROGRAMMER_INVALID; int ret = 0; @@ -156,7 +156,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - read_it = 1; + flags |= ACTION_FLAG_READ; break; case 'w': if (++operation_specified > 1) { @@ -165,7 +165,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - write_it = 1; + flags |= ACTION_FLAG_WRITE; break; case 'v': //FIXME: gracefully handle superfluous -v @@ -179,10 +179,10 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - verify_it = 1; + flags |= ACTION_FLAG_VERIFY; break; case 'n': - if (verify_it) { + if (flags & ACTION_FLAG_VERIFY) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); cli_classic_abort_usage(); } @@ -202,10 +202,10 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - erase_it = 1; + flags |= ACTION_FLAG_ERASE; break; case 'f': - force = 1; + flags |= ACTION_FLAG_FORCE; break; case 'l': if (layoutfile) { @@ -327,7 +327,7 @@ cli_classic_abort_usage(); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if ((flags & ACTION_FLAGS_RWV) && check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -369,7 +369,7 @@ ret = 1; goto out; } - if (layoutfile != NULL && !write_it) { + if (layoutfile != NULL && !(flags & ACTION_FLAG_WRITE)) { msg_gerr("Layout files are currently supported for write operations only.\n"); ret = 1; goto out; @@ -447,11 +447,11 @@ goto out_shutdown; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); - if (!force || !chip_to_probe) { + if (!(flags & ACTION_FLAG_FORCE) || !chip_to_probe) { msg_cinfo("Note: flashrom can never write if the flash chip isn't found " "automatically.\n"); } - if (force && read_it && chip_to_probe) { + if ((flags & ACTION_FLAG_FORCE) && (flags & ACTION_FLAG_READ) && chip_to_probe) { struct registered_master *mst; int compatible_masters = 0; msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n"); @@ -502,27 +502,27 @@ check_chip_supported(fill_flash->chip);
size = fill_flash->chip->total_size * 1024; - if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!force)) { + if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && !(flags & ACTION_FLAG_FORCE)) { msg_cerr("Chip is too big for this programmer (-V gives details). Use --force to override.\n"); ret = 1; goto out_shutdown; }
- if (!(read_it | write_it | verify_it | erase_it)) { + if (!(flags & ACTION_FLAGS_RWEV)) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; }
/* Always verify write operations unless -n is used. */ - if (write_it && !dont_verify_it) - verify_it = 1; + if ((flags & ACTION_FLAG_WRITE) && !dont_verify_it) + flags |= ACTION_FLAG_VERIFY;
/* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ programmer_delay(100000); - ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, filename, flags);
out_shutdown: programmer_shutdown(); Index: flash.h =================================================================== --- flash.h (revision 1832) +++ flash.h (working copy) @@ -121,6 +121,17 @@ #define FEATURE_OTP (1 << 8) #define FEATURE_QPI (1 << 9)
+#define ACTION_FLAG_READ (1 << 1) +#define ACTION_FLAG_WRITE (1 << 2) +#define ACTION_FLAG_ERASE (1 << 3) +#define ACTION_FLAG_VERIFY (1 << 4) +#define ACTION_FLAG_FORCE (1 << 5) + +#define ACTION_FLAGS_RWEV (ACTION_FLAG_READ | ACTION_FLAG_WRITE | \ + ACTION_FLAG_ERASE | ACTION_FLAG_VERIFY) +#define ACTION_FLAGS_RWV (ACTION_FLAG_READ | ACTION_FLAG_WRITE | \ + ACTION_FLAG_VERIFY) + enum test_state { OK = 0, NT = 1, /* Not tested */ @@ -268,7 +279,7 @@ void print_banner(void); void list_programmers_linebreak(int startcol, int cols, int paren); int selfcheck(void); -int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it); +int doit(struct flashctx *flash, const char *filename, int flags); int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename); int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
On Wed, 30 Jul 2014 09:01:43 -0500 (CDT) Nate Case ncase@xes-inc.com wrote:
Condense 'read_it', 'write_it', 'erase_it', 'verify_it', and 'force' into a single 'flags' parameter. This makes the function signatures much more sane, while also making it much easier to add new features/flags in the future.
Signed-off-by: Nate Case ncase@xes-inc.com
Hello Nate and thanks for your patch!
We (mostly Carl-Daniel and myself) have (individually) thought about this (obvious and prominently documented) problem but have not discussed this with each other or in public.
My opinion is that it would make sense to add a new field to struct flashctx that holds various flashrom-wide options (like the action(s) to take or the force switch, but also other things). That would get rid of some other global variables as well. I am not sure about using an integer flag for that though... I'd probably rather use a new struct with bitfields instead.
(I should use less parentheses.) Anyway, before investing any time into this matter, I would like to hear what Carl-Daniel has to say about it (and anybody else too of course! Damnit, parentheses again).
We (mostly Carl-Daniel and myself) have (individually) thought about this (obvious and prominently documented) problem but have not discussed this with each other or in public.
My opinion is that it would make sense to add a new field to struct flashctx that holds various flashrom-wide options (like the action(s) to take or the force switch, but also other things). That would get rid of some other global variables as well. I am not sure about using an integer flag for that though... I'd probably rather use a new struct with bitfields instead.
(I should use less parentheses.) Anyway, before investing any time into this matter, I would like to hear what Carl-Daniel has to say about it (and anybody else too of course! Damnit, parentheses again).
Hi Stefan,
For what it's worth as someone new to this project, my initial thought was to stuff the action parameters into struct flashctx as well. However, after glancing at the existing fields within that structure it didn't seem appropriate (that is, everything else there seems to be describing the flash chip or programmer). On the other hand, even though it's arguably abusing the struct, it's still clearly better than the existing solution.
A new struct like you described instead of the integer flag seems perfectly reasonable to me also. Ultimately I'm just hoping for *something* resembling this to get merged so that I can base future patches on it. Specifically, I have a patch to add a new "read/write/verify_it"-like parameter, which will of course depend on the resolution of this problem.
(and don't feel bad; I've also been guilty of parentheses abuse)
Thanks,
Nate
On Wed, 30 Jul 2014 10:55:10 -0500 (CDT) Nate Case ncase@xes-inc.com wrote:
For what it's worth as someone new to this project, my initial thought was to stuff the action parameters into struct flashctx as well. However, after glancing at the existing fields within that structure it didn't seem appropriate (that is, everything else there seems to be describing the flash chip or programmer). On the other hand, even though it's arguably abusing the struct, it's still clearly better than the existing solution.
struct flashctx is rather new (fall 2011, r1473, ok... not that new anymore) and was introduced to make runtime data available where needed. Previously the pointers to the chip's memory-mapped contents were stuffed into struct flashchip(!). We just have not used flashctx much further yet, but it is definitely the right place for it IMHO.
A new struct like you described instead of the integer flag seems perfectly reasonable to me also. Ultimately I'm just hoping for *something* resembling this to get merged so that I can base future patches on it. Specifically, I have a patch to add a new "read/write/verify_it"-like parameter, which will of course depend on the resolution of this problem.
Carl-Daniel is going on vacation tomorrow and won't be readily available for at least a week. Also, we have a very bad history of merging patches (not only foreign ones, or own too). So, if you don't hear anything from him soonish, I suggest you either implement my proposition (I believe that has a good probability of getting merged, apart from some name bikeshedding), or continue with what you have so far and be prepared to adapt your changes later. After all, your changes so far are not that bold and have a relatively limited extent. That should not cause to many troubles if changed later.
I am interested in hearing more about your further plans. We had patches for a few related functionalities (e.g. setting the status register of SPI flashes manually) and google is using a very extended CLI for unlocking address ranges of flash chips that you might want to look at first.
Carl-Daniel is going on vacation tomorrow and won't be readily available for at least a week. Also, we have a very bad history of merging patches (not only foreign ones, or own too). So, if you don't hear anything from him soonish, I suggest you either implement my proposition (I believe that has a good probability of getting merged, apart from some name bikeshedding), or continue with what you have so far and be prepared to adapt your changes later. After all, your changes so far are not that bold and have a relatively limited extent. That should not cause to many troubles if changed later.
I decided to implement your proposal since I think it's cleaner; I just submitted that as v2 a few minutes ago. Feel free to bikeshed to your heart's content.
I am interested in hearing more about your further plans. We had patches for a few related functionalities (e.g. setting the status register of SPI flashes manually) and google is using a very extended CLI for unlocking address ranges of flash chips that you might want to look at first.
The most immediate need was to add an option to bypass the check_erased_range() call in erase_and_write_block_helper() to speed up erases. Supposedly it reduced erase times from ~13 minutes to ~3 minutes. I don't know if this is a great idea or not since I had little to do with it; I just saw it in an internal svn branch where they implemented it by adding an ugly "erase_check" argument in addition to read_it, write_it, etc.
Thanks,
Nate
On Thu, 31 Jul 2014 11:06:55 -0500 (CDT) Nate Case ncase@xes-inc.com wrote:
Carl-Daniel is going on vacation tomorrow and won't be readily available for at least a week. Also, we have a very bad history of merging patches (not only foreign ones, or own too). So, if you don't hear anything from him soonish, I suggest you either implement my proposition (I believe that has a good probability of getting merged, apart from some name bikeshedding), or continue with what you have so far and be prepared to adapt your changes later. After all, your changes so far are not that bold and have a relatively limited extent. That should not cause to many troubles if changed later.
I decided to implement your proposal since I think it's cleaner; I just submitted that as v2 a few minutes ago. Feel free to bikeshed to your heart's content.
Thanks, that looks ok to me in general. I'd use just 'opts' for the field name, but let's hear what Carl-Daniel thinks about it first.
I am interested in hearing more about your further plans. We had patches for a few related functionalities (e.g. setting the status register of SPI flashes manually) and google is using a very extended CLI for unlocking address ranges of flash chips that you might want to look at first.
The most immediate need was to add an option to bypass the check_erased_range() call in erase_and_write_block_helper() to speed up erases. Supposedly it reduced erase times from ~13 minutes to ~3 minutes. I don't know if this is a great idea or not since I had little to do with it; I just saw it in an internal svn branch where they implemented it by adding an ugly "erase_check" argument in addition to read_it, write_it, etc.
The erase check is one part of the safety net, and allows to bail out early if there are problems. The speedup you mentioned seems unreasonable from such a change alone IMHO. It does only read the erased block, so essentially the whole chip is read (in chunks) one additional time with the check enabled. Anyway, I think you can base your changes on this patch. And I personally would not mind if the infrastructure for the skipping would end up in flashrom as well (but disabled in cli_classic.c).
On Thu, 31 Jul 2014 11:06:55 -0500 (CDT) Nate Case ncase@xes-inc.com wrote:
I decided to implement your proposal since I think it's cleaner; I just submitted that as v2 a few minutes ago. Feel free to bikeshed to your heart's content.
Thanks, that looks ok to me in general. I'd use just 'opts' for the field name, but let's hear what Carl-Daniel thinks about it first.
Sounds good.
The most immediate need was to add an option to bypass the check_erased_range() call in erase_and_write_block_helper() to speed up erases. Supposedly it reduced erase times from ~13 minutes to ~3 minutes. I don't know if this is a great idea or not since I had little to do with it; I just saw it in an internal svn branch where they implemented it by adding an ugly "erase_check" argument in addition to read_it, write_it, etc.
The erase check is one part of the safety net, and allows to bail out early if there are problems. The speedup you mentioned seems unreasonable from such a change alone IMHO. It does only read the erased block, so essentially the whole chip is read (in chunks) one additional time with the check enabled. Anyway, I think you can base your changes on this patch. And I personally would not mind if the infrastructure for the skipping would end up in flashrom as well (but disabled in cli_classic.c).
I can't claim that the 13 > 3 minute speedup came from this change alone with any certainty since I didn't do the testing myself. I see they also implemented a block_erase order reversal patch similar to Pablo's recent submission. So it's possible the benchmarking included that patch as well even though their comments suggested it was the erase check skipping alone.
Thanks,
Nate
Condense 'read_it', 'write_it', 'erase_it', 'verify_it', and 'force' into a single 'action_opts' struct that's available via flashctx. This makes the function signatures much more sane, while also making it much easier to add new features/options in the future.
Signed-off-by: Nate Case ncase@xes-inc.com --- cli_classic.c | 39 ++++++++++++++++++++++----------------- flash.h | 11 ++++++++++- flashrom.c | 47 ++++++++++++++++++++++------------------------- 3 files changed, 54 insertions(+), 43 deletions(-)
v2 changes: Use bitfield struct in flashctx instead of integer flag
Index: flash.h =================================================================== --- flash.h (revision 1832) +++ flash.h (working copy) @@ -207,12 +207,21 @@ enum write_granularity gran; };
+struct action_opts { + unsigned do_read:1; + unsigned do_write:1; + unsigned do_erase:1; + unsigned do_verify:1; + unsigned force:1; +}; + struct flashctx { struct flashchip *chip; chipaddr virtual_memory; /* Some flash devices have an additional register space. */ chipaddr virtual_registers; struct registered_master *mst; + struct action_opts *action; };
/* Timing used in probe routines. ZERO is -2 to differentiate between an unset @@ -268,7 +277,7 @@ void print_banner(void); void list_programmers_linebreak(int startcol, int cols, int paren); int selfcheck(void); -int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it); +int doit(struct flashctx *flash, const char *filename); int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename); int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
Index: cli_classic.c =================================================================== --- cli_classic.c (revision 1832) +++ cli_classic.c (working copy) @@ -98,11 +98,11 @@ struct flashctx *fill_flash; const char *name; int namelen, opt, i, j; - int startchip = -1, chipcount = 0, option_index = 0, force = 0; + int startchip = -1, chipcount = 0, option_index = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + struct action_opts action = {0}; int dont_verify_it = 0, list_supported = 0, operation_specified = 0; enum programmer prog = PROGRAMMER_INVALID; int ret = 0; @@ -142,6 +142,9 @@ if (selfcheck()) exit(1);
+ for (i = 0; i < ARRAY_SIZE(flashes); i++) + flashes[i].action = &action; + setbuf(stdout, NULL); /* FIXME: Delay all operation_specified checks until after command * line parsing to allow --help overriding everything else. @@ -156,7 +159,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - read_it = 1; + action.do_read = 1; break; case 'w': if (++operation_specified > 1) { @@ -165,7 +168,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - write_it = 1; + action.do_write = 1; break; case 'v': //FIXME: gracefully handle superfluous -v @@ -179,10 +182,10 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - verify_it = 1; + action.do_verify = 1; break; case 'n': - if (verify_it) { + if (action.do_verify) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); cli_classic_abort_usage(); } @@ -202,10 +205,10 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - erase_it = 1; + action.do_erase = 1; break; case 'f': - force = 1; + action.force = 1; break; case 'l': if (layoutfile) { @@ -327,7 +330,8 @@ cli_classic_abort_usage(); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if ((action.do_read || action.do_write || action.do_verify) && + check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -369,7 +373,7 @@ ret = 1; goto out; } - if (layoutfile != NULL && !write_it) { + if (layoutfile != NULL && !action.do_write) { msg_gerr("Layout files are currently supported for write operations only.\n"); ret = 1; goto out; @@ -447,11 +451,11 @@ goto out_shutdown; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); - if (!force || !chip_to_probe) { + if (!action.force || !chip_to_probe) { msg_cinfo("Note: flashrom can never write if the flash chip isn't found " "automatically.\n"); } - if (force && read_it && chip_to_probe) { + if (action.force && action.do_read && chip_to_probe) { struct registered_master *mst; int compatible_masters = 0; msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n"); @@ -502,27 +506,28 @@ check_chip_supported(fill_flash->chip);
size = fill_flash->chip->total_size * 1024; - if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!force)) { + if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!action.force)) { msg_cerr("Chip is too big for this programmer (-V gives details). Use --force to override.\n"); ret = 1; goto out_shutdown; }
- if (!(read_it | write_it | verify_it | erase_it)) { + if (!(action.do_read || action.do_write || action.do_verify || + action.do_erase)) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; }
/* Always verify write operations unless -n is used. */ - if (write_it && !dont_verify_it) - verify_it = 1; + if (action.do_write && !dont_verify_it) + action.do_verify = 1;
/* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ programmer_delay(100000); - ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, filename);
out_shutdown: programmer_shutdown(); Index: flashrom.c =================================================================== --- flashrom.c (revision 1832) +++ flashrom.c (working copy) @@ -1850,30 +1850,28 @@ } }
-/* FIXME: This function signature needs to be improved once doit() has a better - * function signature. - */ -int chip_safety_check(const struct flashctx *flash, int force, int read_it, int write_it, int erase_it, - int verify_it) +int chip_safety_check(const struct flashctx *flash) { const struct flashchip *chip = flash->chip; + struct action_opts *opts = flash->action;
- if (!programmer_may_write && (write_it || erase_it)) { + if (!programmer_may_write && (opts->do_write || opts->do_erase)) { msg_perr("Write/erase is not working yet on your programmer in " "its current configuration.\n"); /* --force is the wrong approach, but it's the best we can do * until the generic programmer parameter parser is merged. */ - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); }
- if (read_it || erase_it || write_it || verify_it) { + if (opts->do_read || opts->do_erase || opts->do_write || + opts->do_verify) { /* Everything needs read. */ if (chip->tested.read == BAD) { msg_cerr("Read is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1883,7 +1881,7 @@ return 1; } } - if (erase_it || write_it) { + if (opts->do_erase || opts->do_write) { /* Write needs erase. */ if (chip->tested.erase == NA) { msg_cerr("Erase is not possible on this chip.\n"); @@ -1891,7 +1889,7 @@ } if (chip->tested.erase == BAD) { msg_cerr("Erase is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1901,14 +1899,14 @@ return 1; } } - if (write_it) { + if (opts->do_write) { if (chip->tested.write == NA) { msg_cerr("Write is not possible on this chip.\n"); return 1; } if (chip->tested.write == BAD) { msg_cerr("Write is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1921,19 +1919,18 @@ return 0; }
-/* This function signature is horrible. We need to design a better interface, - * but right now it allows us to split off the CLI code. - * Besides that, the function itself is a textbook example of abysmal code flow. +/* + * FIXME: Abysmal code flow */ -int doit(struct flashctx *flash, int force, const char *filename, int read_it, - int write_it, int erase_it, int verify_it) +int doit(struct flashctx *flash, const char *filename) { uint8_t *oldcontents; uint8_t *newcontents; int ret = 0; unsigned long size = flash->chip->total_size * 1024; + struct action_opts *opts = flash->action;
- if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { + if (chip_safety_check(flash)) { msg_cerr("Aborting.\n"); return 1; } @@ -1949,7 +1946,7 @@ if (flash->chip->unlock) flash->chip->unlock(flash);
- if (read_it) { + if (opts->do_read) { return read_flash_to_file(flash, filename); }
@@ -1973,7 +1970,7 @@ * before we can write. */
- if (erase_it) { + if (opts->do_erase) { /* FIXME: Do we really want the scary warning if erase failed? * After all, after erase the chip is either blank or partially * blank or it has the old contents. A blank chip won't boot, @@ -1987,7 +1984,7 @@ goto out; }
- if (write_it || verify_it) { + if (opts->do_write || opts->do_verify) { if (read_buf_from_file(newcontents, size, filename)) { ret = 1; goto out; @@ -2026,7 +2023,7 @@
// ////////////////////////////////////////////////////////////
- if (write_it) { + if (opts->do_write) { if (erase_and_write_flash(flash, oldcontents, newcontents)) { msg_cerr("Uh oh. Erase/write failed. Checking if anything has changed.\n"); msg_cinfo("Reading current flash chip contents... "); @@ -2047,10 +2044,10 @@ }
/* Verify only if we either did not try to write (verify operation) or actually changed something. */ - if (verify_it && (!write_it || !all_skipped)) { + if (opts->do_verify && (!opts->do_write || !all_skipped)) { msg_cinfo("Verifying flash... ");
- if (write_it) { + if (opts->do_write) { /* Work around chips which need some time to calm down. */ programmer_delay(1000*1000); ret = verify_range(flash, newcontents, 0, size);
-
Nate Case -
Stefan Tauner