Regression with legacy IRQ numbers caused by 9a1091ef0017

Hi all,

Looks like the legacy IRQ numbers are now all wrong at least for omap4 since commit 9a1091ef0017 ("irqchip: gic: Support hierarchy irq domain.").

Instead of this:

# cat /proc/interrupts 
            CPU0       CPU1       
 29:       1124        981       GIC  29  twd
 39:          0          0       GIC  39  TWL6030-PIH
 41:          0          0       GIC  41  l3-dbg-irq
 42:          0          0       GIC  42  l3-app-irq
 44:          0          0       GIC  44  DMA
 45:       7854          0       GIC  45  omap-dma-engine
 52:          0          0       GIC  52  gpmc
...

We now have:

# cat /proc/interrupts 
            CPU0       CPU1       
 16:        343          0       GIC  69  gp_timer
 17:       1160       1017       GIC  29  twd
 18:          0          0       GIC  41  l3-dbg-irq
 19:          1          0       GIC  42  l3-app-irq
 22:       7850          0       GIC  45  omap-dma-engine
 44:          0          0  4a310000.gpio  18  DMA
 61:       2730          0  48055000.gpio   2  eth0
223:          0          0       GIC  52  gpmc
...

So the DMA interrupt using the legacy mapping with something like irq = 12 + OMAP44XX_IRQ_GIC_START now is wrong and unfortunately at least omaps still have a bunch of the legacy interrupts still around.

And that naturally produces all kinds of strange errors like:

WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()

