On 9/11/2018 4:33 PM, Guenter Roeck wrote:
Looking into the patch, clearing the interrupt status at the end of an
interrupt handler is always suspicious and tends to result in race
conditions (because additional interrupts may have arrived while handling
the existing interrupts, or because interrupt handling itself may trigger
another interrupt). With that in mind, the following patch fixes the
problem for me.
Guenter
---
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..c488e6950b7c 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
spin_lock(&bus->lock);
irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ /* Ack all interrupt bits. */
+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
irq_received, irq_handled);
- /* Ack all interrupt bits. */
- writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
My intention of putting the code at the end of interrupt handler was,
to reduce possibility of combined irq calls which is explained in this
patch. But YES, I agree with you. It could make a potential race
condition as you pointed out. I tested your code change and checked that
it works well. Let me take more sufficient test on real H/W. Will share
the test result.
Thanks a lot!
Jae
