[coreboot] [PATCH] flashrom - board enable - reconstruct table.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Apr 20 15:25:33 CEST 2009


On 20.04.2009 14:28, Luc Verhaegen wrote:
> On Mon, Apr 20, 2009 at 01:17:43PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> On 20.04.2009 12:12, Luc Verhaegen wrote:
>>     
>>> Flashrom: board_enables: reconstruct table.
>>>
>>> This patch restores the pciid based board matching table. It makes this
>>> table readable and hackable again, and the only disadvantage is that the
>>> right margin is way beyond the rather dogmatic 80. All 0x0000 pci ids have
>>> been string replaced by 0 to more easily spot missing ids, and extra
>>> comments have been added to explain how the various entries are used.
>>>
>>> Signed-Off-By: Luc Verhaegen <libv at skynet.be>
>>>   
>>>       
>> The extra comments are good, but "coreboot bios" in such a text really
>> hurts. Please change the word BIOS to all-caps and don't refer to
>> coreboot as a BIOS. Thanks.
>>     
>
> These are the lines in question:
>
> * The coreboot ids are used two fold. When a coreboot bios is installed, it
> * uniquely matches the coreboot board identification string. When a legacy
> * bios is installed and when autodetection is not possible, these ids can be
> * used to identify the board through a command line argument.
>
> It is clear and concise.
>
> Let's take the other option: "when coreboot is installed" What does that 
> mean? Did the user check out the tree, did he install some of the utils, 
> or did he install a coreboot bios image into his rom?
>   

Coreboot is NOT a BIOS. If you really insist calling coreboot a BIOS,
why not call it LinuxBIOS?


> Your statement is more about politics than anything else.
>   

There was almost universal agreement about the name change from
LinuxBIOS to coreboot (well, I was unhappy initially, but I soon found
out that the absence of BIOS in the new name is really beneficial). One
of the reasons for the name change was that coreboot is NOT a BIOS nor
should it ever be called one.

Feel free to propose calling coreboot a BIOS in a separate RFC on this list.


>> The documentation part can be committed after one more iteration.
>>     
>
> I would never have altered these comments if it wasn't for the previous 
> flamewar. For those just tuning in, this is the thread in the 
> mailarchives: 
> http://www.coreboot.org/pipermail/coreboot/2009-January/044086.html
>   

Ah yes, I forgot. In
http://www.coreboot.org/pipermail/coreboot/2009-January/044129.html you
promised to "create all board enable functions and table entries for the
foreseeable future for everyone who asks once we switch back to the old
layout [...] next to keeping X from degrading even further." Was that
irony or a real promise?



>> The rest of the patch makes board entries unreadable.
>>     
>
> We had this one before. It makes it unreadable for you. But not for 
> everyone.

Well, more flashrom developers on this list favour the multiline
solution than those opposed to it. That means I'm part of the majority.
And I do help people on IRC add board enables.


>> In the past few weeks, we had quite a few people in #coreboot who wanted
>> to add support for their boards. The presence of struct member names
>> helped immensely to explain how to add board enables. Some even figured
>> it out from similar entries. Lowering the barrier for possible future
>> developers is very important.
>>     
>
> What makes you so certain that it is this change that made people add 4 
> entries in 3 months? Couldn't that be because flashrom just is getting 
> used more? And besides, this growth is not significantly faster than 
> before.
>   

That's a straw man. I didn't claim any of the points you try to question
above.


>> Since IIRC you said some time ago that the multiline layout hinders your
>> workflow, feel free to submit any new board enables in the single-line
>> layout. I'll fix them up.
>>     
>
> Why is my workflow relevant?

If your workflow is irrelevant, why would we want to accommodate it?


> Do you really think that i have all my 
> editors open at 160 chars all the time? My you are referring to my 
> statements about being able to oversee everything more quickly with a 
> condensed table. This is just common sense, everybody will have to 
> acknowledge that one.
>   

Unless you get to define everbody's common sense (which is inherently
subjective), it is unlikely that everybody (in the strict sense) will
have to acknowledge your view.
After all, I'm a person (and thus part of the "everybody" group of
people) and I disagree with you. You could claim that I don't have
common sense, but I hope you won't do that.


> [...]
> What will you do when it reaches a hundred or so entries? Will you be 
> happy with a 1400line mess? 

Is any table with 1400 lines a mess?


> Do you think that this table will stop 
> growing at some point?
>   

Why should it? Hardware will probably have quirks for the foreseeable
future.


> How many actual board enable functions will there be with multiple 
> entries in the table? How much space will the actual enables take 
> compared to the table? 
>   

I expect at least half a dozen entries with flash translation chips
because these boards exist out there.


> How will people tell whether something similar has already been added?
>   

Like they do now.


> Will you impose some entry ordering scheme (which, in light of likely 
> future table growth, is actually a useful change)?
>   

Your patch doesn't touch that issue, so the point is orthogonal to our
discussion.


> It rapidly becomes non-obvious when you don't have a tight table. And i 
> know what you are going to say here: just make sure that they are added 
> in whatever order already. And my answer to that is: you can't. Left or 
> right, entries will be added that aren't ordered, and afterwards no-one 
> will spot them being unordered anymore. In a tight table, you spot them 
> right away and can fix it easily on the next iteration.
>   

See above. There are lots of possible ordering schemes, but deciding
which makes sense is a different matter.


> It really is creating a situation where you are unable to see the forest 
> through the trees.
>   

Grep works nicely.


> Also, you originally suggested to remove lines like = 0x0000; or = NULL; 
> This is a very dangerous path. People will only see the last 2-3 
> entries, and never look beyond. Remove a line, and it will not be 
> thought of for the next entry. This is simply human behaviour.
>   

So you suggest to keep these lines. Fine.


> What you then end up with is a whole range of boards where just a single 
> pair of main ids, the coreboot ids, the string and the enable will be 
> created. No autodetection will be provided any more, as the trivial 
> addition of yet another argument-specified-only board is of course 
> obvious for the person who added the board enable to begin with.
>
> The end result then is that all users will just randomly go through the 
> board names for ages, all the time, some of which will work, as the 
> amount of pci devices really isn't that great. This is quite a dangerous 
> situation and a really bad user experience.
>   

Adding unique subsystem IDs to that struct is hard. First, I know for a
fact that my laptop and a related model have identical subsystem IDs.
That alone is proof that subsystem IDs are not always unique. Unless you
know all subsystem IDs of all boards from a given manufacturer, you
can't say for sure whether the board enable you're adding subsystem IDs
for is actually _not_ going to trigger on another board.
To make this even more fun, which subsystem ID will you use


> With a condensed table, people are forced to pad, and will put more 
> thought into their entries and will feel obliged to add subsystem ids.
>   

You must be joking. If you can _prove_ that, you'll probably get a PhD
in psychology right away.
If pressing tab once really forces people to think for minutes about
something that is neither easy nor obvious, I want a tab key in every
conversation.


> This in turn leads to more autodetection and less people blindly forcing 
> boardnames, and in general a better user experience.
>
> So maybe making it "easier" for people to "understand" what the 
> different entries are for (which actually boils down to being able to 
> ignore how the entries are used), is not a goal that should be pursued 
> above other goals. For existing developers, at least those willing to 
> not religiously stick to dogmas when it makes sense, the condensed table 
> is the better option.
>   

Let me modify one word of your statement to express my opinion:

For existing developers, at least those willing to 
not religiously stick to dogmas when it makes sense, the multiline table 
is the better option.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list