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(); \ 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.
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.
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?
//Peter
On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge peter@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);
but why wouldn't the compiler warn about using a dword where a u8 is required?
void pci_write_config8(device_t dev, unsigned where, uint8_t val)
There's no warning of any kind?
ron
On 04.10.2010 01:35, ron minnich wrote:
On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge peter@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);
but why wouldn't the compiler warn about using a dword where a u8 is required?
void pci_write_config8(device_t dev, unsigned where, uint8_t val)
There's no warning of any kind?
Apparently enabling warnings will break compilation if I understood Uwe correctly.
Regards, Carl-Daniel
On Sun, Oct 3, 2010 at 4:41 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Apparently enabling warnings will break compilation if I understood Uwe correctly.
so we're going to build this complex C pre-processor macro foo, further extending our dependence on the way the compilers work, when the real issue is that we've disabled warnings?
And we'll fix one case, but we won't fix all the other potential errors involving incorrect types?
After all, someone can still assign a u32 to a u8 and still get things lost.
I think we need to fix the real problem, the disabled warnings.
ron
ron minnich wrote:
Apparently enabling warnings will break compilation if I understood Uwe correctly.
so we're going to build this complex C pre-processor macro foo, further extending our dependence on the way the compilers work, when the real issue is that we've disabled warnings?
That's not what I suggested. I certainly don't think that's right. I don't think Carl-Daniel does either. It was just an explanation for why the compiler didn't complain.
I think we need to fix the real problem, the disabled warnings.
I agree. I don't think this thread is so much about that, but it would also be easy to have a check for this error (without special macro foo) in the functions that I'd like to add.
//Peter
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
On Sun, Oct 3, 2010 at 4:39 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
May I suggest alternative names: pci_and8(), pci_or8() pci_and_config8(), pci_or_config8()
I am sure this it bike shedding, but here are a few other options for the name scheme: * pci_bit_and8() * pci_bitwise_and8() * pci_bit_andb() * pci_bitwise_andb()
I am waffling between preferring pci_bitwise_andb() or pci_bitwise_and8() for readability.
Thanks, wt
Warren Turkal wrote:
I am sure this it bike shedding, but here are a few other options for the name scheme:
- pci_bit_and8()
- pci_bitwise_and8()
- pci_bit_andb()
- pci_bitwise_andb()
Don't like _set and _clear at all?
I am waffling between preferring pci_bitwise_andb() or pci_bitwise_and8() for readability.
I think it's a good idea to be consistent with the other functions, which are all using the numeric size.
//Peter
On Sun, Oct 3, 2010 at 10:03 PM, Peter Stuge peter@stuge.se wrote:
Warren Turkal wrote:
I am sure this it bike shedding, but here are a few other options for the name scheme:
- pci_bit_and8()
- pci_bitwise_and8()
- pci_bit_andb()
- pci_bitwise_andb()
Don't like _set and _clear at all?
What about _setbits_config8 and _clearbits_config8? That might be more readable.
I am waffling between preferring pci_bitwise_andb() or pci_bitwise_and8() for readability.
I think it's a good idea to be consistent with the other functions, which are all using the numeric size.
Good point.
Current list as I see it: * pci_{set,clear}bits_config8() * pci_{set,clear}8() * pci_bit_{or,and}8() * pci_bitwise_{or,and}8()
Did I miss any?
I currently prefer pci_{set,clear}bits_config8().
Thanks, wt
On Sun, Oct 3, 2010 at 5:10 PM, Peter Stuge peter@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);
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.)
How and where are compiler warnings disabled for this code?
Thanks, Myles
On Mon, Oct 04, 2010 at 02:02:12PM -0600, Myles Watson wrote:
On Sun, Oct 3, 2010 at 5:10 PM, Peter Stuge peter@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);
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.)
How and where are compiler warnings disabled for this code?
That was a misunderstanding I think, I mentioed (on IRC) warnings being disabled for some parts of the coreboot code (tiny bootblock and others), but that was not related to this specific code (sb700_sm.c).
The file gets compiled with:
gcc -m32 -Wa,--divide -fno-stack-protector -Wl,--build-id=none -MMD -Isrc -Isrc/include -Ibuild -Isrc/arch/i386/include -Isrc/devices/oprom/include -include /home/uwe/v4_bar/build/config.h -Os -nostdinc -pipe -g -nostdlib -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wno-trigraphs -Wstrict-aliasing -Wshadow -Werror -fno-common -ffreestanding -fno-builtin -fomit-frame-pointer -c -o build/southbridge/amd/sb700/sb700_sm.driver.o src/southbridge/amd/sb700/sb700_sm.c
However, that doesn't expose the bug in this case. What would work is to add '-Wconversion', that finds the bug:
src/southbridge/amd/sb700/sb700_sm.c:88: error: conversion to ‘uint8_t’ from ‘u32’ may alter its value
However, it also spews 30 or more of these lines for every single file we have in coreboot, so it's not really a practical option to enable permanently (or at least not at this point in time).
Uwe.
On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge peter@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
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
Peter Stuge peter@stuge.se writes:
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.
I'll add my couple of cents here as well: I think this change is a win. I acknowledge the concern about mixed idioms, but I think the simplification in the general case outweighs it.
I think I prefer the names pci_set_configX and pci_clear_configX, though. In the cases where you need to mix idioms, I feel they make it more obvious that you deal with the same components as pci_{read,write}_configX. I'd also consider using "setb" and "clearb" just to make it explicit that the functions operate on individual bits and not the entire register, but that's secondary.
(Finally, I think it might be worthwhile to add pci_clear_and_set_configX as well.)
On Mon, Oct 4, 2010 at 6:14 PM, ron minnich rminnich@gmail.com wrote:
On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge peter@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
Corey Osgood corey.osgood@gmail.com writes:
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?
Personally, I wouldn't implement this primarily to catch this particular foolish mistake. I think this makes sense on its own as a useful abstraction of a very common pattern that contains more redundancy than needed. The intent of the code becomes clearer and the signal-to-noise ratio increases.
(Also, this has already snuck in, see for instance set_nbcfg_enable_bits in rs780_cmn.c: http://lxr.linux.no/#coreboot+r5917/src/southbridge/amd/rs780/rs780_cmn.c#L1... -- which I think is confusingly named, but which provides exactly this abstraction.)
-
Arne Georg Gleditsch -
Carl-Daniel Hailfinger -
Corey Osgood -
Myles Watson -
Peter Stuge -
ron minnich -
Uwe Hermann -
Warren Turkal