[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[H Hartley Sweeten]

Jamie,

We are seeing a problem on ep93xxthat appears to be caused with the MULTI_IRQ_HANDLER
change to the vic code.

Following is the latest discussion. Maybe you have an idea?

On Sunday, April 01, 2012 11:49 AM, Mika Westerberg wrote:
> On Thu, Mar 29, 2012 at 05:33:49PM -0500, H Hartley Sweeten wrote:
>
>> I tried doing a bit more debugging with the handle_one_vic function. It
>> appears that the timer tick is what's causing the spi dma interrupts grief.
>> I'm just not sure how it's happening or how to fix it...
>> 
>> I modified handle_one_vic to output a message when multiple interrupts
>> are detected in the stat. Then, if multiple interrupts were detected, to output
>> a message with the new calculated stat and the actual stat. These "should"
>> occur one right after the other when multiple interrupts are detected. But
>> that's not what I'm getting. Here's a sample trace with comments:
>> 
>> handle_one_vic: stat:0x00060000 - handling irq:17 now
>> 	stat shows interrupts 17 and 18
>> handle_one_vic: stat:0x00040010 - handling irq:4 now
>> 	stat shows interrupts 4 and 18, 17 was handled
>> handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
>> 	next stat shows interrupt 18, 4 was handled, 18 is pending
>> handle_one_vic: stat:0x00040000 - handling irq:18 now
>> 	stat shows interrupt 18
>> handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
>> 	next stat shows no interrupts, 18 was handled, 4 is pending
>> handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
>> 	next stat shows interrupt 18, it was already handled, none are pending
>> handle_one_vic: stat:0x00040000 - handling irq:18 now
>> 	stat shows interrupt 18 (which was already handled)
>> dma dma1chan1: spurious interrupt: status=00002180
>> 	bang... spurious interrupt
>> 
>> It looks like the timer interrupt (4) is causing vic_handle_irq to start
>> iterating over the VIC's while an iteration is already in progress.  One
>> of the iterations is handling interrupt 18 correctly but, since the stat
>> is only read once, the second iteration also tries to handle it.
>>
>> Any ideas?
>
> Unfortunately no :-/ I've been investigating this also and so far haven't
> found anything which could explain this behaviour. It is good that you found
> that the timer interrupt might have something to do with this. I'm going to
> add some more debugging code and see if that helps to identify the reason for
> this.
>
> It might also be that the ep93xx_dma driver is doing something wrong in its
> interrupt handler which causes the DONE bit to stay asserted even though the
> first thing it does is to write 0 to M2M_INTERRUPT register which is supposed
> to clear the interrupt..

>From what I can tell, the interrupt handler in the ep93xx_dma driver is fine. It
is clearing the interrupt as it should.

The root cause appears to be the timer interrupt causing a new iteration over
the VIC's to start before the current iteration is complete. Both iterations are
reading the vic status register and seeing an interrupt pending for irq 18. One of
the iterations properly handles this interrupt but, because the status register is
only ready once, the other iteration also tries to handle the interrupt. Since it's
already been handled we end up with the spurious interrupt.

So...

1) Are interrupts supposed to be still enabled when vic_handle_irq is called to
handle the pending interrupts the first time? If they "are" disabled, what is
re-enabling them and causing the timer interrupt to start a new iteration?

2) Should the vic status be re-checked after each interrupt is handled in
handle_one_vic? This could cause a problem where an aggressive interrupt,
i.e. the timer on ep93xx, could cause other interrupts to not get handled quickly.

Any ideas?

Regards,
Hartley

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Jamie Iles]

Hi Hartley,

