When an interrupt line is assered, GIC handles interrupt with the flow
(with EOImode == 1):
gic_handle_irq()
`> do_read_iar() => Change int state to active
`> gic_write_eoir() => Drop int priority
`> handle_domain_irq()
`> generic_handle_irq_desc()
`> handle_fasteoi_irq()
`> handle_irq_event() => Peripheral handler and
de-assert int line
`> cond_unmask_eoi_irq()
`> chip->irq_eoi()
`> gic_eoimode1_eoi_irq() => Change int state to inactive
In this flow, it has no explicit memory barrier between the functions
handle_irq_event() and chip->irq_eoi(), it's possible that the
outstanding data has not reached device in handle_irq_event() but the
callback chip->irq_eoi() is invoked, this can lead to state transition
for level triggered interrupt:
Flow | Interrupt state in GIC
---------------------------------+-------------------------------------
Interrupt line is asserted | 'inactive' -> 'pending'
do_read_iar() | 'pending' -> 'pending & active'
handle_irq_event() | Write peripheral register but it's
| not visible for device, so the
| interrupt line is still asserted
chip->irq_eoi() | 'pending & active' -> 'pending'
...
Produce spurious interrupt |
with interrupt ID: 1024 |
| Finally the peripheral reigster is
| updated and the interrupt line is
| deasserted: 'pending' -> 'inactive'
To avoid this potential issue, this patch adds wmb() barrier prior to
invoke EOI operation, this can make sure the interrupt line is
de-asserted in peripheral before deactivating interrupt in GIC. At the
end, this can avoid spurious interrupt.
Reported-by: Orito Takao <[email protected]>
Signed-off-by: Leo Yan <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 4 ++++
drivers/irqchip/irq-gic.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e0f4debe64e1..fcd1477e2274 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -530,6 +530,8 @@ static void gic_irq_nmi_teardown(struct irq_data *d)
static void gic_eoi_irq(struct irq_data *d)
{
+ /* Ensure the data is written to peripheral */
+ wmb();
gic_write_eoir(gic_irq(d));
}
@@ -541,6 +543,8 @@ static void gic_eoimode1_eoi_irq(struct irq_data *d)
*/
if (gic_irq(d) >= 8192 || irqd_is_forwarded_to_vcpu(d))
return;
+ /* Ensure the data is written to peripheral */
+ wmb();
gic_write_dir(gic_irq(d));
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d329ec3d64d8..3e9d4b6dc31b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -222,6 +222,8 @@ static void gic_eoi_irq(struct irq_data *d)
if (hwirq < 16)
hwirq = this_cpu_read(sgi_intid);
+ /* Ensure the data is written to peripheral */
+ wmb();
writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_EOI);
}
@@ -236,6 +238,8 @@ static void gic_eoimode1_eoi_irq(struct irq_data *d)
if (hwirq < 16)
hwirq = this_cpu_read(sgi_intid);
+ /* Ensure the data is written to peripheral */
+ wmb();
writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
}
--
2.25.1
Hi Marc,
On Wed, Sep 01, 2021 at 08:03:52AM +0100, Marc Zyngier wrote:
> On Wed, 01 Sep 2021 07:31:15 +0100,
> Leo Yan <[email protected]> wrote:
> >
> > When an interrupt line is assered, GIC handles interrupt with the flow
> > (with EOImode == 1):
> >
> > gic_handle_irq()
> > `> do_read_iar() => Change int state to active
> > `> gic_write_eoir() => Drop int priority
> > `> handle_domain_irq()
> > `> generic_handle_irq_desc()
> > `> handle_fasteoi_irq()
> > `> handle_irq_event() => Peripheral handler and
> > de-assert int line
> > `> cond_unmask_eoi_irq()
> > `> chip->irq_eoi()
> > `> gic_eoimode1_eoi_irq() => Change int state to inactive
> >
> > In this flow, it has no explicit memory barrier between the functions
> > handle_irq_event() and chip->irq_eoi(), it's possible that the
> > outstanding data has not reached device in handle_irq_event() but the
> > callback chip->irq_eoi() is invoked, this can lead to state transition
> > for level triggered interrupt:
> >
> > Flow | Interrupt state in GIC
> > ---------------------------------+-------------------------------------
> > Interrupt line is asserted | 'inactive' -> 'pending'
> > do_read_iar() | 'pending' -> 'pending & active'
> > handle_irq_event() | Write peripheral register but it's
> > | not visible for device, so the
> > | interrupt line is still asserted
> > chip->irq_eoi() | 'pending & active' -> 'pending'
> > ...
> > Produce spurious interrupt |
> > with interrupt ID: 1024 |
>
> 1024? Surely not.
Sorry for typo, should be 1023.
>
> > | Finally the peripheral reigster is
> > | updated and the interrupt line is
> > | deasserted: 'pending' -> 'inactive'
> >
> > To avoid this potential issue, this patch adds wmb() barrier prior to
> > invoke EOI operation, this can make sure the interrupt line is
> > de-asserted in peripheral before deactivating interrupt in GIC. At the
> > end, this can avoid spurious interrupt.
>
> If you want to ensure completion of device-specific writes, why isn't
> this the job of the device driver to implement whatever semantic it
> desires?
Seems to me, it's a common requirement for all device drivers to
ensure the outstanding transactions to the endpoint to de-assert the
interrupt line before the GIC driver deactivates the interrupt line.
> What if the interrupt is (shock, horror!) driven by a system
> register instead?
Okay, this is good reason that it's not always to need barrier.
> I think this is merely papering over a driver bug, and adds a
> significant cost to all interrupts for no good reasons.
Understand. The memory barrier can be added per device driver.
Thanks for quick response,
Leo
On Wed, 01 Sep 2021 07:31:15 +0100,
Leo Yan <[email protected]> wrote:
>
> When an interrupt line is assered, GIC handles interrupt with the flow
> (with EOImode == 1):
>
> gic_handle_irq()
> `> do_read_iar() => Change int state to active
> `> gic_write_eoir() => Drop int priority
> `> handle_domain_irq()
> `> generic_handle_irq_desc()
> `> handle_fasteoi_irq()
> `> handle_irq_event() => Peripheral handler and
> de-assert int line
> `> cond_unmask_eoi_irq()
> `> chip->irq_eoi()
> `> gic_eoimode1_eoi_irq() => Change int state to inactive
>
> In this flow, it has no explicit memory barrier between the functions
> handle_irq_event() and chip->irq_eoi(), it's possible that the
> outstanding data has not reached device in handle_irq_event() but the
> callback chip->irq_eoi() is invoked, this can lead to state transition
> for level triggered interrupt:
>
> Flow | Interrupt state in GIC
> ---------------------------------+-------------------------------------
> Interrupt line is asserted | 'inactive' -> 'pending'
> do_read_iar() | 'pending' -> 'pending & active'
> handle_irq_event() | Write peripheral register but it's
> | not visible for device, so the
> | interrupt line is still asserted
> chip->irq_eoi() | 'pending & active' -> 'pending'
> ...
> Produce spurious interrupt |
> with interrupt ID: 1024 |
1024? Surely not.
> | Finally the peripheral reigster is
> | updated and the interrupt line is
> | deasserted: 'pending' -> 'inactive'
>
> To avoid this potential issue, this patch adds wmb() barrier prior to
> invoke EOI operation, this can make sure the interrupt line is
> de-asserted in peripheral before deactivating interrupt in GIC. At the
> end, this can avoid spurious interrupt.
If you want to ensure completion of device-specific writes, why isn't
this the job of the device driver to implement whatever semantic it
desires? What if the interrupt is (shock, horror!) driven by a system
register instead?
I think this is merely papering over a driver bug, and adds a
significant cost to all interrupts for no good reasons.
M.
--
Without deviation from the norm, progress is not possible.