logic error in radeonfb.

logic error in radeonfb.

[davej]

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
--- bk-linus/drivers/video/radeonfb.c	2004-01-21 15:58:42.000000000 +0000
+++ linux-2.5/drivers/video/radeonfb.c	2004-01-21 17:48:54.000000000 +0000
@@ -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);

Re: logic error in radeonfb.

[Benjamin Herrenschmidt]

On Fri, 2004-01-23 at 17:35, davej@redhat.com wrote:
> Looks like another instance of a ! in the wrong place.

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.

>     Dave
> 
> diff -urpN --exclude-from=/home/davej/.exclude bk-linus/drivers/video/radeonfb.c linux-2.5/drivers/video/radeonfb.c
> --- bk-linus/drivers/video/radeonfb.c	2004-01-21 15:58:42.000000000 +0000
> +++ linux-2.5/drivers/video/radeonfb.c	2004-01-21 17:48:54.000000000 +0000
> @@ -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);
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: logic error in radeonfb.

[Dave Jones]

On Fri, Jan 23, 2004 at 05:46:35PM +1100, Benjamin Herrenschmidt wrote:
 > On Fri, 2004-01-23 at 17:35, davej@redhat.com wrote:
 > > Looks like another instance of a ! in the wrong place.
 > 
 > 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.

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).

 > 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.

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

	Dave

Re: logic error in radeonfb.

[Benjamin Herrenschmidt]

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

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

Ben.

Re: logic error in radeonfb.

[John Levon]

On Fri, Jan 23, 2004 at 06:54:10AM +0000, Dave Jones wrote:

> 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).

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/patch@1.889.272.4?nav=!-|index.html|stats|!+|index.html|ChangeSet|cset@1.889.272.4

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.

Re: logic error in radeonfb.

[Dave Jones]

On Fri, Jan 23, 2004 at 11:24:44AM +0000, John Levon wrote:

 > > 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).
 > 
 > 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/patch@1.889.272.4?nav=!-|index.html|stats|!+|index.html|ChangeSet|cset@1.889.272.4
 > If you like I can see if I can dig up the gcc patch or the one-liner.

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