44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
...
[<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530) [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
[<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
[<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
...

Quoted 41 lines · Tony Lindgren

Hi all,

Looks like the legacy IRQ numbers are now all wrong at least for omap4 since commit 9a1091ef0017 ("irqchip: gic: Support hierarchy irq domain.").

Instead of this:

# cat /proc/interrupts 
            CPU0       CPU1       
 29:       1124        981       GIC  29  twd
 39:          0          0       GIC  39  TWL6030-PIH
 41:          0          0       GIC  41  l3-dbg-irq
 42:          0          0       GIC  42  l3-app-irq
 44:          0          0       GIC  44  DMA
 45:       7854          0       GIC  45  omap-dma-engine
 52:          0          0       GIC  52  gpmc
...

We now have:

# cat /proc/interrupts 
            CPU0       CPU1       
 16:        343          0       GIC  69  gp_timer
 17:       1160       1017       GIC  29  twd
 18:          0          0       GIC  41  l3-dbg-irq
 19:          1          0       GIC  42  l3-app-irq
 22:       7850          0       GIC  45  omap-dma-engine
 44:          0          0  4a310000.gpio  18  DMA
 61:       2730          0  48055000.gpio   2  eth0
223:          0          0       GIC  52  gpmc
...

So the DMA interrupt using the legacy mapping with something like irq = 12 + OMAP44XX_IRQ_GIC_START now is wrong and unfortunately at least omaps still have a bunch of the legacy interrupts still around.

And that naturally produces all kinds of strange errors like:

WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()

Quoted 8 lines

44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
...
[<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530) [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
[<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
[<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
...

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

There's something else going on which causes the bus errors which we haven't found.

Quoted 7 lines · Russell King - ARM Linux

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

There's something else going on which causes the bus errors which we haven't found.

I think it gets triggered if you enable PREEMPT.

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Russell King - ARM Linux

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

Quoted 4 lines

Quoted 2 lines

There's something else going on which causes the bus errors which we haven't found.

I think it gets triggered if you enable PREEMPT.

That's something which we can try to prove... build running now with CONFIG_PREEMPT=y

Quoted 12 lines · Russell King - ARM Linux

Quoted 10 lines · Tony Lindgren

Quoted 7 lines · Russell King - ARM Linux

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

There's something else going on which causes the bus errors which we haven't found.

I think it gets triggered if you enable PREEMPT.

Quoted 2 lines

That's something which we can try to prove... build running now with CONFIG_PREEMPT=y

Looks like you now have the omap_l3_noc error appear for sdp4430 in your logs after enabling PREEMPT.

I guess that means case closed for this one?

Quoted 19 lines · Tony Lindgren

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Russell King - ARM Linux

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

There's something else going on which causes the bus errors which we haven't found.

I think it gets triggered if you enable PREEMPT.

That's something which we can try to prove... build running now with CONFIG_PREEMPT=y

Looks like you now have the omap_l3_noc error appear for sdp4430 in your logs after enabling PREEMPT.

Quoted 1 line

I guess that means case closed for this one?

I would still like to understand /why/ enabling preempt causes the error. Changing the preempt configuration really should not change what happens on the bus. (Think about it.) It's an indication that there is some other error present.

Unfortunately, the OMAP hardware appears to make it impossible to determine what the access that caused the error was, so it looks like it's pretty much undebuggable.

Quoted 20 lines · Russell King - ARM Linux

Quoted 19 lines · Tony Lindgren

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Russell King - ARM Linux

I don't think we've proven a link there. While you're right that it causes the wrong interrupt to be claimed, I have two kernels here, both claim the same interrupt, one which is multi-platform and issues that strange warning, and one which targets only OMAP4 which doesn't.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

There's something else going on which causes the bus errors which we haven't found.

I think it gets triggered if you enable PREEMPT.

That's something which we can try to prove... build running now with CONFIG_PREEMPT=y

Looks like you now have the omap_l3_noc error appear for sdp4430 in your logs after enabling PREEMPT.

Quoted 6 lines

Quoted 1 line

I guess that means case closed for this one?

I would still like to understand /why/ enabling preempt causes the error. Changing the preempt configuration really should not change what happens on the bus. (Think about it.) It's an indication that there is some other error present.

We have a wrong irq number caused by $subject. And the wrong irq gets triggered before the dma hardware is configured during dma init. And then we get the invalid access error from omap_l3_noc.

Quoted 3 lines

Unfortunately, the OMAP hardware appears to make it impossible to determine what the access that caused the error was, so it looks like it's pretty much undebuggable.

Yeah would be nice to have more info from omap_l3_noc.

Quoted 4 lines · Russell King - ARM Linux

I would still like to understand /why/ enabling preempt causes the error. Changing the preempt configuration really should not change what happens on the bus. (Think about it.) It's an indication that there is some other error present.

We have a wrong irq number caused by $subject. And the wrong irq gets triggered before the dma hardware is configured during dma init. And then we get the invalid access error from omap_l3_noc.

Quoted 9 lines · Tony Lindgren

Quoted 4 lines · Russell King - ARM Linux

I would still like to understand /why/ enabling preempt causes the error. Changing the preempt configuration really should not change what happens on the bus. (Think about it.) It's an indication that there is some other error present.

We have a wrong irq number caused by $subject. And the wrong irq gets triggered before the dma hardware is configured during dma init. And then we get the invalid access error from omap_l3_noc.

... which should happen whether or not preempt is enabled, which is really my point.

We know tha the wrong IRQ gets requested by the driver - and that wrong IRQ is requested whether or not we have preempt enabled. Yet we get the warning whether or not preempt is enabled.

The DMA handler is not registered as a threaded handler, so it's not depending on a context switch to execute omap2_dma_irq_handler().

Another reason why I don't agree with your explanation is that by the time setup_irq() is called, we have already poked at the DMA hardware several times - omap_clear_dma() and omap2_disable_irq_lch() will have been called for each DMA channel - and both will write to the hardware.

What's more is that the only things left after setup_irq() has been called is to possibly reserve the first two DMA channels and print the DMA message (via show_dma_caps). So I see nothing after setup_irq() which would "finish" any unfinished hardware initialisation.

The final reason I don't agree is that I've put a printk() in omap2_dma_irq_handler(), and this does not trigger.

So, I think this has nothing to do with the DMA hardware /at all/, but more to do with the GPIO code, and it suggests that the GPIO code publishes IRQs before it is safe for those IRQs to be used.

Maybe it has to do with omap_gpio_irq_handler() being called... added printk(), nope, that's not called either. So it's not an IRQ which gets triggered at all.

What is called are (in order):

omap_gpio_unmask_irq()
omap_set_gpio_irqenable()
omap_enable_gpio_irqbank()

and this reveals where the problem is, especially when you then add instrumentation into the runtime PM functions - and this reveals that when a GPIO IRQ is requested, these functions are called while the GPIO is runtime suspended.

_That_ is where the *real* problem lies - requesting a GPIO interrupt results in the kernel touching possibly runtime-suspended hardware.

The reason it happens with preempt is that preempt introduces scheduling points during the kernel boot which would not otherwise be there (with preempt disabled, you have to hit an explicit context switch due to contention on some lock or a wait in order for some other thread to run.)

So, the GPIO driver really needs fixing - and I'd suggest fixing it first, before fixing the DMA problem, because the DMA problem allows us to see the GPIO problem.

Quoted 26 lines · Russell King - ARM Linux

Quoted 9 lines · Tony Lindgren

Quoted 4 lines · Russell King - ARM Linux

I would still like to understand /why/ enabling preempt causes the error. Changing the preempt configuration really should not change what happens on the bus. (Think about it.) It's an indication that there is some other error present.

We have a wrong irq number caused by $subject. And the wrong irq gets triggered before the dma hardware is configured during dma init. And then we get the invalid access error from omap_l3_noc.

... which should happen whether or not preempt is enabled, which is really my point.

We know tha the wrong IRQ gets requested by the driver - and that wrong IRQ is requested whether or not we have preempt enabled. Yet we get the warning whether or not preempt is enabled.

The DMA handler is not registered as a threaded handler, so it's not depending on a context switch to execute omap2_dma_irq_handler().

Another reason why I don't agree with your explanation is that by the time setup_irq() is called, we have already poked at the DMA hardware several times - omap_clear_dma() and omap2_disable_irq_lch() will have been called for each DMA channel - and both will write to the hardware.

Quoted 7 lines

What's more is that the only things left after setup_irq() has been called is to possibly reserve the first two DMA channels and print the DMA message (via show_dma_caps). So I see nothing after setup_irq() which would "finish" any unfinished hardware initialisation.

The final reason I don't agree is that I've put a printk() in omap2_dma_irq_handler(), and this does not trigger.

Oh, yes that blows my theory completely then.

Quoted 19 lines

So, I think this has nothing to do with the DMA hardware /at all/, but more to do with the GPIO code, and it suggests that the GPIO code publishes IRQs before it is safe for those IRQs to be used.

Maybe it has to do with omap_gpio_irq_handler() being called... added printk(), nope, that's not called either. So it's not an IRQ which gets triggered at all.

What is called are (in order):

omap_gpio_unmask_irq()
omap_set_gpio_irqenable()
omap_enable_gpio_irqbank()

and this reveals where the problem is, especially when you then add instrumentation into the runtime PM functions - and this reveals that when a GPIO IRQ is requested, these functions are called while the GPIO is runtime suspended.

Quoted 7 lines

_That_ is where the *real* problem lies - requesting a GPIO interrupt results in the kernel touching possibly runtime-suspended hardware.

The reason it happens with preempt is that preempt introduces scheduling points during the kernel boot which would not otherwise be there (with preempt disabled, you have to hit an explicit context switch due to contention on some lock or a wait in order for some other thread to run.)

OK makes sense.

Quoted 3 lines

So, the GPIO driver really needs fixing - and I'd suggest fixing it first, before fixing the DMA problem, because the DMA problem allows us to see the GPIO problem.

Yes we need to fix that.

Quoted 6 lines · Tony Lindgren

Quoted 3 lines · Russell King - ARM Linux

So, the GPIO driver really needs fixing - and I'd suggest fixing it first, before fixing the DMA problem, because the DMA problem allows us to see the GPIO problem.

Yes we need to fix that.

Posted a minimal fix for that one as a separate thread:

[PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Tony Lindgren

Quoted 3 lines · Russell King - ARM Linux

So, the GPIO driver really needs fixing - and I'd suggest fixing it first, before fixing the DMA problem, because the DMA problem allows us to see the GPIO problem.

Quoted 5 lines

Quoted 1 line

Yes we need to fix that.

Posted a minimal fix for that one as a separate thread:

[PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

Thanks, I'll throw that onto the build tree for tonights build.

Hi all,

Looks like the legacy IRQ numbers are now all wrong at least for omap4 since commit 9a1091ef0017 ("irqchip: gic: Support hierarchy irq domain.").

Instead of this:

# cat /proc/interrupts 
            CPU0       CPU1       
 29:       1124        981       GIC  29  twd
 39:          0          0       GIC  39  TWL6030-PIH
 41:          0          0       GIC  41  l3-dbg-irq
 42:          0          0       GIC  42  l3-app-irq
 44:          0          0       GIC  44  DMA
 45:       7854          0       GIC  45  omap-dma-engine
 52:          0          0       GIC  52  gpmc
...

We now have:

# cat /proc/interrupts 
            CPU0       CPU1       
 16:        343          0       GIC  69  gp_timer
 17:       1160       1017       GIC  29  twd
 18:          0          0       GIC  41  l3-dbg-irq
 19:          1          0       GIC  42  l3-app-irq
 22:       7850          0       GIC  45  omap-dma-engine
 44:          0          0  4a310000.gpio  18  DMA
 61:       2730          0  48055000.gpio   2  eth0
223:          0          0       GIC  52  gpmc
...

So the DMA interrupt using the legacy mapping with something like irq = 12 + OMAP44XX_IRQ_GIC_START now is wrong and unfortunately at least omaps still have a bunch of the legacy interrupts still around.

And that naturally produces all kinds of strange errors like:

WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()

44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
...
[<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530) [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
[<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
[<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
...

Looks like the logic changed from:

if (of_property_read_u32(node, "arm,routable-irqs", &nr_routable_irqs))

to just

if (node)

Which now causes irq_domain_add_linear() to be called instead of irq_domain_add_legacy(), which causes the breakage.

Anybody got a sane fix in mind for the -rc series, or should we just revert it for now?

Of course, this is in no way a proper fix, but I suppose the OMAP DT is still missing a few bits...

Quoted 2 lines · On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Of course, this is in no way a proper fix, but I suppose the OMAP DT is still missing a few bits...

I must be missing something here, but all the interrupts are listed correctly in the DT, so what is the omap_hwmod_irq_info actually achieving on omap4 and omap5?

Would it work if we just remove the incorrect copy of the resource and use the one that comes from DT?

Quoted 8 lines · Arnd Bergmann

Quoted 2 lines · On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Of course, this is in no way a proper fix, but I suppose the OMAP DT is still missing a few bits...

I must be missing something here, but all the interrupts are listed correctly in the DT, so what is the omap_hwmod_irq_info actually achieving on omap4 and omap5?

Quoted 2 lines

Would it work if we just remove the incorrect copy of the resource and use the one that comes from DT?

By the look of it, omap_hwmod_irq_info serves multiple purposes:
- low level configuration (pads, probably more stuff)

- interrupt description for some drivers, using resources.

It should be fairly easy to do the latter, but the former looks more tricky (it would push the pad configuration down to the drivers, which is avoided at the moment).

Probably there is a workable strategy, but my knowledge about OMAP is close to *nothing*...

Quoted 4 lines · Marc Zyngier

Quoted 1 line · Arnd Bergmann

On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Probably there is a workable strategy, but my knowledge about OMAP is close to *nothing*...

I have a feeling this might bite other platforms too and we just have not noticed it yet..

Quoted 8 lines · On Thursday 15 January 2015 07:37:48 Tony Lindgren wrote:

Quoted 4 lines · Marc Zyngier

Quoted 1 line · Arnd Bergmann

On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Probably there is a workable strategy, but my knowledge about OMAP is close to *nothing*...

I have a feeling this might bite other platforms too and we just have not noticed it yet..

I'm looking through the entire tree now, scanning for machines that have GIC and use IORESOURCE_IRQ or DEFINE_RES_IRQ in their platform code. Most platforms using GIC are completely converted to DT and have no hardcoded legacy IRQs.

I have checked that cns3xxx and realview are both fine by inspection.

The only one I'm not sure about is shmobile, which looks like it might suffer from the same problem. Simon/Magnus, could you verify this with a multiplatform kernel on any SoC that has GIC and uses devices that have interrupts defined in setup-*.c or board-*.c?

Quoted 17 lines · Arnd Bergmann

Quoted 8 lines · On Thursday 15 January 2015 07:37:48 Tony Lindgren wrote:

Quoted 4 lines · Marc Zyngier

Quoted 1 line · Arnd Bergmann

On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Probably there is a workable strategy, but my knowledge about OMAP is close to *nothing*...

I have a feeling this might bite other platforms too and we just have not noticed it yet..

I'm looking through the entire tree now, scanning for machines that have GIC and use IORESOURCE_IRQ or DEFINE_RES_IRQ in their platform code. Most platforms using GIC are completely converted to DT and have no hardcoded legacy IRQs.

I have checked that cns3xxx and realview are both fine by inspection.

Quoted 4 lines

The only one I'm not sure about is shmobile, which looks like it might suffer from the same problem. Simon/Magnus, could you verify this with a multiplatform kernel on any SoC that has GIC and uses devices that have interrupts defined in setup-*.c or board-*.c?

There are 3 patches floating around for shmobile, converting their non-DT support to directly initializing the GIC instead of relying on irqchip_init().

That's assuming their DT implementation doesn't use any of these device declarations.

If they do, we could use a hack similar to the one I implemented for OMAP, populating the virtual IRQ in the resource at boot time, just after the irqchip initialization.

Thanks,

	M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Quoted 10 lines · Arnd Bergmann

Quoted 2 lines · On Thursday 15 January 2015 13:42:57 Marc Zyngier wrote:

Of course, this is in no way a proper fix, but I suppose the OMAP DT is still missing a few bits...

I must be missing something here, but all the interrupts are listed correctly in the DT, so what is the omap_hwmod_irq_info actually achieving on omap4 and omap5?

Would it work if we just remove the incorrect copy of the resource and use the one that comes from DT?

By the look of it, omap_hwmod_irq_info serves multiple purposes:
- low level configuration (pads, probably more stuff)
- interrupt description for some drivers, using resources.

It should be fairly easy to do the latter, but the former looks more tricky (it would push the pad configuration down to the drivers, which is avoided at the moment).

Probably there is a workable strategy, but my knowledge about OMAP is close to *nothing*...

From what I can tell, we could drop the omap44xx_dss_dispc_irqs,