On 06.01.2010 14:27, Michael Karcher wrote:
Am Mittwoch, den 06.01.2010, 13:58 +0100 schrieb Carl-Daniel Hailfinger:
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.
I agree with Luc here.
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.
OK.
On 04.01.2010 15:15, Michael Karcher wrote:
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.
Yes.
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.
Good point on "??:". If we ever need it, we can still implement it.
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".
Absolutely. We could in theory tell people to run dmidecode six times (with a different string each time), but full dmidecode is likely easier. To be honest, I think creating a small script which collects all data would be user-friendly. That script should be hosted in svn.
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.
Ah, didn't see that. Good choice designing the logic like that.
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.
Yes, let's wait and see.
/* 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.
It probably depends on the compiler, the compiler version and the optimizer flags. In the end, it's just a few nanoseconds we save and thus we should pick the code which is easier to read. No strong preference on my side.
Regards, Carl-Daniel