On 2 April 2012 18:55, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
> Jamie,
>
> We are seeing a problem on ep93xxthat appears to be caused with the MULTI_IRQ_HANDLER
> change to the vic code.
>
> Following is the latest discussion. Maybe you have an idea?
>
> On Sunday, April 01, 2012 11:49 AM, Mika Westerberg wrote:
>> On Thu, Mar 29, 2012 at 05:33:49PM -0500, H Hartley Sweeten wrote:
>>
>>> I tried doing a bit more debugging with the handle_one_vic function. It
>>> appears that the timer tick is what's causing the spi dma interrupts grief.
>>> I'm just not sure how it's happening or how to fix it...
>>>
>>> I modified handle_one_vic to output a message when multiple interrupts
>>> are detected in the stat. Then, if multiple interrupts were detected, to output
>>> a message with the new calculated stat and the actual stat. These "should"
>>> occur one right after the other when multiple interrupts are detected. But
>>> that's not what I'm getting. Here's a sample trace with comments:
>>>
>>> handle_one_vic: stat:0x00060000 - handling irq:17 now
>>> ? ? ?stat shows interrupts 17 and 18
>>> handle_one_vic: stat:0x00040010 - handling irq:4 now
>>> ? ? ?stat shows interrupts 4 and 18, 17 was handled
>>> handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
>>> ? ? ?next stat shows interrupt 18, 4 was handled, 18 is pending
>>> handle_one_vic: stat:0x00040000 - handling irq:18 now
>>> ? ? ?stat shows interrupt 18
>>> handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
>>> ? ? ?next stat shows no interrupts, 18 was handled, 4 is pending
>>> handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
>>> ? ? ?next stat shows interrupt 18, it was already handled, none are pending
>>> handle_one_vic: stat:0x00040000 - handling irq:18 now
>>> ? ? ?stat shows interrupt 18 (which was already handled)
>>> dma dma1chan1: spurious interrupt: status=00002180
>>> ? ? ?bang... spurious interrupt
>>>
>>> It looks like the timer interrupt (4) is causing vic_handle_irq to start
>>> iterating over the VIC's while an iteration is already in progress. ?One
>>> of the iterations is handling interrupt 18 correctly but, since the stat
>>> is only read once, the second iteration also tries to handle it.
>>>
>>> Any ideas?
>>
>> Unfortunately no :-/ I've been investigating this also and so far haven't
>> found anything which could explain this behaviour. It is good that you found
>> that the timer interrupt might have something to do with this. I'm going to
>> add some more debugging code and see if that helps to identify the reason for
>> this.
>>
>> It might also be that the ep93xx_dma driver is doing something wrong in its
>> interrupt handler which causes the DONE bit to stay asserted even though the
>> first thing it does is to write 0 to M2M_INTERRUPT register which is supposed
>> to clear the interrupt..
>
> From what I can tell, the interrupt handler in the ep93xx_dma driver is fine. It
> is clearing the interrupt as it should.
>
> The root cause appears to be the timer interrupt causing a new iteration over
> the VIC's to start before the current iteration is complete. Both iterations are
> reading the vic status register and seeing an interrupt pending for irq 18. One of
> the iterations properly handles this interrupt but, because the status register is
> only ready once, the other iteration also tries to handle the interrupt. Since it's
> already been handled we end up with the spurious interrupt.
>
> So...
>
> 1) Are interrupts supposed to be still enabled when vic_handle_irq is called to
> handle the pending interrupts the first time? If they "are" disabled, what is
> re-enabling them and causing the timer interrupt to start a new iteration?

No, I believe that at this point IRQ's should be disabled.  This gets
called very early by the entry macros, and not much has run before
this gets called.  I can only guess that something inside one of the
interrupt handlers is reenabling interrupts for some period.

> 2) Should the vic status be re-checked after each interrupt is handled in
> handle_one_vic? This could cause a problem where an aggressive interrupt,
> i.e. the timer on ep93xx, could cause other interrupts to not get handled quickly.

No, I think I may have had this when I first introduced this code and
there were objections to the fairness of that, so the current approach
where we try and service each interrupt before restarting was the most
fair way of doing it.

I wonder if looking at IRQ tracing to find out when and where
interrupts are being enabled from might be useful for tracking this
down.

Jamie

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[H Hartley Sweeten]

On Monday, April 02, 2012 11:16 AM, Jamie Iles wrote:
> Hi Hartley,
>
> On 2 April 2012 18:55, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
>> The root cause appears to be the timer interrupt causing a new iteration over
>> the VIC's to start before the current iteration is complete. Both iterations are
>> reading the vic status register and seeing an interrupt pending for irq 18. One of
>> the iterations properly handles this interrupt but, because the status register is
>> only ready once, the other iteration also tries to handle the interrupt. Since it's
>> already been handled we end up with the spurious interrupt.
>>
>> So...
>>
>> 1) Are interrupts supposed to be still enabled when vic_handle_irq is called to
>> handle the pending interrupts the first time? If they "are" disabled, what is
>> re-enabling them and causing the timer interrupt to start a new iteration?
>
> No, I believe that at this point IRQ's should be disabled.  This gets
> called very early by the entry macros, and not much has run before
> this gets called.  I can only guess that something inside one of the
> interrupt handlers is reenabling interrupts for some period.

