logic error in radeonfb.

Looks like another instance of a ! in the wrong place.

Dave

diff -urpN --exclude-from=/home/davej/.exclude bk-linus/drivers/video/radeonfb.c linux-2.5/drivers/video/radeonfb.c
@@ -2319,7 +2319,7 @@ static int radeon_set_backlight_enable(i
 	lvds_gen_cntl |= (LVDS_BL_MOD_EN | LVDS_BLON);
 	if (on && (level > BACKLIGHT_OFF)) {
 		lvds_gen_cntl |= LVDS_DIGON;
-		if (!lvds_gen_cntl & LVDS_ON) {
+		if (!(lvds_gen_cntl & LVDS_ON)) {
 			lvds_gen_cntl &= ~LVDS_BLON;
 			OUTREG(LVDS_GEN_CNTL, lvds_gen_cntl);
 			(void)INREG(LVDS_GEN_CNTL);

Ohh, and _oooold_ bug fixed a long time ago in 2.4. There may actually be another occurence of this one elsewhere iirc. I'll check that. Note that this code is powermac specific anyway and that old radeonfb doesn't work very well on a lot of powermacs, so it's not very urgent. The new radeonfb which has that fixed for a long time will get in along with the fbdev updates as soon as I'm finished cleaning them up.

Ben.

Benjamin Herrenschmidt <[email protected]>

Back then someone came up with a cool one-liner that grepped for suspicious if's with !'s, it seems no-one ever did the same for 2.6, as there were a few others (see seperate mails for patches).

Ok, If this is going to make your merge more difficult, feel free to ignore it.

Dave

The "new" radeonfb is a separate driver, so fixing that alone should be fine.

Ben.

Actually I did make a one-liner for 2.6, and found rather a lot (15 or so if I remember correctly). I also went as far as hacking up something in gcc, but it was too flaky to go in and I haven't got round to fixing it up yet.

Some of them were right under our noses for ages:

http://linus.bkbits.net:8080/linux-2.5/user=levon/[email protected]?nav=!-|index.html|stats|!+|index.html|ChangeSet|[email protected]

If you like I can see if I can dig up the gcc patch or the one-liner.

regards,
john

Khendon's Law:
If the same point is made twice by the same person, the thread is over.

Yep, that's where I got the idea for this (seems no-one did the same for the 2.6 tree). Here's the one-liner..

find ${1:-.} -name "*.c" -type f | xargs grep -En '![a-zA-Z0-9_ ]+(\|[^|]|\&[^&])|([^|]\||[^&]\&) *!' | grep -v SCCS

As well as the misplaced brackets, it also catches if (foo & bar) where && was probably intended (ditto | instead of ||).
It does miss some cases though. Whereas this..

find ${1:-.} -name "*.c" -type f | xargs grep -En 'if[\ ]\(' | grep -v SCCS | grep \)[\ ]\[\|\&][\ ]

turns up a few more, but also a lot more false positives.

And if you think the number of bugs it turned up in the kernel is unfunny, you should see the results when you run it on a source tree of an unpacked distro. It's Un-be-lievable how common this problem is. It's not every week you get to do patches to ~80 projects in 3 days 8-)

I tried coming up with some regexps for other 'stupid errors', things like

	if (foo);
		bar 

or the likes, I got 1-2 real hits out of zillions of lines of code as opposed to the few dozen misplaced brackets and '|' instead of '||'

Fun.

Dave