2010/10/22 Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Given that the version which addresses the review comments apparently is<br>
buggy, I hereby propose to merge the earlier version which worked. It is<br>
here again for your reference with some slight cosmetic fixes which<br>
should not impact functionality.<br>
Error recovery still needs work (see the FIXME comment). It will work in<br>
most cases, but if a given erase command for a block only erased parts<br>
of the block (partial write lock), the code will make incorrect assumptions.<br>
<div class="im"><br>
If anyone feels adventurous, I would love to see logs on Intel/VIA<br>
chipsets with SPI (preferably locked down chipsets or with r1193 reverted).<br>
<br>
</div><div class="im">If you're going to review this, make sure you keep a stack of bananas<br>
(quickly mobilized carbohydrates for your brain), a bucket of ice (to<br>
prevent brain overheating) and a bottle of aspirin handy. If any code is<br>
unclear, please tell me and I'll try to add comments to improve readability.<br>
<br>
This code has been tested. Testing erase (and checking with a separate<br>
readback that erase actually worked) and write (same test with separate<br>
readback) would be highly appreciated. Verbose logs are even more<br>
appreciated.<br>
<br>
I think the code is ready for merge if you trust write/erase to never<br>
fail. The error cases still need to be tested. Should we reread the<br>
whole chip if write/erase failed to make sure our current view of the<br>
chip contents is not stale?<br>
<br>
This patch makes flashrom use real partial writes. If you write an image<br>
full of 0xff, flashrom will erase and detect that no write is needed. If<br>
you write an image which differs only in some parts from the current<br>
flash contents, flashrom will detect that and not touch unchanged areas.<br>
<br>
Fix a long-standing bug in need_erase() for 256 byte granularity as well.<br>
<br>
Nice side benefit: Detailed progress printing.<br>
S means skipped<br>
E means erased<br>
W means written<br>
<br>
</div>Thanks to Andrew Morgan for testing countless iterations of this patch.<br>
<div class="im">Thanks to Richard A. Smith for testing on Dediprog.<br>
</div>Thanks to David Hendricks for the review (will be addressed later).<br>
<div><div></div><div class="h5"><br>
Signed-off-by: Carl-Daniel Hailfinger <<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>><br></div></div></blockquote><div> </div><div>With your latest patch (<a href="http://patchwork.coreboot.org/patch/2160/">http://patchwork.coreboot.org/patch/2160/</a>) applied:<br>
<br>$ sudo ./flashrom -p nicintel_spi -V -w nicintel_spi.rom -c M25P10.RES<br>flashrom v0.9.3-r1215 on FreeBSD 8.1-RELEASE (i386), built with libpci 3.1.7, GCC 4.2.1 20070719 [FreeBSD], little endian<br>flashrom is free software, get the source code at <a href="http://www.flashrom.org">http://www.flashrom.org</a><br>
<br>Calibrating delay loop... OS timer resolution is 4 usecs, 942M loops per second, delay more than 10% too short (got 80% of expected delay), recalculating... 1923M loops per second, 10 myus = 11 us, 100 myus = 138 us, 1000 myus = 981 us, 10000 myus = 10947 us, 16 myus = 33 us, OK.<br>
Initializing nicintel_spi programmer<br>Found "Intel 82541PI Gigabit Ethernet Controller" (8086:107c, BDF 01:03.0).<br>Requested BAR is MEM, 32bit, not prefetchable<br>Probing for ST M25P10.RES, 128 KB: probe_spi_res1: id 0x10<br>
Chip status register is 00<br>Found chip "ST M25P10.RES" (128 KB, SPI) at physical address 0xfffe0000.<br>===<br>This flash part has status UNTESTED for operations: PROBE READ ERASE WRITE<br>The test status of this chip may have been updated in the latest development<br>
version of flashrom. If you are running the latest development version,<br>please email a report to <a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a> if any of the above operations<br>work correctly for you with this flash part. Please include the flashrom<br>
output with the additional -V option for all operations you tested (-V, -Vr,<br>-Vw, -VE), and mention which mainboard or programmer you tested.<br>Please mention your board in the subject line. Thanks for your help!<br>Reading old flash chip contents...<br>
Erasing and writing flash chip... Looking at blockwise erase function 0... trying... 0x000000-0x007fff:W, 0x008000-0x00ffff:W, 0x010000-0x017fff:W, 0x018000-0x01ffff:S<br><br>Done.<br>Verifying flash... VERIFY FAILED at 0x00008000! Expected=0xc6, Read=0x15, failed byte count from 0x00000000-0x0001ffff: 0x8d75<br>
Your flash chip is in an unknown state.<br>Get help on IRC at <a href="http://irc.freenode.net">irc.freenode.net</a> (channel #flashrom) or<br>mail <a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a> with FAILED: your board name in the subject line!<br>
-------------------------------------------------------------------------------<br>DO NOT REBOOT OR POWEROFF! <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div class="h5">
<br>
Index: flashrom-partial_write_rolling_erase_write/flashrom.c<br>
===================================================================<br>
--- flashrom-partial_write_rolling_erase_write/flashrom.c (Revision 1215)<br>
+++ flashrom-partial_write_rolling_erase_write/flashrom.c (Arbeitskopie)<br>
@@ -793,6 +793,7 @@<br>
* Check if the buffer @have can be programmed to the content of @want without<br>
* erasing. This is only possible if all chunks of size @gran are either kept<br>
* as-is or changed from an all-ones state to any other state.<br>
+ *<br>
* The following write granularities (enum @gran) are known:<br>
* - 1 bit. Each bit can be cleared individually.<br>
* - 1 byte. A byte can be written once. Further writes to an already written<br>
@@ -803,10 +804,12 @@<br>
* this function.<br>
* - 256 bytes. If less than 256 bytes are written, the contents of the<br>
* unwritten bytes are undefined.<br>
+ * Warning: This function assumes that @have and @want point to naturally<br>
+ * aligned regions.<br>
*<br>
* @have buffer with current content<br>
* @want buffer with desired content<br>
- * @len length of the verified area<br>
+ * @len length of the checked area<br>
* @gran write granularity (enum, not count)<br>
* @return 0 if no erase is needed, 1 otherwise<br>
*/<br>
@@ -838,7 +841,7 @@<br>
continue;<br>
/* have needs to be in erased state. */<br>
for (i = 0; i < limit; i++)<br>
- if (have[i] != 0xff) {<br>
+ if (have[j * 256 + i] != 0xff) {<br>
result = 1;<br>
break;<br>
}<br>
</div></div>@@ -846,10 +849,102 @@<br>
<div class="im"> break;<br>
}<br>
break;<br>
+ default:<br>
+ msg_cerr("%s: Unsupported granularity! Please report a bug at "<br>
+ "<a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a>\n", __func__);<br>
}<br>
return result;<br>
}<br>
<br>
+/**<br>
+ * Check if the buffer @have needs to be programmed to get the content of @want.<br>
+ * If yes, return 1 and fill in first_start with the start address of the<br>
+ * write operation and first_len with the length of the first to-be-written<br>
+ * chunk. If not, return 0 and leave first_start and first_len undefined.<br>
+ *<br>
+ * Warning: This function assumes that @have and @want point to naturally<br>
+ * aligned regions.<br>
+ *<br>
+ * @have buffer with current content<br>
+ * @want buffer with desired content<br>
+ * @len length of the checked area<br>
+ * @gran write granularity (enum, not count)<br>
</div>+ * @return 0 if no write is needed, 1 otherwise<br>
<div class="im">+ * @first_start offset of the first byte which needs to be written<br>
</div>+ * @first_len length of the first contiguous area which needs to be written<br>
<div class="im">+ *<br>
+ * FIXME: This function needs a parameter which tells it about coalescing<br>
+ * in relation to the max write length of the programmer and the max write<br>
+ * length of the chip.<br>
+ */<br>
+static int get_next_write(uint8_t *have, uint8_t *want, int len,<br>
</div>+ int *first_start, int *first_len,<br>
+ enum write_granularity gran)<br>
+{<br>
+ int result = 0;<br>
+ int i, limit;<br>
<div class="im">+<br>
+ /* The passed in variable might be uninitialized. */<br>
+ *first_len = 0;<br>
</div><div class="im">+ switch (gran) {<br>
+ case write_gran_1bit:<br>
+ case write_gran_1byte:<br>
</div><div class="im">+ for (i = 0; i < len; i++) {<br>
+ if (have[i] != want[i]) {<br>
+ if (!result) {<br>
</div><div class="im">+ /* First location where have and want<br>
+ * differ.<br>
</div><div class="im">+ */<br>
+ result = 1;<br>
+ *first_start = i;<br>
+ }<br>
+ } else {<br>
+ if (result) {<br>
</div><div class="im">+ /* First location where have and want<br>
+ * do not differ anymore.<br>
+ */<br>
</div>+ *first_len = i - *first_start;<br>
<div class="im">+ break;<br>
+ }<br>
+ }<br>
+ }<br>
+ /* Did the loop terminate without setting first_len? */<br>
</div><div class="im">+ if (result && ! *first_len)<br>
+ *first_len = i - *first_start;<br>
</div><div class="im">+ break;<br>
+ case write_gran_256bytes:<br>
</div>+ for (i = 0; i < len / 256; i++) {<br>
+ limit = min(256, len - i * 256);<br>
<div class="im">+ /* Are 'have' and 'want' identical? */<br>
</div>+ if (memcmp(have + i * 256, want + i * 256, limit)) {<br>
+ if (!result) {<br>
<div class="im">+ /* First location where have and want<br>
+ * differ.<br>
</div>+ */<br>
+ result = 1;<br>
+ *first_start = i * 256;<br>
+ }<br>
+ } else {<br>
+ if (result) {<br>
<div class="im">+ /* First location where have and want<br>
+ * do not differ anymore.<br>
+ */<br>
</div>+ *first_len = i * 256 - *first_start;<br>
<div class="im">+ break;<br>
+ }<br>
+ }<br>
+ }<br>
+ /* Did the loop terminate without setting first_len? */<br>
</div>+ if (result && ! *first_len)<br>
+ *first_len = min(i * 256 - *first_start, len);<br>
+ break;<br>
<div class="im">+ default:<br>
+ msg_cerr("%s: Unsupported granularity! Please report a bug at "<br>
+ "<a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a>\n", __func__);<br>
+ }<br>
+ return result;<br>
+}<br>
</div><div class="im">+<br>
/* This function generates various test patterns useful for testing controller<br>
* and chip communication as well as chip behaviour.<br>
*<br>
</div>@@ -1203,7 +1298,8 @@<br>
<div class="im"> return ret;<br>
}<br>
<br>
-/* This function shares a lot of its structure with erase_flash().<br>
+/* This function shares a lot of its structure with erase_and_write_flash() and<br>
+ * walk_eraseregions().<br>
* Even if an error is found, the function will keep going and check the rest.<br>
*/<br>
static int selfcheck_eraseblocks(struct flashchip *flash)<br>
</div>@@ -1271,10 +1367,67 @@<br>
<div><div></div><div class="h5"> return ret;<br>
}<br>
<br>
+static int erase_and_write_block_helper(struct flashchip *flash,<br>
+ unsigned int start, unsigned int len,<br>
+ uint8_t *oldcontents,<br>
+ uint8_t *newcontents,<br>
+ int (*erasefn) (struct flashchip *flash,<br>
+ unsigned int addr,<br>
+ unsigned int len))<br>
+{<br>
+ int starthere = 0;<br>
+ int lenhere = 0;<br>
+ int ret = 0;<br>
+ int skip = 1;<br>
+ int writecount = 0;<br>
+ enum write_granularity gran = write_gran_256bytes; /* FIXME */<br>
+<br>
+ /* oldcontents and newcontents are opaque to walk_eraseregions, and<br>
+ * need to be adjusted here to keep the impression of proper abstraction<br>
+ */<br>
+ oldcontents += start;<br>
+ newcontents += start;<br>
+ msg_cdbg(":");<br>
+ /* FIXME: Assume 256 byte granularity for now to play it safe. */<br>
+ if (need_erase(oldcontents, newcontents, len, gran)) {<br>
+ msg_cdbg("E");<br>
+ ret = erasefn(flash, start, len);<br>
+ if (ret)<br>
+ return ret;<br>
+ /* Erase was successful. Adjust oldcontents. */<br>
+ memset(oldcontents, 0xff, len);<br>
+ skip = 0;<br>
+ }<br>
</div></div>+ while (get_next_write(oldcontents + starthere,<br>
<div class="im">+ newcontents + starthere,<br>
+ len - starthere, &starthere,<br>
</div>+ &lenhere, gran)) {<br>
<div><div></div><div class="h5">+ if (!writecount++)<br>
+ msg_cdbg("W");<br>
+ /* Needs the partial write function signature. */<br>
+ ret = flash->write(flash, newcontents + starthere,<br>
+ start + starthere, lenhere);<br>
+ if (ret)<br>
+ return ret;<br>
+ starthere += lenhere;<br>
+ skip = 0;<br>
+ }<br>
+ if (skip)<br>
+ msg_cdbg("S");<br>
+ return ret;<br>
+}<br>
+<br>
static int walk_eraseregions(struct flashchip *flash, int erasefunction,<br>
int (*do_something) (struct flashchip *flash,<br>
unsigned int addr,<br>
- unsigned int len))<br>
+ unsigned int len,<br>
+ uint8_t *param1,<br>
+ uint8_t *param2,<br>
+ int (*erasefn) (<br>
+ struct flashchip *flash,<br>
+ unsigned int addr,<br>
+ unsigned int len)),<br>
+ void *param1, void *param2)<br>
{<br>
int i, j;<br>
unsigned int start = 0;<br>
</div></div>@@ -1286,21 +1439,34 @@<br>
<div><div></div><div class="h5"> */<br>
len = eraser.eraseblocks[i].size;<br>
for (j = 0; j < eraser.eraseblocks[i].count; j++) {<br>
- msg_cdbg("0x%06x-0x%06x, ", start,<br>
+ /* Print this for every block except the first one. */<br>
+ if (i || j)<br>
+ msg_cdbg(", ");<br>
+ msg_cdbg("0x%06x-0x%06x", start,<br>
start + len - 1);<br>
- if (do_something(flash, start, len))<br>
+ if (do_something(flash, start, len, param1, param2,<br>
+ eraser.block_erase)) {<br>
+ msg_cdbg("\n");<br>
return 1;<br>
+ }<br>
start += len;<br>
}<br>
}<br>
+ msg_cdbg("\n");<br>
return 0;<br>
}<br>
<br>
-int erase_flash(struct flashchip *flash)<br>
+int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)<br>
{<br>
int k, ret = 0, found = 0;<br>
+ uint8_t *curcontents;<br>
+ unsigned long size = flash->total_size * 1024;<br>
<br>
- msg_cinfo("Erasing flash chip... ");<br>
+ curcontents = (uint8_t *) malloc(size);<br>
+ /* Copy oldcontents to curcontents to avoid clobbering oldcontents. */<br>
+ memcpy(curcontents, oldcontents, size);<br>
+<br>
+ msg_cinfo("Erasing and writing flash chip... ");<br>
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {<br>
struct block_eraser eraser = flash->block_erasers[k];<br>
<br>
</div></div>@@ -1324,12 +1490,19 @@<br>
<div class="im"> }<br>
found = 1;<br>
msg_cdbg("trying... ");<br>
- ret = walk_eraseregions(flash, k, eraser.block_erase);<br>
+ ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents);<br>
msg_cdbg("\n");<br>
/* If everything is OK, don't try another erase function. */<br>
if (!ret)<br>
break;<br>
</div>+ /* FIXME: Reread the whole chip here so we know the current<br>
+ * chip contents? curcontents might be up to date, but this<br>
+ * code is only reached if something failed, and then we don't<br>
+ * know exactly what failed, and how.<br>
+ */<br>
<div class="im"> }<br>
+ /* Free the scratchpad. */<br>
+ free(curcontents);<br>
if (!found) {<br>
msg_cerr("ERROR: flashrom has no erase function for this flash chip.\n");<br>
return 1;<br>
</div>@@ -1338,7 +1511,7 @@<br>
<div class="im"> if (ret) {<br>
msg_cerr("FAILED!\n");<br>
} else {<br>
- msg_cinfo("SUCCESS.\n");<br>
+ msg_cinfo("Done.\n");<br>
}<br>
return ret;<br>
}<br>
</div>@@ -1637,6 +1810,19 @@<br>
<div class="im"> programmer_shutdown();<br>
return ret;<br>
}<br>
+<br>
+ oldcontents = (uint8_t *) malloc(size);<br>
+ /* Assume worst case: All bits are 0. */<br>
+ memset(oldcontents, 0x00, size);<br>
+ newcontents = (uint8_t *) malloc(size);<br>
+ /* Assume best case: All bits should be 1. */<br>
+ memset(newcontents, 0xff, size);<br>
+ /* Side effect of the assumptions above: Default write action is erase<br>
+ * because newcontents looks like a completely erased chip, and<br>
+ * oldcontents being completely 0x00 means we have to erase everything<br>
+ * before we can write.<br>
+ */<br>
+<br>
if (erase_it) {<br>
/* FIXME: Do we really want the scary warning if erase failed?<br>
* After all, after erase the chip is either blank or partially<br>
</div>@@ -1644,15 +1830,14 @@<br>
<div class="im"> * so if the user wanted erase and reboots afterwards, the user<br>
* knows very well that booting won't work.<br>
*/<br>
- if (erase_flash(flash)) {<br>
+ if (erase_and_write_flash(flash, oldcontents, newcontents)) {<br>
emergency_help_message();<br>
- programmer_shutdown();<br>
- return 1;<br>
+ ret = 1;<br>
}<br>
+ programmer_shutdown();<br>
+ return ret;<br>
}<br>
<br>
- newcontents = (uint8_t *) calloc(size, sizeof(char));<br>
-<br>
if (write_it || verify_it) {<br>
if (read_buf_from_file(newcontents, size, filename)) {<br>
programmer_shutdown();<br>
</div>@@ -1665,8 +1850,6 @@<br>
<div class="im"> #endif<br>
}<br>
<br>
- oldcontents = (uint8_t *) calloc(size, sizeof(char));<br>
-<br>
/* Read the whole chip to be able to check whether regions need to be<br>
* erased and to give better diagnostics in case write fails.<br>
* The alternative would be to read only the regions which are to be<br>
</div>@@ -1686,9 +1869,9 @@<br>
<div class="im"> // ////////////////////////////////////////////////////////////<br>
<br>
if (write_it) {<br>
- if (erase_flash(flash)) {<br>
- msg_cerr("Uh oh. Erase failed. Checking if anything "<br>
- "changed.\n");<br>
+ if (erase_and_write_flash(flash, oldcontents, newcontents)) {<br>
+ msg_cerr("Uh oh. Erase/write failed. Checking if "<br>
+ "anything changed.\n");<br>
if (!flash->read(flash, newcontents, 0, size)) {<br>
if (!memcmp(oldcontents, newcontents, size)) {<br>
msg_cinfo("Good. It seems nothing was "<br>
</div>@@ -1702,26 +1885,6 @@<br>
<div><div></div><div class="h5"> programmer_shutdown();<br>
return 1;<br>
}<br>
- msg_cinfo("Writing flash chip... ");<br>
- ret = flash->write(flash, newcontents, 0, size);<br>
- if (ret) {<br>
- msg_cerr("Uh oh. Write failed. Checking if anything "<br>
- "changed.\n");<br>
- if (!flash->read(flash, newcontents, 0, size)) {<br>
- if (!memcmp(oldcontents, newcontents, size)) {<br>
- msg_cinfo("Good. It seems nothing was "<br>
- "changed.\n");<br>
- nonfatal_help_message();<br>
- programmer_shutdown();<br>
- return 1;<br>
- }<br>
- }<br>
- emergency_help_message();<br>
- programmer_shutdown();<br>
- return 1;<br>
- } else {<br>
- msg_cinfo("COMPLETE.\n");<br>
- }<br>
}<br>
<br>
if (verify_it) {<br>
<br>
<br>
--<br>
<a href="http://www.hailfinger.org/" target="_blank">http://www.hailfinger.org/</a><br>
<br>
<br>
_______________________________________________<br>
flashrom mailing list<br>
<a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a><br>
<a href="http://www.flashrom.org/mailman/listinfo/flashrom" target="_blank">http://www.flashrom.org/mailman/listinfo/flashrom</a><br>
</div></div></blockquote></div><br>