Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
properly") moved interrupt acknowledgment to the end of the interrupt
handler. In part this was done because the AST2500 datasheet says:
I2CD10 Interrupt Status Register
bit 2 Receive Done Interrupt status
S/W needs to clear this status bit to allow next data receiving.
Acknowledging Receive Done before receive data was handled resulted in
receive errors on high speed I2C busses.
However, interrupt acknowledgment was not only moved to the end of the
interrupt handler for Receive Done Interrupt status, but for all interrupt
status bits. This could result in race conditions if a second interrupt was
received during interrupt handling and not handled but still acknowledged
at the end of the interrupt handler.
Acknowledge only "Receive Done Interrupt status" late in the interrupt
handler to solve the problem.
Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
Cc: Jae Hyun Yoo <[email protected]>
Cc: Joel Stanley <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/i2c/busses/i2c-aspeed.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..3d518e09369f 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -552,6 +552,9 @@ 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 interrupts except for Rx done */
+ writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
+ bus->base + ASPEED_I2C_INTR_STS_REG);
irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -584,8 +587,10 @@ 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);
+ /* Ack Rx done */
+ if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
+ writel(ASPEED_I2CD_INTR_RX_DONE,
+ bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.7.4
On 9/13/2018 8:30 PM, Guenter Roeck wrote:
> Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
> properly") moved interrupt acknowledgment to the end of the interrupt
> handler. In part this was done because the AST2500 datasheet says:
>
> I2CD10 Interrupt Status Register
> bit 2 Receive Done Interrupt status
> S/W needs to clear this status bit to allow next data receiving.
>
> Acknowledging Receive Done before receive data was handled resulted in
> receive errors on high speed I2C busses.
>
> However, interrupt acknowledgment was not only moved to the end of the
> interrupt handler for Receive Done Interrupt status, but for all interrupt
> status bits. This could result in race conditions if a second interrupt was
> received during interrupt handling and not handled but still acknowledged
> at the end of the interrupt handler.
>
> Acknowledge only "Receive Done Interrupt status" late in the interrupt
> handler to solve the problem.
>
> Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> Cc: Jae Hyun Yoo <[email protected]>
> Cc: Joel Stanley <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ 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 interrupts except for Rx done */
> + writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ 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);
> + /* Ack Rx done */
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + writel(ASPEED_I2CD_INTR_RX_DONE,
> + bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
Looks good to me. Thanks! :)
Acked-by: Jae Hyun Yoo <[email protected]>
> Looks good to me. Thanks! :)
>
> Acked-by: Jae Hyun Yoo <[email protected]>
Does that mean I need to revert "[PATCH i2c-next v6] i2c: aspeed: Handle
master/slave combined irq events properly" in i2c/for-next? And apply
this to i2c/for-current?
(and please quote only relevat parts of a message when replying)
On 9/17/2018 9:34 AM, Wolfram Sang wrote:
>
>> Looks good to me. Thanks! :)
>>
>> Acked-by: Jae Hyun Yoo <[email protected]>
>
> Does that mean I need to revert "[PATCH i2c-next v6] i2c: aspeed: Handle
> master/slave combined irq events properly" in i2c/for-next? And apply
> this to i2c/for-current?
>
> (and please quote only relevat parts of a message when replying)
>
No need to revert "[PATCH i2c-next v6] i2c: aspeed: Handle master/slave
combined irq events properly". This patch should be applied on top of
the patch.
(Will reduce message size when replying. Thanks!)
Jae
On Mon, Sep 17, 2018 at 10:16:59AM -0700, Jae Hyun Yoo wrote:
> On 9/17/2018 9:34 AM, Wolfram Sang wrote:
> >
> >>Looks good to me. Thanks! :)
> >>
> >>Acked-by: Jae Hyun Yoo <[email protected]>
> >
> >Does that mean I need to revert "[PATCH i2c-next v6] i2c: aspeed: Handle
> >master/slave combined irq events properly" in i2c/for-next? And apply
> >this to i2c/for-current?
> >
>
> No need to revert "[PATCH i2c-next v6] i2c: aspeed: Handle master/slave
> combined irq events properly". This patch should be applied on top of
> the patch.
>
Yes, sorry, I should have tagged the patch as for-next.
Guenter
On Fri, 14 Sep 2018 at 13:00, Guenter Roeck <[email protected]> wrote:
>
> Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
> properly") moved interrupt acknowledgment to the end of the interrupt
> handler. In part this was done because the AST2500 datasheet says:
>
> I2CD10 Interrupt Status Register
> bit 2 Receive Done Interrupt status
> S/W needs to clear this status bit to allow next data receiving.
>
> Acknowledging Receive Done before receive data was handled resulted in
> receive errors on high speed I2C busses.
>
> However, interrupt acknowledgment was not only moved to the end of the
> interrupt handler for Receive Done Interrupt status, but for all interrupt
> status bits. This could result in race conditions if a second interrupt was
> received during interrupt handling and not handled but still acknowledged
> at the end of the interrupt handler.
>
> Acknowledge only "Receive Done Interrupt status" late in the interrupt
> handler to solve the problem.
>
> Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> Cc: Jae Hyun Yoo <[email protected]>
> Cc: Joel Stanley <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
Thanks for getting to the bottom of this Guenter. I gave it a spin on
Romulus (ast2500) and Palmetto (ast2400) without issue.
Tested-by: Joel Stanley <[email protected]>
Cheers,
Joel
On Mon, Sep 17, 2018 at 6:11 PM Joel Stanley <[email protected]> wrote:
>
> On Fri, 14 Sep 2018 at 13:00, Guenter Roeck <[email protected]> wrote:
> >
> > Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
> > properly") moved interrupt acknowledgment to the end of the interrupt
> > handler. In part this was done because the AST2500 datasheet says:
> >
> > I2CD10 Interrupt Status Register
> > bit 2 Receive Done Interrupt status
> > S/W needs to clear this status bit to allow next data receiving.
> >
> > Acknowledging Receive Done before receive data was handled resulted in
> > receive errors on high speed I2C busses.
> >
> > However, interrupt acknowledgment was not only moved to the end of the
> > interrupt handler for Receive Done Interrupt status, but for all interrupt
> > status bits. This could result in race conditions if a second interrupt was
> > received during interrupt handling and not handled but still acknowledged
> > at the end of the interrupt handler.
> >
> > Acknowledge only "Receive Done Interrupt status" late in the interrupt
> > handler to solve the problem.
> >
> > Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> > Cc: Jae Hyun Yoo <[email protected]>
> > Cc: Joel Stanley <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
>
> Thanks for getting to the bottom of this Guenter. I gave it a spin on
> Romulus (ast2500) and Palmetto (ast2400) without issue.
>
> Tested-by: Joel Stanley <[email protected]>
>
Nice work! Thanks!
Acked-by: Brendan Higgins <[email protected]>
On Thu, Sep 13, 2018 at 08:30:10PM -0700, Guenter Roeck wrote:
> Commit 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events
> properly") moved interrupt acknowledgment to the end of the interrupt
> handler. In part this was done because the AST2500 datasheet says:
>
> I2CD10 Interrupt Status Register
> bit 2 Receive Done Interrupt status
> S/W needs to clear this status bit to allow next data receiving.
>
> Acknowledging Receive Done before receive data was handled resulted in
> receive errors on high speed I2C busses.
>
> However, interrupt acknowledgment was not only moved to the end of the
> interrupt handler for Receive Done Interrupt status, but for all interrupt
> status bits. This could result in race conditions if a second interrupt was
> received during interrupt handling and not handled but still acknowledged
> at the end of the interrupt handler.
>
> Acknowledge only "Receive Done Interrupt status" late in the interrupt
> handler to solve the problem.
>
> Fixes: 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> Cc: Jae Hyun Yoo <[email protected]>
> Cc: Joel Stanley <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
Applied to for-next, thanks!