[flashrom] DMI matching patch

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Wed Jan 6 14:27:28 CET 2010


Am Mittwoch, den 06.01.2010, 13:58 +0100 schrieb Carl-Daniel Hailfinger:
> On 04.01.2010 15:15, Michael Karcher wrote:
> > You seem
> > to want to use DMI for a good board identification (which is fair
> > enough, I also started with that intention), whereas now we decided to
> > use DMI only as an emergency helper in case the PCI IDs seem obviously
> > not good enough, which should be the exception instead of the regular
> > case.
> I also collect superiotool output for known good boards. We don't need
> superiotool output for those boards, but I believe that output may be
> useful for data mining later. [...]
> My desire to get dmidecode data for all boards is motivated by the same
> reasons.
OK, point taken. No problem

> IMHO PCI ID matching should be the primary match (we seem to agree here)
> and DMI should only be used as additional match to narrow down the
> results (we seem to agree on that point as well).
Yes, definitely.

> Just to make this very clear: I consider a pure DMI match (without PCI
> matching) to be a really bad idea because it gives us two orthogonal
> sets of IDs (PCI+DMI) without any way to correlate them.
Right. My first idea was to provide a DMI ID -> coreboot ID mapping that
could be kept out of the board_enable structure. In that case, a pure
DMI match would be used to identify the board, but then the usual PCI-ID
cross checking you also have for coreboot IDs applies. Luc didn't like
this idea, because it would scatter the information for one mainboard
into two tables. I think his point is valid, which made the patch as it
is now. I don't think one approach is technically much superior to the
other one.

> If you think functions like getline would be useful, copy them into
> flashrom.c where we already have min() and max() and others.
OK, will keep that in mind. It's not really needed here, though. The
fixed-size buffer seems good enough.

> > Fair enough. I added it. Specifying one string to match is not
> > increasing complexity much, although it will be a three-character hit on
> > the board_enable table (if you specify like "bp:^A8V MX$" for baseboard
Oops, I trapped into the trap I set myself. It should be "mp:^A8V MX$"
of course. But as adding BIOS identifiers could be needed soon, I really
think using "m" for the baseboard is the lesser evil.
> > product match for example). Luc, please NACK if you think the it is too
> > much.
> This also documents the used string in case someone later wants to
> modify the matching in any way. There's always the ability to specify
> "any:" or something like that in case the board vendors store the same
> identifier in different DMI strings based on BIOS revision.
I don't think we need the "any:" prefix (I think I would have written it
as "??:") at all. The match-any policy was in just to shorten the table
entry.

> > As written in the introduction text, this is something I would like to
> > avoid. You have a point in "it's nice if it's always there", but this
> > makes the "we have DMI because it's nice" and "we *need* to match the
> > DMI info to be sure we have the right board" indistinguishable.
[...]
> OK, as long as we have the dmidecode info around (mailing list post) in
> case someone later finds another board with the same PCI IDs.
So, the bot command for "summary" should add "dmidecode output, but
clear out your serial numbers/UUIDs".

> > +		if (ferror(dmidecode_pipe))
> > +		{
> > +			printf_debug("DMI pipe read error\n");
> > +			pclose(dmidecode_pipe);
> > +			goto out_free;
> Logic question: If there is any error reading one DMI string, we skip
> reading all subsequent strings AFAICS. Is that intentional?
The logic is that dmidecode should not fail at all. If reading one
string fails, the goto (or return it was before the malloc() sneaked in)
jumps over the has_dmi_support = 1 line, so DMI will be completely
disabled of reading one string fails. As DMI will be completely
disabled, reading further strings is useless. We might want to change
that policy if it turns out that some dmidecode version doesn't
implement some identifier strings.


> > +		/* chomp trailing newline */
> > +		if (answerbuf[0] != 0 &&
> > +		    answerbuf[strlen(answerbuf) - 1] == '\n')
> > +			answerbuf[strlen(answerbuf) - 1] = 0;
> I noticed that the three expressions above could be simplified if we
> calculateed strlen(answerbuf) unconditionally. Micro-optimization
> though, can be postponed.
Yeah, style question. It will introduce a temporary variable that's not
in yet, but avoid the double strlen calculation. I trusted the gcc
optimizer to catch it, but didn't check whether it really does.

> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Thanks for your Ack.
Luc, do you see any problems with the patch? Should we consider removing
the string-selection prefix? I think the horse has been beaten to death
and the code as it is now should be a fair compromise, but as the board
enable table currently is kind of "your property", I would like to hear
your opinion on it.

Regards,
  Michael Karcher





More information about the flashrom mailing list