[PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

As fasteoi type chips never had to define their ack() method before the
recent Ingo's change to handle_fasteoi_irq(), any attempt to execute handler
in thread resulted in the kernel crash. So, define their ack() methods to be
the same as their eoi() ones...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
Since there was no feedback on three solutions I suggested, I'm going the way
of least resistance and making the fasteoi type chips behave the way that
handle_fasteoi_irq() is expecting from them...

 arch/powerpc/platforms/cell/interrupt.c |    1 +
 arch/powerpc/platforms/iseries/irq.c    |    1 +
 arch/powerpc/platforms/pseries/xics.c   |    2 ++
 arch/powerpc/sysdev/mpic.c              |    1 +
 4 files changed, 5 insertions(+)

Index: linux-2.6/arch/powerpc/platforms/cell/interrupt.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/cell/interrupt.c
+++ linux-2.6/arch/powerpc/platforms/cell/interrupt.c
@@ -83,6 +83,7 @@ static struct irq_chip iic_chip = {
 	.typename = " CELL-IIC ",
 	.mask = iic_mask,
 	.unmask = iic_unmask,
+	.ack = iic_eoi,
 	.eoi = iic_eoi,
 };
 
Index: linux-2.6/arch/powerpc/platforms/iseries/irq.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/iseries/irq.c
+++ linux-2.6/arch/powerpc/platforms/iseries/irq.c
@@ -282,6 +282,7 @@ static struct irq_chip iseries_pic = {
 	.shutdown	= iseries_shutdown_IRQ,
 	.unmask		= iseries_enable_IRQ,
 	.mask		= iseries_disable_IRQ,
+	.ack		= iseries_end_IRQ,
 	.eoi		= iseries_end_IRQ
 };
 
Index: linux-2.6/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6/arch/powerpc/platforms/pseries/xics.c
@@ -477,6 +477,7 @@ static struct irq_chip xics_pic_direct =
 	.startup = xics_startup,
 	.mask = xics_mask_irq,
 	.unmask = xics_unmask_irq,
+	.ack = xics_eoi_direct,
 	.eoi = xics_eoi_direct,
 	.set_affinity = xics_set_affinity
 };
@@ -487,6 +488,7 @@ static struct irq_chip xics_pic_lpar = {
 	.startup = xics_startup,
 	.mask = xics_mask_irq,
 	.unmask = xics_unmask_irq,
+	.ack = xics_eoi_lpar,
 	.eoi = xics_eoi_lpar,
 	.set_affinity = xics_set_affinity
 };
Index: linux-2.6/arch/powerpc/sysdev/mpic.c
===================================================================
--- linux-2.6.orig/arch/powerpc/sysdev/mpic.c
+++ linux-2.6/arch/powerpc/sysdev/mpic.c
@@ -718,6 +718,7 @@ static int mpic_set_irq_type(unsigned in
 static struct irq_chip mpic_irq_chip = {
 	.mask		= mpic_mask_irq,
 	.unmask		= mpic_unmask_irq,
+	.ack		= mpic_end_irq,
 	.eoi		= mpic_end_irq,
 	.set_type	= mpic_set_irq_type,
 };

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Sun, 2006-11-19 at 22:43 +0300, Sergei Shtylyov wrote:
> As fasteoi type chips never had to define their ack() method before the
> recent Ingo's change to handle_fasteoi_irq(), any attempt to execute handler
> in thread resulted in the kernel crash. So, define their ack() methods to be
> the same as their eoi() ones...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> ---
> Since there was no feedback on three solutions I suggested, I'm going the way
> of least resistance and making the fasteoi type chips behave the way that
> handle_fasteoi_irq() is expecting from them...

Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi
handler is being changed and what is the rationale for adding an ack
that was not necessary before ?

Cheers,
Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Mon, 2006-11-20 at 07:00 +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2006-11-19 at 22:43 +0300, Sergei Shtylyov wrote:
> > As fasteoi type chips never had to define their ack() method before the
> > recent Ingo's change to handle_fasteoi_irq(), any attempt to execute handler
> > in thread resulted in the kernel crash. So, define their ack() methods to be
> > the same as their eoi() ones...
> > 
> > Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> > 
> > ---
> > Since there was no feedback on three solutions I suggested, I'm going the way
> > of least resistance and making the fasteoi type chips behave the way that
> > handle_fasteoi_irq() is expecting from them...
> 
> Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi
> handler is being changed and what is the rationale for adding an ack
> that was not necessary before ?

To be more precise, I don't see in what circumstances a fasteoi type PIC
would need an ack routine that does something different than the eoi...
and if it always does the same thing, why not just call eoi ?

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi 
> handler is being changed and what is the rationale for adding an ack 
> that was not necessary before ?

dont worry, it's -rt only stuff.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>>As fasteoi type chips never had to define their ack() method before the
>>>recent Ingo's change to handle_fasteoi_irq(), any attempt to execute handler
>>>in thread resulted in the kernel crash. So, define their ack() methods to be
>>>the same as their eoi() ones...

>>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>>---
>>>Since there was no feedback on three solutions I suggested, I'm going the way
>>>of least resistance and making the fasteoi type chips behave the way that
>>>handle_fasteoi_irq() is expecting from them...

>>Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi
>>handler is being changed and what is the rationale for adding an ack
>>that was not necessary before ?

    It's changed in the RT patch for the case of threaded IRQ. This patch is 
not for the mainline kernels.

> To be more precise, I don't see in what circumstances a fasteoi type PIC
> would need an ack routine that does something different than the eoi...
> and if it always does the same thing, why not just call eoi ?

    Because Ingo decided that calling mask() and ack() methods was a better 
than calling mask() and eoi(). Here's the thread:

http://ozlabs.org/pipermail/linuxppc-dev/2006-October/026546.html

> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Sun, 2006-11-19 at 21:06 +0100, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi 
> > handler is being changed and what is the rationale for adding an ack 
> > that was not necessary before ?
> 
> dont worry, it's -rt only stuff.

Still, I'm curious :-) Besides, there have been people talking about
having -rt work on ppc64 so ...

What do you need an ack() for on fasteoi ? On all fasteoi controllers I
have, ack is implicit by obtaining the vector number and all there is is
an eoi...

Cheers,
Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> > dont worry, it's -rt only stuff.
> 
> Still, I'm curious :-) Besides, there have been people talking about 
> having -rt work on ppc64 so ...

ok :)

> What do you need an ack() for on fasteoi ? On all fasteoi controllers 
> I have, ack is implicit by obtaining the vector number and all there 
> is is an eoi...

it's a compatibility hack only. Threaded handlers are a different type 
of flow, but often the fasteoi handler is not changed to the threaded 
handler so i changed it to be a threaded handler too.

threaded handlers need a mask() + an ack(), because that's the correct 
model to map them to kernel threads - threaded handlers can be delayed 
for a long time if something higher-prio is preempting them.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

>>>Wait wait wait .... Can somebody (Ingo ?) explain me why the fasteoi 
>>>handler is being changed and what is the rationale for adding an ack 
>>>that was not necessary before ?

