- Introduce a variable in doit() that allows to influence read-before-write and its consequences. - Rename handle_romentries to build_new_image and a description of its purpose. - Modify build_new_image so that it still works even if the old content is not read before.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
--- This is completely wrong.
When the user wants to skip most of the chip AND wants to avoid the safety net, it makes no sense at all to read most of the chip in build_new_image. This is needed because the write/skip logic works by comparing the old content with the new content.
In the case that old content is already known (because we have read the whole chip before), this is no problem and even nice from a architectural point of view. Allowing to skip the reading breaks the concept though. This patch provides the ugly code that is necessary so that this concept does not explode. It does not fix the concept itself. In the case the user wants to write the whole chip, this provides a real benefit. The smaller the region is that he wants to update, the small the benefit becomes.
The real solution would be to provide the erase and write logic not just a complete image, from which it can deduce what it can and should skip, but also a layout-like map of what the user wants to be changed (or something similar that allows the following). If the old contents are known, then fine, but if not, read only the parts needed due to erase block overheads and not everything unnecessarily. --- flash.h | 2 +- flashrom.c | 42 ++++++++++++++++++++++-------------------- layout.c | 42 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/flash.h b/flash.h index 4b1cca2..c3644ce 100644 --- a/flash.h +++ b/flash.h @@ -257,7 +257,7 @@ int cli_classic(int argc, char *argv[]); /* layout.c */ int read_romlayout(char *name); int find_romentry(char *name); -int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents); +int build_new_image(struct flashchip *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents);
/* spi.c */ struct spi_command { diff --git a/flashrom.c b/flashrom.c index 07cfdd9..b0e3b15 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1865,6 +1865,7 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it, uint8_t *newcontents; int ret = 0; unsigned long size = flash->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"); @@ -1931,28 +1932,27 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it,
/* 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 + * 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. */ - msg_cinfo("Reading old flash chip contents... "); - if (flash->read(flash, oldcontents, 0, size)) { - ret = 1; - msg_cinfo("FAILED.\n"); - goto out; + if (read_all_first) { + msg_cinfo("Reading old flash chip contents... "); + if (flash->read(flash, oldcontents, 0, size)) { + ret = 1; + msg_cinfo("FAILED.\n"); + goto out; + } } msg_cinfo("done.\n");
- // This should be moved into each flash part's code to do it - // cleanly. This does the job. - handle_romentries(flash, oldcontents, newcontents); + /* Build a new image from the given layout. */ + build_new_image(flash, read_all_first, oldcontents, newcontents);
- // //////////////////////////////////////////////////////////// - - if (write_it) { - if (erase_and_write_flash(flash, oldcontents, newcontents)) { - msg_cerr("Uh oh. Erase/write failed. Checking if " - "anything changed.\n"); + 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 changed... "); if (!flash->read(flash, newcontents, 0, size)) { if (!memcmp(oldcontents, newcontents, size)) { msg_cinfo("Good. It seems nothing was " @@ -1961,11 +1961,13 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it, ret = 1; goto out; } - } - emergency_help_message(); - ret = 1; - goto out; - } + } else + msg_cerr("failed.\n"); + } else + msg_cerr("\n"); + emergency_help_message(); + ret = 1; + goto out; }
if (verify_it) { diff --git a/layout.c b/layout.c index 6b9627a..8283e36 100644 --- a/layout.c +++ b/layout.c @@ -213,7 +213,7 @@ int find_romentry(char *name) return -1; }
-struct rom_entry *get_next_included_romentry(unsigned int start) +static struct rom_entry *get_next_included_romentry(unsigned int start) { int i; unsigned int best_start = UINT_MAX; @@ -240,11 +240,18 @@ struct rom_entry *get_next_included_romentry(unsigned int start) return best_entry; }
-int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) +/** + * 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 flashchip *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents) { unsigned int start = 0; struct rom_entry *entry; unsigned int size = flash->total_size * 1024; + unsigned int copy_size = 0;
/* If no layout file was specified or the layout file was empty, assume * that the user wants to flash the complete new image. @@ -258,13 +265,38 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne entry = get_next_included_romentry(start); /* No more romentries for remaining region? */ if (entry == NULL) { + copy_size = size - start; + if (!oldcontents_valid) { + /* oldcontents is a zero-filled buffer. By + * reading into oldcontents, we avoid a rewrite + * of identical regions even if an initial full + * chip read didn't happen. */ + msg_gdbg("reading last old chunk 0x%06x (0x%06x)\n", + start, copy_size); + flash->read(flash, oldcontents + start, + start, + copy_size); + } memcpy(newcontents + start, oldcontents + start, - size - start); + copy_size); break; } - if (entry->start > start) + if (entry->start > start) { + copy_size = entry->start - start; + if (!oldcontents_valid) { + /* oldcontents is a zero-filled buffer. By + * reading into oldcontents, we avoid a rewrite + * of identical regions even if an initial full + * chip read didn't happen. */ + msg_gdbg("reading some chunk 0x%06x (0x%06x)\n", + start, copy_size); + flash->read(flash, oldcontents + start, + start, + copy_size); + } memcpy(newcontents + start, oldcontents + start, - entry->start - start); + copy_size); + } /* Skip to location after current romentry. */ start = entry->end + 1; /* Catch overflow. */