I don't see anything in the relevant ep93xx drivers that would be re-enabling the
interrupts.

>
>> 2) Should the vic status be re-checked after each interrupt is handled in
>> handle_one_vic? This could cause a problem where an aggressive interrupt,
>> i.e. the timer on ep93xx, could cause other interrupts to not get handled quickly.
>
> No, I think I may have had this when I first introduced this code and
> there were objections to the fairness of that, so the current approach
> where we try and service each interrupt before restarting was the most
> fair way of doing it.

I remember seeing this discussion on the list. I agree the current approach
is the most "fair" way of handling the interrupts.

> I wonder if looking at IRQ tracing to find out when and where
> interrupts are being enabled from might be useful for tracking this
> down.

I'm willing to do the IRQ tracing... How... ;-)

I don't see anything in the Kernel Hacking options to enable IRQ tracing.

Regards,
Hartley

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Jamie Iles]

On 2 April 2012 19:27, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
> On Monday, April 02, 2012 11:16 AM, Jamie Iles wrote:
>> On 2 April 2012 18:55, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
>>> 2) Should the vic status be re-checked after each interrupt is handled in
>>> handle_one_vic? This could cause a problem where an aggressive interrupt,
>>> i.e. the timer on ep93xx, could cause other interrupts to not get handled quickly.
>>
>> No, I think I may have had this when I first introduced this code and
>> there were objections to the fairness of that, so the current approach
>> where we try and service each interrupt before restarting was the most
>> fair way of doing it.
>
> I remember seeing this discussion on the list. I agree the current approach
> is the most "fair" way of handling the interrupts.
>
>> I wonder if looking at IRQ tracing to find out when and where
>> interrupts are being enabled from might be useful for tracking this
>> down.
>
> I'm willing to do the IRQ tracing... How... ;-)
>
> I don't see anything in the Kernel Hacking options to enable IRQ tracing.

It looks like the irqsoff tracer has a dependency on
!ARCH_USES_GETTIMEOFFSET which is defined for ep93xx.  I'll have a
think to see if there's another way to tackle this, but at the moment
there's nothing that springs to mind...

Jamie

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Mika Westerberg]

On Mon, Apr 02, 2012 at 07:16:11PM +0100, Jamie Iles wrote:
> > 1) Are interrupts supposed to be still enabled when vic_handle_irq is called to
> > handle the pending interrupts the first time? If they "are" disabled, what is
> > re-enabling them and causing the timer interrupt to start a new iteration?
> 
> No, I believe that at this point IRQ's should be disabled.  This gets
> called very early by the entry macros, and not much has run before
> this gets called.  I can only guess that something inside one of the
> interrupt handlers is reenabling interrupts for some period.

I finally managed to dig up my old TS-7260 board which has SPI eeprom and this
is much easier to debug as the traffic is minimal compared to MMC over SPI.

I added some debug code once I was able to reproduce this and here's the trace
with my added comments.

[  176.860000] dma dma1chan0: M2M: submit: control=0xb40500c
[  176.860000] dma dma1chan1: M2M: submit: control=0xb80280c

Here we submit both RX and TX requests.

[  176.860000] VIC0: start <stat=0x60000>

We got immediately two interrupts as the transfer size is pretty small. The
stat field shows that both bits 17 and 18 are set (DMA RX and TX interrupts).
We are now at the beginning of handle_one_vic().

[  176.860000] VIC0: <IRQ17>

We start processing the first (RX) interrupt here.

[  176.860000] dma dma1chan0: <IRQ17>
[  176.860000] dma dma1chan0: M2M: begin intr: control=0xb40500c
[  176.860000] dma dma1chan0: M2M: DONE end intr: control=0xb405000
[  176.860000] dma dma1chan0: </IRQ17>

The interrupt was handled by the ep93xx_dma driver for RX channel.

[  176.860000] VIC0: start <stat=0x40000>

Another call to handle_one_vic() starts here and it shows that bit 18 is set
(DMA TX interrupt).

[  176.860000] VIC0: <IRQ18>

and we start processing that.

[  176.860000] dma dma1chan1: <IRQ18>