>>dont worry, it's -rt only stuff.

> Still, I'm curious :-) Besides, there have been people talking about
> having -rt work on ppc64 so ...

> What do you need an ack() for on fasteoi ? On all fasteoi controllers I
> have, ack is implicit by obtaining the vector number and all there is is
> an eoi...

    I must not that this whole ack() vs eoi() stuff is misleading. For example,
in 8259 driver, mask_ack() method actually sends EOI to PIC, not ACK's an IRQ 
-- the actual ACK is implicit on x86 and is used to read the interrupt vector 
form 8259 on PPC. So, IMO, there probably should only have been either ack() 
or eoi() method in the first place. Though I'm not familiar with ARM from 
which genirq stuff originated...

> Cheers,
> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Ingo Molnar wrote:

>>What do you need an ack() for on fasteoi ? On all fasteoi controllers 
>>I have, ack is implicit by obtaining the vector number and all there 
>>is is an eoi...

> it's a compatibility hack only. Threaded handlers are a different type 
> of flow, but often the fasteoi handler is not changed to the threaded 
> handler so i changed it to be a threaded handler too.

   The fasteoi flow seem to only had been used for x86 IOAPIC in the RT patch 
only *before* PPC took to using them in the mainline...

> threaded handlers need a mask() + an ack(), because that's the correct

    Not all of them. This could be customized on type-by-type basis. I.e. we 
could call eoi() instead of ack() for fasteoi chips without having to resort 
to the duplicated ack/eoi handlers.

> model to map them to kernel threads - threaded handlers can be delayed 
> for a long time if something higher-prio is preempting them.
> 
> 	Ingo

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Sun, 2006-11-19 at 23:26 +0300, Sergei Shtylyov wrote:

>     I must not that this whole ack() vs eoi() stuff is misleading. For example,
> in 8259 driver, mask_ack() method actually sends EOI to PIC, not ACK's an IRQ 
> -- the actual ACK is implicit on x86 and is used to read the interrupt vector 
> form 8259 on PPC. So, IMO, there probably should only have been either ack() 
> or eoi() method in the first place. Though I'm not familiar with ARM from 
> which genirq stuff originated...

They are different concepts. Ack clears the event on the PIC, it's
tyically necessary for resetting the edge detection logic for edge
interrupts and has to happen before the handler is called.

On MPIC or XICS, this is implicit by reading the vector. On some more
dumb controllers, this has to be done explicitely.

EOI is a more "high level" thing that some "intelligent" PICs that
automatically raise the priority do to restore the priority to what it
was before the interrupt occured.

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Sun, 2006-11-19 at 23:31 +0300, Sergei Shtylyov wrote:

>    The fasteoi flow seem to only had been used for x86 IOAPIC in the RT patch 
> only *before* PPC took to using them in the mainline...

I don't think so, I asked for the fasteoi to be created while porting
ppc to genirq :-)

> > threaded handlers need a mask() + an ack(), because that's the correct
> 
>     Not all of them. This could be customized on type-by-type basis. I.e. we 
> could call eoi() instead of ack() for fasteoi chips without having to resort 
> to the duplicated ack/eoi handlers.

I still don't see how ack() makes sense in the context of a fasteoi...
You can either just not EOI until it's handled, but you'll indeed
introduce delays for other interrupts of the same priority or lower, or
you can mask() and then eoi(), which is, I think, what Apple does, to
deliver the interrupt to a thread (and later unmask).

In any case, I don't see the need for a separate ack().

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>    I must not that this whole ack() vs eoi() stuff is misleading. For example,
>>in 8259 driver, mask_ack() method actually sends EOI to PIC, not ACK's an IRQ 
>>-- the actual ACK is implicit on x86 and is used to read the interrupt vector 
>>form 8259 on PPC. So, IMO, there probably should only have been either ack() 
>>or eoi() method in the first place. Though I'm not familiar with ARM from 
>>which genirq stuff originated...

> They are different concepts. Ack clears the event on the PIC, it's
> tyically necessary for resetting the edge detection logic for edge
> interrupts and has to happen before the handler is called.

    I know 8259. :-)
    It also resets the corresponding IRQ bit in IRR, and sets it in ISR where 
it's then cleared on EOI command.

> On MPIC or XICS, this is implicit by reading the vector. On some more
> dumb controllers, this has to be done explicitely.

    This is not implicit -- CPU has to read INTACK reg. on OpenPIC. Really 
implicit method is in action on x86 where CPU issues dual ACK bus cycle to get 
the vector form 8259...

> EOI is a more "high level" thing that some "intelligent" PICs that
> automatically raise the priority do to restore the priority to what it
> was before the interrupt occured.

    Thank you, I know. Even 8259 has the notion of priority and EOI works the 
same way here.

> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

>     This is not implicit -- CPU has to read INTACK reg. on OpenPIC.

Which is what I meant... "implicit by reading the vector".

> Really implicit method is in action on x86 where CPU issues dual ACK bus cycle to get 
> the vector form 8259...

Which you can do on OpenPIC too. It's the same idea. The concept that
obtaining the vector performs the ack.

> > EOI is a more "high level" thing that some "intelligent" PICs that
> > automatically raise the priority do to restore the priority to what it
> > was before the interrupt occured.
> 
>     Thank you, I know. Even 8259 has the notion of priority and EOI works the 
> same way here.

Ok, so why would you need an ack there then while eoi is just what you
need ? :-)

Also, that's interesting what you are saying about 8259... I should be
able to convert ppc's i8259 to use fasteoi too...

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>   The fasteoi flow seem to only had been used for x86 IOAPIC in the RT patch 
>>only *before* PPC took to using them in the mainline...

> I don't think so, I asked for the fasteoi to be created while porting
> ppc to genirq :-)

    Oh, I was unaware of such details. :-)

>>>threaded handlers need a mask() + an ack(), because that's the correct

>>    Not all of them. This could be customized on type-by-type basis. I.e. we 
>>could call eoi() instead of ack() for fasteoi chips without having to resort 
>>to the duplicated ack/eoi handlers.

> I still don't see how ack() makes sense in the context of a fasteoi...

    It doesn't make sense even in the context of 8259 with its level/edge 
flows. That's what I'm talking about...

> You can either just not EOI until it's handled, but you'll indeed
> introduce delays for other interrupts of the same priority or lower, or
> you can mask() and then eoi(), which is, I think, what Apple does, to
> deliver the interrupt to a thread (and later unmask).
> In any case, I don't see the need for a separate ack().

   Yeah, that's what the threaded versions of flow handlers are doing. Except 
they're calling ack() to actually EOI an IRQ.

> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

>>On MPIC or XICS, this is implicit by reading the vector. On some more
>>dumb controllers, this has to be done explicitely.

>     This is not implicit -- CPU has to read INTACK reg. on OpenPIC. Really 

    Sorry, it's the same for x86 version of OpenPIC according to spec. I meant 
the PPC implementation here.

>>Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

