This is a small patch set to fix some shortcomings how Orion bridge
irqs are handled. The patches are based on v3.13-rc8 and should go
into v3.14. They can possibly also marked for -stable down to v3.10.
This patches are the result of a discussion about a stale watchdog irq,
that can accidentially trigger the watchdog's irq handler and cause a
reset [1].
The first patch will add a write to clear already pending interrupts
on init. The second patch replaces handle_level_irq with handle_edge_irq
which is more appropriate for bridge irqs which are edge-triggered.
The last patch finally, fixes stale interrupts by installing an
.irq_enable callback, that will clear a possible pending interrupt
before unmasking it.
[1] http://www.spinics.net/lists/arm-kernel/msg302106.html
Sebastian Hesselbarth (3):
irqchip: orion: clear bridge cause register on init
irqchip: orion: use handle_edge_irq on bridge irqs
irqchip: orion: clear stale interrupts in irq_enable
drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
--
1.8.5.2
It is good practice to mask and clear pending irqs on init. We already
mask all irqs, so also clear the bridge irq cause register.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/irqchip/irq-orion.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index e51d40031884..4137c3d15284 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -180,8 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
- /* mask all interrupts */
+ /* mask and clear all interrupts */
writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
+ writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
irq_set_handler_data(irq, domain);
irq_set_chained_handler(irq, orion_bridge_irq_handler);
--
1.8.5.2
Bridge irqs are edge-triggered, i.e. they get asserted on low-to-high
transitions and not on the level of the downstream interrupt line.
This replaces handle_level_irq by the more appropriate handle_edge_irq.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/irqchip/irq-orion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 4137c3d15284..1f636f719065 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -143,7 +143,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
}
ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
- handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
+ handle_edge_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
if (ret) {
pr_err("%s: unable to alloc irq domain gc\n", np->name);
return ret;
--
1.8.5.2
Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
them before unmask. This installs an .irq_enable callback to ensure stale
irqs are cleared before initial unmask.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/irqchip/irq-orion.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 1f636f719065..80b13c1a0947 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -123,6 +123,18 @@ static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
}
}
+/*
+ * Bridge IRQ_CAUSE is asserted regardless of IRQ_MASK register.
+ * To avoid interrupt events on stale irqs, we clear them before unmask.
+ */
+static void orion_bridge_irq_enable(struct irq_data *d)
+{
+ struct irq_chip_type *ct = irq_data_get_chip_type(d);
+
+ ct->chip.irq_ack(d);
+ ct->chip.irq_unmask(d);
+}
+
static int __init orion_bridge_irq_init(struct device_node *np,
struct device_node *parent)
{
@@ -176,6 +188,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE;
gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK;
+ gc->chip_types[0].chip.irq_enable = orion_bridge_irq_enable;
gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit;
gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
--
1.8.5.2
On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
> Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
> IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
> them before unmask. This installs an .irq_enable callback to ensure stale
> irqs are cleared before initial unmask.
I'm not sure if putting this in irq_enable is correct. I think this
should only happen at irq_startup.
The question boils down to what is supposed to happen with this code
sequence:
disable_irq(..);
write(.. something to cause an interrupt edge ..);
.. synchronize ..
enable_irq(..);
Do we get the interrupt or not?
I found this message from Linus long ago:
http://yarchive.net/comp/linux/edge_triggered_interrupts.html
> Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
> when you disable an edge-triggered interrupt, and the edge happens while
> the interrupt is disabled, we will re-play the interrupt at enable time.
> Exactly so that drivers can have an easier time and don't have to
> normally worry about whether something is edge or level-triggered.
And found this note in Documentation/DocBook/genericirq.tmpl:
> This prevents losing edge interrupts on hardware which does
> not store an edge interrupt event while the interrupt is disabled at
> the hardware level.
So I think it is very clear that the chip driver should not discard
edges that happened while the interrupt was disabled.
Regards,
Jason
On 01/23/2014 11:52 PM, Jason Gunthorpe wrote:
> On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
>> Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
>> IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
>> them before unmask. This installs an .irq_enable callback to ensure stale
>> irqs are cleared before initial unmask.
>
> I'm not sure if putting this in irq_enable is correct. I think this
> should only happen at irq_startup.
>
> The question boils down to what is supposed to happen with this code
> sequence:
>
> disable_irq(..);
> write(.. something to cause an interrupt edge ..);
> .. synchronize ..
> enable_irq(..);
>
> Do we get the interrupt or not?
Jason,
I get the point and actually I'd chosen .irq_enable because using
.irq_startup didn't work. I rechecked this and now it works.. maybe
it is getting too late for me. I'll send a v2 of this patch shortly.
Sebastian
> I found this message from Linus long ago:
> http://yarchive.net/comp/linux/edge_triggered_interrupts.html
>> Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
>> when you disable an edge-triggered interrupt, and the edge happens while
>> the interrupt is disabled, we will re-play the interrupt at enable time.
>> Exactly so that drivers can have an easier time and don't have to
>> normally worry about whether something is edge or level-triggered.
>
> And found this note in Documentation/DocBook/genericirq.tmpl:
>
>> This prevents losing edge interrupts on hardware which does
>> not store an edge interrupt event while the interrupt is disabled at
>> the hardware level.
>
> So I think it is very clear that the chip driver should not discard
> edges that happened while the interrupt was disabled.
>
> Regards,
> Jason
>
Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
them before unmask. This installs an .irq_startup callback to ensure stale
irqs are cleared before initial unmask.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Changelog:
v1->v2:
- use .irq_startup instead of .irq_enable (Reported by Jason Gunthorpe)
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/irqchip/irq-orion.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 1f636f719065..0dfdc5c824a1 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -123,6 +123,19 @@ static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
}
}
+/*
+ * Bridge IRQ_CAUSE is asserted regardless of IRQ_MASK register.
+ * To avoid interrupt events on stale irqs, we clear them before unmask.
+ */
+static unsigned int orion_bridge_irq_startup(struct irq_data *d)
+{
+ struct irq_chip_type *ct = irq_data_get_chip_type(d);
+
+ ct->chip.irq_ack(d);
+ ct->chip.irq_unmask(d);
+ return 0;
+}
+
static int __init orion_bridge_irq_init(struct device_node *np,
struct device_node *parent)
{
@@ -176,6 +189,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE;
gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK;
+ gc->chip_types[0].chip.irq_startup = orion_bridge_irq_startup;
gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit;
gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
--
1.8.5.2
On Thu, Jan 23, 2014 at 03:52:08PM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
> > Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
> > IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
> > them before unmask. This installs an .irq_enable callback to ensure stale
> > irqs are cleared before initial unmask.
>
> I'm not sure if putting this in irq_enable is correct. I think this
> should only happen at irq_startup.
>
> The question boils down to what is supposed to happen with this code
> sequence:
>
> disable_irq(..);
> write(.. something to cause an interrupt edge ..);
> .. synchronize ..
> enable_irq(..);
>
> Do we get the interrupt or not?
The answer is... yes, the interrupt should be delivered after the
interrupt is re-enabled.
> I found this message from Linus long ago:
> http://yarchive.net/comp/linux/edge_triggered_interrupts.html
> > Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
> > when you disable an edge-triggered interrupt, and the edge happens while
> > the interrupt is disabled, we will re-play the interrupt at enable time.
> > Exactly so that drivers can have an easier time and don't have to
> > normally worry about whether something is edge or level-triggered.
>
> And found this note in Documentation/DocBook/genericirq.tmpl:
>
> > This prevents losing edge interrupts on hardware which does
> > not store an edge interrupt event while the interrupt is disabled at
> > the hardware level.
>
> So I think it is very clear that the chip driver should not discard
> edges that happened while the interrupt was disabled.
Correct.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
On Thu, Jan 23, 2014 at 11:38:04PM +0100, Sebastian Hesselbarth wrote:
> It is good practice to mask and clear pending irqs on init. We already
> mask all irqs, so also clear the bridge irq cause register.
>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: Gregory Clement <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/irqchip/irq-orion.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
> index e51d40031884..4137c3d15284 100644
> --- a/drivers/irqchip/irq-orion.c
> +++ b/drivers/irqchip/irq-orion.c
> @@ -180,8 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
> gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
> gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
>
> - /* mask all interrupts */
> + /* mask and clear all interrupts */
> writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
> + writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
>
This looks a bit bogus to me, now that we are clearing the cause upon
irq_startup(). Don't have a strong opinion, it's just that I fail to see
why we'd want or need this change...
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> This is a small patch set to fix some shortcomings how Orion bridge
> irqs are handled. The patches are based on v3.13-rc8 and should go
> into v3.14. They can possibly also marked for -stable down to v3.10.
>
> This patches are the result of a discussion about a stale watchdog irq,
> that can accidentially trigger the watchdog's irq handler and cause a
> reset [1].
>
> The first patch will add a write to clear already pending interrupts
> on init. The second patch replaces handle_level_irq with handle_edge_irq
> which is more appropriate for bridge irqs which are edge-triggered.
> The last patch finally, fixes stale interrupts by installing an
> .irq_enable callback, that will clear a possible pending interrupt
> before unmasking it.
>
> [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
>
> Sebastian Hesselbarth (3):
> irqchip: orion: clear bridge cause register on init
> irqchip: orion: use handle_edge_irq on bridge irqs
> irqchip: orion: clear stale interrupts in irq_enable
>
> drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3). Once I
see that the outstanding pull requests for mvebu/irqchip have been
merged into mainline, I'll be changing the name of this branch to
mvebu/irqchip-fixes. I just don't want to upset the applecart atm.
Oh yeah, Cc'd for stable back to v3.10.
thx,
Jason.
On Tue, Feb 04, 2014 at 11:54:15PM -0500, Jason Cooper wrote:
> On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> > This is a small patch set to fix some shortcomings how Orion bridge
> > irqs are handled. The patches are based on v3.13-rc8 and should go
> > into v3.14. They can possibly also marked for -stable down to v3.10.
> >
> > This patches are the result of a discussion about a stale watchdog irq,
> > that can accidentially trigger the watchdog's irq handler and cause a
> > reset [1].
> >
> > The first patch will add a write to clear already pending interrupts
> > on init. The second patch replaces handle_level_irq with handle_edge_irq
> > which is more appropriate for bridge irqs which are edge-triggered.
> > The last patch finally, fixes stale interrupts by installing an
> > .irq_enable callback, that will clear a possible pending interrupt
> > before unmasking it.
> >
> > [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
> >
> > Sebastian Hesselbarth (3):
> > irqchip: orion: clear bridge cause register on init
> > irqchip: orion: use handle_edge_irq on bridge irqs
> > irqchip: orion: clear stale interrupts in irq_enable
> >
> > drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
>
> Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3). Once I
> see that the outstanding pull requests for mvebu/irqchip have been
> merged into mainline, I'll be changing the name of this branch to
> mvebu/irqchip-fixes. I just don't want to upset the applecart atm.
>
> Oh yeah, Cc'd for stable back to v3.10.
>
If you want to pick this:
Tested-by: Ezequiel Garcia <[email protected]>
Tested Kirkwood Topkick and Dove Cubox. Quite frankly, I haven't checked
this series prevents an "errant watchdog", but just pursued a boot test.
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
On Thu, Feb 06, 2014 at 02:10:40PM -0300, Ezequiel Garcia wrote:
> On Tue, Feb 04, 2014 at 11:54:15PM -0500, Jason Cooper wrote:
> > On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> > > This is a small patch set to fix some shortcomings how Orion bridge
> > > irqs are handled. The patches are based on v3.13-rc8 and should go
> > > into v3.14. They can possibly also marked for -stable down to v3.10.
> > >
> > > This patches are the result of a discussion about a stale watchdog irq,
> > > that can accidentially trigger the watchdog's irq handler and cause a
> > > reset [1].
> > >
> > > The first patch will add a write to clear already pending interrupts
> > > on init. The second patch replaces handle_level_irq with handle_edge_irq
> > > which is more appropriate for bridge irqs which are edge-triggered.
> > > The last patch finally, fixes stale interrupts by installing an
> > > .irq_enable callback, that will clear a possible pending interrupt
> > > before unmasking it.
> > >
> > > [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
> > >
> > > Sebastian Hesselbarth (3):
> > > irqchip: orion: clear bridge cause register on init
> > > irqchip: orion: use handle_edge_irq on bridge irqs
> > > irqchip: orion: clear stale interrupts in irq_enable
> > >
> > > drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
> > > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3). Once I
> > see that the outstanding pull requests for mvebu/irqchip have been
> > merged into mainline, I'll be changing the name of this branch to
> > mvebu/irqchip-fixes. I just don't want to upset the applecart atm.
> >
> > Oh yeah, Cc'd for stable back to v3.10.
> >
>
> If you want to pick this:
>
> Tested-by: Ezequiel Garcia <[email protected]>
Added, thanks for testing!
thx,
Jason.