Hi,
recently I found a bogus semicolon an if statement. So I was searching for
other possible problems with following command:
find linux-2.6.4-rc2 -name "*.c" -exec grep -Hn "\<if[[:space:]]*(\([^()]\|
([^()]*)\)*)[[:space:]]*;" {} \;
That found following possible problems:
linux-2.6.4-rc2/arch/um/kernel/tt/tracer.c:257: if(WIFEXITED(status)) ;
linux-2.6.4-rc2/arch/i386/kernel/io_apic.c:2200: if (check_nmi_watchdog() <
0);
linux-2.6.4-rc2/arch/i386/kernel/io_apic.c:2224: if (check_nmi_watchdog() <
0);
linux-2.6.4-rc2/drivers/atm/iphase.c:155: if (!desc1) ;
linux-2.6.4-rc2/drivers/net/wan/pc300_drv.c:3664: if (card->chan[i].d.dev);
linux-2.6.4-rc2/drivers/net/tokenring/ibmtr.c:613: else if
(ti->shared_ram_paging == 0xf); /* No paging in adapter */
linux-2.6.4-rc2/drivers/usb/misc/speedtch.c:92:#define DEBUG_ON(x) do { if
(x); } while (0)
linux-2.6.4-rc2/drivers/usb/media/w9968cf.c:3374: if (tuner.tuner != 0);
linux-2.6.4-rc2/drivers/isdn/capi/capiutil.c:438: else if (c <= 0x0f);
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:385: if (Read_hfc(cs,
HFCSX_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:415: if (Read_hfc(cs,
HFCSX_INT_S2));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:1290: if (Read_hfc(cs,
HFCSX_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:131: if (Read_hfc(cs,
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:161: if (Read_hfc(cs,
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:1543: if (Read_hfc(cs,
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/s390/scsi/zfcp_erp.c:2057: if (status ==
ZFCP_ERP_SUCCEEDED) ; /* no further action */
linux-2.6.4-rc2/drivers/scsi/sg.c:356: if (ppos != &filp->f_pos) ; /* FIXME:
Hmm. Seek to the right place, or fail? */
linux-2.6.4-rc2/drivers/scsi/sg.c:514: if (ppos != &filp->f_pos) ; /* FIXME:
Hmm. Seek to the right place, or fail? */
Best regards
Thomas Schlichter
P.S.: Wouldn't it be nice if gcc complained about these mistakes?
Thomas Schlichter <[email protected]> writes:
> P.S.: Wouldn't it be nice if gcc complained about these mistakes?
Among these 18 cases are 13 false positives. :-)
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstra?e 5, 90409 N?rnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Am Dienstag, 9. M?rz 2004 00:43 schrieb Andreas Schwab:
> Thomas Schlichter <[email protected]> writes:
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
>
> Among these 18 cases are 13 false positives. :-)
Yes, but at least it's a very bad coding style, and a warning may be useful to
find the _real_ bugs...
Thomas
Andreas Schwab <[email protected]> wrote:
>
> Thomas Schlichter <[email protected]> writes:
>
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
>
> Among these 18 cases are 13 false positives. :-)
Rename Thomas's script to crappy-code-detector.sh and its hit rate goes to
100% ;)
Thomas Schlichter <[email protected]> wrote:
>
> P.S.: Wouldn't it be nice if gcc complained about these mistakes?
It does, with -W. But -W creates vast amounts of less useful warnings.
On Mon, 08 Mar 2004 at 16:29 +0000, Andrew Morton wrote:
> Andreas Schwab <[email protected]> wrote:
> >
> > Thomas Schlichter <[email protected]> writes:
> >
> > > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
> >
> > Among these 18 cases are 13 false positives. :-)
>
> Rename Thomas's script to crappy-code-detector.sh and its hit rate goes to
> 100% ;)
>
Was this patch so crappy ? http://tinyurl.com/2jbe4 ,
- check_nmi_watchdog();
+ if (check_nmi_watchdog() < 0);
+ timer_ack = !cpu_has_tsc
regards,
Phil
Am Dienstag, 9. M?rz 2004 08:01 schrieb Philippe Elie:
> On Mon, 08 Mar 2004 at 16:29 +0000, Andrew Morton wrote:
~~ snip ~~
> > Rename Thomas's script to crappy-code-detector.sh and its hit rate goes
> > to 100% ;)
>
> Was this patch so crappy ? http://tinyurl.com/2jbe4 ,
>
> - check_nmi_watchdog();
> + if (check_nmi_watchdog() < 0);
> + timer_ack = !cpu_has_tsc
Well, exactly this code made me look for other possible problems... ;-)
As I wrote a few days ago I have problems with that ChangeSet,
(http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
so I did examine it closer.
My results are:
1. The semicolons behind the if()'s cannot be there intentionally.
2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and
as my CPU has a TSC, this cannot be correct for me:
- timer_ack = 1;
+ if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
+ timer_ack = 1;
+ else
+ timer_ack = !cpu_has_tsc;
I changed that if(...) to
if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
which works fine for me here, but I am not 100% sure if this is what the
author of the original patch ment and if it still fixes the original
problem...
The patch which makes these changes is attached. Perhaps someone else wants to
test it, too...
Thomas Schlichter
P.S.: I tested it with an AMD Athlon 3000+ on a board with a VIA KT400 chipset
and enabled ACPI and IO-APIC.
Am Dienstag, 9. M?rz 2004 01:33 schrieb Andrew Morton:
> Thomas Schlichter <[email protected]> wrote:
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
>
> It does, with -W. But -W creates vast amounts of less useful warnings.
Ah, OK... Thanks!
On Tue, 09 Mar 2004 at 12:08 +0000, Thomas Schlichter wrote:
> As I wrote a few days ago I have problems with that ChangeSet,
> (http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
> so I did examine it closer.
errmm, http://tinyurl.com/2jbe4
Maciej, you wrote this patch, any comment ?
> so I did examine it closer.
>
> My results are:
> 1. The semicolons behind the if()'s cannot be there intentionally.
> 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and
> as my CPU has a TSC, this cannot be correct for me:
> - timer_ack = 1;
> + if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> + timer_ack = 1;
> + else
> + timer_ack = !cpu_has_tsc;
I don't get the figure, this code in check_timer() is called by
setup_IO_APIC so APIC_INTEGRATED(ver) is always 0 ?
> I changed that if(...) to
> if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
> which works fine for me here, but I am not 100% sure if this is what the
> author of the original patch ment and if it still fixes the original
> problem...
regards,
Phil
On Wed, 10 Mar 2004, Philippe Elie wrote:
> > 1. The semicolons behind the if()'s cannot be there intentionally.
> > 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and
> > as my CPU has a TSC, this cannot be correct for me:
> > - timer_ack = 1;
> > + if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> > + timer_ack = 1;
> > + else
> > + timer_ack = !cpu_has_tsc;
>
> I don't get the figure, this code in check_timer() is called by
> setup_IO_APIC so APIC_INTEGRATED(ver) is always 0 ?
Nope, that will catch the older external "local" APICs like the 82489s (P5
etc), most IOAPICs will return 0x11 or higher for version#
On Tue, 9 Mar 2004, Thomas Schlichter wrote:
> 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and
> as my CPU has a TSC, this cannot be correct for me:
> - timer_ack = 1;
> + if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> + timer_ack = 1;
> + else
> + timer_ack = !cpu_has_tsc;
>
> I changed that if(...) to
> if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
> which works fine for me here, but I am not 100% sure if this is what the
> author of the original patch ment and if it still fixes the original
> problem...
You need timer_ack set to one when either:
1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
(i.e. the 82489DX),
or:
2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
way; we check three variations) and the TSC is non-functional (absent or
disabled).
Since you have an integrated APIC and you use the TSC, you may have
timer_ack set to zero. That saves a few (possibly slow) I/O accesses and
works around problems that may arise due 8259A clone (in)compatibility or
bugs in SMM firmware.
Maciej
--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +
On Wed, 10 Mar 2004, Philippe Elie wrote:
> > As I wrote a few days ago I have problems with that ChangeSet,
> > (http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
> > so I did examine it closer.
>
> errmm, http://tinyurl.com/2jbe4
>
> Maciej, you wrote this patch, any comment ?
Yep, that's a stupid typo, but the bug would only trigger for a system
that would have:
1. a discrete 82489DX APIC,
2. a functional TSC,
3. a timer interrupt working through the I/O APIC,
4. a working I/O APIC NMI watchdog.
Such systems used to actually exist, but you'd have a hard time trying to
find one.
Here's an obvious update. Thomas, thanks for spotting it.
Maciej
--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +
patch-2.6.4-timer_ack-fix-0
diff -up --recursive --new-file linux-2.6.4.macro/arch/i386/kernel/io_apic.c linux-2.6.4/arch/i386/kernel/io_apic.c
--- linux-2.6.4.macro/arch/i386/kernel/io_apic.c 2004-03-17 17:09:29.000000000 +0000
+++ linux-2.6.4/arch/i386/kernel/io_apic.c 2004-03-17 17:11:07.000000000 +0000
@@ -2195,7 +2195,7 @@ static inline void check_timer(void)
disable_8259A_irq(0);
setup_nmi();
enable_8259A_irq(0);
- if (check_nmi_watchdog() < 0);
+ if (check_nmi_watchdog() < 0)
timer_ack = !cpu_has_tsc;
}
return;
@@ -2219,7 +2219,7 @@ static inline void check_timer(void)
add_pin_to_irq(0, 0, pin2);
if (nmi_watchdog == NMI_IO_APIC) {
setup_nmi();
- if (check_nmi_watchdog() < 0);
+ if (check_nmi_watchdog() < 0)
timer_ack = !cpu_has_tsc;
}
return;
"Maciej W. Rozycki" <[email protected]> wrote:
>
> You need timer_ack set to one when either:
>
> 1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
> (i.e. the 82489DX),
>
> or:
>
> 2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
> way; we check three variations) and the TSC is non-functional (absent or
> disabled).
>
I still have a couple of NMI patches in -mm:
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
What should we do with these?
On Wed, 17 Mar 2004, Andrew Morton wrote:
> I still have a couple of NMI patches in -mm:
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>
> What should we do with these?
I think we should ask Mikael Pettersson as he is the local APIC watchdog
expert. Mikael?
--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +
Maciej W. Rozycki writes:
> On Wed, 17 Mar 2004, Andrew Morton wrote:
>
> > I still have a couple of NMI patches in -mm:
> >
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
> >
> > What should we do with these?
>
> I think we should ask Mikael Pettersson as he is the local APIC watchdog
> expert. Mikael?
Will do. Is there a problem with them, or do you just want them
reviewed for merging into 2.6.5-rc?
/Mikael
Mikael Pettersson <[email protected]> wrote:
>
> Maciej W. Rozycki writes:
> > On Wed, 17 Mar 2004, Andrew Morton wrote:
> >
> > > I still have a couple of NMI patches in -mm:
> > >
> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
> > >
> > > What should we do with these?
> >
> > I think we should ask Mikael Pettersson as he is the local APIC watchdog
> > expert. Mikael?
>
> Will do. Is there a problem with them, or do you just want them
> reviewed for merging into 2.6.5-rc?
>
They seem to work OK - I did a batch of testing with various setups. But a
retest wouldn't hurt.
We mainly need a review and general finish-it-off-and-bless-it please.
Am Mittwoch, 17. M?rz 2004 17:51 schrieb Maciej W. Rozycki:
~~ snip ~~
> You need timer_ack set to one when either:
>
> 1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
> (i.e. the 82489DX),
>
> or:
>
> 2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
> way; we check three variations) and the TSC is non-functional (absent or
> disabled).
>
> Since you have an integrated APIC and you use the TSC, you may have
> timer_ack set to zero. That saves a few (possibly slow) I/O accesses and
> works around problems that may arise due 8259A clone (in)compatibility or
> bugs in SMM firmware.
>
> Maciej
Well, my timer interrupt goes through the IO-APIC but I do have a functional
TSC. Nevertheless my system requires timer_ack to be set... If it isn't, my
CPU does not utilize its C2 state...
Thomas
On Fri, 19 Mar 2004, Thomas Schlichter wrote:
> Well, my timer interrupt goes through the IO-APIC but I do have a functional
> TSC. Nevertheless my system requires timer_ack to be set... If it isn't, my
> CPU does not utilize its C2 state...
Hmm, I wonder if there's any relationship between the state of the local
APIC and your observation. Can you please see if the following hack
changes anything (this assumes you have your timer IRQ directly connected
to an I/O APIC input)?
--
+ Maciej W. Rozycki, Technical University of Gdansk, Poland +
+--------------------------------------------------------------+
+ e-mail: [email protected], PGP key available +
--- io_apic.c.macro 2004-03-19 20:13:44.000000000 +0000
+++ io_apic.c 2004-03-19 20:15:23.000000000 +0000
@@ -1613,7 +1613,7 @@ static inline void check_timer(void)
timer_ack = 1;
else
timer_ack = !cpu_has_tsc;
- enable_8259A_irq(0);
+ disable_8259A_irq(0);
pin1 = find_isa_irq_pin(0, mp_INT);
pin2 = find_isa_irq_pin(0, mp_ExtINT);
Am Freitag, 19. M?rz 2004 21:30 schrieb Maciej W. Rozycki:
> On Fri, 19 Mar 2004, Thomas Schlichter wrote:
> > Well, my timer interrupt goes through the IO-APIC but I do have a
> > functional TSC. Nevertheless my system requires timer_ack to be set... If
> > it isn't, my CPU does not utilize its C2 state...
>
> Hmm, I wonder if there's any relationship between the state of the local
> APIC and your observation. Can you please see if the following hack
> changes anything (this assumes you have your timer IRQ directly connected
> to an I/O APIC input)?
I had to apply this hack by hand as your line numbers don't match mine (I use
2.6.4-mm2) but I' sorry, this hack doesn't change anything for me... ;-(
Thomas
On Thu, 18 Mar 2004 01:52:36 -0800, Andrew Morton wrote:
>Mikael Pettersson <[email protected]> wrote:
>>
>> Maciej W. Rozycki writes:
>> > On Wed, 17 Mar 2004, Andrew Morton wrote:
>> >
>> > > I still have a couple of NMI patches in -mm:
>> > >
>> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
>> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>> > >
>> > > What should we do with these?
>> >
>> > I think we should ask Mikael Pettersson as he is the local APIC watchdog
>> > expert. Mikael?
>>
>> Will do. Is there a problem with them, or do you just want them
>> reviewed for merging into 2.6.5-rc?
>>
>
>They seem to work OK - I did a batch of testing with various setups. But a
>retest wouldn't hurt.
>
>We mainly need a review and general finish-it-off-and-bless-it please.
'nmi_watchdog-local-apic-fix.patch' looks Ok, although I
would prefer if it didn't put a 'static' on nmi_perfctr_msr:
the perfctr driver needs access to it.
I'm not happy with 'nmi-1-hz.patch'. The real problem is that
SMP with nmi_watchdog=2 initialises the lapic NMI watchdog but
doesn't check it and therefore doesn't reduce nmi_hz.
This is an SMP bug, but instead of fixing it the patch removes
the check from UP and changes nmi.c to compensate.
This would be Ok if check_nmi_watchdog() with nmi_watchdog=2
was redundant, but I don't believe it is. It has exposed bugs
and unexpected hardware changes before, and so should remain.
I propose the patch below instead. It changes smpboot.c to do a
check_nmi_watchdog() at the appropriate place, which fixes the
high NMI frequency problem w/o changing anything else.
I've verified that it solves the problem on my MP-capable UP box.
/Mikael
diff -ruN linux-2.6.5-rc2/arch/i386/kernel/smpboot.c linux-2.6.5-rc2.smpboot-lapic-watchdog-fix/arch/i386/kernel/smpboot.c
--- linux-2.6.5-rc2/arch/i386/kernel/smpboot.c 2004-03-11 14:01:25.000000000 +0100
+++ linux-2.6.5-rc2.smpboot-lapic-watchdog-fix/arch/i386/kernel/smpboot.c 2004-03-20 17:21:30.113862000 +0100
@@ -1107,6 +1107,9 @@
}
}
+ if (nmi_watchdog == NMI_LOCAL_APIC)
+ check_nmi_watchdog();
+
smpboot_setup_io_apic();
setup_boot_APIC_clock();
On Wed, 17 Mar 2004, Andrew Morton wrote:
> I still have a couple of NMI patches in -mm:
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
That looks like a decent bug fix.
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
Didn't _you_ want that one? ;)
On Sat, 20 Mar 2004, Zwane Mwaikambo wrote:
> On Wed, 17 Mar 2004, Andrew Morton wrote:
>
> > I still have a couple of NMI patches in -mm:
> >
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
>
> That looks like a decent bug fix.
>
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>
> Didn't _you_ want that one? ;)
Excuse this little outburt, i seem to be reading an old mbox.