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();