Re: [Patch V4 11/42] x86, ioapic: kill unused global variable timer_through_8259

Maciej W. Rozycki
2014-06-10T21:57:36Z

On Tue, 10 Jun 2014, Jiang Liu wrote:

> >> index d4aba16e6bbf..94a56c233e87 100644
> >> --- a/arch/x86/kernel/apic/io_apic.c
> >> +++ b/arch/x86/kernel/apic/io_apic.c
> >> @@ -2638,8 +2638,6 @@ static int __init disable_timer_pin_setup(char *arg)
> >> }
> >> early_param("disable_timer_pin_1", disable_timer_pin_setup);
> >>
> >> -int timer_through_8259 __initdata;
> >> -
> >> /*
> >> * This code may look a bit paranoid, but it's supposed to cooperate with
> >> * a wide range of boards and BIOS bugs. Fortunately only the timer IRQ
> >> @@ -2744,7 +2742,6 @@ static inline void __init check_timer(void)
> >> legacy_pic->unmask(0);
> >> if (timer_irq_works()) {
> >> apic_printk(APIC_QUIET, KERN_INFO "....... works.\n");
> >> - timer_through_8259 = 1;
> >> goto out;
> >> }
> >> /*
> >
> > So how does the new NMI watchdog handle systems that use this '8259A
> > Virtual Wire' mode, pretty common on pre-P6 computers?
> Hi Maciej,
> We just kill the useless variable "timer_through_8259", 8259 virtual
> wire mode is still supported.

No doubt it is useless, the question is why it is unused. If you tracked
it down, you'd come up with this change of mine:

commit 35542c5ebced864776d90d83d1e255016fd4c084
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date: Wed May 21 22:10:22 2008 +0100

x86: I/O APIC: clean up the 8259A on a NMI watchdog failure

There is no point in keeping the 8259A enabled if the I/O APIC NMI
watchdog has failed and the 8259A is not used to pass through regular
timer interrupts. This fixes problems with some systems where some logic
gets confused.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

that introduced that variable. And to answer myself:

commit 072b198a4ad48bd722ec6d203d65422a4698eae7
Author: Don Zickus <dzickus@redhat.com>
Date: Fri Nov 12 11:22:24 2010 -0500

x86, nmi_watchdog: Remove all stub function calls from old nmi_watchdog

Now that the bulk of the old nmi_watchdog is gone, remove all
the stub variables and hooks associated with it.

This touches lots of files mainly because of how the io_apic
nmi_watchdog was implemented. Now that the io_apic nmi_watchdog
is forever gone, remove all its fingers.

Most of this code was not being exercised by virtue of
nmi_watchdog != NMI_IO_APIC, so there shouldn't be anything to
risky here.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: fweisbec@gmail.com
Cc: gorcunov@openvz.org
LKML-Reference: <1289578944-28564-3-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

So the I/O APIC watchdog is gone and '8259A Virtual Wire' mode systems are
not handled anymore. Well, I guess it was a useful hack for its days, but
I understand that maintaining that rather hairy code to support a
minuscule set of old SMP systems (P5 or even i486 UP systems virtually
never had an I/O APIC, unless you just didn't populate additional
processor sockets; P5 SMPs were usually only DP though) still in existence
(mine is still all right, thanks, after 17 years; even the original boxed
CPU fans work great :) ) was considered not worth it. Oh well...

In that case though I think we should clean up some stale comment
references to that watchdog in arch/x86/kernel/apic/io_apic.c too. It may
be worthwhile if code was scrutinised at that time too WRT implicit
hardware setup dependencies as indicated by these comments, as these
comments having been left behind clearly indicate that such a step has
never been made.

And for the record, your clean-up logically belongs to this change:

commit 5f2b0ba4d94b3ac23cbc4b7f675d98eb677a760a
Author: Don Zickus <dzickus@redhat.com>
Date: Fri Nov 12 11:22:23 2010 -0500

x86, nmi_watchdog: Remove the old nmi_watchdog

Now that we have a new nmi_watchdog that is more generic and
sits on top of the perf subsystem, we really do not need the old
nmi_watchdog any more.

In addition, the old nmi_watchdog doesn't really work if you are
using the default clocksource, hpet. The old nmi_watchdog code
relied on local apic interrupts to determine if the cpu is still
alive. With hpet as the clocksource, these interrupts don't
increment any more and the old nmi_watchdog triggers false
postives.

This piece removes the old nmi_watchdog code and stubs out any
variables and functions calls. The stubs are the same ones used
by the new nmi_watchdog code, so it should be well tested.

Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: fweisbec@gmail.com
Cc: gorcunov@openvz.org
LKML-Reference: <1289578944-28564-2-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

that removed the lone use of `timer_through_8259'. So it's been lingering
out there for some 3.5 years. Thanks for sorting this out.

Maciej