* logic error in radeonfb.
@ 2004-01-23 6:35 davej
2004-01-23 6:46 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: davej @ 2004-01-23 6:35 UTC (permalink / raw)
To: linux-kernel; +Cc: torvalds, akpm, benh
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: logic error in radeonfb.
2004-01-23 6:35 logic error in radeonfb davej
@ 2004-01-23 6:46 ` Benjamin Herrenschmidt
2004-01-23 6:54 ` Dave Jones
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2004-01-23 6:46 UTC (permalink / raw)
To: davej; +Cc: Linux Kernel list, Linus Torvalds, Andrew Morton
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: logic error in radeonfb.
2004-01-23 6:46 ` Benjamin Herrenschmidt
@ 2004-01-23 6:54 ` Dave Jones
2004-01-23 7:03 ` Benjamin Herrenschmidt
2004-01-23 11:24 ` John Levon
0 siblings, 2 replies; 6+ messages in thread
From: Dave Jones @ 2004-01-23 6:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Linux Kernel list, Linus Torvalds, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: logic error in radeonfb.
2004-01-23 6:54 ` Dave Jones
@ 2004-01-23 7:03 ` Benjamin Herrenschmidt
2004-01-23 11:24 ` John Levon
1 sibling, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2004-01-23 7:03 UTC (permalink / raw)
To: Dave Jones; +Cc: Linux Kernel list, Linus Torvalds, Andrew Morton
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: logic error in radeonfb.
2004-01-23 6:54 ` Dave Jones
2004-01-23 7:03 ` Benjamin Herrenschmidt
@ 2004-01-23 11:24 ` John Levon
2004-01-23 13:53 ` Dave Jones
1 sibling, 1 reply; 6+ messages in thread
From: John Levon @ 2004-01-23 11:24 UTC (permalink / raw)
To: Dave Jones, Linux Kernel list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: logic error in radeonfb.
2004-01-23 11:24 ` John Levon
@ 2004-01-23 13:53 ` Dave Jones
0 siblings, 0 replies; 6+ messages in thread
From: Dave Jones @ 2004-01-23 13:53 UTC (permalink / raw)
To: John Levon; +Cc: Linux Kernel list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-01-23 13:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-01-23 6:35 logic error in radeonfb davej
2004-01-23 6:46 ` Benjamin Herrenschmidt
2004-01-23 6:54 ` Dave Jones
2004-01-23 7:03 ` Benjamin Herrenschmidt
2004-01-23 11:24 ` John Levon
2004-01-23 13:53 ` Dave Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox