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

[Maciej W. Rozycki]

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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
    Signed-off-by: Ingo Molnar <mingo@xxxxxxx>

that introduced that variable.  And to answer myself:

commit 072b198a4ad48bd722ec6d203d65422a4698eae7
Author: Don Zickus <dzickus@xxxxxxxxxx>
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@xxxxxxxxxx>
    Cc: fweisbec@xxxxxxxxx
    Cc: gorcunov@xxxxxxxxxx
    LKML-Reference: <1289578944-28564-3-git-send-email-dzickus@xxxxxxxxxx>
    Signed-off-by: Ingo Molnar <mingo@xxxxxxx>

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@xxxxxxxxxx>
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@xxxxxxxxxx>
    Cc: fweisbec@xxxxxxxxx
    Cc: gorcunov@xxxxxxxxxx
    LKML-Reference: <1289578944-28564-2-git-send-email-dzickus@xxxxxxxxxx>
    Signed-off-by: Ingo Molnar <mingo@xxxxxxx>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/