On Wed, Jan 06, 2010 at 02:27:28PM +0100, 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.
It also tries to decouple DMI matching from the pciids and opens the door for full dmi handling (dmi info which might be altered when bioses are updated) and no pciid matching. This would end up being a big mess that at one point later we cannot fix anymore.
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.
Hrm. If this positional information is really necessary (and i think it isn't), then at least use an enum and get the compiler to catch typos.
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.
This whole thing is why i was initially very weary of *^$ in these strings. People end up trying to go beyond when they see something like that, it's something psychological that i have seen at play more often in these cases.
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".
And even if those things are in there, who cares :)
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.
Sounds good. Give up on dmi when it seems to bugger up is good enough not to get weird things happening. And the last statement reads as "only fix the problem if/when it presents itself" which is what i like for something like this, where it is clear that the issue can be worked around easily and cleanly.
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.
As said, i do not like the string selection. _IF_ it is deemed really necessary, and i do not think it is, at least not yet, then we should use an enum and an extra entry.
For now, i would like to see just the string matching, and the relevant dmidecodes being collected so that possible future string selection can be implemented with some googling.
Luc Verhaegen.