David,
On Wed, 4 Mar 2009, David Brownell wrote:
> On Tuesday 03 March 2009, Thomas Gleixner wrote:
> > >
> > > > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> > >
> > > I did check them out, as noted earlier in this thread.
> > >
> > > The significant omission is lack of support for chaining
> > > such threads. Example, an I2C device that exposes
> > > several dozen IRQs with mask/ack/... operations that
> > > require I2C access.
> >
> > Well, the significant omission is on your side.
>
> The facts don't quite match up with that story though ... for
> starters, as I've already pointed out in this thread, I didn't
> write that code (or even "create a private form of abuse" as
> you put it). My name isn't even on the copyright.
>
> I did however clean it up a lot, in hope that such cleanup
> would make later updates easier. Anyone could tell such
> updates would be needed. In fact ...
Sorry, did not realize that it was not your design in the first place.
> Your IRQ threading patches appeared well after this driver went
> to mainline. So I did talk to "us" about those problems, earlier,
> but it doesn't seem to have gotten your attention until now.
Fair enough. I did not realize the horror of this chip until now. From
what you told me at KS I figured it would be a halfways straight
forward thing.
> You're referring to the second issue. The code in
> question doesn't actually have any dependency on
> hardirq context though.
Err. handle_IRQ_event was never meant to run in thread context,
neither the handle_TYPE functions.
> Assuming all IRQ configuring and dispatching runs with IRQs
> disabled. Your threaded IRQ patches kick in only *after*
> dispatching has been done. So it affects just one of the
> three main unusual bits of behavior involved here.
>
> Which mess were you thinking of? :)
None, there is no mess in the irq code.
> > The problem you described is straight forward and as I said before
> > it's not rocket science to provide support for that in the genirq
> > code. Your use case does not need to use the chained handler setup at
> > all, it just needs to request the main IRQ as a simple type and handle
> > the ack/mask/unmask from the two handler parts.
>
> When there is a "main IRQ" that calls the handlers, that's
> exactly what chaining involves ...
And how does this rabulistic nit picking help us here ? :)
Again: the chained_handler functionality was never designed to run in
a thread.
> > The only change in the generic code which is needed is a new handler
> > function for the chained irqs "handle_irq_simple_threaded()" which is
> > designed to handle the calls from thread context.
>
> I'm not 100% sure that's right; the dispatching is a bit quirky.
> That is however where I'll start.
>
> The top level handler (for the PIH module) could easily use a
> "handle_irq_simple_threaded()", yes ... but the secondary (SIH)
> handlers have a few oddball behaviors including mixes of edge
> and level trigger modes.
I took a closer look at this code and the more I look the more it
confuses me.
You told me that the demux handler runs the secondary handlers in its
thread context, but looking at the code there is a work queue as well.
The mask/unmask functions which are called for the secondary handlers
are just queueing work. I really do not understand the logic here:
primary interrupt happens
->primary thread is woken up
primary thread runs
-> primary thread raises secondary irq via
generic_handle_irq(irq), which results in:
desc->handle_irq(irq, desc);
The secondary handler has is set to: handle_edge_irq and
handle_edge_irq() does: desc->chip->ack(irq);
But the irqchip, which is set for those secondary irqs has a NULL ack
function. How can this work at all ?
I'm probably missing something and I would appreciate if you could
shed some light on this. An abstract description of the requirements
of the hardware w/o any reference to the current implementation would
be definitely helpful.
Thanks,
tglx
