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.
Regards, Carl-Daniel