[flashrom] [PATCH] Handle erase failure in partial write, clean up

[Carl-Daniel Hailfinger]

Handle erase failure in partial write.
Clean up erase function checking.
Update a few comments and messages to improve readability.

The erase failure handling is a genuine bugfix which is needed on locked
down chipsets and in the unlikely case that write fails halfway through
or an incorrect chip definition is used.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: flashrom-cleanup_erase_and_write/flashrom.c
===================================================================
--- flashrom-cleanup_erase_and_write/flashrom.c	(Revision 1236)
+++ flashrom-cleanup_erase_and_write/flashrom.c	(Arbeitskopie)
@@ -1429,57 +1429,84 @@
 	return 0;
 }
 
+static int check_block_eraser(struct flashchip *flash, int k, int log)
+{
+	struct block_eraser eraser = flash->block_erasers[k];
+
+	if (log)
+		msg_cdbg("Looking at blockwise erase function %i... ", k);
+	if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
+		if (log)
+			msg_cdbg("not defined. ");
+		return 1;
+	}
+	if (!eraser.block_erase && eraser.eraseblocks[0].count) {
+		if (log)
+			msg_cdbg("eraseblock layout is known, but no "
+				"matching block erase function found. ");
+		return 1;
+	}
+	if (eraser.block_erase && !eraser.eraseblocks[0].count) {
+		if (log)
+			msg_cdbg("block erase function found, but "
+				"eraseblock layout is unknown. ");
+		return 1;
+	}
+	return 0;
+}
+
 int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
 {
-	int k, ret = 0, found = 0;
+	int k, ret = 0;
 	uint8_t *curcontents;
 	unsigned long size = flash->total_size * 1024;
+	int usable_erasefunctions = 0;
 
+	for (k = 0; k < NUM_ERASEFUNCTIONS; k++)
+		if (!check_block_eraser(flash, k, 0))
+			usable_erasefunctions++;
+	msg_cinfo("Erasing and writing flash chip... ");
+	if (!usable_erasefunctions) {
+		msg_cerr("ERROR: flashrom has no erase function for this flash "
+			 "chip.\n");
+		return 1;
+	}
+
 	curcontents = (uint8_t *) malloc(size);
 	/* Copy oldcontents to curcontents to avoid clobbering oldcontents. */
 	memcpy(curcontents, oldcontents, size);
 
-	msg_cinfo("Erasing and writing flash chip... ");
 	for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
-		struct block_eraser eraser = flash->block_erasers[k];
-
-		msg_cdbg("Looking at blockwise erase function %i... ", k);
-		if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
-			msg_cdbg("not defined. "
-				"Looking for another erase function.\n");
+		if (check_block_eraser(flash, k, 1) && usable_erasefunctions) {
+			msg_cdbg("Looking for another erase function.\n");
 			continue;
 		}
-		if (!eraser.block_erase && eraser.eraseblocks[0].count) {
-			msg_cdbg("eraseblock layout is known, but no "
-				"matching block erase function found. "
-				"Looking for another erase function.\n");
-			continue;
-		}
-		if (eraser.block_erase && !eraser.eraseblocks[0].count) {
-			msg_cdbg("block erase function found, but "
-				"eraseblock layout is unknown. "
-				"Looking for another erase function.\n");
-			continue;
-		}
-		found = 1;
+		usable_erasefunctions--;
 		msg_cdbg("trying... ");
 		ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents);
 		msg_cdbg("\n");
 		/* If everything is OK, don't try another erase function. */
 		if (!ret)
 			break;
-		/* FIXME: Reread the whole chip here so we know the current
-		 * chip contents? curcontents might be up to date, but this
-		 * code is only reached if something failed, and then we don't
-		 * know exactly what failed, and how.
+		/* Write/erase failed, so try to find out what the current chip
+		 * contents are. If no usable erase functions remain, we could
+		 * abort the loop instead of continuing, the effect is the same.
+		 * The only difference is whether the reason for other unusable
+		 * functions is printed or not. If in doubt, verbosity wins.
 		 */
+		if (!usable_erasefunctions)
+			continue;
+		if (flash->read(flash, curcontents, 0, size)) {
+			/* Now we are truly screwed. Read failed as well. */
+			msg_cerr("Can't read anymore!\n");
+			/* We have no idea about the flash chip contents, so
+			 * retrying with another erase function is pointless.
+			 */
+			break;
+		}
 	}
 	/* Free the scratchpad. */
 	free(curcontents);
-	if (!found) {
-		msg_cerr("ERROR: flashrom has no erase function for this flash chip.\n");
-		return 1;
-	}
 
 	if (ret) {
 		msg_cerr("FAILED!\n");
@@ -1827,18 +1854,15 @@
 	 * preserved, but in that case we might perform unneeded erase which
 	 * takes time as well.
 	 */
-	msg_cdbg("Reading old flash chip contents...\n");
+	msg_cdbg("Reading current flash chip contents...\n");
 	if (flash->read(flash, oldcontents, 0, size)) {
 		ret = 1;
 		goto out;
 	}
 
-	// This should be moved into each flash part's code to do it 
-	// cleanly. This does the job.
+	/* Build a new image from the given layout. */
 	handle_romentries(flash, oldcontents, newcontents);
 
-	// ////////////////////////////////////////////////////////////
-
 	if (write_it) {
 		if (erase_and_write_flash(flash, oldcontents, newcontents)) {
 			msg_cerr("Uh oh. Erase/write failed. Checking if "


-- 
http://www.hailfinger.org/