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