[coreboot] PCI register read/mod/write code

ron minnich rminnich at gmail.com
Tue Oct 5 00:14:34 CEST 2010


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




More information about the coreboot mailing list