Signed-off-by: Alexander Couzens lynxis@fe80.eu --- Hi everyone,
I've rewritten read/write/verify functions to get romregions/layouts working in all cases. A write to a specific region required before a complete read of the chip. Because of this it wasn't possible to write to a specific region, when another region is locked (ME lockdown). And flashrom is faster now when writing to a rom region.
Best, lynxis
I've melt all my commits into a single one. A small commit version can be found at https://github.com/lynxis/flashrom/tree/v1/
cli_classic.c | 5 - flash.h | 13 +++ flashrom.c | 336 +++++++++++++++++++++++++++++++++++----------------------- layout.c | 23 ++-- 4 files changed, 228 insertions(+), 149 deletions(-)
diff --git a/cli_classic.c b/cli_classic.c index 8588881..613c7aa 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -368,11 +368,6 @@ int main(int argc, char *argv[]) ret = 1; goto out; } - if (layoutfile != NULL && !write_it) { - msg_gerr("Layout files are currently supported for write operations only.\n"); - ret = 1; - goto out; - }
if (process_include_args()) { ret = 1; diff --git a/flash.h b/flash.h index 03b26e7..ec85398 100644 --- a/flash.h +++ b/flash.h @@ -337,6 +337,19 @@ __attribute__((format(printf, 2, 3))); #define msg_cspew(...) print(MSG_SPEW, __VA_ARGS__) /* chip debug spew */
/* layout.c */ + +#define MAX_ROMLAYOUT 32 + +typedef struct { + chipoff_t start; + chipoff_t end; + unsigned int included; + char name[256]; +} romentry_t; + +extern romentry_t rom_entries[]; +extern int num_rom_entries; + int register_include_arg(char *name); int process_include_args(void); int read_romlayout(const char *name); diff --git a/flashrom.c b/flashrom.c index 9b82d4c..b2c59c2 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1434,45 +1434,67 @@ static int erase_and_write_block_helper(struct flashctx *flash, 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) +static int walk_eraseregions_offset_len( + 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 *oldcontent, + void *newcontent, + unsigned offset, + unsigned len) { int i, j; unsigned int start = 0; - unsigned int len; + unsigned int eraselen; 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++) { + eraselen = eraser.eraseblocks[i].size; + for (j = 0; j < eraser.eraseblocks[i].count; j++, start += eraselen) { + if (start < offset) + continue; + /* 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)) { + msg_cdbg("0x%06x-0x%06x", start, start + eraselen - 1); + if (do_something(flash, start, eraselen, oldcontent, newcontent, eraser.block_erase)) { return 1; } - start += len; } } msg_cdbg("\n"); return 0; }
+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 *oldcontent, + void *newcontent) +{ + return walk_eraseregions_offset_len(flash, erasefunction, do_something, oldcontent, newcontent, 0, flash->chip->total_size); +} + static int check_block_eraser(const struct flashctx *flash, int k, int log) { struct block_eraser eraser = flash->chip->block_erasers[k]; @@ -1562,7 +1584,7 @@ int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents, uint8_t } return ret; } - +/* FIXME: Unused! static void nonfatal_help_message(void) { msg_gerr("Good, writing to the flash chip apparently didn't do anything.\n"); @@ -1579,7 +1601,7 @@ static void nonfatal_help_message(void) "the programmer and the flash chip. If you think the error is caused by flashrom\n" "please report this on IRC at chat.freenode.net (channel #flashrom) or\n" "mail flashrom@flashrom.org, thanks!\n"); -} +} */
static void emergency_help_message(void) { @@ -1895,153 +1917,201 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int 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 read_flash(struct flashctx *flash, const char *filename) { int ret = 0; - unsigned long size = flash->chip->total_size * 1024; + int err = 0; /* used as ret for read_flash if a single partition fails */ + size_t flashsize = flash->chip->total_size * 1024; + int i;
- if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { - msg_cerr("Aborting.\n"); - return 1; - } + uint8_t *contents = calloc(1, flashsize);
- if (normalize_romentries(flash)) { - msg_cerr("Requested regions can not be handled. Aborting.\n"); - return 1; - } + for (i=0; i < num_rom_entries; i++) { + chipoff_t start = rom_entries[i].start; + chipoff_t end = rom_entries[i].end; + chipsize_t length = end - start;
- /* Given the existence of read locks, we want to unlock for read, - * erase and write. - */ - if (flash->chip->unlock) - flash->chip->unlock(flash); + /* ignore entries not included */ + if (!rom_entries[i].included) + continue;
- if (read_it) { - return read_flash_to_file(flash, filename); + ret = flash->chip->read(flash, contents + start, start, length); + if (ret) { + msg_cerr("Read for part %s (0x%x - 0x%x) failed!", rom_entries[i].name, start, end); + err = -1; + continue; + } }
- oldcontents = malloc(size); - if (!oldcontents) { - msg_gerr("Out of memory!\n"); - exit(1); + ret = write_buf_to_file(contents, flashsize, filename); + if (ret) { + msg_cerr("Writing to file %s failed!", filename); + err = -1; } + + free(contents); + return err; +} + +int write_flash(struct flashctx *flash, const char *filename) { + int ret = 0; + size_t flashsize = flash->chip->total_size * 1024; + int i; + + uint8_t *oldcontents = calloc(1, flashsize); + uint8_t *newcontents = calloc(1, flashsize); + /* 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); + memset(oldcontents, 0x00, flashsize); + + /* Assume worst case: All bits are 0. */ + memset(newcontents, 0xff, flashsize); /* 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) { + if(read_buf_from_file(newcontents, flashsize, filename)) { + msg_cerr("Can not read file %s\n. Aborting.\n", filename); + return -1; + } + +#if CONFIG_INTERNAL == 1 + /* FIXME: move this code into a small function and move it out of write_flash */ + if (programmer == PROGRAMMER_INTERNAL && cb_check_image(newcontents, flashsize) < 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"); + return 1; + } + } +#endif + + for (i=0; i < num_rom_entries; i++) { + chipoff_t start = rom_entries[i].start; + chipoff_t end = rom_entries[i].end; + chipsize_t length = end - start; + + /* ignore entries not included */ + if (!rom_entries[i].included) + continue; + + /* read specified flash region */ + ret = flash->chip->read(flash, oldcontents + start, start, length); + if(ret) { + msg_cerr("Can not read flash position 0x%x len: 0x%x\n. Ret: %d Ignoring.\n", start, length, ret); + return -1; + } + + int k; + for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { + if (k != 0) + msg_cinfo("Looking for another erase function.\n"); + + msg_cdbg("Trying erase function %i... ", k); + if (check_block_eraser(flash, k, 1)) + continue; + + ret = walk_eraseregions_offset_len(flash, k, &erase_and_write_block_helper, oldcontents, newcontents, start, length); + + /* If everything is OK, don't try another erase function. */ + if (!ret) + break; + } + + if (!ret) { + msg_cinfo("Finished flashing %d - region %s", ret, rom_entries[i].name); + } else { + msg_cerr("Failed flashing!"); + emergency_help_message(); /* 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; - } + return 0; +}
-#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 +int verify_flash(struct flashctx *flash, const char *filename) { + int ret = 0; + int verified = 0; + size_t flashsize = flash->chip->total_size * 1024; + int i; + + /* we alloc here the whole flashsize to be sure we have enought space */ + uint8_t *filecontents = calloc(1, flashsize); + + if(read_buf_from_file(filecontents, flashsize, filename)) { + msg_cerr("Can not read file %s\n. Aborting.\n", filename); + return -1; }
- /* 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 would be to read only the regions which are to be - * preserved, but in that case we might perform unneeded erase which - * takes time as well. - */ - msg_cinfo("Reading old flash chip contents... "); - if (flash->chip->read(flash, oldcontents, 0, size)) { - ret = 1; - msg_cinfo("FAILED.\n"); - goto out; + for (i=0; i < num_rom_entries; i++) { + chipoff_t start = rom_entries[i].start; + chipoff_t end = rom_entries[i].end; + chipsize_t length = end - start; + + /* ignore entries not included */ + if (!rom_entries[i].included) + continue; + + ret = verify_range(flash, filecontents + start, start, length); + if (ret) + verified = -1; } - msg_cinfo("done.\n");
- /* Build a new image taking the given layout into account. */ - build_new_image(flash, oldcontents, newcontents); + return verified; +} + +/* 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) +{ + + int ret = 0;
- // //////////////////////////////////////////////////////////// + if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { + msg_cerr("Aborting.\n"); + return 1; + }
- if (write_it) { - 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... "); - 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; - } + if (normalize_romentries(flash)) { + msg_cerr("Requested regions can not be handled. Aborting.\n"); + return 1; }
- /* 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... "); + /* Given the existence of read locks, we want to unlock for read, + * erase and write. + */ + if (flash->chip->unlock) + flash->chip->unlock(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"); + if (read_it) { + return read_flash(flash, filename); }
-out: - free(oldcontents); - free(newcontents); + if (write_it) { + ret = write_flash(flash, filename); + if (ret) + return ret; + } + + if (verify_it) { + ret = verify_flash(flash, filename); + if (ret) + return ret; + } + + msg_cinfo("done.\n"); + return ret; } diff --git a/layout.c b/layout.c index a12eb28..9bdf9b6 100644 --- a/layout.c +++ b/layout.c @@ -26,18 +26,9 @@ #include "flash.h" #include "programmer.h"
-#define MAX_ROMLAYOUT 32 - -typedef struct { - chipoff_t start; - chipoff_t end; - unsigned int included; - char name[256]; -} romentry_t; - /* rom_entries store the entries specified in a layout file and associated run-time data */ -static romentry_t rom_entries[MAX_ROMLAYOUT]; -static int num_rom_entries = 0; /* the number of successfully parsed rom_entries */ +romentry_t rom_entries[MAX_ROMLAYOUT]; +int num_rom_entries = 0; /* the number of successfully parsed rom_entries */
/* include_args holds the arguments specified at the command line with -i. They must be processed at some point * so that desired regions are marked as "included" in the rom_entries list. */ @@ -254,6 +245,16 @@ int normalize_romentries(const struct flashctx *flash) } }
+ /* create a partition over the complete flash when no layout given */ + if (num_rom_entries == 0) { + rom_entries[0].start = 0x0; + rom_entries[0].end = flash->chip->total_size * 1024 - 1; + rom_entries[0].included = 1; + strncpy(rom_entries[0].name, "complete flash", 15); + + num_rom_entries = 1; + } + return ret; }
On Sun, 12 Oct 2014 22:37:23 +0200 Alexander Couzens lynxis@fe80.eu wrote:
Signed-off-by: Alexander Couzens lynxis@fe80.eu
Hi everyone,
I've rewritten read/write/verify functions to get romregions/layouts working in all cases. A write to a specific region required before a complete read of the chip. Because of this it wasn't possible to write to a specific region, when another region is locked (ME lockdown). And flashrom is faster now when writing to a rom region.
Hi,
we have discussed this patch in Prague already but I was actually looking at it at last. It is a bit convoluted/big so maybe I have overlooked something, but I have not seen anything my patch set does not or worse than this. I'll rethink if your approach with the single layout region when no explicit region is given could improve my approach, thanks for that input. Apart from that I would rather improve my patch set where deemed necessary... if you have any additional inputs on my approach I'd love to hear them though!
-
Alexander Couzens -
Stefan Tauner