>    Yeah, that's what the threaded versions of flow handlers are doing. Except 
> they're calling ack() to actually EOI an IRQ.

Ok well, them the fix is to fix them not the PIC code :-)

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

>     Sorry, it's the same for x86 version of OpenPIC according to spec. I meant 
> the PPC implementation here.

Same OpenPIC.. it can deal with both INTACK cycles and reading from an
INTACK register. On some PPC's, we actually have logic in the bridge to
generate the INTACK cycle (which exist on PCI) though we only use that
with 8259's currently as it's not really more efficient than just
reading from the INTACK register of the OpenPIC.

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Sun, 2006-11-19 at 21:23 +0100, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > > dont worry, it's -rt only stuff.
> > 
> > Still, I'm curious :-) Besides, there have been people talking about 
> > having -rt work on ppc64 so ...
> 
> ok :)
> 
> > What do you need an ack() for on fasteoi ? On all fasteoi controllers 
> > I have, ack is implicit by obtaining the vector number and all there 
> > is is an eoi...
> 
> it's a compatibility hack only. Threaded handlers are a different type 
> of flow, but often the fasteoi handler is not changed to the threaded 
> handler so i changed it to be a threaded handler too.
> 
> threaded handlers need a mask() + an ack(), because that's the correct 
> model to map them to kernel threads - threaded handlers can be delayed 
> for a long time if something higher-prio is preempting them.

Well, the principle of controllers that do fasteoi is that all
interrupts of the same priority or below are masked until eoi happens...
so I still don't see why you need that...

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>>EOI is a more "high level" thing that some "intelligent" PICs that
>>>automatically raise the priority do to restore the priority to what it
>>>was before the interrupt occured.

>>    Thank you, I know. Even 8259 has the notion of priority and EOI works the 
>>same way here.

> Ok, so why would you need an ack there then while eoi is just what you
> need ? :-)

    Don't ask me, ask the genirq authors. :-)

> Also, that's interesting what you are saying about 8259... I should be
> able to convert ppc's i8259 to use fasteoi too...

    I'm not sure it's feasible. The idea behind level/edge flows is to 
eliminate the interrupt priority I think. That's why they EOI ASAP (with the 
level handler masking IRQ before that) -- this way the other interrupts may 
come thru.
    I used to think that fasteoi was intended for SMP PICs which are 
intelligent enough to mask off the interrupts pending delivery or handling on 
CPUs and unmask them upon receiving EOI -- just like x86 IOAPIC does. This 
way, the acceptance of the lower priority interrupts shouldn't be hindered on 
the other CPUs. Maybe the scheme is different for OpenPIC (I know it has the 
different interrupt distribution scheme from IOAPIC)?

> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

>     I'm not sure it's feasible. The idea behind level/edge flows is to 
> eliminate the interrupt priority I think. That's why they EOI ASAP (with the 
> level handler masking IRQ before that) -- this way the other interrupts may 
> come thru.

Well, the idea behind the level/edge flow is not exactly that afaik.
It's more like having tailored handlers for level/edge on PICs that are
not intelligent to auto-mask with a priority mecanism (ie. dumb PICs
which are very common in the embedded field, and for example, on ARM
where genirq takes its roots).

>     I used to think that fasteoi was intended for SMP PICs which are 
> intelligent enough to mask off the interrupts pending delivery or handling on 
> CPUs and unmask them upon receiving EOI -- just like x86 IOAPIC does.

In general, PICs that are intelligent enough to mask off, wether using
something as you describe or using priorities. I don't feel the need of
going through hoops to allow lower or same priority interrupts in.
First, if you really need an interrupt to be serviced quick, then you
can just give it a higher priority. In the general case however, I do
-not- want to allow interrupts to stack up. Imagine a big IBM machine
with hundreds interrupt lines, what happens to the kernel stack if we
let them interrupt each other ?

>  This 
> way, the acceptance of the lower priority interrupts shouldn't be hindered on 
> the other CPUs. Maybe the scheme is different for OpenPIC (I know it has the 
> different interrupt distribution scheme from IOAPIC)?

I don't think there is a real need to let lower priority interrupts in
on a CPU that is currently handling a higher priority one.

In the case of RT, though, with delayed delivery, then, the option to
mask and EOI right away is always possible.

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

> it's a compatibility hack only. Threaded handlers are a different type 
> of flow, but often the fasteoi handler is not changed to the threaded 
> handler so i changed it to be a threaded handler too.
> 
> threaded handlers need a mask() + an ack(), because that's the correct 
> model to map them to kernel threads - threaded handlers can be delayed 
> for a long time if something higher-prio is preempting them.

I've re-read the old thread and I disagree with you there. I think the
right approach was proposed initially by Daniel Walker.

If my understanding of "threaded handlers" is correct, that is delaying
the delivery of the interrupt to a thread while restoring the ability to
process further interrupts of the same or lower priority immediately.

There is no such thing as an explicit "ack" on fasteoi controllers. The
ack is implicit with the reading of the vector (either via a special
cycle on the bus or via reading the intack register for example on
mpic).

So by doing a mask followed by an eoi, you essentially mask the
interrupt preventing further delivery of that interrupt and lower the
CPU priority in the PIC thus allowing processing of further interrupts.

Are there other fasteoi controllers than the ones I have on powerpc
anyway ?

There is one thing I disagree with in the initial patch from Daniel
however, it's the fact that he can't deal with fasteoi handlers that
have no mask/unmask.

I have such a controller, on Cell, though currently I do have an empty
mask and unmask for it, I've been thinking about removing them (and
fixing callers to test for NULL). This is the Cell's top level
controller and it is edge only (at this level, on Cell, all interrupts
are messages).

Thus it can perfectly well cope with soft-masking (delayed disable if
you prefer) and that is compatible with using a fasteoi handler. There
should be no reason why that wouldn't work for -rt as well.

Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> If my understanding of "threaded handlers" is correct, that is 
> delaying the delivery of the interrupt to a thread while restoring the 
> ability to process further interrupts of the same or lower priority 
> immediately.

correct. It's basically a different type of 'flow' of handling an 
interrupt. It's a host-side genirq-level detail that should have no 
irqchip level impact at all.

The only detail is that sometimes a threaded flow /cannot/ be 
implemented if the irqchip lacks certain methods. (such as a 
mask/unmask)

i.e. Sergei's patch tweaking the irqchip data structures is wrong - the 
correct approach is what i do for i386/x86_64: install a different 
"threaded" flow handler. I prefer this to tweaking the existing 
'fasteoi' handler, to make the act of supporting a threaded flow design 
explicit. (and to allow a mixed threaded/non-threaded flow setup) I 
didnt take Daniel's prior patch for that reason: he tried to tweak the 
fasteoi flow handler - which is an almost good approach but not good 
enough :-)

> There is no such thing as an explicit "ack" on fasteoi controllers. 
> The ack is implicit with the reading of the vector (either via a 
> special cycle on the bus or via reading the intack register for 
> example on mpic).

well, even if it's coupled to the reading of the vector, there is an ack 
initiated by the host. This is not really a fundamental thing, it's API 
semantics of the hardware interface.