The dma driver prints out that it started to handle the interrupt.

Now, I added WARN_ON_ONCE(irq == 18) at the beginning of the handler which
shows the situation nicely. Comments in the backtrace should be read starting
from the bottom (ep93xx_dma_tx_submit). I also numbered them.

[  176.860000] ------------[ cut here ]------------
[  176.860000] WARNING: at drivers/dma/ep93xx_dma.c:748 ep93xx_dma_interrupt+0x16c/0x194()
[  176.860000] Modules linked in:
[  176.860000] [<c0019e74>] (unwind_backtrace+0x0/0xf0) from [<c0022724>] (warn_slowpath_common+0x48/0x60)
[  176.860000] [<c0022724>] (warn_slowpath_common+0x48/0x60) from [<c0022758>] (warn_slowpath_null+0x1c/0x24)
[  176.860000] [<c0022758>] (warn_slowpath_null+0x1c/0x24) from [<c01cfa64>] (ep93xx_dma_interrupt+0x16c/0x194)
[  176.860000] [<c01cfa64>] (ep93xx_dma_interrupt+0x16c/0x194) from [<c005a960>] (handle_irq_event_percpu+0x50/0x1a8)
[  176.860000] [<c005a960>] (handle_irq_event_percpu+0x50/0x1a8) from [<c005aaf4>] (handle_irq_event+0x3c/0x5c)
[  176.860000] [<c005aaf4>] (handle_irq_event+0x3c/0x5c) from [<c005ca24>] (handle_level_irq+0x90/0x11c)
[  176.860000] [<c005ca24>] (handle_level_irq+0x90/0x11c) from [<c005a2a4>] (generic_handle_irq+0x2c/0x40)
[  176.860000] [<c005a2a4>] (generic_handle_irq+0x2c/0x40) from [<c001528c>] (handle_IRQ+0x30/0x84)
[  176.860000] [<c001528c>] (handle_IRQ+0x30/0x84) from [<c0008608>] (vic_handle_irq+0x108/0x1f8)
[  176.860000] [<c0008608>] (vic_handle_irq+0x108/0x1f8) from [<c0014054>] (__irq_svc+0x34/0x60)

3. And here we start to handle the second interrupt.

[  176.860000] Exception stack(0xc503bde0 to 0xc503be28)
[  176.860000] bde0: 00000000 00000000 c503be18 20000013 00000040 00000011 c503a000 c0454ef8
[  176.860000] be00: c0459940 c043e2d8 00000000 c0459964 00000102 c503be28 c0028218 c002822c
[  176.860000] be20: 20000013 ffffffff
[  176.860000] [<c0014054>] (__irq_svc+0x34/0x60) from [<c002822c>] (__do_softirq+0x58/0x13c)
[  176.860000] [<c002822c>] (__do_softirq+0x58/0x13c) from [<c00286fc>] (irq_exit+0x54/0x60)

