On Mon, Jan 04, 2010 at 03:15:11PM +0100, Michael Karcher wrote:
Hello Carl-Daniel,
thanks for your review.
There seems to be a fundamental opinion difference what DMI is used for between you on the one side and me and Luc on the other side. 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.
Using DMI as emergency rescue has the advantage that it still works on most systems even if DMI is unavailable (boards having DMI info can not be matched by PCI IDs with the matching policy of this patch). Collecting DMI info on boards with good PCI subsystem IDs (like the MSI boards that have the internal 4-digit product number BCD-coded in the subsystem IDs) is not useful, in my opinion.
The usage of DMI as breaker explains the loose matching policy of "any DMI string matches", as the IDs that should go into the table should be unique enough even with this weak policy (I for example can't imagine any HP system, as the manufacturer has already been established by subsystem manufacturer IDs, where "^VL420$" is going to match in the wrong string). If you want to use DMI as general board identification mechanism, independent of the PCI IDs, one would of course have to check more than one field.
Agreed.
I'm not so happy about the static allocation here. Can't we use malloc() for the read buffer and strdup() for filling in dmistrings?
Of course we can. On the other hand, I don't really see the point in using dynamic allocation here. If it's about wasted memory, we should approach "struct flashchips" first. The fixed-size arrays in it waste a lot more. Nevertheless, I change it to dynamic allocation. Too bad the nice getline function that reads one line and allocates dynamically is GNU-only and not standard C or at least POSIX.
This is a run-once tool, the results are not stored, and we do not go and run dmidecode several times. Everything filling up any memory should be ran once.
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 product match for example). Luc, please NACK if you think the it is too much.
That's way too contrived for my taste. If you really believe it is necessary, then you should provide an enum entry right in front of the string.
But then the board enable table is taking another big hit, and this i would like to avoid at all cost. We already have pci ids and pci subsystem ids, this is just an extra bit of info that will be the last in precedence for any match.
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. I understand your point of tighter matches being better, but I don't think that's worth a hard dependency on dmidecode if we get along nicely without.
I agree, this is a lowlevel tool and the less we depend on externals the better. That being said, if we have any board that requires dmidecode to be able to make a succesfull match, we already depend fully on dmidecode.
Luc Verhaegen.