[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 <c-d.hailfinger.devel.2006 at gmx.net> [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 <c-d.hailfinger.devel.2006 at gmx.net>
>>     
>
>
> Nice! We should indeed make this configurable.
>
> Acked-by: Stefan Reinauer <stepan at coreboot.org>
>   

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 flashrom at flashrom.org!\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

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