so the question is not 'is there an ACK' (all non-MSI-type-of IRQ 
delivery mechanisms have some sort of ACK mechanism), but what is the 
precise structure of ACK-ing an IRQ that the host recieves.

on PPC64, 'get the vector' initiates an ACK as well - is that done 
before handle_irq() is done?

> So by doing a mask followed by an eoi, you essentially mask the 
> interrupt preventing further delivery of that interrupt and lower the 
> CPU priority in the PIC thus allowing processing of further 
> interrupts.

correct, that's what should happen.

> Are there other fasteoi controllers than the ones I have on powerpc 
> anyway ?

well, if you mean the x86 APICs, there you get the vector 'for free' as 
part of the IRQ entry call sequence, and there's an EOI register in the 
local APIC that notifies the IRQ hardware, lowers the CPU priority, etc. 
We have that as an ->eoi handler right now.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Ingo Molnar wrote:

> i.e. Sergei's patch tweaking the irqchip data structures is wrong - the 
> correct approach is what i do for i386/x86_64: install a different 
> "threaded" flow handler.

    Let me remind you that the patch was following *your* change in 
handle_fasteoi_irq(). Do not present it as my own invention.

> I prefer this to tweaking the existing 
> 'fasteoi' handler, to make the act of supporting a threaded flow design 

    You do, indeed? :-)

> explicit. (and to allow a mixed threaded/non-threaded flow setup) I 
> didnt take Daniel's prior patch for that reason: he tried to tweak the 
> fasteoi flow handler - which is an almost good approach but not good 
> enough :-)

    Yeah, your way was *really* better. :-/

> on PPC64, 'get the vector' initiates an ACK as well - is that done 
> before handle_irq() is done?

    Exactly. How else do_IRQ() would know the vector?

>>So by doing a mask followed by an eoi, you essentially mask the 
>>interrupt preventing further delivery of that interrupt and lower the 
>>CPU priority in the PIC thus allowing processing of further 
>>interrupts.

> correct, that's what should happen.

    eoi() would be a more proper name than ack() then.

>>Are there other fasteoi controllers than the ones I have on powerpc 
>>anyway ?

> well, if you mean the x86 APICs, there you get the vector 'for free' as 
> part of the IRQ entry call sequence, and there's an EOI register in the 
> local APIC that notifies the IRQ hardware, lowers the CPU priority, etc. 
> We have that as an ->eoi handler right now.

    Yes. And my perception from that fragment of RT patch was that fasteoi was 
targeted only at SMP interrupt controllers which like IOAPIC are sensible 
enough to not trouble the other CPUs with an interrupt that's already been 
taken by a certain CPU for handling, i.e. that interrupt being effectively 
masked off until EOI arrives (well, that's so for the level triggered but not 
the edge-triggered IRQs, if I don't mistake), so there's no need for explicit 
masking and at the same time that interrupt isn't hindering the delivery of 
the lower-priority interrupts to any CPU (unlike that would be happening with 
8259)...
    Now it turns out that my understading was wrong, and fasteoi could even 
fit for 8259 (according to Ben)?

> 	Ingo

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:
>>    I'm not sure it's feasible. The idea behind level/edge flows is to 
>>eliminate the interrupt priority I think. That's why they EOI ASAP (with the 
>>level handler masking IRQ before that) -- this way the other interrupts may 
>>come thru.

> Well, the idea behind the level/edge flow is not exactly that afaik.
> It's more like having tailored handlers for level/edge on PICs that are
> not intelligent to auto-mask with a priority mecanism (ie. dumb PICs
> which are very common in the embedded field, and for example, on ARM
> where genirq takes its roots).

    That was a conclusion to which I came after looking at the 8259 code (that 
PIC being full capable of the priority masking).

>>    I used to think that fasteoi was intended for SMP PICs which are 
>>intelligent enough to mask off the interrupts pending delivery or handling on 
>>CPUs and unmask them upon receiving EOI -- just like x86 IOAPIC does.

> In general, PICs that are intelligent enough to mask off, wether using
> something as you describe or using priorities. I don't feel the need of
> going through hoops to allow lower or same priority interrupts in.
> First, if you really need an interrupt to be serviced quick, then you
> can just give it a higher priority. In the general case however, I do
> -not- want to allow interrupts to stack up. Imagine a big IBM machine
> with hundreds interrupt lines, what happens to the kernel stack if we
> let them interrupt each other ?

    Well, such machines are SMP usually... :-)

>> This 
>>way, the acceptance of the lower priority interrupts shouldn't be hindered on 
>>the other CPUs. Maybe the scheme is different for OpenPIC (I know it has the 
>>different interrupt distribution scheme from IOAPIC)?

> I don't think there is a real need to let lower priority interrupts in
> on a CPU that is currently handling a higher priority one.

    Nevertheless, 8259 drivers are doing exactly this on UP machines -- and 
they were doing this before and after genirq conversion...

> Ben.

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Daniel Walker]

On Mon, 2006-11-20 at 11:01 +0100, Ingo Molnar wrote:

> correct. It's basically a different type of 'flow' of handling an 
> interrupt. It's a host-side genirq-level detail that should have no 
> irqchip level impact at all.
> 
> The only detail is that sometimes a threaded flow /cannot/ be 
> implemented if the irqchip lacks certain methods. (such as a 
> mask/unmask)
> 
> i.e. Sergei's patch tweaking the irqchip data structures is wrong - the 
> correct approach is what i do for i386/x86_64: install a different 
> "threaded" flow handler. I prefer this to tweaking the existing 
> 'fasteoi' handler, to make the act of supporting a threaded flow design 
> explicit. (and to allow a mixed threaded/non-threaded flow setup) I 
> didnt take Daniel's prior patch for that reason: he tried to tweak the 
> fasteoi flow handler - which is an almost good approach but not good 
> enough :-)
> 

It makes porting to powerpc for instance harder because some controllers
have ack(), and some don't.. Some have mask(), and some don't.. So you
end up with what Sergei is doing which is flat out make ack == eoi ..
Where you have multiple irq chip types each one really needs an
individual evaluation ..

I don't really agree with your method since it increase the porting
effort, and I don't see a gain from it.. In fact the changes that you
make seem like it would be more difficult to support a simple reversal
from a thread to an interrupt context handler, since your permanently
changing the "flow handler" , as you called it, no matter what the
context is. So the person using this code will have to investigate this
new flow handler, which will result is a very anti-climactic ending and
lots of useless work since it's really making ack == eoi (at least for
powerpc). 

Daniel

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> It makes porting to powerpc for instance harder because some 
> controllers have ack(), and some don't.. Some have mask(), and some 
> don't.. So you end up with what Sergei is doing which is flat out make 
> ack == eoi .. Where you have multiple irq chip types each one really 
> needs an individual evaluation ..

