ron minnich wrote:
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.
Other kids doing it isn't neccessarily a good reason.
simple complex mixed idioms.
I think the ratio of simple vs. complex operations is significant. The majority I've seen in the code are simple, but granted I haven't read every file. I agree that mixed idioms are annoying if nothing else, but I think the benefit from replacing all the simple cases is important enough to do it.
Those complex cases will stand out more and may thus get more careful review, hopefully finding bugs earlier. There's a high ratio of noise and repetition in the quoted code.
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.
Maybe we could try them on for size for a while anyway? I wrote a semantic patch to have coccinelle do this change. The spatch isn't complete however, I should also try to make it remove variables that are now unused. Anyway, both .cocci and resulting .patch are attached.
I was surprised but happy to discover that comments in the third file src/cpu/amd/quadcore/quadcore.c actually match the code almost word for word after the change. Apparently I'm not completely alone in my way of thinking about these things. ;)
Testing this it became obvious why I prefer set and clear: they take un-modified bits as input, whereas pci_and8() would require callers to do the ~() trick, which I also think is very nice to get rid of.
Commands used to generate patch and diffstat: grep -lr =.*pci_read_config src|grep -v '/.svn/'|uniq|xargs spatch -sp_file pci_set_clear.cocci > pci_set_clear.patch sed -e '/^---/{h;d}' -e '/^+++/{p;x}' -e 's,^--- src/,+++ src/,' pci_set_clear.patch |sed 's,^+++ /tmp,--- /tmp,'|diffstat > pci_set_clear.diffstat
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.
I completely agree that we need to get the deal with warnings sorted. I will admit that I used the bug a little bit as an excuse to bring these functions up again, because I associated strongly when looking at the bug.
//Peter