[flashrom] DMI matching patch

[Carl-Daniel Hailfinger]

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

-- 
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."