[PATCH] Avoid scary warning if nothing changed

[Carl-Daniel Hailfinger]

If flashrom encounters an erase or write error, it will complain loudly
and tell the user that the flash chip is in an unknown state. Most of
the time the erase/write failed completely, and this case can be
detected by always reading the full chip before any write action and
comparing the new contents against the old contents after any failed
erase/write.

This should spit out a much more harmless warning if write/erase failed,
but the flash chip is completely unmodified.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-always_read_no_scary_warning/flash.h
===================================================================
--- flashrom-always_read_no_scary_warning/flash.h	(Revision 1023)
+++ flashrom-always_read_no_scary_warning/flash.h	(Arbeitskopie)
@@ -562,7 +562,7 @@
 int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len);
 int erase_flash(struct flashchip *flash);
 struct flashchip *probe_flash(struct flashchip *first_flash, int force);
-int read_flash(struct flashchip *flash, char *filename);
+int read_flash_write_file(struct flashchip *flash, char *filename);
 void check_chip_supported(struct flashchip *flash);
 int check_max_decode(enum chipbustype buses, uint32_t size);
 int min(int a, int b);
Index: flashrom-always_read_no_scary_warning/cli_classic.c
===================================================================
--- flashrom-always_read_no_scary_warning/cli_classic.c	(Revision 1023)
+++ flashrom-always_read_no_scary_warning/cli_classic.c	(Arbeitskopie)
@@ -419,7 +419,7 @@
    			exit(1);
    		}
    		printf("Please note that forced reads most likely contain garbage.\n");
-			return read_flash(flashes[0], filename);
+			return read_flash_write_file(flashes[0], filename);
    	}
    	// FIXME: flash writes stay enabled!
    	programmer_shutdown();
Index: flashrom-always_read_no_scary_warning/flashrom.c
===================================================================
--- flashrom-always_read_no_scary_warning/flashrom.c	(Revision 1023)
+++ flashrom-always_read_no_scary_warning/flashrom.c	(Arbeitskopie)
@@ -1003,12 +1003,10 @@
    return ret;
 }

-int read_flash(struct flashchip *flash, char *filename)
+int write_buf_to_file(unsigned char *buf, unsigned long size, char *filename)
 {
    unsigned long numbytes;
    FILE *image;
-	unsigned long size = flash->total_size * 1024;
-	unsigned char *buf = calloc(size, sizeof(char));

if (!filename) {
    	msg_gerr("Error: No filename specified.\n");
@@ -1018,23 +1016,32 @@
    	perror(filename);
    	exit(1);
    }
-	msg_cinfo("Reading flash... ");
-	if (!flash->read) {
-		msg_cinfo("FAILED!\n");
-		msg_cerr("ERROR: flashrom has no read function for this flash chip.\n");
-		return 1;
-	} else
-		flash->read(flash, buf, 0, size);

numbytes = fwrite(buf, 1, size, image);
    fclose(image);
-	free(buf);
    msg_cinfo("%s.\n", numbytes == size ? "done" : "FAILED");
    if (numbytes != size)
    	return 1;
    return 0;
 }

+int read_flash_write_file(struct flashchip *flash, char *filename)
+{
+	unsigned long size = flash->total_size * 1024;
+	unsigned char *buf = calloc(size, sizeof(char));
+
+	msg_cinfo("Reading flash... ");
+	if (!flash->read) {
+		msg_cinfo("FAILED!\n");
+		msg_cerr("ERROR: flashrom has no read function for this flash chip.\n");
+		return 1;
+	}
+	if (flash->read(flash, buf, 0, size))
+		return 1;
+
+	return write_buf_to_file(buf, flash->total_size * 1024, filename);
+}
+
 /* This function shares a lot of its structure with erase_flash().
  * Even if an error is found, the function will keep going and check the rest.
  */
@@ -1169,6 +1176,19 @@
    return ret;
 }

