Messages in this thread | | | Date | Wed, 29 Oct 2014 10:11:45 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 1/3] genirq: Add support for priority-drop/deactivate interrupt controllers |
| |
On 28/10/14 20:14, Thomas Gleixner wrote: > On Tue, 28 Oct 2014, Marc Zyngier wrote: >> On 28/10/14 15:32, Thomas Gleixner wrote: >>> Let me make a few assumptions and correct me if I'm wrong as usual. >>> >>> 1) The startup/shutdown procedure for such an interrupt is the >>> expensive mask/unmask which you want to avoid for the actual >>> handling case >> >> Indeed. >> >>> 2) In case of an actual interrupt the flow (ignoring locking) is: >>> >>> handle_xxx_irq() >>> >>> mask_irq(); /* chip->irq_mask() maps to EOI */ >>> >>> if (!action || irq_disabled()) >>> return; >>> >>> handle_actions(); >>> >>> if (irq_threads_active() || irq_disabled()) >>> return; >>> >>> unmask_irq(); /* chip->irq_unmask() maps to DIR */ >>> >>> So that is handle_level_irq() with the chip callbacks being: >>> >>> irq_startup = gic_unmask >>> irq_shutdown = gic_mask >>> irq_unmask = gic_dir >>> irq_mask = gic_eoi >> >> So while this works really well for the interrupt handling part, it will >> break [un]mask_irq(). This is because you can only write to EOI for an >> interrupt that you have ACKed just before (anything else and the GIC >> state machine goes crazy). Basically, any use for EOI/DIR outside of the >> interrupt context itself (hardirq or thread) is really dangerous. > > I really doubt that the DIR invocation is dangerous outside of > interrupt context. Otherwise your threaded scheme would not work at > all as the DIR invocation happens in thread context.
There is a small restriction in the use of DIR (quoting the spec):
"If the interrupt identified in the GICC_DIR is not active, and is not a spurious interrupt, the effect of the register write is UNPREDICTABLE. This means any GICC_DIR write must identify an interrupt for which there has been a valid GICC_EOIR or GICC_AEOIR write."
I think that affect the irq_enable you describe below.
> The nice thing about the lazy irq disable code is that the irq_mask(), > i.e. EOI, invocation actually happens always in hard interrupt > context. We should never invoke irq_mask() from any other context if > you supply a startup/shutdown function. > >> If we had a flag like IRQCHIP_UNMASK_IS_STARTUP, we could distinguish >> this particular case, but that's borderline ugly. > > Indeed. But I don't think it is required. See also below. > >>> 4) In the lazy irq disable case if the interrupt fires mask_irq() >>> [EOI] is good enough to silence it. >>> >>> Though in the enable_irq() case you cannot rely on the automatic >>> resend of the interrupt when you unmask [DIR]. So we need to make >>> sure that even in the level case (dunno whether that's supported in >>> that mode) we end up calling the irq_retrigger() callback. But >>> that's rather simple to achieve with a new chip flag. >> >> I think this one breaks for the same reason as above. And an interrupt >> masked with EOI cannot easily be restarted without clearing the ACTIVE >> bit (and everything becomes even more of a complete madness). > > So we already established that irq_mask()/EOI will only be called from > the actual interrupt context and irq_unmask()/DIR must be safe to be > called from any context in order to make the EOI/DIR based threaded > optimization work. > > So the only interesting code path is enable_irq() which invokes > irq_enable() and then the resend/retrigger machinery. > > irq_enable() calls chip->irq_unmask(), i.e. DIR. So that clears the > ACTIVE bit and then the IRQ either gets resent by hardware (in case of > level as the device interrupt is still active) or retriggered by the > irq_retrigger() callback.
The problem I see here is for an interrupt that has been flagged as disabled with irq_disabled(), but that hasn't fired. We'd end up doing a DIR on something that hasn't had an EOI first. I think that's the only wrinkle in this scheme.
I'll implement something today, that will help me thinking.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |