[PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo <hidden>
Date: 2018-09-11 23:58:47
Also in:
linux-aspeed, linux-i2c, lkml, openbmc
On 9/11/2018 4:33 PM, Guenter Roeck wrote:
quoted hunk ↗ jump to hunk
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