+void nonfatal_help_message(void)
+{
+	msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
+		"This means we have to add special support for your board, "
+		  "programmer or flash chip.\n"
+		"Please report this on IRC at irc.freenode.net (channel "
+		  "#flashrom) or\n"
+		"mail [e-mail]!\n"
+		"-------------------------------------------------------------"
+		  "------------------\n"
+		"You may now reboot or simply leave the machine running.\n");
+}
+
 void emergency_help_message(void)
 {
    msg_gerr("Your flash chip is in an unknown state.\n"
@@ -1330,7 +1350,7 @@
  */
 int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it)
 {
-	uint8_t *buf;
+	uint8_t *buf, *oldflashcontents;
    unsigned long numbytes;
    FILE *image;
    int ret = 0;
@@ -1338,7 +1358,31 @@

size = flash->total_size * 1024;
    buf = (uint8_t *) calloc(size, sizeof(char));
+	oldflashcontents = (uint8_t *) calloc(size, sizeof(char));

+	/* FIXME: This ignores --noverify. */
+	if (read_it || write_it || erase_it) {
+		/* Useful for chips with read lock. */
+		if (flash->unlock)
+			flash->unlock(flash);
+
+		msg_cinfo("Reading flash... ");
+		if (!flash->read) {
+			msg_cinfo("FAILED!\n");
+			msg_cerr("ERROR: flashrom has no read function for "
+				 "this flash chip.\n");
+			msg_cerr("Aborting.\n");
+			programmer_shutdown();
+			return 1;
+		}
+		if (flash->read(flash, oldflashcontents, 0, size)) {
+			msg_cinfo("FAILED!\n");
+			msg_cerr("Aborting.\n");
+			programmer_shutdown();
+			return 1;
+		}
+	}
+
    if (erase_it) {
    	if (flash->tested & TEST_BAD_ERASE) {
    		msg_cerr("Erase is not working on this chip. ");
@@ -1354,18 +1398,26 @@
    		flash->unlock(flash);

if (erase_flash(flash)) {
+			msg_cerr("Uh oh. Erase failed. Checking if anything "
+				 "changed.\n");
+			if (!flash->read(flash, buf, 0, size)) {
+				if (!memcmp(oldflashcontents, buf, 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 if (read_it) {
-		if (flash->unlock)
-			flash->unlock(flash);
+		ret = write_buf_to_file(buf, flash->total_size * 1024, filename);

-		if (read_flash(flash, filename)) {
-			programmer_shutdown();
-			return 1;
-		}
+		programmer_shutdown();
+		return ret;
    } else {
    	struct stat image_stat;

@@ -1421,8 +1473,7 @@
    	}
    }

-	// This should be moved into each flash part's code to do it 
-	// cleanly. This does the job.
+	/* FIXME: handle_romentries should reuse oldflashcontents. */
    handle_romentries(buf, flash);

// ////////////////////////////////////////////////////////////
@@ -1436,7 +1487,18 @@
    	}
    	ret = flash->write(flash, buf);
    	if (ret) {
-			msg_cerr("FAILED!\n");
+			msg_cerr("Uh oh. Write failed. Checking if anything "
+				 "changed.\n");
+			/* FIXME: Should we really reuse buf here? */
+			if (!flash->read(flash, buf, 0, size)) {
+				if (!memcmp(oldflashcontents, buf, size)) {
+					msg_cinfo("Good. It seems nothing was "
+						  "changed.\n");
+					nonfatal_help_message();
+					programmer_shutdown();
+					return 1;
+				}
+			}
    		emergency_help_message();
    		programmer_shutdown();
    		return 1;

[Michael Karcher]

Am Dienstag, den 01.06.2010, 04:54 +0200 schrieb Carl-Daniel Hailfinger:
> -int read_flash(struct flashchip *flash, char *filename);
> +int read_flash_write_file(struct flashchip *flash, char *filename);

Cumbersome name. Use "read_flash_to_file".
> +int read_flash_write_file(struct flashchip *flash, char *filename)
> +{
> unsigned long size = flash->total_size * 1024;
> unsigned char *buf = calloc(size, sizeof(char));
> msg_cinfo("Reading flash... ");
> if (!flash->read) {
> msg_cinfo("FAILED!\n");
> msg_cerr("ERROR: flashrom has no read function for this flash chip.\n");
> return 1;
> }
> if (flash->read(flash, buf, 0, size))
> return 1;

I know you didn't change that code. Do we really want no error message
here?
> return write_buf_to_file(buf, flash->total_size * 1024, filename);
>
> +}
> +void nonfatal_help_message(void)
> +{
> msg_gerr("Writing to the flash chip apparently didn't do anything.\n"

"Trying to write to the flash chip..."
> "This means we have to add special support for your board, "
>   "programmer or flash chip.\n"
> "Please report this on IRC at irc.freenode.net (channel "
>   "#flashrom) or\n"
> "mail [e-mail]!\n"
> "-------------------------------------------------------------"
>   "------------------\n"
> "You may now reboot or simply leave the machine running.\n");

"As nothing changed, powering off or rebooting the machine is not
dangerous".
> 	msg_cerr("FAILED!\n");
> 	msg_cerr("Uh oh. Write failed. Checking if anything "
> 		 "changed.\n");
> 	/* FIXME: Should we really reuse buf here? */
> 	if (!flash->read(flash, buf, 0, size)) {
> 		if (!memcmp(oldflashcontents, buf, size)) {
> 			msg_cinfo("Good. It seems nothing was "
> 				  "changed.\n");
> 			nonfatal_help_message();
> 			programmer_shutdown();
> 			return 1;
> 		}
> 	}

Wait a moment... How can the chip be unmodified after passing an erase
check? Only if it was empty before - no need to panic in that case. I
suggest to remove the "did anything change" check from this piece of
code.

Regards,
  Michael Karcjer

[Carl-Daniel Hailfinger]

On 10.06.2010 16:33, Michael Karcher wrote:
> Am Dienstag, den 01.06.2010, 04:54 +0200 schrieb Carl-Daniel Hailfinger:
> > -int read_flash(struct flashchip *flash, char *filename);
> > +int read_flash_write_file(struct flashchip *flash, char *filename);
>
> Cumbersome name. Use "read_flash_to_file".

Done.
> > +int read_flash_write_file(struct flashchip *flash, char *filename)
> > +{
> > unsigned long size = flash->total_size * 1024;
> > unsigned char *buf = calloc(size, sizeof(char));
> > msg_cinfo("Reading flash... ");
> > if (!flash->read) {
> > msg_cinfo("FAILED!\n");
> > msg_cerr("ERROR: flashrom has no read function for this flash chip.\n");
> > return 1;
> > }
> > if (flash->read(flash, buf, 0, size))
> > return 1;
>
> I know you didn't change that code. Do we really want no error message
> here?

Fixed.

I moved the code listed above to a separate patch:
[PATCH] Refactor/fix read-to-file
> > return write_buf_to_file(buf, flash->total_size * 1024, filename);
> >
> > +}
> > +void nonfatal_help_message(void)
> > +{
> > msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
>
> "Trying to write to the flash chip..."

Hm. An earlier message says "writing", not "trying to write". With your
change, we'd see messages like this:

Writing flash chip...
Trying to write to the flash chip apparently didn't do anything.

That looks odd. I agree that my proposed message wasn't really good
style, but the replacement isn't perfect either. Maybe a native speaker
can comment.
> > "This means we have to add special support for your board, "
> >   "programmer or flash chip.\n"
> > "Please report this on IRC at irc.freenode.net (channel "
> >   "#flashrom) or\n"
> > "mail [e-mail]!\n"
> > "-------------------------------------------------------------"
> >   "------------------\n"
> > "You may now reboot or simply leave the machine running.\n");
>
> "As nothing changed, powering off or rebooting the machine is not
> dangerous".

Should this message be printed in addition to or as replacement for "You
may now reboot..."?

How should we tell the user that rebooting is unsafe if a previous write
failed?
Example scenario: Chip is filled with data, user tries to write, but
write fails and the chip is now erased. We print a scary warning. User
retries, the write fails again but this time nothing changed (the chip
was already erased), so we print the non-scary warning. The user is
confused and reboots. Boom!
> > 	msg_cerr("FAILED!\n");
> > 	msg_cerr("Uh oh. Write failed. Checking if anything "
> > 		 "changed.\n");
> > 	/* FIXME: Should we really reuse buf here? */
> > 	if (!flash->read(flash, buf, 0, size)) {
> > 		if (!memcmp(oldflashcontents, buf, size)) {
> > 			msg_cinfo("Good. It seems nothing was "
> > 				  "changed.\n");
> > 			nonfatal_help_message();
> > 			programmer_shutdown();
> > 			return 1;
> > 		}
> > 	}
>
> Wait a moment... How can the chip be unmodified after passing an erase
> check? Only if it was empty before - no need to panic in that case. I
> suggest to remove the "did anything change" check from this piece of
> code.

What about a partially failed write? If the chip was erased before, the
erase check will pass. If write fails completely, the chip is still
erased. If write fails partially, you have a partially written chip.
Total write failure is not a problem with already erased chips, but
partial write failure is indeed a problem.
Did I understand your thoughts correctly or did you mean something else?

Regards,
Carl-Daniel

[Michael Karcher]

Am Montag, den 28.06.2010, 15:35 +0200 schrieb Carl-Daniel Hailfinger:
> > > +void nonfatal_help_message(void)
> > > +{
> > > msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
> >
> > "Trying to write to the flash chip..."
>
> Hm. An earlier message says "writing", not "trying to write". With your
> change, we'd see messages like this:
>
> Writing flash chip...
> Trying to write to the flash chip apparently didn't do anything.
> That looks odd. I agree that my proposed message wasn't really good
> style, but the replacement isn't perfect either. Maybe a native speaker
> can comment.

Hmm. While trying to defend my position, I think your message isn't as
bad as is initially thought. I think it would be confusing to talk about
having "written" something and in the same sentence tell that this did
not do anything. Actually, nothing was written to the memory, even if
you sent write cycles to the chipset intending they end up in the chip
and get forwarded to the array. The average user the "soft" error
message is intended for doesn't know about the write cycle stuff, but
will be happy to know that nothing was written.
> > > "-------------------------------------------------------------"
> > >   "------------------\n"
> > > "You may now reboot or simply leave the machine running.\n");
> >
> > "As nothing changed, powering off or rebooting the machine is not
> > dangerous".
>
> Should this message be printed in addition to or as replacement for "You
> may now reboot..."?

Replacement. I dislike the explicit license to "leave the machine
running". Of *course* I can leave the machine running, as long until it
crashes, no matter whether the flash chip is OK. What you want to say,
if I understand it correctly, is that rebooting is safe, but it is not
needed to reboot now. I think my suggestion makes it more clear.
> How should we tell the user that rebooting is unsafe if a previous write
> failed?
> Example scenario: [...]

Excellent point, but it still applies to both the original and my
reworded message. Something like "If you had no flashrom failures
before, powering off or rebooting the machine is not dangerous, as the
contents of your flash chip kept unchange in this run of flashrom".
> > > 	msg_cerr("FAILED!\n");
> > > 	msg_cerr("Uh oh. Write failed. Checking if anything "
> > > 		 "changed.\n");
> > > 	/* FIXME: Should we really reuse buf here? */
> > > 	if (!flash->read(flash, buf, 0, size)) {
> > > 		if (!memcmp(oldflashcontents, buf, size)) {
> > > 			msg_cinfo("Good. It seems nothing was "
> > > 				  "changed.\n");
> > > 			nonfatal_help_message();
> > > 			programmer_shutdown();
> > > 			return 1;
> > > 		}
> > > 	}
> >
> > Wait a moment... How can the chip be unmodified after passing an erase
> > check? Only if it was empty before - no need to panic in that case. I
> > suggest to remove the "did anything change" check from this piece of
> > code.
> What about a partially failed write? If the chip was erased before, the
> erase check will pass.

Right.
> If write fails completely, the chip is still erased.

And it would only trigger the "soft" error message at this point with
your patch, without the patch, it will trigger the "hard" error message.
> If write fails partially, you have a partially written chip.

In that case, both the old and the new version show the "hard" error
message.
> Total write failure is not a problem with already erased chips, but
> partial write failure is indeed a problem.

I am not arguing against verifying the write after sucessfull erase
(think of old dying chips!), I just claim that the extra verify to print
the "soft" error message is unneeded in this case and we should keep the
old behaviour of always printing the hard error message on write
failure. Think of reading SPI and the time consumed by that.
> Did I understand your thoughts correctly or did you mean something else?

I didn't really understand what was your point was, but maybe you didn't
get my intention.

Regards,
  Michael Karcher