Re: [patch 3/3] genirq: mark io_apic level interrupts to avoid resend

[Jarek Poplawski <jarkao2@o2.pl>]

On Tue, Aug 14, 2007 at 10:10:32AM +0200, Thomas Gleixner wrote:
> On Tue, 2007-08-14 at 09:12 +0200, Jarek Poplawski wrote:
> > > No, the point is that the resend is suppressed for all interrupts which
> > > are marked with IRQ_LEVEL:
> > > 
> > >         /*
> > >          * We do not resend level type interrupts. Level type
> > >          * interrupts are resent by hardware when they are still
> > >          * active.
> > >          */
> > >         if ((status & (IRQ_LEVEL | IRQ_PENDING | IRQ_REPLAY)) == IRQ_PENDING) {
> > > 	....
> > > 
> > > This is not witchcraft, this is how the hardware works.
> > 
> > Sorry! It's probably something with my English: I like this flag very
> > much! But I simply can't find where this flag is set for any irq using
> > handle_level_irq, or, otherwise, can't understand why it's not set
> > (because in this case I don't think not setting IRQ_PENDING by the
> > handler should be enough).
> 
> handle_level_irq() does not set the PENDING bit on delayed disable.
> 
> > > 
> > It's different because e.g. for x86_64 fasteoi level type irqs were
> > masked during disable_irq, so there was very small probability any
> > irq were skipped, plus the state of io_apic was different from this
> > point (regarding this irq). Now it's for sure many interrupts could
> > be 'missing'.
> 
> No. Let me explain:
> 
> Before delayed disable:
> 
> 	irq_disable(); /* Mask in hardware */
> 	....
> 	-> Interrupt line is asserted. No interrupt due to hardware mask
> 	....
> 	irq_enable();  /* Unmask */
> 	
> 	-> When interrupt line is still active, then the interrupt is
> 	   invoked. Otherwise nothing happens
> 
> Delayed disable (with level fix):
> 
> 	irq_disable(); /* Do not mask in hardware */
> 	....
> 	-> Interrupt line is asserted.
> 	---> interrupt handler is invoked: interrupt is masked in
> 	     hardware
> 	....
> 	irq_enable();  /* Unmask */
> 	
> 	-> When interrupt line is still active, then the interrupt is
> 	   invoked. Otherwise nothing happens
> 
> Can we agree, that this is the same ?

Of course, not! IMHO, far less changes can broke some drivers.

> 
> > BTW, of course, my knowledge of this is very limited, but I wonder
> > about these level type irqs used e.g. by apics. 'Normal' chips hold
> > some data until it's read by a driver, so there is something more
> > needed than an ack by io_apic. But isn't there any possibility
> > some level type irqs possible for IPIs or local interrupts (82489DX?)
> > could be missing here? Are we sure there is no hardware using level
> > type irqs in a similar way (drop after acking)?
> 
> Level type interrupts _are_ active as long as the hardware pin of the
> interrupt line is driven by a device to the active level. 
> 
> When the hardware pin is kept at the active level by a device, the ACK
> of the interrupt controller does not change the interrupt line of the
> device. The interrupt comes back again immediately.
> 
> Edge type interrupts are different:
...
Thanks for explanation. But, I'd be really glad if you could hint me
too about this level type possible sometimes e.g. for IPIs: I've
read the message needs ack (EOI) only, and hard to believe IPI is
repeated on and on after this. So, if such "thing" is level type
and hits IRQ_PENDING (or even IRQ_INPROGRESS) in handle_fasteoi_irq,
then is acked - isn't it dumped?

> > > > there is no reason to endanger even a small number of users/admins
> > > > for stresses like this, done to Marcin or Jean-Baptiste, when it's
> > > > possible to do this safer without much changes.
> > > 
> > > Safer in what way ? 
> > 
> > Because, if there were a visible config option or kernel parameter
> > e.g.  with a comment like "legacy level irq handling - obsolete",
> > some people would be happy when they find it's useful for them, and
> > you would know about the problem much sooner, as well.
> 
> Well, there is nothing legacy. level type interrupts do not need the
> resend mechanism at all. This misfeature was introduced with the delayed
> disable and went unnoticed until now.

http://dictionary.reference.com/browse/legacy
"2. anything handed down from the past [...]"

I hope, you are right, really! On the other hand we wouldn't have this
discussion at all, if right opinions were always enough.

Thanks,
Jarek P.