- 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. - Add copy_old_content() to ease the pain for future patches.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
---
TODO: add UI
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 smaller 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 | 30 +++++++++++++++++++++++++----- 3 files changed, 48 insertions(+), 26 deletions(-)
diff --git a/flash.h b/flash.h index e51b6d4..6151e1c 100644 --- a/flash.h +++ b/flash.h @@ -292,7 +292,7 @@ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); int register_include_arg(char *name); int process_include_args(void); int read_romlayout(char *name); -int handle_romentries(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents); +int build_new_image(struct flashctx *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents);
/* spi.c */ struct spi_command { diff --git a/flashrom.c b/flashrom.c index f1a6165..708dba0 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1679,6 +1679,7 @@ int doit(struct flashctx *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"); @@ -1745,28 +1746,27 @@ int doit(struct flashctx *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 " @@ -1775,11 +1775,13 @@ int doit(struct flashctx *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 4bd1560..dac9dac 100644 --- a/layout.c +++ b/layout.c @@ -338,7 +338,26 @@ static int read_content_from_file(romlayout_t *entry, uint8_t *newcontents) return 0; }
-int handle_romentries(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents) +static void 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 into + * oldcontents, we avoid a rewrite of identical regions even if + * an initial full chip read didn't happen. */ + msg_gdbg2("Read a chunk starting from 0x%06x (len=0x%06x).\n", + start, size); + flash->read(flash, oldcontents + start, start, size); + } + memcpy(newcontents + start, oldcontents + start, size); +} + +/** + * 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, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents) { unsigned int start = 0; romlayout_t *entry; @@ -357,14 +376,15 @@ int handle_romentries(struct flashctx *flash, uint8_t *oldcontents, uint8_t *new entry = get_next_included_romentry(start); /* No more romentries for remaining region? */ if (!entry) { - memcpy(newcontents + start, oldcontents + start, - size - start); + copy_old_content(flash, oldcontents_valid, oldcontents, + newcontents, start, size - start); break; } /* For non-included region, copy from old content. */ if (entry->start > start) - memcpy(newcontents + start, oldcontents + start, - entry->start - start); + copy_old_content(flash, oldcontents_valid, oldcontents, + newcontents, start, + entry->start - start); /* For included region, copy from file if specified. */ if (read_content_from_file(entry, newcontents) != 0) return 1;