this isnt really a problem. The current situation is simply hacky, 
because right now there's no 'threaded' flow type at all. The x86 code 
just moves the code away from fasteoi:

 #ifdef CONFIG_PREEMPT_HARDIRQS
                set_irq_chip_and_handler_name(irq, &ioapic_chip,
                                            handle_level_irq, "level-threaded");#else
                set_irq_chip_and_handler_name(irq, &ioapic_chip,
                                              handle_fasteoi_irq, "fasteoi");
 #endif

what should happen is a handle_thread_irq irq-flow handler that will 
first mask, and then ack or eoi (whichever callbacks is available), and 
thus can and will handle both fasteoi, edge and level irqs.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> >on PPC64, 'get the vector' initiates an ACK as well - is that done 
> >before handle_irq() is done?
> 
>    Exactly. How else do_IRQ() would know the vector?

the reason i'm asking is that in this case masking is a bit late at this 
point and there's a chance for a repeat interrupt.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Daniel Walker]

On Mon, 2006-11-20 at 17:42 +0100, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > It makes porting to powerpc for instance harder because some 
> > controllers have ack(), and some don't.. Some have mask(), and some 
> > don't.. So you end up with what Sergei is doing which is flat out make 
> > ack == eoi .. Where you have multiple irq chip types each one really 
> > needs an individual evaluation ..
> 
> this isnt really a problem. The current situation is simply hacky, 
> because right now there's no 'threaded' flow type at all. The x86 code 
> just moves the code away from fasteoi:
> 
>  #ifdef CONFIG_PREEMPT_HARDIRQS
>                 set_irq_chip_and_handler_name(irq, &ioapic_chip,
>                                             handle_level_irq, "level-threaded");#else
>                 set_irq_chip_and_handler_name(irq, &ioapic_chip,
>                                               handle_fasteoi_irq, "fasteoi");
>  #endif
> 
> what should happen is a handle_thread_irq irq-flow handler that will 
> first mask, and then ack or eoi (whichever callbacks is available), and 
> thus can and will handle both fasteoi, edge and level irqs.

If it still calls for arch level changes, I have an aversion to those ..
Maybe if it was internal to the set_irq_chip_and_handler_name() macro?

Daniel

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Ingo Molnar wrote:

>>>on PPC64, 'get the vector' initiates an ACK as well - is that done 
>>>before handle_irq() is done?

>>   Exactly. How else do_IRQ() would know the vector?

> the reason i'm asking is that in this case masking is a bit late at this 
> point and there's a chance for a repeat interrupt.

    How's that? Acknowledge != EOI on OpenPIC (as well as 8259).
    Acknoledging sets the bit in the in-service register preventing all the 
interrupts with same or lower prioriry from being accepted.

> 	Ingo

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Hello.
> 
> Ingo Molnar wrote:
> 
> >>>on PPC64, 'get the vector' initiates an ACK as well - is that done 
> >>>before handle_irq() is done?
> 
> >>  Exactly. How else do_IRQ() would know the vector?
> 
> >the reason i'm asking is that in this case masking is a bit late at this 
> >point and there's a chance for a repeat interrupt.
> 
>    How's that? Acknowledge != EOI on OpenPIC (as well as 8259). 
> Acknoledging sets the bit in the in-service register preventing all 
> the interrupts with same or lower prioriry from being accepted.

ah, ok. The x86 APIC does that automatically, and only an EOI has to be 
passed. So i guess we are in wild agreement ;)

i'm hacking up something now to see whether it makes sense to introduce 
a central threaded flow type, or whether it's better the branch off the 
current flow types (as the code does it right now).

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> i'm hacking up something now to see whether it makes sense to 
> introduce a central threaded flow type, or whether it's better the 
> branch off the current flow types (as the code does it right now).

ok, a central flow type caused more problems than good - the main 
complication is that the handler needs to know the true 'flow' 
(edge/level/fasteoi, etc.) anyway, even in the threaded case.

So i rather went on making the existing flow handlers more 
threading-friendly, and undoing the x86_64 and i386 arch changes to make 
sure that the default handlers all work fine. Does the patch below 
(against -rt4) do the trick for your on PPC too?

[ also, i excluded old-style do_IRQ() platforms from irq threading -
  those hopelessly mix all the flow types that makes robust threading
  support not really possible. ]

	Ingo

Index: linux/arch/i386/kernel/io_apic.c
===================================================================
--- linux.orig/arch/i386/kernel/io_apic.c
+++ linux/arch/i386/kernel/io_apic.c
@@ -1272,22 +1272,12 @@ static struct irq_chip ioapic_chip;
 static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL) {
-#ifdef CONFIG_PREEMPT_HARDIRQS
+			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					    handle_level_irq, "level-threaded");
-#else
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_fasteoi_irq, "fasteoi");
-#endif
-	} else {
-#ifdef CONFIG_PREEMPT_HARDIRQS
+					 handle_fasteoi_irq, "fasteoi");
+	else {
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge-threaded");
-#else
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge");
-#endif
+					 handle_edge_irq, "edge");
 	}
 	set_intr_gate(vector, interrupt[irq]);
 }
Index: linux/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux.orig/arch/x86_64/kernel/io_apic.c
+++ linux/arch/x86_64/kernel/io_apic.c
@@ -787,22 +787,12 @@ static struct irq_chip ioapic_chip;
 static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL) {
-#ifdef CONFIG_PREEMPT_HARDIRQS
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					    handle_level_irq, "level-threaded");
-#else
+			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_fasteoi_irq, "fasteoi");
-#endif
-	} else {
-#ifdef CONFIG_PREEMPT_HARDIRQS
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge-threaded");
-#else
+	else {
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_edge_irq, "edge");
-#endif
 	}
 }
 
Index: linux/kernel/Kconfig.preempt
===================================================================
--- linux.orig/kernel/Kconfig.preempt
+++ linux/kernel/Kconfig.preempt
@@ -105,7 +105,7 @@ config PREEMPT_SOFTIRQS
 config PREEMPT_HARDIRQS
 	bool "Thread Hardirqs"
 	default n
-#	depends on PREEMPT
+	depends on !GENERIC_HARDIRQS_NO__DO_IRQ
 	help
 	  This option reduces the latency of the kernel by 'threading'
           hardirqs. This means that all (or selected) hardirqs will run
Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -238,8 +238,10 @@ static inline void mask_ack_irq(struct i
 	if (desc->chip->mask_ack)
 		desc->chip->mask_ack(irq);
 	else {
-		desc->chip->mask(irq);
-		desc->chip->ack(irq);
+		if (desc->chip->mask)
+			desc->chip->mask(irq);
+		if (desc->chip->mask)
+			desc->chip->ack(irq);
 	}
 }
 
Index: linux/kernel/irq/handle.c
===================================================================
--- linux.orig/kernel/irq/handle.c
+++ linux/kernel/irq/handle.c
@@ -266,12 +266,6 @@ fastcall notrace unsigned int __do_IRQ(u
 		goto out;
 
 	/*
-	 * hardirq redirection to the irqd process context:
-	 */
-	if (redirect_hardirq(desc))
-		goto out_no_end;
-
-	/*
 	 * Edge triggered interrupts need to remember
 	 * pending events.
 	 * This applies to any hw interrupts that allow a second
@@ -303,7 +297,6 @@ out:
 	 * disabled while the handler was running.
 	 */
 	desc->chip->end(irq);
-out_no_end:
 	spin_unlock(&desc->lock);
 
 	return 1;

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Daniel Walker]

