Re: [patch 2.6.18] genirq: remove oops with fasteoi irq_chip descriptors

[David Brownell <david-b@pacbell.net>]

On Wednesday 27 September 2006 2:38 pm, Thomas Gleixner wrote:
> On Fri, 2006-09-22 at 06:43 -0700, David Brownell wrote:
> > The irq handler code can oops when used with an irq_chip with just
> > enable/disable/eoi methods, appropriate for handle_fasteoi_irq(),
> > either by (a) uninstalling, or (b) using it with a chained handler.
> > 
> > The problem was that the original code expected there would always
> > be mask/unmask/ack methods, and the fix is to instead use methods
> > which are always present and which more closely correspond to the
> > flag manipulation being done.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> NACK. 

Explain yourself then ...


> # grep startup kernel/irq/manage.c
> 307:                    if (desc->chip->startup)
> 308:                            desc->chip->startup(irq);
> 
> # grep shutdown kernel/irq/manage.c
> 386:                            if (desc->chip->shutdown)
> 387:                                    desc->chip->shutdown(irq);
> 
> startup() and shutdown() are optional and mostly there to keep the not
> converted do_IRQ() users working.

As you should know, irq_chip_set_defaults() ensures that every
irqchip has startup() and shutdown() methods.  Their default
implementations use enable() and disable() ... which in turn
have default implementations using mask()/unmask(), for use
with non-EIO handlers. 

So what's the correct fix then ... use enable() and disable()?
Oopsing isn't OK...


> This patch will simply break _all_ ARM and PowerPC in one go and as well
> the i386 and x86_64 conversion which is in -mm.

I can understand breaking patches in MM that I've not seen, but not about
breaking anything else.

It was certainly _tested_ on a 2.6.18 ARM board, so you're clearly wrong
about at least that part of your feedback as well as the bits about the
shartup and shutdown calls being "optional" (in any practical sense, since
they are in fact _always_ present).

- Dave


> 	tglx
> 
> > --- d26.rc4test.orig/kernel/irq/chip.c	2006-09-21 16:41:11.000000000 -0700
> > +++ d26.rc4test/kernel/irq/chip.c	2006-09-21 18:04:07.000000000 -0700
> > @@ -482,10 +482,8 @@ __set_irq_handler(unsigned int irq,
> >  
> >  	/* Uninstall? */
> >  	if (handle == handle_bad_irq) {
> > -		if (desc->chip != &no_irq_chip) {
> > -			desc->chip->mask(irq);
> > -			desc->chip->ack(irq);
> > -		}
> > +		if (desc->chip != &no_irq_chip)
> > +			desc->chip->shutdown(irq);
> >  		desc->status |= IRQ_DISABLED;
> >  		desc->depth = 1;
> >  	}
> > @@ -495,7 +493,7 @@ __set_irq_handler(unsigned int irq,
> >  		desc->status &= ~IRQ_DISABLED;
> >  		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
> >  		desc->depth = 0;
> > -		desc->chip->unmask(irq);
> > +		desc->chip->startup(irq);
> >  	}
> >  	spin_unlock_irqrestore(&desc->lock, flags);
> >  }
>