From: Andreas Schwab <hidden> Date: 2012-04-07 12:27:26
Benjamin Herrenschmidt [off-list ref] writes:
It's arguable that this irq_set_irq_type(,NONE) shouln't be there but
still ... it's been around for ever and things worked :-) So something
-else- is causing the problem and I'd like to understand what exactly.
AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5
irq_set_irq_type(,NONE) was actually a no-op.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-04-11 01:14:33
On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote:
Benjamin Herrenschmidt [off-list ref] writes:
quoted
It's arguable that this irq_set_irq_type(,NONE) shouln't be there but
still ... it's been around for ever and things worked :-) So something
-else- is causing the problem and I'd like to understand what exactly.
AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5
irq_set_irq_type(,NONE) was actually a no-op.
So I'm still a bit baffled... ie, I understand some of what's happening
but not why it breaks things, I haven't yet managed to reproduce but I
haven't tried too hard just yet (was away from the HW) :
Basically, we end up setting the mpic to IRQ_TYPE_LEVEL_LOW as a result
of a request to set it to IRQ_TYPE_NONE, ie, we apply a "default"
setting instead of just setting it to "none" (in part because we don't
have a way in HW to disable the trigger completely. We then write that
into the irq_desc with irqd_set_trigger_type().
Then we return IRQ_SET_MASK_OK_NOCOPY, which means that the generic
__irq_set_trigger() code will read back what we've set in the desc and
adjust things accordingly, rather than writing the original value in,
ie, our descriptor ends up being effectively set to IRQ_TYPE_LEVEL_LOW
rather than IRQ_TYPE_NONE.
Later, when we actually try to set it to IRQ_TYPE_LEVEL_LOW as a result
of parsing the device-tree, we then hit the code
irq_create_of_mapping(), at the bottom, which will skip the call to
irq_set_irq_type() if we are trying to set it to the same type that it's
already set to....
But I don't see why that breaks anything, ie we -have- set things to
LEVEL_LOW as a result of IRQ_TYPE_NONE, which is the right setting, so
things should work despite the fact that we skip the second call.
Out of curiosity, can you try removing the check in irqdomain and always
apply the setting regardless of the previous value (ie, remove the check
of type against irqd_get_trigger at the end of irq_create_of_mapping),
see if that makes a difference ?
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-04-11 01:33:40
On Wed, 2012-04-11 at 11:13 +1000, Benjamin Herrenschmidt wrote:
On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote:
quoted
Benjamin Herrenschmidt [off-list ref] writes:
quoted
It's arguable that this irq_set_irq_type(,NONE) shouln't be there but
still ... it's been around for ever and things worked :-) So something
-else- is causing the problem and I'd like to understand what exactly.
AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5
irq_set_irq_type(,NONE) was actually a no-op.
So I'm still a bit baffled... ie, I understand some of what's happening
but not why it breaks things, I haven't yet managed to reproduce but I
haven't tried too hard just yet (was away from the HW) :
Allright, I have a repro-case, I'll dig.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-04-11 05:30:15
On Wed, 2012-04-11 at 11:33 +1000, Benjamin Herrenschmidt wrote:
On Wed, 2012-04-11 at 11:13 +1000, Benjamin Herrenschmidt wrote:
quoted
On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote:
quoted
Benjamin Herrenschmidt [off-list ref] writes:
quoted
It's arguable that this irq_set_irq_type(,NONE) shouln't be there but
still ... it's been around for ever and things worked :-) So something
-else- is causing the problem and I'd like to understand what exactly.
AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5
irq_set_irq_type(,NONE) was actually a no-op.
So I'm still a bit baffled... ie, I understand some of what's happening
but not why it breaks things, I haven't yet managed to reproduce but I
haven't tried too hard just yet (was away from the HW) :
Allright, I have a repro-case, I'll dig.
Ok, so it's Grant's fault :-)
So basically, it's quite subtle and I'm only 99% sure of the details but
I believe what happens is:
- The audio interrupts get virq 64 and 65 (so above NR_IRQS)
- The reverse map isn't pre-filled at map time (we should probably do
it nowadays), we do it lazily so ...
- The audio interrupt happens, it tries to pre-fill the reverse map
and ... fails (see below)
- This cause irq_linear_revmap() to returns 0
- This hits another bug in mpic where when that happens it doesn't EOI
the interrupt, which means the priority remains stuck high and we don't
get any other interrupt.
There's thus two bugs here:
- We don't properly establish the reverse mapping for 64 and 65 (well,
not always, again see below)
- We don't EOI an interrupt we can't reverse map (we should warn & EOI,
I'll send a patch for that).
The second problem is a bug we shouldn't hit if the first one didn't
happen, the first one comes from way irq_find_mapping() works. Basically
when the revmap entry is 0, we call it to find the mapping & populate
the revmap... and that fails.
That fails because it will only search up to irq_virq_count which is
statically initialized to NR_IRQS, which in our case is 64 so it won't
find our interrupts 64 and 65...
The reason it works if you don't do the set_type(NONE) is a fluke, it
will crash as soon as you actually do audio:
The set_type(NONE) has the effect in mpic to configure the interrupt to
level low (default). This is also the idle state of the audio interrupt
(which is positive edge), and the MPIC appears to be latching it (yeah
odd, it does seem to latch level interrupts).
So as soon as the audio driver enables it, it fires, triggering the bug.
Without the set_type(NONE) the occurence of the bug is delayed to the
fist time you get a real audio interrupt.
Now what is the solution ? Well, there's several things I think we need
to do:
- MPIC should properly EOI interrupts it can't revmap (I'll fix that)
- MPIC should probably not do the set_type(NONE) on map, it's not
useful
- However, we do have a risk of discrepency between the default trigger
type in the irq_desc vs. the default HW value at startup/map time, so we
do need to do something to reconcile them (that was the intend of NONE I
think)... without that, we risk having irq_create_of_mapping() not
setting the trigger because it thinks it thinks it's right... What do
you think is best here ? We can probably read the HW config and sync the
irq desc appropriately ...
- The whole business with irq_virq_count needs fixing. Basically the
default value shouldn't be NR_IRQ. I suggest making it 0 and have all
the use sites do something like:
max = irq_virq_count ? irq_virq_count : nr_irqs;
(Grant, can you take care of that ?)
- We still need to clear up all the NR_IRQS occurences in arch/powerpc
Cheers,
Ben.
From: Grant Likely <hidden> Date: 2012-04-11 20:57:12
On Wed, 11 Apr 2012 15:29:42 +1000, Benjamin Herrenschmidt [off-list ref] wrote:
On Wed, 2012-04-11 at 11:33 +1000, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2012-04-11 at 11:13 +1000, Benjamin Herrenschmidt wrote:
quoted
On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote:
quoted
Benjamin Herrenschmidt [off-list ref] writes:
quoted
It's arguable that this irq_set_irq_type(,NONE) shouln't be there but
still ... it's been around for ever and things worked :-) So something
-else- is causing the problem and I'd like to understand what exactly.
AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5
irq_set_irq_type(,NONE) was actually a no-op.
So I'm still a bit baffled... ie, I understand some of what's happening
but not why it breaks things, I haven't yet managed to reproduce but I
haven't tried too hard just yet (was away from the HW) :
Allright, I have a repro-case, I'll dig.
Ok, so it's Grant's fault :-)
I pretty much expected it would be. :-p
So basically, it's quite subtle and I'm only 99% sure of the details but
I believe what happens is:
- The audio interrupts get virq 64 and 65 (so above NR_IRQS)
- The reverse map isn't pre-filled at map time (we should probably do
it nowadays), we do it lazily so ...
Hmmm... I though I had merged a patch that does that.
/me goes to look again...
Okay, I did write that patch, but I never merged it because it didn't
get much review and I was already nervous about the other irq_domain
changes. I'll post it again and ask for feedback.
- The whole business with irq_virq_count needs fixing. Basically the
default value shouldn't be NR_IRQ. I suggest making it 0 and have all
the use sites do something like:
max = irq_virq_count ? irq_virq_count : nr_irqs;
(Grant, can you take care of that ?)
Yeah, I've got a different way to fix it though. There is exactly one
user of irq_virq_count in-tree right now: PS3. Also, irq_virq_count
is only useful for the NOMAP mapping. So, instead of having a single
global irq_virq_count values, I've dropped it entirely and added a
max_irq argument to irq_domain_add_nomap(). That makes it a property
of an individual nomap irq domain instead of a global system settting.
Hopefully I'll have a draft patch ready today.
g.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-04-11 21:38:06
On Wed, 2012-04-11 at 14:57 -0600, Grant Likely wrote:
Yeah, I've got a different way to fix it though. There is exactly one
user of irq_virq_count in-tree right now: PS3. Also, irq_virq_count
is only useful for the NOMAP mapping. So, instead of having a single
global irq_virq_count values, I've dropped it entirely and added a
max_irq argument to irq_domain_add_nomap(). That makes it a property
of an individual nomap irq domain instead of a global system settting.
Hopefully I'll have a draft patch ready today.
That works for me. I'll send patches for cleanup MPIC as well.
One thing tho (Thomas, Russell) is that I like using set_irq_trigger to
establish the "defaults" in mpic, ie, it does the descriptor locking
etc... for me, I'd rather avoid open coding all of that. What I need is
a "variant" that doesn't actually change the trigger but instead
initializes the irq_desc with whatever settings the HW currently has
(ie, I need to make sure things are properly in sync) though other
implementations may want to use that for defaults.
Any objection to defining something like IRQ_TYPE_DEFAULT ?
I was thinking about making it equal to IRQ_TYPE_SENSE_MASK since that
can obviously not be a valid trigger value and is distinct from
IRQ_TYPE_NONE.
Cheers,
Ben.
From: Thomas Gleixner <hidden> Date: 2012-04-11 21:47:32
On Thu, 12 Apr 2012, Benjamin Herrenschmidt wrote:
On Wed, 2012-04-11 at 14:57 -0600, Grant Likely wrote:
quoted
Yeah, I've got a different way to fix it though. There is exactly one
user of irq_virq_count in-tree right now: PS3. Also, irq_virq_count
is only useful for the NOMAP mapping. So, instead of having a single
global irq_virq_count values, I've dropped it entirely and added a
max_irq argument to irq_domain_add_nomap(). That makes it a property
of an individual nomap irq domain instead of a global system settting.
Hopefully I'll have a draft patch ready today.
That works for me. I'll send patches for cleanup MPIC as well.
One thing tho (Thomas, Russell) is that I like using set_irq_trigger to
establish the "defaults" in mpic, ie, it does the descriptor locking
etc... for me, I'd rather avoid open coding all of that. What I need is
a "variant" that doesn't actually change the trigger but instead
initializes the irq_desc with whatever settings the HW currently has
(ie, I need to make sure things are properly in sync) though other
implementations may want to use that for defaults.
Any objection to defining something like IRQ_TYPE_DEFAULT ?
I was thinking about making it equal to IRQ_TYPE_SENSE_MASK since that
can obviously not be a valid trigger value and is distinct from
IRQ_TYPE_NONE.
From: Grant Likely <hidden> Date: 2012-04-19 18:42:43
On Thu, 12 Apr 2012 07:37:28 +1000, Benjamin Herrenschmidt [off-list ref] wrote:
On Wed, 2012-04-11 at 14:57 -0600, Grant Likely wrote:
quoted
Yeah, I've got a different way to fix it though. There is exactly one
user of irq_virq_count in-tree right now: PS3. Also, irq_virq_count
is only useful for the NOMAP mapping. So, instead of having a single
global irq_virq_count values, I've dropped it entirely and added a
max_irq argument to irq_domain_add_nomap(). That makes it a property
of an individual nomap irq domain instead of a global system settting.
Hopefully I'll have a draft patch ready today.
That works for me. I'll send patches for cleanup MPIC as well.
Okay, I'll wait on these. The MPIC fixes will need to be applied
before I can apply the automatic revmapping and hint removal patches.
Those patches will also need testing before I apply to linux-next.
g.