[coreboot] PCI register read/mod/write code

Corey Osgood corey.osgood at gmail.com
Tue Oct 5 10:32:48 CEST 2010


On Mon, Oct 4, 2010 at 6:14 PM, ron minnich <rminnich at gmail.com> wrote:
> On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge <peter at stuge.se> wrote:
>> Rudolf just found a bug in the sb700 code:
>>
>> u32 dword;
>> ..
>> dword = pci_read_config8(dev, 0x64);
>> dword |= 1 << 10;
>> pci_write_config8(dev, 0x64, dword);
>
>
> Actually, I don't even have a problem with this construct. Why?
> Because it's in just about every kernel I've ever worked with. It's a
> common technique. Sure, in this case, it's a trivially simple change
> and you can write a function for it. But, as soon as things get more
> complex, with multiline tests and bit sets, you can't use the
> functions, and we're back to the same type of coding. Then we end up
> with mixed idioms.
>
> It's a good idea for our code base to adhere to such common idioms. It
> makes for an easier time for people coming in from, e.g., Linux. I
> don't find the functions easier.
>
> Also, as pointed out, the proposed functions solve one special case.
> Better to fix the real problem, which is that the compiler can tell us
> about this type of error but we're not letting it. That will fix all
> such problems, not just the pci subsystem.
>
> Just another penny or so.
>
> ron

I'm fully in agreement with Ron here. Lets fix whatever's broken so we
can let the compiler tell us when we make foolish mistakes, rather
then writing some mess that's sure to cause some WTFs down the road.
This is *one* instance in which someone used an 8-bit instead of a
16-bit function by mistake. Lets not go nuts trying to idiot-proof
things. Instead of hacking away at all the pci functions, and almost
definitely causing some unintended breakage, can't we just pay a
little more attention during reviews?

-Corey




More information about the coreboot mailing list