[flashrom] [PATCH 4/7] Make read before write configurable (infrastructure part)

[Stefan Tauner]

 - Introduce a variable in doit() that allows to influence
   read-before-write and its consequences.
 - Rename handle_romentries to build_new_image and a description of
   its purpose.
 - Modify build_new_image so that it still works even if the old content
   is not read before.
 - Add copy_old_content() to ease the pain for future patches.

Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

---

TODO: add UI

This is completely wrong.

When the user wants to skip most of the chip AND wants to avoid the
safety net, it makes no sense at all to read most of the chip in
build_new_image. This is needed because the write/skip logic works by
comparing the old content with the new content.

In the case that old content is already known (because we have read
the whole chip before), this is no problem and even nice from a
architectural point of view. Allowing to skip the reading breaks the
concept though. This patch provides the ugly code that is necessary
so that this concept does not explode. It does not fix the concept
itself. In the case the user wants to write the whole chip, this
provides a real benefit. The smaller the region is that he wants to
update, the smaller the benefit becomes.

The real solution would be to provide the erase and write logic not
just a complete image, from which it can deduce what it can and
should skip, but also a layout-like map of what the user wants to be
changed (or something similar that allows the following). If the old
contents are known, then fine, but if not, read only the parts
needed due to erase block overheads and not everything unnecessarily.
---
 flash.h    |    2 +-
 flashrom.c |   42 ++++++++++++++++++++++--------------------
 layout.c   |   30 +++++++++++++++++++++++++-----
 3 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/flash.h b/flash.h
index f2988bc..eb5f144 100644
--- a/flash.h
+++ b/flash.h
@@ -258,7 +258,7 @@ int cli_classic(int argc, char *argv[]);
 int register_include_arg(char *name);
 int process_include_args(void);
 int read_romlayout(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 {
diff --git a/flashrom.c b/flashrom.c
index ad1e042..564d815 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1871,6 +1871,7 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it,
 	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, read_it, write_it, erase_it, verify_it)) {
 		msg_cerr("Aborting.\n");
@@ -1937,28 +1938,27 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it,
 
 	/* 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_cinfo("Reading old flash chip contents... ");
-	if (flash->read(flash, oldcontents, 0, size)) {
-		ret = 1;
-		msg_cinfo("FAILED.\n");
-		goto out;
+	if (read_all_first) {
+		msg_cinfo("Reading old flash chip contents... ");
+		if (flash->read(flash, oldcontents, 0, size)) {
+			ret = 1;
+			msg_cinfo("FAILED.\n");
+			goto out;
+		}
 	}
 	msg_cinfo("done.\n");
 
-	// 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_and_write_flash(flash, oldcontents, newcontents)) {
-			msg_cerr("Uh oh. Erase/write failed. Checking if "
-				 "anything changed.\n");
+	if (write_it && erase_and_write_flash(flash, oldcontents, newcontents)) {
+		msg_cerr("Uh oh. Erase/write failed.");
+		if (read_all_first) {
+			msg_cerr("Checking if anything changed... ");
 			if (!flash->read(flash, newcontents, 0, size)) {
 				if (!memcmp(oldcontents, newcontents, size)) {
 					msg_cinfo("Good. It seems nothing was "
@@ -1967,11 +1967,13 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it,
 					ret = 1;
 					goto out;
 				}
-			}
-			emergency_help_message();
-			ret = 1;
-			goto out;
-		}
+			} else
+				msg_cerr("failed.\n");
+		} else
+			msg_cerr("\n");
+		emergency_help_message();
+		ret = 1;
+		goto out;
 	}
 
 	if (verify_it) {
diff --git a/layout.c b/layout.c
index f5764c5..2e386d6 100644
--- a/layout.c
+++ b/layout.c
@@ -338,7 +338,26 @@ static int read_content_from_file(romlayout_t *entry, uint8_t *newcontents)
 	return 0;
 }
 
-int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
+static void copy_old_content(struct flashchip *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents, unsigned int start, unsigned int size)
+{
+	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. */
+		msg_gdbg2("Read a chunk starting from 0x%06x (len=0x%06x).\n",
+			  start, size);
+		flash->read(flash, oldcontents + start, start, size);
+	}
+	memcpy(newcontents + start, oldcontents + start, size);
+}
+
+/**
+ * Modify @newcontents so that it contains the data that should be on the chip
+ * eventually. In the case the user wants to update only parts of it, copy
+ * the chunks to be preserved from @oldcontents to @newcontents. If @oldcontents
+ * is not valid, we need to fetch the current data from the chip first.
+ */
+int build_new_image(struct flashchip *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents)
 {
 	unsigned int start = 0;
 	romlayout_t *entry;
@@ -357,14 +376,15 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne
 		entry = get_next_included_romentry(start);
 		/* No more romentries for remaining region? */
 		if (entry == NULL) {
-			memcpy(newcontents + start, oldcontents + start,
-			       size - start);
+			copy_old_content(flash, oldcontents_valid, oldcontents,
+					 newcontents, start, size - start);
 			break;
 		}
 		/* For non-included region, copy from old content. */
 		if (entry->start > start)
-			memcpy(newcontents + start, oldcontents + start,
-			       entry->start - start);
+			copy_old_content(flash, oldcontents_valid, oldcontents,
+					 newcontents, start,
+					 entry->start - start);
 		/* For included region, copy from file if specified. */
 		if (read_content_from_file(entry, newcontents) < 0)
 			return 1;
-- 
1.7.1