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

[Paul Kocialkowski]

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.

> 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.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20151111/0cd4c93f/attachment.asc>