* [PATCH] ARM: vic: re-read status register before dispatching each IRQ handler
@ 2012-04-04 10:05 Will Deacon
2012-04-04 19:07 ` H Hartley Sweeten
0 siblings, 1 reply; 3+ messages in thread
From: Will Deacon @ 2012-04-04 10:05 UTC (permalink / raw)
To: linux-arm-kernel
handle_IRQ may briefly cause interrupts to be re-enabled during soft IRQ
processing on the exit path, leading to nested handling of VIC interrupts.
Since the current code does not re-read the VIC_IRQ_STATUS register, this
can lead to multiple invocations of the same interrupt handler and
spurious interrupts to be reported.
This patch changes the VIC interrupt dispatching code to re-read the
status register each time, avoiding duplicate invocations of the same
handler.
Acked-and-tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Reviewed-by: Jamie Iles <jamie@jamieiles.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
Taken against v3.4-rc1 and tested on Versatile AB 926 (+ PL011 NULL
pointer fix). I'll put this in the patch system tomorrow if there aren't
any objections.
arch/arm/common/vic.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index 7a66311..6b1de5d 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -435,11 +435,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_find_mapping(vic->domain, irq), regs);
- stat &= ~(1 << irq);
handled = 1;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] ARM: vic: re-read status register before dispatching each IRQ handler
2012-04-04 10:05 [PATCH] ARM: vic: re-read status register before dispatching each IRQ handler Will Deacon
@ 2012-04-04 19:07 ` H Hartley Sweeten
2012-04-05 9:23 ` Will Deacon
0 siblings, 1 reply; 3+ messages in thread
From: H Hartley Sweeten @ 2012-04-04 19:07 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, April 04, 2012 3:05 AM, Will Deacon wrote:
>
> handle_IRQ may briefly cause interrupts to be re-enabled during soft IRQ
> processing on the exit path, leading to nested handling of VIC interrupts.
>
> Since the current code does not re-read the VIC_IRQ_STATUS register, this
> can lead to multiple invocations of the same interrupt handler and
> spurious interrupts to be reported.
>
> This patch changes the VIC interrupt dispatching code to re-read the
> status register each time, avoiding duplicate invocations of the same
> handler.
>
> Acked-and-tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Reviewed-by: Jamie Iles <jamie@jamieiles.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>
> Taken against v3.4-rc1 and tested on Versatile AB 926 (+ PL011 NULL
> pointer fix). I'll put this in the patch system tomorrow if there aren't
> any objections.
>
> arch/arm/common/vic.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index 7a66311..6b1de5d 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -435,11 +435,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_find_mapping(vic->domain, irq), regs);
> - stat &= ~(1 << irq);
> handled = 1;
> }
You probably should fix the comment above the function:
/*
* Handle each interrupt in a single VIC. Returns non-zero if we've
* handled at least one interrupt. This does a single read of the
* status register and handles all interrupts in order from LSB first.
*/
You are no longer doing a single read. You might mention the problem
caused by the single read so that no one tries to "optimize" the loop
and reintroduce the problem.
Regards,
Hartley
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] ARM: vic: re-read status register before dispatching each IRQ handler
2012-04-04 19:07 ` H Hartley Sweeten
@ 2012-04-05 9:23 ` Will Deacon
0 siblings, 0 replies; 3+ messages in thread
From: Will Deacon @ 2012-04-05 9:23 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Apr 04, 2012 at 08:07:34PM +0100, H Hartley Sweeten wrote:
> On Wednesday, April 04, 2012 3:05 AM, Will Deacon wrote:
> > diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> > index 7a66311..6b1de5d 100644
> > --- a/arch/arm/common/vic.c
> > +++ b/arch/arm/common/vic.c
> > @@ -435,11 +435,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_find_mapping(vic->domain, irq), regs);
> > - stat &= ~(1 << irq);
> > handled = 1;
> > }
>
> You probably should fix the comment above the function:
Sorry, missed that. v2 below.
Thanks,
Will
Author: Will Deacon <will.deacon@arm.com>
Date: Wed Apr 4 10:13:49 2012 +0100
ARM: vic: re-read status register before dispatching each IRQ handler
handle_IRQ may briefly cause interrupts to be re-enabled during soft IRQ
processing on the exit path, leading to nested handling of VIC interrupts.
Since the current code does not re-read the VIC_IRQ_STATUS register, this
can lead to multiple invocations of the same interrupt handler and
spurious interrupts to be reported.
This patch changes the VIC interrupt dispatching code to re-read the
status register each time, avoiding duplicate invocations of the same
handler.
Acked-and-tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Reviewed-by: Jamie Iles <jamie@jamieiles.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index 7a66311..7e288f9 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -427,19 +427,18 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
/*
* Handle each interrupt in a single VIC. Returns non-zero if we've
- * handled at least one interrupt. This does a single read of the
- * status register and handles all interrupts in order from LSB first.
+ * handled at least one interrupt. This reads the status register
+ * before handling each interrupt, which is necessary given that
+ * handle_IRQ may briefly re-enable interrupts for soft IRQ handling.
*/
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_find_mapping(vic->domain, irq), regs);
- stat &= ~(1 << irq);
handled = 1;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-04-05 9:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-04 10:05 [PATCH] ARM: vic: re-read status register before dispatching each IRQ handler Will Deacon
2012-04-04 19:07 ` H Hartley Sweeten
2012-04-05 9:23 ` Will Deacon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.