Re: [flashrom] [PATCH] Always read chip before writing, avoid emergency messages

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [101015 15:22]:
> Always read the flash chip before writing. This will allow flashrom to
> skip erase of already-erased blocks and to skip write of blocks which
> already have the wanted contents.
>
> Avoid emergency messages by checking if the chip contents after a failed
> write operation (erase/write) are unchanged.
>
> Keep the emergency messages after a failed pure erase. That part is
> debatable because if someone wants erase, he pretty sure doesn't care
> about the flash contents anymore.
>
> Please note that this introduces additional overhead of a full chip read
> before write. This is frowned upon by people with slow programmers.
> Maybe we should make this configurable. Either way, this patch needs a
> review to make sure I'm not going in the wrong direction.
>
> Signed-off-by: Carl-Daniel Hailfinger

Nice! We should indeed make this configurable.

Acked-by: Stefan Reinauer
> Index: flashrom-always_read_no_scary_warning/flashrom.c
>
> --- flashrom-always_read_no_scary_warning/flashrom.c	(Revision 1212)
> +++ flashrom-always_read_no_scary_warning/flashrom.c	(Arbeitskopie)
> @@ -1343,6 +1343,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");
>
> +}

Since we are reading anyways now, it would be very nice to check whether
the image to write is different from the image that was just read and
not attempt any write at all in this case. (Possibly with a message a la
"Trying to write identical image. Skipped"
> 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
>  * blank or it has the old contents. A blank chip won't boot,
>  * so if the user wanted erase and reboots afterwards, the user
>  * knows very well that booting won't work.
>  */

We certainly want to know when an erase failed, though.
> }
>
> }
> newcontents = (uint8_t *) calloc(size, sizeof(char));

Maybe using malloc and memset would increase the readability at that
point. On first glance it was somewhat unclear why calloc was used with
byte sized members.
> /* 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
> * preserved, but in that case we might perform unneeded erase which
> * takes time as well.
> */
> msg_cdbg("Reading old flash chip contents...\n");
> if (flash->read(flash, oldcontents, 0, size)) {
> programmer_shutdown();
> return 1;
> }

So what should the "skip the read" option be called? --unsafe?
> 	msg_cerr("Uh oh. Write failed. Checking if anything "
> 		 "changed.\n");

Should the tool really say "Uh oh."? ;-)

Good stuff!

Best regards,
Stefan