On Mon, 2006-11-20 at 18:55 +0100, Ingo Molnar wrote:
> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -238,8 +238,10 @@ static inline void mask_ack_irq(struct i
>         if (desc->chip->mask_ack)
>                 desc->chip->mask_ack(irq);
>         else {
> -               desc->chip->mask(irq);
> -               desc->chip->ack(irq);
> +               if (desc->chip->mask)
> +                       desc->chip->mask(irq);
> +               if (desc->chip->mask)
> +                       desc->chip->ack(irq);
>         }
>  } 


Did you mean to check ->mask both times here?

Daniel

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> > -               desc->chip->mask(irq);
> > -               desc->chip->ack(irq);
> > +               if (desc->chip->mask)
> > +                       desc->chip->mask(irq);
> > +               if (desc->chip->mask)
> > +                       desc->chip->ack(irq);
> >         }
> >  } 
> 
> Did you mean to check ->mask both times here?

good catch - fixed patch below.

	Ingo

Index: linux/arch/i386/kernel/io_apic.c
===================================================================
--- linux.orig/arch/i386/kernel/io_apic.c
+++ linux/arch/i386/kernel/io_apic.c
@@ -1272,22 +1272,12 @@ static struct irq_chip ioapic_chip;
 static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL) {
-#ifdef CONFIG_PREEMPT_HARDIRQS
+			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					    handle_level_irq, "level-threaded");
-#else
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_fasteoi_irq, "fasteoi");
-#endif
-	} else {
-#ifdef CONFIG_PREEMPT_HARDIRQS
+					 handle_fasteoi_irq, "fasteoi");
+	else {
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge-threaded");
-#else
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge");
-#endif
+					 handle_edge_irq, "edge");
 	}
 	set_intr_gate(vector, interrupt[irq]);
 }
Index: linux/arch/x86_64/kernel/io_apic.c
===================================================================
--- linux.orig/arch/x86_64/kernel/io_apic.c
+++ linux/arch/x86_64/kernel/io_apic.c
@@ -787,22 +787,12 @@ static struct irq_chip ioapic_chip;
 static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL) {
-#ifdef CONFIG_PREEMPT_HARDIRQS
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					    handle_level_irq, "level-threaded");
-#else
+			trigger == IOAPIC_LEVEL)
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_fasteoi_irq, "fasteoi");
-#endif
-	} else {
-#ifdef CONFIG_PREEMPT_HARDIRQS
-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
-					      handle_edge_irq, "edge-threaded");
-#else
+	else {
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_edge_irq, "edge");
-#endif
 	}
 }
 
Index: linux/kernel/Kconfig.preempt
===================================================================
--- linux.orig/kernel/Kconfig.preempt
+++ linux/kernel/Kconfig.preempt
@@ -105,7 +105,7 @@ config PREEMPT_SOFTIRQS
 config PREEMPT_HARDIRQS
 	bool "Thread Hardirqs"
 	default n
-#	depends on PREEMPT
+	depends on !GENERIC_HARDIRQS_NO__DO_IRQ
 	help
 	  This option reduces the latency of the kernel by 'threading'
           hardirqs. This means that all (or selected) hardirqs will run
Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -238,8 +238,10 @@ static inline void mask_ack_irq(struct i
 	if (desc->chip->mask_ack)
 		desc->chip->mask_ack(irq);
 	else {
-		desc->chip->mask(irq);
-		desc->chip->ack(irq);
+		if (desc->chip->mask)
+			desc->chip->mask(irq);
+		if (desc->chip->ack)
+			desc->chip->ack(irq);
 	}
 }
 
Index: linux/kernel/irq/handle.c
===================================================================
--- linux.orig/kernel/irq/handle.c
+++ linux/kernel/irq/handle.c
@@ -266,12 +266,6 @@ fastcall notrace unsigned int __do_IRQ(u
 		goto out;
 
 	/*
-	 * hardirq redirection to the irqd process context:
-	 */
-	if (redirect_hardirq(desc))
-		goto out_no_end;
-
-	/*
 	 * Edge triggered interrupts need to remember
 	 * pending events.
 	 * This applies to any hw interrupts that allow a second
@@ -303,7 +297,6 @@ out:
 	 * disabled while the handler was running.
 	 */
 	desc->chip->end(irq);
-out_no_end:
 	spin_unlock(&desc->lock);
 
 	return 1;

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Ingo Molnar wrote:

>>i'm hacking up something now to see whether it makes sense to 
>>introduce a central threaded flow type, or whether it's better the 
>>branch off the current flow types (as the code does it right now).

> ok, a central flow type caused more problems than good - the main 
> complication is that the handler needs to know the true 'flow' 
> (edge/level/fasteoi, etc.) anyway, even in the threaded case.

> So i rather went on making the existing flow handlers more 
> threading-friendly, and undoing the x86_64 and i386 arch changes to make 
> sure that the default handlers all work fine. Does the patch below 
> (against -rt4) do the trick for your on PPC too?

    Not without my patch.

> 	Ingo
> 
> Index: linux/arch/i386/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/io_apic.c
> +++ linux/arch/i386/kernel/io_apic.c
> @@ -1272,22 +1272,12 @@ static struct irq_chip ioapic_chip;
>  static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
>  {
>  	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
> -			trigger == IOAPIC_LEVEL) {
> -#ifdef CONFIG_PREEMPT_HARDIRQS
> +			trigger == IOAPIC_LEVEL)
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					    handle_level_irq, "level-threaded");
> -#else
> -		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					      handle_fasteoi_irq, "fasteoi");
> -#endif
> -	} else {
> -#ifdef CONFIG_PREEMPT_HARDIRQS
> +					 handle_fasteoi_irq, "fasteoi");
> +	else {
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					      handle_edge_irq, "edge-threaded");
> -#else
> -		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					      handle_edge_irq, "edge");
> -#endif
> +					 handle_edge_irq, "edge");

    Hm, why force edge flow on edge-triggered IRQs?

