[flashrom] [PATCH 3/3] ENE Embedded Debug Interface (EDI) and ENE KB9012 EC internal flash support

[Nico Huber]

On 11.11.2015 16:34, Paul Kocialkowski wrote:
> Le dimanche 08 novembre 2015 à 15:53 +0100, Nico Huber a écrit :
>> Hi Paul,
>>
>> On 24.10.2015 13:19, Paul Kocialkowski wrote:
>>> I will probably send v2 right away, feel free to follow up the
>>> discussion on some of these comments in there, it'll probably be
>>> easier.
>> I'll keep some of the general discussion here. Some new comments
>> will come inline with v2.
>>
>>>>> +	unsigned char buffer[edi_read_buffer_length];
>>>>> +	unsigned int i;
>>>>> +	int rc;
>>>>> +
>>>>> +	edi_read_cmd((unsigned char *)cmd, address);
>>>>> +
>>>>> +	memset(buffer, 0, sizeof(buffer));
>>>> Why?
>>>
>>> Same thing here, spi_send_command *should* fill-in the whole buffer,
>>> but in case it doesn't, I'd rather have known values instead, just in
>>> case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
>>>
>>> This is probably not a hard requirement either, but seems like a good
>>> programming habit to produce reliable code to me.
>> What bothers me is that you imply that spi_send_command() may behave
>> wrong. That is very defensive programming. And if spi_send_command()
>> really does it wrong, we'd never know.
> 
> So I guess the point here is that spi_send_command will let us know in
> its return code when it failed to fill the whole buffer. I can work with
> that as a hard assumption. Some functions, such as read, read at most
> the requested number of bytes, thus this programming habit becomes
> relevant.
Pedantic me says: In that case you shouldn't look at the bytes you
didn't request to read. They have the old value anyway.

>> Well, that's generally why I prefer to never initialize unless it's
>> necessary. So many errors out there that we can't see due to over
>> initialization :-/
> 
> At the end of the day, I prefer to have initialized data mistakenly
> interpreted than random garbage (but of course, that's only a fallback
> and it indicates that there is a problem somewhere else). I guess the
> question is whether we want to have a chance to spot those kinds of
> issues at run time by not initializing data and having better odds of
> spotting a hard crash or have it keep going with a fallback path. For
> flashrom, I'd say it's okay to have a hard fail. For other projects,
> where it is vital for the program to keep running no matter what, I
> wouldn't agree.
Crashing is never an option. If it's vital for a program to keep run-
ning, it has to check for garbage every time it looks into a simple buf-
fer of data without guarantees on its content. Sticking to the example,
spi_send_command() is allowed to fill the buffer with garbage (that will
be the case when there is garbage on the SPI line). Overinitialization
only gives you the feeling of being safe. You're never safe!

Nico