[flashrom] [PATCH] Real partial writes
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Oct 21 02:08:41 CEST 2010
On 21.10.2010 02:00, Andrew Morgan wrote:
> On 20/10/10 05:06, Carl-Daniel Hailfinger wrote:
>> If you're going to test this, please be advised that I have not tested
>> execution, only compilation. Testing erase (and checking with a separate
>> readback that erase actually worked) and write (same test with separate
>> readback) would be highly appreciated. Verbose logs are even more
>> appreciated.
> Write seems work, but erase on its own doesn't. Some blocks aren't
> erased: (good = erased, bad = not erased)
>
> 0x000000-0x000fff, 0x001000-0x001fff, good
> 0x002000-0x002fff, bad
> 0x003000-0x003fff, 0x004000-0x004fff, 0x005000-0x005fff, good
> 0x006000-0x006fff, bad
> 0x007000-0x007fff, good
> 0x008000-0x008fff, bad
I suspect that there are more bugs in the code than I'd like. Please try
the patch below which has more debug output.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Index: flashrom-partial_write_rolling_erase_write/flashrom.c
===================================================================
--- flashrom-partial_write_rolling_erase_write/flashrom.c (Revision 1215)
+++ flashrom-partial_write_rolling_erase_write/flashrom.c (Arbeitskopie)
@@ -793,6 +793,7 @@
* Check if the buffer @have can be programmed to the content of @want without
* erasing. This is only possible if all chunks of size @gran are either kept
* as-is or changed from an all-ones state to any other state.
+ *
* The following write granularities (enum @gran) are known:
* - 1 bit. Each bit can be cleared individually.
* - 1 byte. A byte can be written once. Further writes to an already written
@@ -803,10 +804,12 @@
* this function.
* - 256 bytes. If less than 256 bytes are written, the contents of the
* unwritten bytes are undefined.
+ * Warning: This function assumes that @have and @want point to naturally
+ * aligned regions.
*
* @have buffer with current content
* @want buffer with desired content
- * @len length of the verified area
+ * @len length of the checked area
* @gran write granularity (enum, not count)
* @return 0 if no erase is needed, 1 otherwise
*/
@@ -846,10 +849,102 @@
break;
}
break;
+ default:
+ msg_cerr("%s: Unsupported granularity! Please report a bug at "
+ "flashrom at flashrom.org\n", __func__);
}
return result;
}
+/**
+ * Check if the buffer @have needs to be programmed to get the content of @want.
+ * If yes, return 1 and fill in first_start with the start address of the
+ * write operation and first_len with the length of the first to-be-written
+ * chunk. If not, return 0 and leave first_start and first_len undefined.
+ *
+ * Warning: This function assumes that @have and @want point to naturally
+ * aligned regions.
+ *
+ * @have buffer with current content
+ * @want buffer with desired content
+ * @len length of the checked area
+ * @gran write granularity (enum, not count)
+ * @return 0 if no write is needed, 1 otherwise
+ * @first_start offset of the first byte which needs to be written
+ * @first_len length of the first contiguous area which needs to be written
+ *
+ * FIXME: This function needs a parameter which tells it about coalescing
+ * in relation to the max write length of the programmer and the max write
+ * length of the chip.
+ */
+static int get_next_write(uint8_t *have, uint8_t *want, int len,
+ int *first_start, int *first_len,
+ enum write_granularity gran)
+{
+ int result = 0;
+ int i, j, limit;
+
+ /* The passed in variable might be uninitialized. */
+ *first_len = 0;
+ switch (gran) {
+ case write_gran_1bit:
+ case write_gran_1byte:
+ for (i = 0; i < len; i++) {
+ if (have[i] != want[i]) {
+ if (!result) {
+ /* First location where have and want
+ * differ.
+ */
+ result = 1;
+ *first_start = i;
+ }
+ } else {
+ if (result) {
+ /* First location where have and want
+ * do not differ anymore.
+ */
+ *first_len = i - *first_start;
+ break;
+ }
+ }
+ }
+ /* Did the loop terminate without setting first_len? */
+ if (result && ! *first_len)
+ *first_len = i - *first_start;
+ break;
+ case write_gran_256bytes:
+ for (j = 0; j < len / 256; j++) {
+ limit = min(256, len - j * 256);
+ /* Are 'have' and 'want' identical? */
+ if (memcmp(have + j * 256, want + j * 256, limit)) {
+ if (!result) {
+ /* First location where have and want
+ * differ.
+ */
+ result = 1;
+ *first_start = j * 256;
+ }
+ } else {
+ if (result) {
+ /* First location where have and want
+ * do not differ anymore.
+ */
+ *first_len = j * 256 - *first_start;
+ break;
+ }
+ }
+ }
+ /* Did the loop terminate without setting first_len? */
+ if (result && ! *first_len)
+ *first_len = min(j * 256 - *first_start, len);
+ break;
+ default:
+ msg_cerr("%s: Unsupported granularity! Please report a bug at "
+ "flashrom at flashrom.org\n", __func__);
+ }
+ return result;
+}
+
/* This function generates various test patterns useful for testing controller
* and chip communication as well as chip behaviour.
*
@@ -1203,7 +1298,8 @@
return ret;
}
-/* This function shares a lot of its structure with erase_flash().
+/* This function shares a lot of its structure with erase_and_write_flash() and
+ * walk_eraseregions().
* Even if an error is found, the function will keep going and check the rest.
*/
static int selfcheck_eraseblocks(struct flashchip *flash)
@@ -1271,10 +1367,59 @@
return ret;
}
+static int erase_and_write_block_helper(struct flashchip *flash,
+ unsigned int start, unsigned int len,
+ uint8_t *oldcontents,
+ uint8_t *newcontents,
+ int (*erasefn) (struct flashchip *flash,
+ unsigned int addr,
+ unsigned int len))
+{
+ int starthere = 0;
+ int lenhere = 0;
+ int ret = 0;
+ enum write_granularity gran = write_gran_256bytes; /* FIXME */
+
+ /* oldcontents and newcontents are opaque to walk_eraseregions, and
+ * need to be adjusted here to keep the impression of proper abstraction
+ */
+ oldcontents += start;
+ newcontents += start;
+ /* FIXME: Assume 256 byte granularity for now to play it safe. */
+ if (need_erase(oldcontents, newcontents, len, gran)) {
+ msg_cdbg(" E");
+ ret = erasefn(flash, start, len);
+ if (ret)
+ return ret;
+ /* Erase was successful. Adjust oldcontents. */
+ memset(oldcontents + start, 0xff, len);
+ } else
+ msg_cdbg(" e");
+ while (get_next_write(oldcontents + starthere,
+ newcontents + starthere,
+ len - starthere, &starthere,
+ &lenhere, gran)) {
+ msg_cdbg("W");
+ /* Needs the partial write function signature. */
+ ret = flash->write(flash, newcontents + starthere, start + starthere, lenhere);
+ if (ret)
+ break;
+ starthere += lenhere;
+ }
+ return ret;
+}
+
static int walk_eraseregions(struct flashchip *flash, int erasefunction,
int (*do_something) (struct flashchip *flash,
unsigned int addr,
- unsigned int len))
+ unsigned int len,
+ uint8_t *param1,
+ uint8_t *param2,
+ int (*erasefn) (
+ struct flashchip *flash,
+ unsigned int addr,
+ unsigned int len)),
+ void *param1, void *param2)
{
int i, j;
unsigned int start = 0;
@@ -1286,21 +1431,22 @@
*/
len = eraser.eraseblocks[i].size;
for (j = 0; j < eraser.eraseblocks[i].count; j++) {
- msg_cdbg("0x%06x-0x%06x, ", start,
+ msg_cdbg("0x%06x-0x%06x", start,
start + len - 1);
- if (do_something(flash, start, len))
+ if (do_something(flash, start, len, param1, param2, eraser.block_erase))
return 1;
+ msg_cdbg(", ");
start += len;
}
}
return 0;
}
-int erase_flash(struct flashchip *flash)
+int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
{
int k, ret = 0, found = 0;
- msg_cinfo("Erasing flash chip... ");
+ msg_cinfo("Erasing and writing flash chip... ");
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
struct block_eraser eraser = flash->block_erasers[k];
@@ -1324,7 +1470,7 @@
}
found = 1;
msg_cdbg("trying... ");
- ret = walk_eraseregions(flash, k, eraser.block_erase);
+ ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, oldcontents, newcontents);
msg_cdbg("\n");
/* If everything is OK, don't try another erase function. */
if (!ret)
@@ -1637,6 +1783,19 @@
programmer_shutdown();
return ret;
}
+
+ oldcontents = (uint8_t *) malloc(size);
+ /* Assume worst case: All bits are 0. */
+ memset(oldcontents, 0x00, size);
+ newcontents = (uint8_t *) malloc(size);
+ /* Assume best case: All bits should be 1. */
+ memset(newcontents, 0xff, size);
+ /* Side effect of the assumptions above: Default write action is erase
+ * because newcontents looks like a completely erased chip, and
+ * oldcontents being completely 0x00 means we have to erase everything
+ * before we can 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
@@ -1644,15 +1803,13 @@
* so if the user wanted erase and reboots afterwards, the user
* knows very well that booting won't work.
*/
- if (erase_flash(flash)) {
+ if (erase_and_write_flash(flash, oldcontents, newcontents)) {
emergency_help_message();
programmer_shutdown();
return 1;
}
}
- newcontents = (uint8_t *) calloc(size, sizeof(char));
-
if (write_it || verify_it) {
if (read_buf_from_file(newcontents, size, filename)) {
programmer_shutdown();
@@ -1665,8 +1822,6 @@
#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
@@ -1686,9 +1841,9 @@
// ////////////////////////////////////////////////////////////
if (write_it) {
- if (erase_flash(flash)) {
- msg_cerr("Uh oh. Erase failed. Checking if anything "
- "changed.\n");
+ if (erase_and_write_flash(flash, oldcontents, newcontents)) {
+ msg_cerr("Uh oh. Erase/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 "
@@ -1701,24 +1856,6 @@
emergency_help_message();
programmer_shutdown();
return 1;
- }
- 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;
- }
- }
- emergency_help_message();
- programmer_shutdown();
- return 1;
} else {
msg_cinfo("COMPLETE.\n");
}
--
http://www.hailfinger.org/
More information about the flashrom
mailing list