>  	}
>  	set_intr_gate(vector, interrupt[irq]);
>  }
> Index: linux/arch/x86_64/kernel/io_apic.c
> ===================================================================
> --- linux.orig/arch/x86_64/kernel/io_apic.c
> +++ linux/arch/x86_64/kernel/io_apic.c
> @@ -787,22 +787,12 @@ static struct irq_chip ioapic_chip;
>  static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
>  {
>  	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
> -			trigger == IOAPIC_LEVEL) {
> -#ifdef CONFIG_PREEMPT_HARDIRQS
> -		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					    handle_level_irq, "level-threaded");
> -#else
> +			trigger == IOAPIC_LEVEL)
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					      handle_fasteoi_irq, "fasteoi");
> -#endif
> -	} else {
> -#ifdef CONFIG_PREEMPT_HARDIRQS
> -		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> -					      handle_edge_irq, "edge-threaded");
> -#else
> +	else {
>  		set_irq_chip_and_handler_name(irq, &ioapic_chip,
>  					      handle_edge_irq, "edge");

     Same here...

> -#endif
>  	}
>  }
>  
> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -238,8 +238,10 @@ static inline void mask_ack_irq(struct i
>  	if (desc->chip->mask_ack)
>  		desc->chip->mask_ack(irq);
>  	else {
> -		desc->chip->mask(irq);
> -		desc->chip->ack(irq);
> +		if (desc->chip->mask)
> +			desc->chip->mask(irq);
> +		if (desc->chip->mask)
> +			desc->chip->ack(irq);
>  	}
>  }

    Hmm, that just won't do for PPC threaded fasteoi flows! What you'll get is 
a threaded IRQ with EOI *never ever* issued, unless my PPC patch is also in...

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> >-#else
> >-		set_irq_chip_and_handler_name(irq, &ioapic_chip,
> >-					      handle_edge_irq, "edge");
> >-#endif
> >+					 handle_edge_irq, "edge");
> 
>    Hm, why force edge flow on edge-triggered IRQs?

it doesnt do that. This, ontop of -rt4 restores it to the vanilla arch 
code.

> > 	if (desc->chip->mask_ack)
> > 		desc->chip->mask_ack(irq);
> > 	else {
> >-		desc->chip->mask(irq);
> >-		desc->chip->ack(irq);
> >+		if (desc->chip->mask)
> >+			desc->chip->mask(irq);
> >+		if (desc->chip->mask)
> >+			desc->chip->ack(irq);
> > 	}
> > }
> 
>    Hmm, that just won't do for PPC threaded fasteoi flows! What you'll 
> get is a threaded IRQ with EOI *never ever* issued, unless my PPC 
> patch is also in...

