Am Donnerstag, den 31.12.2009, 17:22 +0100 schrieb Carl-Daniel Hailfinger:
thanks for your patch. Overall, I like it, but some design decisions may need to be revisited.
That doesn't sound too bad for something as new and still in discussion as this DMI patch. Thanks for your review.
On 30.12.2009 15:02, Michael Karcher wrote:
Matching via DMI: If a board is not uniquely identifiable by PCI device/subsystem IDs, a string can be specified to be looked for (case-sensitive, substring) for now in one of the following items:
- System Manufacturer
- System Product Name
- System Version
- Baseboard Manufacturer
- Baseboard Product Name
- Baseboard Version
I believe the substring match is suboptimal.
You got a point there.
For example, if you want to differentiate between the Asus M2A-VM and the Asus M2A-VM HDMI, only the HDMI variant can be matched safely.
The idea was that some manufacturers (HP I think) seem to put board ID and some revision or different ID into the same string, so we can't match full strings only.
This may be a stupid question, but how do AWDFLASH etc. match their boards? Their mechanism is designed to work well under most circumstances, and the manufacturers check it for ambiguities.
They don't do like we do. The vendor tools don't need to match the board to find out how to program it, because all vendor tools call into the BIOS or some code snippet provided with the BIOS update to do the board enable stuff. The only thing they have to check is whether the new BIOS matches the board. They do that by simply comparing some magic strings.
Vendors found out that one flash program that supports all boards is a horrible, unmaintainable mess of special cases, and as vendors can rely on their own BIOS in the running system, they just put the board enable code for that board into the BIOS. That's something we can't do with flashrom, as flashrom should work with coreboot, and coreboot currently does not support BIOS support.
The match is only made if DMI info is available and the string matches. If no DMI info is available, a warning is printed as the board can not be autodetected.
I'd make that warning conditional on the absence of a coreboot table. Otherwise you'll get a warning on a lot of systems with coreboot.
You will only get that warning if the PCI IDs match and the DMI is missing. When you run on coreboot, you don't even get into board_match_pci_card_ids(), as board_match_coreboot_name() already has chosen the right board.
It's still open to discussion whether we add an DMI override switch to specify a string that will definitely match, and whether this switch is only used if no DMI is available or whether it overrides or augments DMI data.
I think DMI should be used as additional matching help if PCI IDs are already correct. Especially in case multiple vendors have the same Product Name string, the PCI ID matching is crucial.
This is already implemented. DMI is only matched if PCI matches.
DMI data is currently read using dmidecode. This tool is available for all major platforms except MacOS X. I heard that there also is a MacOS X version of dmidecode, but didn't investigate that.
We might want to specify a dmidecode path/binary as optional internal programmer parameter for installations which don't have a native dmidecode in their path. Maybe something like flashrom -p internal:dmidecode=c:\cygwin\bin\dmidecode.exe That's something I'd leave to a followup patch, though.
I agree on that. And that makes agaran's point that the fixed-size command line buffer is bad more important. PATH_MAX+40 should be enough, and then snprintf for extra protection.
[three coding style hints snipped] Thanks again.
Regards, Michael Karcher