[flashrom] [PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Fri, 03 Feb 2012 02:13:56 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > +	nph = buf[2];
> > +	msg_cdbg2("SFDP number of parameter headers (NPH) = %d (+ 1 mandatory)"
> > +		  "\n", nph);
> > +
> > +	/* Fetch all parameter headers, even if we don't use them all (yet). */
> > +	hbuf = malloc(sizeof(struct sfdp_tbl_hdr) * (nph + 1));
> > +	hdrs = malloc((nph + 1) * 8);  
> 
> Why is 8 a magic unexplained constant for hdrs allocation, but sizeof
> struct sfdp_tbl_hdr (which is 8 as well) is used for hbuf allocation?
> Did you mix up the two by accident?

ooops. actually it should be the other way around.

rationale: we know exactly how big hbuf should be. it is filled by
spi_sfdp_read and read by index. actually we would only need 7 bytes
because the last byte of each header is unused, but this would
complicate the call(s) to spi_sfdp_read.

hdrs OTOH is accessed by the struct members only and we dont know the
size of the struct beforehand.

> And why is (nph+1) the first factor
> in the second malloc and the second factor in the first malloc?

just to test the reviewer of course.

> > +		tbuf = malloc(len);
> > +		if (tbuf == NULL) {
> > +			msg_gerr("Out of memory!\n");  
> 
> insert the following code:
> ret = 0;
^ this is not needed because ret is only changed in the loop later.
i have now refined the whole error handling inside the loop anyway, see
end of mail.

> goto cleanup_hdrs;

> 
> > +		exit(1); /* FIXME: shutdown gracefully */  
> 
> and kill the exit(1). It would be nice to change the probe interface to
> return 0 on success... that would allow us to return detailed errors.
> OTOH, we might want to use the probe interface to return match accuracy,
> in which case 0 would be nomatch. Comments appreciated.

the only thing i can think about at the moment regarding this: this
thread is already complicated enough :)

we could do both... returning a struct is always an option, if it is
really needed.
is the accuracy really needed? the only use of it would be to order the
matches for the user, or define limits of "accuracy amount" where we
decide on our own...? i think we have more important problems to solve,
but if you want to discuss this further, please start another RFC
thread with more details.

> > +		}
> > +		if (sfdp_fetch_pt(flash, tmp32, tbuf, len)){
> > +			msg_cerr("Fetching SFDP parameter table %d failed.\n",
> > +				 i);
> > +			free(tbuf);
> > +			break;
> > +		}
> > +		if (i == 0) { /* Mandatory JEDEC SFDP parameter table */
> > +			if (hdrs[i].id != 0)
> > +				msg_cdbg("ID of the mandatory JEDEC SFDP "  
> 
> msg_cerr, then cleanup and return 0. Maybe even ask user to report?

the question is if this would ever safe anyone from bricking something.
if not, it is much better to ignore it due to how well *cough* vendors
have implemented the standard so far (david's winbond chip works
flawlessly but has the ID set to 0xe5 - does that ring a bell btw?).
we only have the two samples from winbond and macronix, but it is
quite evident that vendors rather understood "first table is the jedec
flash parameter table" than the scheme of parameter table IDs.
At this point i think it might even be a good idea to introduce a
"yes_i_have_verified_the_SFDP_parameters_to_be_reasonable" option. No
ID check (especially not against 0) will make the whole thing much
safer imo.

> > +
> > +			if (len != 9 * 4 && len != 4 * 4) {
> > +				msg_cdbg("Length of the mandatory JEDEC SFDP "  
> 
> msg_cerr, then cleanup and return 0. Maybe even ask user to report?

see end of mail

> > +					 "parameter table is %d B instead of "
> > +					 "36 B (i.e. 9 double words) as "
> > +					 "demanded by JESD216, skipping "
> > +					 "parsing.\n", len);
> > +			} else if (sfdp_fill_flash(flash, tbuf, len) == 0)
> > +				ret = 1;

the main idea of the loop is to iterate over the various sfdp parameter
headers and contents. while doing that we want to extract and print any
information we can derive from it and eventually use it to fill a
struct flash. the error handling now guarantees that we will look at
all tables except in the case of OOM. since the tables are all
independent in theory we should not abort the probing by enforcing ret
= 0 if other tables are bogus or we cant retrieve them. this also
allows us to dump as much information as possible without adding other
code (i have done that previously to debug the macronix chips and it was
awful.

attached patch is untested. i will do that tomorrow, but wanted to give
you the chance to reply early.
one thing i remember to be missing is the 4k handling. apart from that
and the loop error handling i hope we are quite finished. \o/
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-support-for-SFDP-JESD216.patch
Type: text/x-patch
Size: 19509 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20120209/5a5cf023/attachment.patch>