Index: flashrom-partial_write_rolling_erase_write/flashrom.c

20 Oct 2010

If you're going to review this, make sure you keep a stack of bananas
(quickly mobilized carbohydrates for your brain), a bucket of ice (to
prevent brain overheating) and a bottle of aspirin handy. If any code is
unclear, please tell me and I'll try to add comments to improve readability.
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.
This patch makes flashrom use real partial writes. If you write an image
full of 0xff, flashrom will erase and detect that no write is needed. If
you write an image which differs only in some parts from the current
flash contents, flashrom will detect that and not touch unchanged areas.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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@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,58 @@
    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)) {
+		ret = erasefn(flash, start, len);
+		if (ret)
+			return ret;
+		/* Erase was successful. Adjust oldcontents. */
+		memset(oldcontents + start, 0xff, len);
+	}
+	while (get_next_write(oldcontents + starthere,
+			      newcontents + starthere,
+			      len - starthere, &starthere,
+			      &lenhere, gran)) {
+		/* 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;
@@ -1288,7 +1432,7 @@
    	for (j = 0; j < eraser.eraseblocks[i].count; j++) {
    		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;
    		start += len;
    	}
@@ -1296,11 +1440,11 @@
    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 +1468,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 +1781,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 +1801,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 +1820,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 +1839,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 +1854,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/



    

[PATCH] Real partial writes

Index: flashrom-partial_write_rolling_erase_write/flashrom.c

Index: flashrom-partial_write_rolling_erase_write/flashrom.c

Index: flashrom-partial_write_rolling_erase_write/flashrom.c

Index: flashrom-partial_write_rolling_erase_write/flashrom.c