Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/17949 )
Change subject: Kill doit() ......................................................................
Kill doit()
No words can describe this feeling.
v2: Rejoice while removing more, orphaned code (layout.c).
Change-Id: Id81177c50b4410e68dcf8ebab48386a94cd9b714 Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/17949 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: David Hendricks david.hendricks@gmail.com --- M cli_classic.c M flash.h M flashrom.c M layout.c 4 files changed, 91 insertions(+), 418 deletions(-)
Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c index 984a4da..00baf49 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -530,24 +530,28 @@ goto out_shutdown; }
- /* Always verify write operations unless -n is used. */ - if (write_it && !dont_verify_it) - verify_it = 1; + if (layoutfile) + flashrom_layout_set(fill_flash, get_global_layout());
- /* Map the selected flash chip again. */ - if (map_flash(fill_flash) != 0) { - ret = 1; - goto out_shutdown; - } + flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, !!force); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, !!force_boardmismatch); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, true);
/* 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); + if (read_it) + ret = do_read(fill_flash, filename); + else if (erase_it) + ret = do_erase(fill_flash); + else if (write_it) + ret = do_write(fill_flash, filename); + else if (verify_it) + ret = do_verify(fill_flash, filename);
- unmap_flash(fill_flash); out_shutdown: programmer_shutdown(); out: diff --git a/flash.h b/flash.h index db8430c..7c7ecd5 100644 --- a/flash.h +++ b/flash.h @@ -285,9 +285,12 @@ 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 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); +int do_read(struct flashctx *, const char *filename); +int do_erase(struct flashctx *); +int do_write(struct flashctx *, const char *const filename); +int do_verify(struct flashctx *, const char *const filename);
/* Something happened that shouldn't happen, but we can go on. */ #define ERROR_NONFATAL 0x100 @@ -354,7 +357,6 @@ int process_include_args(void); int read_romlayout(const char *name); int normalize_romentries(const struct flashctx *flash); -int build_new_image(struct flashctx *flash, bool oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents); void layout_cleanup(void);
/* spi.c */ diff --git a/flashrom.c b/flashrom.c index 3be3d32..325a0c1 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1374,6 +1374,7 @@ #endif }
+static int read_by_layout(struct flashctx *, uint8_t *); int read_flash_to_file(struct flashctx *flash, const char *filename) { unsigned long size = flash->chip->total_size * 1024; @@ -1391,7 +1392,7 @@ ret = 1; goto out_free; } - if (flash->chip->read(flash, buf, 0, size)) { + if (read_by_layout(flash, buf)) { msg_cerr("Read operation failed!\n"); ret = 1; goto out_free; @@ -1470,97 +1471,6 @@ return ret; }
-static int erase_and_write_block_helper(struct flashctx *flash, - unsigned int start, unsigned int len, - uint8_t *curcontents, - uint8_t *newcontents, - int (*erasefn) (struct flashctx *flash, - unsigned int addr, - unsigned int len)) -{ - unsigned int starthere = 0, lenhere = 0; - int ret = 0, skip = 1, writecount = 0; - enum write_granularity gran = flash->chip->gran; - - /* curcontents and newcontents are opaque to walk_eraseregions, and - * need to be adjusted here to keep the impression of proper abstraction - */ - curcontents += start; - newcontents += start; - msg_cdbg(":"); - if (need_erase(curcontents, newcontents, len, gran)) { - msg_cdbg("E"); - ret = erasefn(flash, start, len); - if (ret) - return ret; - if (check_erased_range(flash, start, len)) { - msg_cerr("ERASE FAILED!\n"); - return -1; - } - /* Erase was successful. Adjust curcontents. */ - memset(curcontents, 0xff, len); - skip = 0; - } - /* get_next_write() sets starthere to a new value after the call. */ - while ((lenhere = get_next_write(curcontents + starthere, - newcontents + starthere, - len - starthere, &starthere, gran))) { - if (!writecount++) - msg_cdbg("W"); - /* Needs the partial write function signature. */ - ret = flash->chip->write(flash, newcontents + starthere, - start + starthere, lenhere); - if (ret) - return ret; - starthere += lenhere; - skip = 0; - } - if (skip) - msg_cdbg("S"); - else - all_skipped = false; - return ret; -} - -static int walk_eraseregions(struct flashctx *flash, int erasefunction, - int (*do_something) (struct flashctx *flash, - unsigned int addr, - unsigned int len, - uint8_t *param1, - uint8_t *param2, - int (*erasefn) ( - struct flashctx *flash, - unsigned int addr, - unsigned int len)), - void *param1, void *param2) -{ - int i, j; - unsigned int start = 0; - unsigned int len; - struct block_eraser eraser = flash->chip->block_erasers[erasefunction]; - - for (i = 0; i < NUM_ERASEREGIONS; i++) { - /* count==0 for all automatically initialized array - * members so the loop below won't be executed for them. - */ - len = eraser.eraseblocks[i].size; - for (j = 0; j < eraser.eraseblocks[i].count; j++) { - /* Print this for every block except the first one. */ - if (i || j) - msg_cdbg(", "); - msg_cdbg("0x%06x-0x%06x", start, - start + len - 1); - if (do_something(flash, start, len, param1, param2, - eraser.block_erase)) { - return 1; - } - start += len; - } - } - msg_cdbg("\n"); - return 0; -} - static int check_block_eraser(const struct flashctx *flash, int k, int log) { struct block_eraser eraser = flash->chip->block_erasers[k]; @@ -1584,71 +1494,6 @@ } // TODO: Once erase functions are annotated with allowed buses, check that as well. return 0; -} - -int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents) -{ - int k, ret = 1; - uint8_t *curcontents; - unsigned long size = flash->chip->total_size * 1024; - unsigned int usable_erasefunctions = count_usable_erasers(flash); - - msg_cinfo("Erasing and writing flash chip... "); - curcontents = malloc(size); - if (!curcontents) { - msg_gerr("Out of memory!\n"); - exit(1); - } - /* Copy oldcontents to curcontents to avoid clobbering oldcontents. */ - memcpy(curcontents, oldcontents, size); - - for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - if (k != 0) - msg_cinfo("Looking for another erase function.\n"); - if (!usable_erasefunctions) { - msg_cinfo("No usable erase functions left.\n"); - break; - } - msg_cdbg("Trying erase function %i... ", k); - if (check_block_eraser(flash, k, 1)) - continue; - usable_erasefunctions--; - ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, - curcontents, newcontents); - /* If everything is OK, don't try another erase function. */ - if (!ret) - break; - /* Write/erase failed, so try to find out what the current chip - * contents are. If no usable erase functions remain, we can - * skip this: the next iteration will break immediately anyway. - */ - if (!usable_erasefunctions) - continue; - /* Reading the whole chip may take a while, inform the user even - * in non-verbose mode. - */ - msg_cinfo("Reading current flash chip contents... "); - if (flash->chip->read(flash, curcontents, 0, size)) { - /* Now we are truly screwed. Read failed as well. */ - msg_cerr("Can't read anymore! Aborting.\n"); - /* We have no idea about the flash chip contents, so - * retrying with another erase function is pointless. - */ - break; - } - msg_cinfo("done. "); - } - /* Free the scratchpad. */ - free(curcontents); - - if (ret) { - msg_cerr("FAILED!\n"); - } else { - if (all_skipped) - msg_cinfo("\nWarning: Chip content is identical to the requested image.\n"); - msg_cinfo("Erase/write done.\n"); - } - return ret; }
static const struct flashrom_layout *get_layout(const struct flashctx *const flashctx) @@ -2323,170 +2168,6 @@ 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. - */ -int doit(struct flashctx *flash, int force, const char *filename, int read_it, - int write_it, int erase_it, int verify_it) -{ - uint8_t *oldcontents; - uint8_t *newcontents; - int ret = 0; - unsigned long size = flash->chip->total_size * 1024; - int read_all_first = 1; /* FIXME: Make this configurable. */ - - if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { - msg_cerr("Aborting.\n"); - return 1; - } - - if (normalize_romentries(flash)) { - msg_cerr("Requested regions can not be handled. Aborting.\n"); - return 1; - } - - /* Given the existence of read locks, we want to unlock for read, - * erase and write. - */ - if (flash->chip->unlock) - flash->chip->unlock(flash); - - if (read_it) { - return read_flash_to_file(flash, filename); - } - - oldcontents = malloc(size); - if (!oldcontents) { - msg_gerr("Out of memory!\n"); - exit(1); - } - /* Assume worst case: All bits are 0. */ - memset(oldcontents, 0x00, size); - newcontents = malloc(size); - if (!newcontents) { - msg_gerr("Out of memory!\n"); - exit(1); - } - /* Assume best case: All bits should be 1. */ - memset(newcontents, 0xff, size); - /* Side effect of the assumptions above: Default write action is erase - * because newcontents looks like a completely erased chip, and - * oldcontents being completely 0x00 means we have to erase everything - * before we can write. - */ - - if (erase_it) { - /* 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, - * so if the user wanted erase and reboots afterwards, the user - * knows very well that booting won't work. - */ - if (erase_and_write_flash(flash, oldcontents, newcontents)) { - emergency_help_message(); - ret = 1; - } - goto out; - } - - if (write_it || verify_it) { - if (read_buf_from_file(newcontents, size, filename)) { - ret = 1; - goto out; - } - -#if CONFIG_INTERNAL == 1 - if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, size) < 0) { - if (force_boardmismatch) { - msg_pinfo("Proceeding anyway because user forced us to.\n"); - } else { - msg_perr("Aborting. You can override this with " - "-p internal:boardmismatch=force.\n"); - ret = 1; - goto out; - } - } -#endif - } - - /* Read the whole chip to be able to check whether regions need to be - * erased and to give better diagnostics in case write fails. - * The alternative is to read only the regions which are to be - * preserved, but in that case we might perform unneeded erase which - * takes time as well. - */ - if (read_all_first) { - msg_cinfo("Reading old flash chip contents... "); - if (flash->chip->read(flash, oldcontents, 0, size)) { - ret = 1; - msg_cinfo("FAILED.\n"); - goto out; - } - } - msg_cinfo("done.\n"); - - /* Build a new image taking the given layout into account. */ - if (build_new_image(flash, read_all_first, oldcontents, newcontents)) { - msg_gerr("Could not prepare the data to be written, aborting.\n"); - ret = 1; - goto out; - } - - // //////////////////////////////////////////////////////////// - - if (write_it && erase_and_write_flash(flash, oldcontents, newcontents)) { - msg_cerr("Uh oh. Erase/write failed. "); - if (read_all_first) { - msg_cerr("Checking if anything has changed.\n"); - msg_cinfo("Reading current flash chip contents... "); - if (!flash->chip->read(flash, newcontents, 0, size)) { - msg_cinfo("done.\n"); - if (!memcmp(oldcontents, newcontents, size)) { - nonfatal_help_message(); - ret = 1; - goto out; - } - msg_cerr("Apparently at least some data has changed.\n"); - } else - msg_cerr("Can't even read anymore!\n"); - emergency_help_message(); - ret = 1; - goto out; - } else - msg_cerr("\n"); - emergency_help_message(); - ret = 1; - goto out; - } - - /* Verify only if we either did not try to write (verify operation) or actually changed something. */ - if (verify_it && (!write_it || !all_skipped)) { - msg_cinfo("Verifying flash... "); - - if (write_it) { - /* Work around chips which need some time to calm down. */ - programmer_delay(1000*1000); - ret = verify_range(flash, newcontents, 0, size); - /* If we tried to write, and verification now fails, we - * might have an emergency situation. - */ - if (ret) - emergency_help_message(); - } else { - ret = compare_range(newcontents, oldcontents, 0, size); - } - if (!ret) - msg_cinfo("VERIFIED.\n"); - } - -out: - free(oldcontents); - free(newcontents); - return ret; -} - -/** @private */ static int prepare_flash_access(struct flashctx *const flash, const bool read_it, const bool write_it, const bool erase_it, const bool verify_it) @@ -2512,7 +2193,6 @@ return 0; }
-/** @private */ static void finalize_flash_access(struct flashctx *const flash) { unmap_flash(flash); @@ -2783,3 +2463,74 @@ }
/** @} */ /* end flashrom-ops */ + +int do_read(struct flashctx *const flash, const char *const filename) +{ + if (prepare_flash_access(flash, true, false, false, false)) + return 1; + + const int ret = read_flash_to_file(flash, filename); + + finalize_flash_access(flash); + + return ret; +} + +int do_erase(struct flashctx *const flash) +{ + const int ret = flashrom_flash_erase(flash); + + /* + * 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, + * so if the user wanted erase and reboots afterwards, the user + * knows very well that booting won't work. + */ + if (ret) + emergency_help_message(); + + return ret; +} + +int do_write(struct flashctx *const flash, const char *const filename) +{ + const size_t flash_size = flash->chip->total_size * 1024; + int ret = 1; + + uint8_t *const newcontents = malloc(flash_size); + if (!newcontents) { + msg_gerr("Out of memory!\n"); + goto _free_ret; + } + + if (read_buf_from_file(newcontents, flash_size, filename)) + goto _free_ret; + + ret = flashrom_image_write(flash, newcontents, flash_size); + +_free_ret: + free(newcontents); + return ret; +} + +int do_verify(struct flashctx *const flash, const char *const filename) +{ + const size_t flash_size = flash->chip->total_size * 1024; + int ret = 1; + + uint8_t *const newcontents = malloc(flash_size); + if (!newcontents) { + msg_gerr("Out of memory!\n"); + goto _free_ret; + } + + if (read_buf_from_file(newcontents, flash_size, filename)) + goto _free_ret; + + ret = flashrom_image_verify(flash, newcontents, flash_size); + +_free_ret: + free(newcontents); + return ret; +} diff --git a/layout.c b/layout.c index 3d09011..2d53295 100644 --- a/layout.c +++ b/layout.c @@ -202,33 +202,6 @@ layout.num_entries = 0; }
-struct romentry *get_next_included_romentry(unsigned int start) -{ - int i; - unsigned int best_start = UINT_MAX; - struct romentry *best_entry = NULL; - struct romentry *cur; - - /* First come, first serve for overlapping regions. */ - for (i = 0; i < layout.num_entries; i++) { - cur = &layout.entries[i]; - if (!cur->included) - continue; - /* Already past the current entry? */ - if (start > cur->end) - continue; - /* Inside the current entry? */ - if (start >= cur->start) - return cur; - /* Entry begins after start. */ - if (best_start > cur->start) { - best_start = cur->start; - best_entry = cur; - } - } - return best_entry; -} - /* Validate and - if needed - normalize layout entries. */ int normalize_romentries(const struct flashctx *flash) { @@ -251,61 +224,4 @@ }
return ret; -} - -static int copy_old_content(struct flashctx *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents, unsigned int start, unsigned int size) -{ - if (!oldcontents_valid) { - /* oldcontents is a zero-filled buffer. By reading the current data into oldcontents here, we - * avoid a rewrite of identical regions even if an initial full chip read didn't happen. */ - msg_gdbg2("Read a chunk starting at 0x%06x (len=0x%06x).\n", start, size); - int ret = flash->chip->read(flash, oldcontents + start, start, size); - if (ret != 0) { - msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", start, start + size - 1); - return 1; - } - } - memcpy(newcontents + start, oldcontents + start, size); - return 0; -} - -/** - * Modify @newcontents so that it contains the data that should be on the chip eventually. In the case the user - * wants to update only parts of it, copy the chunks to be preserved from @oldcontents to @newcontents. If - * @oldcontents is not valid, we need to fetch the current data from the chip first. - */ -int build_new_image(struct flashctx *flash, bool oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents) -{ - unsigned int start = 0; - struct romentry *entry; - unsigned int size = flash->chip->total_size * 1024; - - /* If no regions were specified for inclusion, assume - * that the user wants to write the complete new image. - */ - if (num_include_args == 0) - return 0; - - /* Non-included romentries are ignored. - * The union of all included romentries is used from the new image. - */ - while (start < size) { - entry = get_next_included_romentry(start); - /* No more romentries for remaining region? */ - if (!entry) { - copy_old_content(flash, oldcontents_valid, oldcontents, newcontents, start, - size - start); - break; - } - /* For non-included region, copy from old content. */ - if (entry->start > start) - copy_old_content(flash, oldcontents_valid, oldcontents, newcontents, start, - entry->start - start); - /* Skip to location after current romentry. */ - start = entry->end + 1; - /* Catch overflow. */ - if (!start) - break; - } - return 0; }
-
Nico Huber (Code Review)