[coreboot] PCI register read/mod/write code

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Oct 4 01:39:49 CEST 2010

On 04.10.2010 01:10, Peter Stuge 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);
> And I'm ranting now, because a pci_set8() macro/function could have
> found this bug at compile time, and because I don't like these
> constructs. (Compiler warnings would also have indicated a problem.
> They're currently disabled for this code.)
> Carl-Daniel is working on a patch, meanwhile consider this:
> #define pci_set8(dev, reg, val) do { \
>   if ((a) & ~0xff) can_only_set_low_8_bits(); \

If val is a variable, this will cause linker errors. However, if we use
__builtin_constant_p to apply this check only to constants, it would work.

>   pci_set8_nocheck((dev), (reg), (val)); \
> } while(0)
> u8 pci_set8_nocheck(device_t dev, u32 reg, u8 val) {
>   u8 tmp = pci_read_config8(dev, reg);
>   tmp |= val;
>   pci_write_config8(dev, reg, tmp);
>   return tmp;
> }
> $ gcc -g -o a a.c
> /tmp/cchpQWSr.o: In function `main':
> /tmp/a.c:11: undefined reference to `can_only_set_low_8_bits'
> collect2: ld returned 1 exit status
> Now, this is ugly, and warnings is the only right way, but I still
> very much think that one pci_set8() or pci_clear8() call is way
> better than code to read+mod+write. The reason is that it's how I
> think about these operations; "set bits xy in reg foo."
> Read+mod+write is one lower level of abstraction, and not relevant
> in the context of setting the bits. Ie. I don't want to write/see
> *how* bits get set, I just want to write/see *that* bits get set.

Absolutely. If we assume that the amount of errors per line of code is
constant, reducing the line count by a factor of 3 will reduce the error
rate by a factor of 3 as well. Besides that, improved readability is a
nice side effect.

> I completely understand that everyone else may not have the same
> mental model of these operations, but I hope many enough do..
> (Yes, we've discussed before and it seemed not to be the case.)
> Carl-Daniel mentioned that there may be a risk for confusion between
> pci_set8() and pci_write_config8(), and this was also noted before.
> Isn't it really reasonable to expect that people can actually keep
> track of how those two functions are different?
> I understand that my taste is more terse than most, and if consensus
> is that _set8 _clear8 etc. names suck, then I'd be happy with other
> fun names too, as long as read+mod+write blocks can be replaced with
> single lines of code.

May I suggest alternative names:
pci_and8(), pci_or8()
pci_and_config8(), pci_or_config8()

I don't really care about the _config part of the name, but I think
"and" and "or" make it really obvious what the functions do. And if we
ever want a function which sets and clears bits at the same time,
pci_and_or_config8() makes the function purpose and the parameter order
very clear.

I will post a patch once a naming decision has been reached.

> Like the CAR thing I think this really helps write- and readability,
> and even thinkability, for our code. The latter helps make further
> abstraction easier, which allows more refactoring. Bad goal?

You can also fit more code into a window, and that might help you get a
better overview besides allowing you to read less code if you only care
about accesses to one register.
A further benefit of such read-modify-write functions is avoidance of
register/device mixups. For example, consider the code snippets below:

val = pci_read_config8(dev, 0x13);
val |= 1;
pci_write_config8(dev, 0x18);

val = pci_read_config8(dev, 0x13);
val |= 1;
pci_write_config8(dev2, 0x13);

We can safely assume that neither the changed register nor the changed
device was intentional if no comment is attached. Avoiding such bugs
(which may be caused by cut-paste-modify actions) is worthwhile
especially if we can't pay anyone to look for such mixups in the code.



More information about the coreboot mailing list