Re: [PATCH] x86: check_timer cleanup

["Maciej W. Rozycki" <macro@linux-mips.org>]

On Mon, 9 Feb 2009, Ingo Molnar wrote:

> > Impact: remove extra unmask, and pair disable/enable
> > 
> > for edge trigger io-apic routing, already unmasked via setup_IO_APIC_irq,
> > so don't unmask it again.
> > 
> > also do call local_irq_disable() between timer_irq_works(), between it does call
> > local_irq_enable() inside.
> > also remove not needed apic version reading for 64bit
> > 
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > 
> > ---
> >  arch/x86/kernel/io_apic.c |   30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> applied to tip/x86/apic, thanks Yinghai. Maybe this even solves some of the spurious 
> detection troubles we had in this code.

 I'll see if I can find time to look into it in details, but it looks 
dodgy to me.  The thing is setup_timer_IRQ0_pin() is special -- due to 
line rerouting it may not necessarily be called for an input already set 
up by setup_IO_APIC_irqs().  I recall keeping this mask set until ready 
for a reason when doing the cleanup last year -- I think old code actually 
used to do what the change now reintroduces.  What's the reason of this 
change?

 And timer_irq_works() used to restore the local interrupt mask within 
itself -- perhaps it should be brought back rather than cluttering code 
with local_irq_disable() calls throughout?

 What problems are you referring to, Ingo?  Perhaps they should be looked 
into specifically?

  Maciej