Always read the flash chip before writing. This will allow flashrom to skip erase of already-erased blocks and to skip write of blocks which already have the wanted contents.
Avoid emergency messages by checking if the chip contents after a failed write operation (erase/write) are unchanged.
Keep the emergency messages after a failed pure erase. That part is debatable because if someone wants erase, he pretty sure doesn't care about the flash contents anymore.
Please note that this introduces additional overhead of a full chip read before write. This is frowned upon by people with slow programmers. Maybe we should make this configurable. Either way, this patch needs a review to make sure I'm not going in the wrong direction.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-always_read_no_scary_warning/flash.h =================================================================== --- flashrom-always_read_no_scary_warning/flash.h (Revision 1212) +++ flashrom-always_read_no_scary_warning/flash.h (Arbeitskopie) @@ -239,7 +239,7 @@ /* layout.c */ int read_romlayout(char *name); int find_romentry(char *name); -int handle_romentries(uint8_t *buffer, struct flashchip *flash); +int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents);
/* spi.c */ struct spi_command { Index: flashrom-always_read_no_scary_warning/layout.c =================================================================== --- flashrom-always_read_no_scary_warning/layout.c (Revision 1212) +++ flashrom-always_read_no_scary_warning/layout.c (Arbeitskopie) @@ -205,7 +205,7 @@ return -1; }
-int handle_romentries(uint8_t *buffer, struct flashchip *flash) +int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { int i;
@@ -225,13 +225,12 @@ // normal will be updated and the rest will be kept.
for (i = 0; i < romimages; i++) { - if (rom_entries[i].included) continue;
- flash->read(flash, buffer + rom_entries[i].start, - rom_entries[i].start, - rom_entries[i].end - rom_entries[i].start + 1); + memcpy(newcontents + rom_entries[i].start, + oldcontents + rom_entries[i].start, + rom_entries[i].end - rom_entries[i].start + 1); }
return 0; Index: flashrom-always_read_no_scary_warning/flashrom.c =================================================================== --- flashrom-always_read_no_scary_warning/flashrom.c (Revision 1212) +++ flashrom-always_read_no_scary_warning/flashrom.c (Arbeitskopie) @@ -1343,6 +1343,19 @@ return ret; }
+void nonfatal_help_message(void) +{ + msg_gerr("Writing to the flash chip apparently didn't do anything.\n" + "This means we have to add special support for your board, " + "programmer or flash chip.\n" + "Please report this on IRC at irc.freenode.net (channel " + "#flashrom) or\n" + "mail flashrom@flashrom.org!\n" + "-------------------------------------------------------------" + "------------------\n" + "You may now reboot or simply leave the machine running.\n"); +} + void emergency_help_message(void) { msg_gerr("Your flash chip is in an unknown state.\n" @@ -1602,9 +1615,10 @@ */ int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) { - uint8_t *buf; + uint8_t *oldcontents; + uint8_t *newcontents; int ret = 0; - unsigned long size; + unsigned long size = flash->total_size * 1024;
if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) { msg_cerr("Aborting.\n"); @@ -1612,9 +1626,6 @@ return 1; }
- size = flash->total_size * 1024; - buf = (uint8_t *) calloc(size, sizeof(char)); - /* Given the existence of read locks, we want to unlock for read, * erase and write. */ @@ -1627,6 +1638,12 @@ return ret; } 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_flash(flash)) { emergency_help_message(); programmer_shutdown(); @@ -1634,33 +1651,71 @@ } }
+ newcontents = (uint8_t *) calloc(size, sizeof(char)); + if (write_it || verify_it) { - if (read_buf_from_file(buf, flash->total_size * 1024, filename)) { + if (read_buf_from_file(newcontents, size, filename)) { programmer_shutdown(); return 1; }
#if CONFIG_INTERNAL == 1 - show_id(buf, size, force); + if (programmer == PROGRAMMER_INTERNAL) + show_id(newcontents, size, force); #endif }
+ oldcontents = (uint8_t *) calloc(size, sizeof(char)); + + /* 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_cdbg("Reading old flash chip contents...\n"); + if (flash->read(flash, oldcontents, 0, size)) { + programmer_shutdown(); + return 1; + } + // This should be moved into each flash part's code to do it // cleanly. This does the job. - handle_romentries(buf, flash); + handle_romentries(flash, oldcontents, newcontents);
// ////////////////////////////////////////////////////////////
if (write_it) { if (erase_flash(flash)) { + msg_cerr("Uh oh. Erase failed. Checking if anything " + "changed.\n"); + if (!flash->read(flash, newcontents, 0, size)) { + if (!memcmp(oldcontents, newcontents, size)) { + msg_cinfo("Good. It seems nothing was " + "changed.\n"); + nonfatal_help_message(); + programmer_shutdown(); + return 1; + } + } emergency_help_message(); programmer_shutdown(); return 1; } msg_cinfo("Writing flash chip... "); - ret = flash->write(flash, buf, 0, flash->total_size * 1024); + ret = flash->write(flash, newcontents, 0, size); if (ret) { - msg_cerr("FAILED!\n"); + msg_cerr("Uh oh. Write failed. Checking if anything " + "changed.\n"); + if (!flash->read(flash, newcontents, 0, size)) { + if (!memcmp(oldcontents, newcontents, size)) { + msg_cinfo("Good. It seems nothing was " + "changed.\n"); + nonfatal_help_message(); + programmer_shutdown(); + return 1; + } + } emergency_help_message(); programmer_shutdown(); return 1; @@ -1673,7 +1728,7 @@ /* Work around chips which need some time to calm down. */ if (write_it) programmer_delay(1000*1000); - ret = verify_flash(flash, buf); + ret = verify_flash(flash, newcontents); /* If we tried to write, and verification now fails, we * might have an emergency situation. */
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [101015 15:22]:
Always read the flash chip before writing. This will allow flashrom to skip erase of already-erased blocks and to skip write of blocks which already have the wanted contents.
Avoid emergency messages by checking if the chip contents after a failed write operation (erase/write) are unchanged.
Keep the emergency messages after a failed pure erase. That part is debatable because if someone wants erase, he pretty sure doesn't care about the flash contents anymore.
Please note that this introduces additional overhead of a full chip read before write. This is frowned upon by people with slow programmers. Maybe we should make this configurable. Either way, this patch needs a review to make sure I'm not going in the wrong direction.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Nice! We should indeed make this configurable.
Acked-by: Stefan Reinauer stepan@coreboot.org
Index: flashrom-always_read_no_scary_warning/flashrom.c
--- flashrom-always_read_no_scary_warning/flashrom.c (Revision 1212) +++ flashrom-always_read_no_scary_warning/flashrom.c (Arbeitskopie) @@ -1343,6 +1343,19 @@ return ret; }
+void nonfatal_help_message(void) +{
- msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
"This means we have to add special support for your board, ""programmer or flash chip.\n""Please report this on IRC at irc.freenode.net (channel ""#flashrom) or\n""mail flashrom@flashrom.org!\n""-------------------------------------------------------------""------------------\n""You may now reboot or simply leave the machine running.\n");+}
Since we are reading anyways now, it would be very nice to check whether the image to write is different from the image that was just read and not attempt any write at all in this case. (Possibly with a message a la "Trying to write identical image. Skipped"
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.*/
We certainly want to know when an erase failed, though.
}}
- newcontents = (uint8_t *) calloc(size, sizeof(char));
Maybe using malloc and memset would increase the readability at that point. On first glance it was somewhat unclear why calloc was used with byte sized members.
- /* 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_cdbg("Reading old flash chip contents...\n");
- if (flash->read(flash, oldcontents, 0, size)) {
programmer_shutdown();return 1;- }
So what should the "skip the read" option be called? --unsafe?
msg_cerr("Uh oh. Write failed. Checking if anything ""changed.\n");
Should the tool really say "Uh oh."? ;-)
Good stuff!
Best regards, Stefan
On 19.10.2010 00:49, Stefan Reinauer wrote:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [101015 15:22]:
Always read the flash chip before writing. This will allow flashrom to skip erase of already-erased blocks and to skip write of blocks which already have the wanted contents.
Avoid emergency messages by checking if the chip contents after a failed write operation (erase/write) are unchanged.
Keep the emergency messages after a failed pure erase. That part is debatable because if someone wants erase, he pretty sure doesn't care about the flash contents anymore.
Please note that this introduces additional overhead of a full chip read before write. This is frowned upon by people with slow programmers. Maybe we should make this configurable. Either way, this patch needs a review to make sure I'm not going in the wrong direction.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Nice! We should indeed make this configurable.
Acked-by: Stefan Reinauer stepan@coreboot.org
Thanks, committed in r1215.
Index: flashrom-always_read_no_scary_warning/flashrom.c
--- flashrom-always_read_no_scary_warning/flashrom.c (Revision 1212) +++ flashrom-always_read_no_scary_warning/flashrom.c (Arbeitskopie) @@ -1343,6 +1343,19 @@ return ret; }
+void nonfatal_help_message(void) +{
- msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
"This means we have to add special support for your board, ""programmer or flash chip.\n""Please report this on IRC at irc.freenode.net (channel ""#flashrom) or\n""mail flashrom@flashrom.org!\n""-------------------------------------------------------------""------------------\n""You may now reboot or simply leave the machine running.\n");+}
Since we are reading anyways now, it would be very nice to check whether the image to write is different from the image that was just read and not attempt any write at all in this case. (Possibly with a message a la "Trying to write identical image. Skipped"
Will come in a followup patch which starts real partial 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.*/We certainly want to know when an erase failed, though.
Yes, that's why the code still complains. Maybe I should reword the comment. We want to know that the erase failed, but I think there is no need for additional info on top of that (i.e. if anything changed).
}}
- newcontents = (uint8_t *) calloc(size, sizeof(char));
Maybe using malloc and memset would increase the readability at that point. On first glance it was somewhat unclear why calloc was used with byte sized members.
I want kzalloc(size_t size). It's what the Linux kernel uses, and it make sense.
- /* 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_cdbg("Reading old flash chip contents...\n");
- if (flash->read(flash, oldcontents, 0, size)) {
programmer_shutdown();return 1;- }
So what should the "skip the read" option be called? --unsafe?
I'll probably make this dependent on --verify or so. We'll see. Maybe --verify-level=2.
msg_cerr("Uh oh. Write failed. Checking if anything ""changed.\n");Should the tool really say "Uh oh."? ;-)
Yup, it sounds appropriate if you say it out loud.
Good stuff!
Thanks!
Regards, Carl-Daniel
Overall the patch looks good and didn't cause any failures with some testing when I ran thru a basic set of read / erase / write / partial write tests.
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Always read the flash chip before writing. This will allow flashrom to skip erase of already-erased blocks and to skip write of blocks which already have the wanted contents.
Good idea in general IMHO, however as you mention a config option would be nice.
Also, perhaps oldcontents should be written out if verification fails. That could wait for a follow-up patch.
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
-int handle_romentries(uint8_t *buffer, struct flashchip *flash) +int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { int i;
@@ -225,13 +225,12 @@ // normal will be updated and the rest will be kept.
for (i = 0; i < romimages; i++) {
if (rom_entries[i].included) continue;flash->read(flash, buffer + rom_entries[i].start,rom_entries[i].start,rom_entries[i].end - rom_entries[i].start + 1);
memcpy(newcontents + rom_entries[i].start,oldcontents + rom_entries[i].start,rom_entries[i].end - rom_entries[i].start + 1); }
As discussed on IRC, this function can probably use a better name and comments... in another patch :-)
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
@@ -1634,33 +1651,71 @@ } }
newcontents = (uint8_t *) calloc(size, sizeof(char));
+1 on Stefan's comment here... malloc / memset is probably considered better form by some. But that's just a matter of preference.
On 19.10.2010 02:59, David Hendricks wrote:
Overall the patch looks good and didn't cause any failures with some testing when I ran thru a basic set of read / erase / write / partial write tests.
Thanks for testing.
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger wrote:
Always read the flash chip before writing. This will allow flashrom to skip erase of already-erased blocks and to skip write of blocks which already have the wanted contents.
Good idea in general IMHO, however as you mention a config option would be nice.
We can decide later about adding it to the command line. The patch below is intended to provide the infrastructure, and then we can hook it up in a subsequent patch.
Also, perhaps oldcontents should be written out if verification fails. That could wait for a follow-up patch.
I'm not really sure that is a good idea. If anything, the user probably expects flashrom to recover the thing automatically, and that's actually pretty easy with partial write. Just call write again with the old chip contents.
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger wrote:
-int handle_romentries(uint8_t *buffer, struct flashchip *flash) +int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { int i;
As discussed on IRC, this function can probably use a better name and comments... in another patch :-)
See below.
On Fri, Oct 15, 2010 at 6:22 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
@@ -1634,33 +1651,71 @@ } }
newcontents = (uint8_t *) calloc(size, sizeof(char));+1 on Stefan's comment here... malloc / memset is probably considered better form by some. But that's just a matter of preference.
I find malloc/memset to be a bit unwieldy because there's the danger of forgetting the memset. Maybe I'll just use kzalloc from the Linux kernel.
Make reading the whole chip before write configurable.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-initial_read_configurable/flash.h =================================================================== --- flashrom-initial_read_configurable/flash.h (Revision 1215) +++ flashrom-initial_read_configurable/flash.h (Arbeitskopie) @@ -239,7 +239,7 @@ /* 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 { Index: flashrom-initial_read_configurable/layout.c =================================================================== --- flashrom-initial_read_configurable/layout.c (Revision 1215) +++ flashrom-initial_read_configurable/layout.c (Arbeitskopie) @@ -33,15 +33,13 @@
#define MAX_ROMLAYOUT 16
-typedef struct { +static struct { unsigned int start; unsigned int end; unsigned int included; char name[256]; -} romlayout_t; +} rom_entries[MAX_ROMLAYOUT];
-static romlayout_t rom_entries[MAX_ROMLAYOUT]; - #if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */ static char *def_name = "DEFAULT";
@@ -205,7 +203,7 @@ return -1; }
-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) { int i;
@@ -225,12 +223,24 @@ // normal will be updated and the rest will be kept.
for (i = 0; i < romimages; i++) { + unsigned int romentry_len; + if (rom_entries[i].included) continue;
- memcpy(newcontents + rom_entries[i].start, + romentry_len = rom_entries[i].end - rom_entries[i].start + 1; + 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. + */ + flash->read(flash, oldcontents + rom_entries[i].start, + rom_entries[i].start, + romentry_len); + } + memcpy(newcontents + rom_entries[i].start, oldcontents + rom_entries[i].start, - rom_entries[i].end - rom_entries[i].start + 1); + romentry_len); }
return 0; Index: flashrom-initial_read_configurable/flashrom.c =================================================================== --- flashrom-initial_read_configurable/flashrom.c (Revision 1215) +++ flashrom-initial_read_configurable/flashrom.c (Arbeitskopie) @@ -1619,6 +1619,7 @@ 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, filename, read_it, write_it, erase_it, verify_it)) { msg_cerr("Aborting.\n"); @@ -1669,34 +1670,37 @@
/* 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_cdbg("Reading old flash chip contents...\n"); - if (flash->read(flash, oldcontents, 0, size)) { - programmer_shutdown(); - return 1; + if (read_all_first) { + msg_cdbg("Reading old flash chip contents...\n"); + if (flash->read(flash, oldcontents, 0, size)) { + programmer_shutdown(); + return 1; + } }
- // 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_flash(flash)) { - msg_cerr("Uh oh. Erase failed. Checking if anything " - "changed.\n"); - if (!flash->read(flash, newcontents, 0, size)) { - if (!memcmp(oldcontents, newcontents, size)) { - msg_cinfo("Good. It seems nothing was " - "changed.\n"); - nonfatal_help_message(); - programmer_shutdown(); - return 1; + msg_cerr("Uh oh. Erase failed. "); + if (read_all_first) { + msg_cerr("Checking if anything changed.\n"); + if (!flash->read(flash, newcontents, 0, size)) { + if (!memcmp(oldcontents, newcontents, size)) { + msg_cinfo("Good. It seems nothing was " + "changed.\n"); + nonfatal_help_message(); + programmer_shutdown(); + return 1; + } } + } else { + msg_cerr("\n"); } emergency_help_message(); programmer_shutdown(); @@ -1705,16 +1709,20 @@ msg_cinfo("Writing flash chip... "); ret = flash->write(flash, newcontents, 0, size); if (ret) { - msg_cerr("Uh oh. Write failed. Checking if anything " - "changed.\n"); - if (!flash->read(flash, newcontents, 0, size)) { - if (!memcmp(oldcontents, newcontents, size)) { - msg_cinfo("Good. It seems nothing was " - "changed.\n"); - nonfatal_help_message(); - programmer_shutdown(); - return 1; + msg_cerr("Uh oh. Write failed. "); + if (read_all_first) { + msg_cerr("Checking if anything changed.\n"); + if (!flash->read(flash, newcontents, 0, size)) { + if (!memcmp(oldcontents, newcontents, size)) { + msg_cinfo("Good. It seems nothing was " + "changed.\n"); + nonfatal_help_message(); + programmer_shutdown(); + return 1; + } } + } else { + msg_cerr("\n"); } emergency_help_message(); programmer_shutdown();
This is a rebased and enhanced version of carl-daniel's patch: http://www.flashrom.org/pipermail/flashrom/2010-October/005202.html
Stefan Tauner (3): layout: change return type and name of find_next_included_romentry Make read before write configurable (infrastructure part) an obvious but dubios refactor opportunity
flash.h | 2 +- flashrom.c | 42 ++++++++++++++++++++------------------ layout.c | 66 +++++++++++++++++++++++++++++++++++++++-------------------- 3 files changed, 66 insertions(+), 44 deletions(-)
- rename from find_next_included_romentry to get_next_included_romentry - return a pointer to a rom_entry instead of just its index
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- layout.c | 38 +++++++++++++++++++------------------- 1 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/layout.c b/layout.c index d719a05..6b9627a 100644 --- a/layout.c +++ b/layout.c @@ -34,14 +34,12 @@ static int romimages = 0;
#define MAX_ROMLAYOUT 32
-typedef struct { +static struct rom_entry { unsigned int start; unsigned int end; unsigned int included; char name[256]; -} romlayout_t; - -static romlayout_t rom_entries[MAX_ROMLAYOUT]; +} rom_entries[MAX_ROMLAYOUT];
#if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */ static char *def_name = "DEFAULT"; @@ -215,26 +213,28 @@ int find_romentry(char *name) return -1; }
-int find_next_included_romentry(unsigned int start) +struct rom_entry *get_next_included_romentry(unsigned int start) { int i; unsigned int best_start = UINT_MAX; - int best_entry = -1; + struct rom_entry *best_entry = NULL; + struct rom_entry *cur;
/* First come, first serve for overlapping regions. */ for (i = 0; i < romimages; i++) { - if (!rom_entries[i].included) + cur = &rom_entries[i]; + if (!cur->included) continue; /* Already past the current entry? */ - if (start > rom_entries[i].end) + if (start > cur->end) continue; /* Inside the current entry? */ - if (start >= rom_entries[i].start) - return i; + if (start >= cur->start) + return cur; /* Entry begins after start. */ - if (best_start > rom_entries[i].start) { - best_start = rom_entries[i].start; - best_entry = i; + if (best_start > cur->start) { + best_start = cur->start; + best_entry = cur; } } return best_entry; @@ -243,7 +243,7 @@ int find_next_included_romentry(unsigned int start) int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { unsigned int start = 0; - int entry; + struct rom_entry *entry; unsigned int size = flash->total_size * 1024;
/* If no layout file was specified or the layout file was empty, assume @@ -255,18 +255,18 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne * The union of all included romentries is used from the new image. */ while (start < size) { - entry = find_next_included_romentry(start); + entry = get_next_included_romentry(start); /* No more romentries for remaining region? */ - if (entry < 0) { + if (entry == NULL) { memcpy(newcontents + start, oldcontents + start, size - start); break; } - if (rom_entries[entry].start > start) + if (entry->start > start) memcpy(newcontents + start, oldcontents + start, - rom_entries[entry].start - start); + entry->start - start); /* Skip to location after current romentry. */ - start = rom_entries[entry].end + 1; + start = entry->end + 1; /* Catch overflow. */ if (!start) break;
- 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. */
it probably makes sense, even if the code savings are minimal, because of readability. i split it out so that you can decide. as i explained in the previous patch i hope it wont stay long anyway ;)
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- layout.c | 50 +++++++++++++++++++------------------------------- 1 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/layout.c b/layout.c index 8283e36..fe29852 100644 --- a/layout.c +++ b/layout.c @@ -240,6 +240,19 @@ static struct rom_entry *get_next_included_romentry(unsigned int start) return best_entry; }
+static void copy_old_content(struct flashchip *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 @@ -251,7 +264,6 @@ int build_new_image(struct flashchip *flash, int oldcontents_valid, uint8_t *old 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. @@ -265,38 +277,14 @@ int build_new_image(struct flashchip *flash, int oldcontents_valid, uint8_t *old 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, - copy_size); + copy_old_content(flash, oldcontents_valid, oldcontents, + newcontents, start, size - start); break; } - 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, - copy_size); - } + 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. */
-
Carl-Daniel Hailfinger -
David Hendricks -
Stefan Reinauer -
Stefan Tauner