* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).