2. This is where the first interrupt is finished and it shows that irq_exit()
allows softirqs to run (enabling hardirqs). So we get another interrupt (as
the line is still active for IRQ18 which we didn't handle yet).

[  176.860000] [<c00286fc>] (irq_exit+0x54/0x60) from [<c0015290>] (handle_IRQ+0x34/0x84)
[  176.860000] [<c0015290>] (handle_IRQ+0x34/0x84) from [<c0008608>] (vic_handle_irq+0x108/0x1f8)
[  176.860000] [<c0008608>] (vic_handle_irq+0x108/0x1f8) from [<c0014054>] (__irq_svc+0x34/0x60)
[  176.860000] Exception stack(0xc503beb0 to 0xc503bef8)
[  176.860000] bea0:                                     c503bef0 00000000 ffffffff c503a030
[  176.860000] bec0: 60000013 00000002 c506cf58 60000013 c5147f30 c513d805 c5137dbc c5137dac
[  176.860000] bee0: 00000001 c503bef8 c0332b08 c0332b0c 60000013 ffffffff
[  176.860000] [<c0014054>] (__irq_svc+0x34/0x60) from [<c0332b0c>] (_raw_spin_unlock_irqrestore+0x10/0x38)

1. First interrupt happens here.

[  176.860000] [<c0332b0c>] (_raw_spin_unlock_irqrestore+0x10/0x38) from [<c01cf2fc>] (ep93xx_dma_tx_submit+0x6c/0x90)
[  176.860000] [<c01cf2fc>] (ep93xx_dma_tx_submit+0x6c/0x90) from [<c02130cc>] (ep93xx_spi_work+0x478/0x5d8)
[  176.860000] [<c02130cc>] (ep93xx_spi_work+0x478/0x5d8) from [<c0035888>] (process_one_work+0x128/0x398)
[  176.860000] [<c0035888>] (process_one_work+0x128/0x398) from [<c0037c14>] (worker_thread+0x160/0x340)
[  176.860000] [<c0037c14>] (worker_thread+0x160/0x340) from [<c003bf60>] (kthread+0x90/0x9c)
[  176.860000] [<c003bf60>] (kthread+0x90/0x9c) from [<c0015370>] (kernel_thread_exit+0x0/0x8)
[  176.860000] ---[ end trace e258991e05d70c3d ]---


[  176.860000] dma dma1chan1: M2M: begin intr: control=0xb80280c
[  176.860000] dma dma1chan1: M2M: DONE end intr: control=0xb802800
[  176.860000] dma dma1chan1: </IRQ18>

Now back to the trace we see that IRQ18 is now handled and we return from
there.

[  176.860000] VIC0: </IRQ18>
[  176.860000] VIC0: finished <stat=0x0, loops=1, handled=1>
[  177.150000] VIC0: </IRQ17>

At this point we are back in the first vic_handle_irq() where the stat still
shows that there is an interupt 18 pending (which we handled already).

[  177.150000] VIC0: <IRQ18>
[  177.150000] dma dma1chan1: <IRQ18>
[  177.150000] dma dma1chan1: got interrupt while active list is empty

And the driver complains that we already handled that interrupt.

[  177.150000] VIC0: </IRQ18>
[  177.150000] VIC0: finished <stat=0x0, loops=2, handled=1>

Sorry if the above is bit messy - I didn't find any other way to present this.

Anyway it looks like handle_IRQ() enables interrupts when it is finished with
the current interrupt which then causes hw to interrupt second time resulting
failure in case of ep93xx.

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Russell King - ARM Linux]

On Mon, Apr 02, 2012 at 11:00:34PM +0300, Mika Westerberg wrote:
> Anyway it looks like handle_IRQ() enables interrupts when it is finished with
> the current interrupt which then causes hw to interrupt second time resulting
> failure in case of ep93xx.

Soft IRQ processing in the irq exit path will enable interrupts, and
this is probably where the problem is showing up.

You've identified an important difference between the level 1 interrupt
controller handlers and the chained handlers, and I suggest that folk
re-implement their level 1 interrupt handlers in the same way as the
assembly code was: re-read the interrupt register each time round the
loop.

Otherwise, we're going to have to move all the L1 stuff up a level,
so that you only have one irq_enter() ... irq_exit() around the L1
handler.  That means you'll avoid running any softirqs until you've
cleared down all interrupts in the system, which might be bad news for
softirq latency (and it'd be different from x86 behaviour, where softirqs
are run after each non-nested IRQ is handled.)

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Will Deacon]

On Mon, Apr 02, 2012 at 10:46:14PM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 02, 2012 at 11:00:34PM +0300, Mika Westerberg wrote:
> > Anyway it looks like handle_IRQ() enables interrupts when it is finished with
> > the current interrupt which then causes hw to interrupt second time resulting
> > failure in case of ep93xx.
> 
> Soft IRQ processing in the irq exit path will enable interrupts, and
> this is probably where the problem is showing up.
> 
> You've identified an important difference between the level 1 interrupt
> controller handlers and the chained handlers, and I suggest that folk
> re-implement their level 1 interrupt handlers in the same way as the
> assembly code was: re-read the interrupt register each time round the
> loop.

I'm also seeing this on the Versatile AB, with spurious interrupts reported
from eth0.

As you suggested, re-reading the VIC status solves the problem:


diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index dcb004a..cb6b49a 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
        u32 stat, irq;
        int handled = 0;
 
-       stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
-       while (stat) {
+       while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
                irq = ffs(stat) - 1;
                handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
-               stat &= ~(1 << irq);
                handled = 1;
        }


Will

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Mika Westerberg]

On Tue, Apr 03, 2012 at 06:08:12PM +0100, Will Deacon wrote:

> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index dcb004a..cb6b49a 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
>         u32 stat, irq;
>         int handled = 0;
>  
> -       stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
> -       while (stat) {
> +       while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>                 irq = ffs(stat) - 1;
>                 handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
> -               stat &= ~(1 << irq);
>                 handled = 1;
>         }
> 

I can confirm that the above patch fixes spurious interrupts on ep93xx.

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[H Hartley Sweeten]

On Tuesday, April 03, 2012 10:08 AM, Will Deacon wrote:
> On Mon, Apr 02, 2012 at 10:46:14PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Apr 02, 2012 at 11:00:34PM +0300, Mika Westerberg wrote:
>>> Anyway it looks like handle_IRQ() enables interrupts when it is finished with
>>> the current interrupt which then causes hw to interrupt second time resulting
>>> failure in case of ep93xx.
>> 
>> Soft IRQ processing in the irq exit path will enable interrupts, and
>> this is probably where the problem is showing up.
>> 
>> You've identified an important difference between the level 1 interrupt
>> controller handlers and the chained handlers, and I suggest that folk
>> re-implement their level 1 interrupt handlers in the same way as the
>> assembly code was: re-read the interrupt register each time round the
>> loop.
>
> I'm also seeing this on the Versatile AB, with spurious interrupts reported
> from eth0.
>
> As you suggested, re-reading the VIC status solves the problem:
>
>
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index dcb004a..cb6b49a 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
>         u32 stat, irq;
>         int handled = 0;
>  
> -       stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
> -       while (stat) {
> +       while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>                 irq = ffs(stat) - 1;
>                 handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
> -               stat &= ~(1 << irq);
>                 handled = 1;
>         }

This should be posted as a proper patch with your Signed-off-by.

But for what it's worth, on ep93xx:

Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Will Deacon]

On Tue, Apr 03, 2012 at 06:46:40PM +0100, H Hartley Sweeten wrote:
> On Tuesday, April 03, 2012 10:08 AM, Will Deacon wrote:
> > diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> > index dcb004a..cb6b49a 100644
> > --- a/arch/arm/common/vic.c
> > +++ b/arch/arm/common/vic.c
> > @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
> >         u32 stat, irq;
> >         int handled = 0;
> >  
> > -       stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
> > -       while (stat) {
> > +       while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
> >                 irq = ffs(stat) - 1;
> >                 handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
> > -               stat &= ~(1 << irq);
> >                 handled = 1;
> >         }
> 
> This should be posted as a proper patch with your Signed-off-by.

If Jamie's happy with the change, I'll repost.

> But for what it's worth, on ep93xx:
> 
> Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks,

Will

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Jamie Iles]

On 4 April 2012 10:11, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Apr 03, 2012 at 06:46:40PM +0100, H Hartley Sweeten wrote:
>> On Tuesday, April 03, 2012 10:08 AM, Will Deacon wrote:
>> > diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
>> > index dcb004a..cb6b49a 100644
>> > --- a/arch/arm/common/vic.c
>> > +++ b/arch/arm/common/vic.c
>> > @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
>> > ? ? ? ? u32 stat, irq;
>> > ? ? ? ? int handled = 0;
>> >
>> > - ? ? ? stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
>> > - ? ? ? while (stat) {
>> > + ? ? ? while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>> > ? ? ? ? ? ? ? ? irq = ffs(stat) - 1;
>> > ? ? ? ? ? ? ? ? handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
>> > - ? ? ? ? ? ? ? stat &= ~(1 << irq);
>> > ? ? ? ? ? ? ? ? handled = 1;
>> > ? ? ? ? }
>>
>> This should be posted as a proper patch with your Signed-off-by.
>
> If Jamie's happy with the change, I'll repost.

Sure, lgtm.

Reviewed-by: Jamie Iles <jamie@jamieiles.com>

Jamie

[BUG?] vic MULTI_IRQ_HANDLER (was [PATCH] ep93xx: Implement double buffering for M2M DMA channels)

[Will Deacon]

On Wed, Apr 04, 2012 at 10:12:56AM +0100, Jamie Iles wrote:
> On 4 April 2012 10:11, Will Deacon <will.deacon@arm.com> wrote:
> >
> > If Jamie's happy with the change, I'll repost.
> 
> Sure, lgtm.
> 
> Reviewed-by: Jamie Iles <jamie@jamieiles.com>

Cheers Jamie, I've posted the patch individually here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/092698.html

Will