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

[Carl-Daniel Hailfinger]

On 19.10.2010 00:49, Stefan Reinauer wrote:
> 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

Thanks, committed in r1215.
> > 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"

Will come in a followup patch which starts real partial 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
> >  * 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.

Yes, that's why the code still complains. Maybe I should reword the
comment. We want to know that the erase failed, but I think there is no
need for additional info on top of that (i.e. if anything changed).
> > }
> >
> > }
> > 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.

I want kzalloc(size_t size). It's what the Linux kernel uses, and it
make sense.
> > /* 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?

I'll probably make this dependent on --verify or so. We'll see. Maybe
--verify-level=2.
> > 	msg_cerr("Uh oh. Write failed. Checking if anything "
> > 		 "changed.\n");
>
> Should the tool really say "Uh oh."? ;-)

Yup, it sounds appropriate if you say it out loud.
> Good stuff!

Thanks!

Regards,
Carl-Daniel