Re: [PATCH] IRQ: prevent enabling of previously disabled interrupt

[Andi Kleen <ak@muc.de>]

I guess the best person to review this is Ingo.

Full quote:

On Tue, Mar 07, 2006 at 03:55:45AM -0800, Andrew Morton wrote:
> "lgeek@frontiernet.net" <lgeek@frontiernet.net> wrote:
> >
> > Hi,
> >    This fix prevents re-disabling and enabling of a previously disabled 
> > interrupt in 2.6.16-rc5.  On an SMP system with irq balancing enabled; 
> > If an interrupt is disabled from within its own interrupt context with 
> > disable_irq_nosync and is also earmarked for processor migration, the 
> > interrupt is blindly moved to the other processor and enabled without 
> > regard for its current "enabled" state.  If there is an interrupt  
> > pending, it will unexpectedly invoke the irq handler on the new irq 
> > owning processor (even though the irq was previously disabled)
> > 
> >    The more intuitive fix would be to invoke disable_irq_nosync and 
> > enable_irq, but since we already have the desc->lock from __do_IRQ, we 
> > cannot call them directly.  Instead we can use the same logic to 
> > disable and enable found in disable_irq_nosync and enable_irq, with 
> > regards to the desc->depth.
> > 
> >    This now prevents a disabled interrupt from being re-disabled, and 
> > more importantly prevents a disabled interrupt from being incorrectly 
> > enabled on a different processor.
> > 
> > Signed-off-by: Bryan Holty <lgeek@frontiernet.net>
> > 
> > --- 2.6.16-rc5/include/linux/irq.h
> > +++ b/include/linux/irq.h
> > @@ -155,9 +155,13 @@
> > 	 * Being paranoid i guess!
> > 	 */
> > 	if (unlikely(!cpus_empty(tmp))) {
> > -		desc->handler->disable(irq);
> > +		if (likely(!desc->depth++))
> > +			desc->handler->disable(irq);
> > +
> > 		desc->handler->set_affinity(irq,tmp);
> > -		desc->handler->enable(irq);
> > +
> > +		if (likely(!--desc->depth))
> > +			desc->handler->enable(irq);
> > 	}
> > 	cpus_clear(pending_irq_cpumask[irq]);
> > }
> 
> But desc->lock isn't held here.  We need that for the update to ->depth (at
> least).
> 
> And we can't take it here because one of the two ->end callers in __do_IRQ
> already holds that lock.  Possibly we should require that ->end callers
> hold the lock, but that would incur considerable cost for cpu-local
> interrupts.
> 
> So we'd need to require that ->end gets called outside the lock for
> non-CPU-local interrupts.  I'm not sure what the implications of that would
> be - the ->end handlers don't need to be threaded at present and perhaps we
> could put hardware into a bad state?
> 
> Or we add a new ->local_end, just for the CPU-local IRQs.