ok, how about the patch below in addition?

	Ingo

Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -396,7 +398,7 @@ handle_fasteoi_irq(unsigned int irq, str
 	 */
 	if (redirect_hardirq(desc)) {
 		mask_ack_irq(desc, irq);
-		goto out_unlock;
+		goto out;
 	}
 
 	desc->status &= ~IRQ_PENDING;

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> >    Hmm, that just won't do for PPC threaded fasteoi flows! What you'll 
> > get is a threaded IRQ with EOI *never ever* issued, unless my PPC 
> > patch is also in...
> 
> ok, how about the patch below in addition?

or rather, the one below. Untested.

	Ingo

Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -392,11 +394,12 @@ handle_fasteoi_irq(unsigned int irq, str
 	desc->status |= IRQ_INPROGRESS;
 
 	/*
-	 * In the threaded case we fall back to a mask+ack sequence:
+	 * In the threaded case we fall back to a mask+eoi sequence:
 	 */
 	if (redirect_hardirq(desc)) {
-		mask_ack_irq(desc, irq);
-		goto out_unlock;
+		if (desc->chip->mask)
+			desc->chip->mask(irq);
+		goto out;
 	}
 
 	desc->status &= ~IRQ_PENDING;

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Ingo Molnar wrote:

>>>   Hmm, that just won't do for PPC threaded fasteoi flows! What you'll 
>>>get is a threaded IRQ with EOI *never ever* issued, unless my PPC 
>>>patch is also in...

>>ok, how about the patch below in addition?

> or rather, the one below. Untested.

    Actually, it's been tested since it's close to Daniel's original variant. 
Should do it.

> 	Ingo

> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -392,11 +394,12 @@ handle_fasteoi_irq(unsigned int irq, str
>  	desc->status |= IRQ_INPROGRESS;
>  
>  	/*
> -	 * In the threaded case we fall back to a mask+ack sequence:
> +	 * In the threaded case we fall back to a mask+eoi sequence:
>  	 */
>  	if (redirect_hardirq(desc)) {
> -		mask_ack_irq(desc, irq);
> -		goto out_unlock;
> +		if (desc->chip->mask)
> +			desc->chip->mask(irq);
> +		goto out;
>  	}
>  
>  	desc->status &= ~IRQ_PENDING;
> 
> 

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Ingo Molnar]

* Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> >>	if (redirect_hardirq(desc)) {
> >>-		mask_ack_irq(desc, irq);
> >>-		goto out_unlock;
> 
>    That label would generate a warning though. Should be gotten rid of now.

yep, gone in my tree.

	Ingo

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

>>>>  Hmm, that just won't do for PPC threaded fasteoi flows! What you'll 
>>>>get is a threaded IRQ with EOI *never ever* issued, unless my PPC 
>>>>patch is also in...

>>>ok, how about the patch below in addition?

>>or rather, the one below. Untested.

>     Actually, it's been tested since it's close to Daniel's original variant. 
> Should do it.

    Can't say that about the interrupt controllers without mask() method -- 
that *really* needs testing...

>>Index: linux/kernel/irq/chip.c
>>===================================================================
>>--- linux.orig/kernel/irq/chip.c
>>+++ linux/kernel/irq/chip.c
>>@@ -392,11 +394,12 @@ handle_fasteoi_irq(unsigned int irq, str
>> 	desc->status |= IRQ_INPROGRESS;
>> 
>> 	/*
>>-	 * In the threaded case we fall back to a mask+ack sequence:
>>+	 * In the threaded case we fall back to a mask+eoi sequence:
>> 	 */
>> 	if (redirect_hardirq(desc)) {
>>-		mask_ack_irq(desc, irq);
>>-		goto out_unlock;

    That label would generate a warning though. Should be gotten rid of now.

>>+		if (desc->chip->mask)
>>+			desc->chip->mask(irq);
>>+		goto out;
>> 	}
>> 
>> 	desc->status &= ~IRQ_PENDING;
>>
>>

WBR, Sergei

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Mon, 2006-11-20 at 11:01 +0100, Ingo Molnar wrote:

> so the question is not 'is there an ACK' (all non-MSI-type-of IRQ 
> delivery mechanisms have some sort of ACK mechanism), but what is the 
> precise structure of ACK-ing an IRQ that the host recieves.
> 
> on PPC64, 'get the vector' initiates an ACK as well - is that done 
> before handle_irq() is done?

Yes.

> > So by doing a mask followed by an eoi, you essentially mask the 
> > interrupt preventing further delivery of that interrupt and lower the 
> > CPU priority in the PIC thus allowing processing of further 
> > interrupts.
> 
> correct, that's what should happen.
> 
> > Are there other fasteoi controllers than the ones I have on powerpc 
> > anyway ?
> 
> well, if you mean the x86 APICs, there you get the vector 'for free' as 
> part of the IRQ entry call sequence, and there's an EOI register in the 
> local APIC that notifies the IRQ hardware, lowers the CPU priority, etc. 
> We have that as an ->eoi handler right now.

Ok, so that's like me. Which means that what you need is a specific thre
aded_fasteoi flow handler that does mask & eoi, not ack.

Note that I still think it would work in the absence of mask too if the
controller only does edge interrupts, as it is the case for the cell.

Cheers,
Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

On Mon, 2006-11-20 at 17:56 +0100, Ingo Molnar wrote:
> * Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
> > >on PPC64, 'get the vector' initiates an ACK as well - is that done 
> > >before handle_irq() is done?
> > 
> >    Exactly. How else do_IRQ() would know the vector?
> 
> the reason i'm asking is that in this case masking is a bit late at this 
> point and there's a chance for a repeat interrupt.

What do you mean by a bit late ?

You can't mask before you know what interrupt occured so you don't
really have a choice there :-) I'm pretty sure that mask + eoi is what
Apple does on Darwin too though.

Cheers,
Ben.

Re: [PATCH] 2.6.18-rt7: PowerPC: fix breakage in threaded fasteoi type IRQ handlers

[Benjamin Herrenschmidt]

> Index: linux/kernel/irq/chip.c
> ===================================================================
> --- linux.orig/kernel/irq/chip.c
> +++ linux/kernel/irq/chip.c
> @@ -238,8 +238,10 @@ static inline void mask_ack_irq(struct i
>  	if (desc->chip->mask_ack)
>  		desc->chip->mask_ack(irq);
>  	else {
> -		desc->chip->mask(irq);
> -		desc->chip->ack(irq);
> +		if (desc->chip->mask)
> +			desc->chip->mask(irq);
> +		if (desc->chip->mask)
> +			desc->chip->ack(irq);

Looks like a typo to me :-)

Cheers,
Ben.

[PATCH 2.6.21-rt2] PowerPC: revert fix for threaded fasteoi IRQ handlers

[Sergei Shtylyov]

Revert the change to the "fasteoi" type chips as after handle_fasteoi_irq() had
been fixed, they've become meaningless (and even dangerous -- as was the case
with Celleb that has been fixed earlier)...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
The patch in question wasn't even initially accepted but then was erroneously
restored along with the TOD patch.  I've asked to revert it but to no avail,
so here's the formal patch to revert it at last...

 arch/powerpc/platforms/iseries/irq.c  |    1 -
 arch/powerpc/platforms/pseries/xics.c |    2 --
 arch/powerpc/sysdev/mpic.c            |    1 -
 3 files changed, 4 deletions(-)

Index: linux-2.6/arch/powerpc/platforms/iseries/irq.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/iseries/irq.c
+++ linux-2.6/arch/powerpc/platforms/iseries/irq.c
@@ -279,7 +279,6 @@ static struct irq_chip iseries_pic = {
 	.shutdown	= iseries_shutdown_IRQ,
 	.unmask		= iseries_enable_IRQ,
 	.mask		= iseries_disable_IRQ,
-	.ack		= iseries_end_IRQ,
 	.eoi		= iseries_end_IRQ
 };
 
Index: linux-2.6/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6/arch/powerpc/platforms/pseries/xics.c
@@ -456,7 +456,6 @@ static struct irq_chip xics_pic_direct =
 	.startup = xics_startup,
 	.mask = xics_mask_irq,
 	.unmask = xics_unmask_irq,
-	.ack = xics_eoi_direct,
 	.eoi = xics_eoi_direct,
 	.set_affinity = xics_set_affinity
 };
@@ -467,7 +466,6 @@ static struct irq_chip xics_pic_lpar = {
 	.startup = xics_startup,
 	.mask = xics_mask_irq,
 	.unmask = xics_unmask_irq,
-	.ack = xics_eoi_lpar,
 	.eoi = xics_eoi_lpar,
 	.set_affinity = xics_set_affinity
 };
Index: linux-2.6/arch/powerpc/sysdev/mpic.c
===================================================================
--- linux-2.6.orig/arch/powerpc/sysdev/mpic.c
+++ linux-2.6/arch/powerpc/sysdev/mpic.c
@@ -776,7 +776,6 @@ static int mpic_set_irq_type(unsigned in
 static struct irq_chip mpic_irq_chip = {
 	.mask		= mpic_mask_irq,
 	.unmask		= mpic_unmask_irq,
-	.ack		= mpic_end_irq,
 	.eoi		= mpic_end_irq,
 	.set_type	= mpic_set_irq_type,
 };

Re: [PATCH 2.6.21-rt2] PowerPC: revert fix for threaded fasteoi IRQ handlers

[Sergei Shtylyov]

Hello, I wrote:

> Revert the change to the "fasteoi" type chips as after handle_fasteoi_irq() had
> been fixed, they've become meaningless (and even dangerous -- as was the case
> with Celleb that has been fixed earlier)...

> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> ---
> The patch in question wasn't even initially accepted but then was erroneously
> restored along with the TOD patch.  I've asked to revert it but to no avail,
> so here's the formal patch to revert it at last...

    Now that the -rt patch has been first release in the broken-out version, 
let me tell you that the following 3 patches in the series can be just 
annihilated:

preempt-irqs-ppc-ack-irq-fixups.patch
preempt-irqs-ppc-fix-b5.patch
preempt-irqs-ppc-fix-more-fasteoi.patch

as all that the latter two are doing is undoing the former one.

WBR, Sergei

Re: [PATCH 2.6.21-rt2] PowerPC: revert fix for threaded fasteoi IRQ handlers

[Thomas Gleixner]

On Thu, 2007-07-12 at 20:47 +0400, Sergei Shtylyov wrote:
>     Now that the -rt patch has been first release in the broken-out version, 
> let me tell you that the following 3 patches in the series can be just 
> annihilated:
> 
> preempt-irqs-ppc-ack-irq-fixups.patch
> preempt-irqs-ppc-fix-b5.patch
> preempt-irqs-ppc-fix-more-fasteoi.patch
> 
> as all that the latter two are doing is undoing the former one.

Really ? I know for quite a while. 

We kept some of those files to document contributions. Go read back on
the LKML archives, where we were accused more than once not to keep
track of these things.

They can be folded together and fixed up, one thing after the other
please.

	tglx

Re: [PATCH 2.6.21-rt2] PowerPC: revert fix for threaded fasteoi IRQ handlers

[Sergei Shtylyov]

Hello.

Thomas Gleixner wrote:

>>    Now that the -rt patch has been first release in the broken-out version, 
>>let me tell you that the following 3 patches in the series can be just 
>>annihilated:

>>preempt-irqs-ppc-ack-irq-fixups.patch
>>preempt-irqs-ppc-fix-b5.patch
>>preempt-irqs-ppc-fix-more-fasteoi.patch

>>as all that the latter two are doing is undoing the former one.

> Really ? I know for quite a while. 

    Just reminding. :-)

> We kept some of those files to document contributions. Go read back on
> the LKML archives, where we were accused more than once not to keep
> track of these things.

    Yeah, I figured that out. This case is somewhat special though since the 
patch that started this should't even have been there in the firts place.

> They can be folded together and fixed up, one thing after the other
> please.

    OK, no hurry. :-)

> 	tglx

WBR, Sergei