RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
> In AST2600 there have a slow peripheral bus between CPU and i2c > controller. > Therefore GIC i2c interrupt status clear have delay timing, when CPU > issue write clear i2c controller interrupt status. > To avoid this issue, the driver need have read after write clear at > i2c ISR. > > Signed-off-by: ryan_chen >Applied to for-current with a Fixes tag, thanks! Please, try to add one next >time and please also check how the subsystem formats the $subject line. [Ryan Chen] Thanks your review, will add fixes tag at subject.
RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
> > And is there maybe a Fixes: tag for it? > > [Ryan Chen] Yes it is a fix patch. > > I meant this (from submitting-patches.rst): >It fixes the original implementation of the driver basically. It's just a >classic posted-write fix. The write to clear the pending interrupt is >asynchronous, so you can get spurrious ones if you return from the handler >before it has percolated to the HW. >I assume it's just more visible on the 2600 because of the cores are >significantly faster but the IO bus is still as dumb. >Ryan: You could always add a Fixed-by: tag that specifies the commit that >added the initial driver... [Ryan Chen] Thanks Ben.
Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
On Wed, Apr 29, 2020 at 11:37:37AM +0800, ryan_chen wrote: > In AST2600 there have a slow peripheral bus between CPU > and i2c controller. > Therefore GIC i2c interrupt status clear have delay timing, > when CPU issue write clear i2c controller interrupt status. > To avoid this issue, the driver need have read after write > clear at i2c ISR. > > Signed-off-by: ryan_chen Applied to for-current with a Fixes tag, thanks! Please, try to add one next time and please also check how the subsystem formats the $subject line. signature.asc Description: PGP signature
Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
On Wed, 2020-04-29 at 11:03 +0200, Wolfram Sang wrote: > > And is there maybe a Fixes: tag for it? > > [Ryan Chen] Yes it is a fix patch. > > I meant this (from submitting-patches.rst): It fixes the original implementation of the driver basically. It's just a classic posted-write fix. The write to clear the pending interrupt is asynchronous, so you can get spurrious ones if you return from the handler before it has percolated to the HW. I assume it's just more visible on the 2600 because of the cores are significantly faster but the IO bus is still as dumb. Ryan: You could always add a Fixed-by: tag that specifies the commit that added the initial driver... Cheers, Ben.
Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
On Wed, 2020-04-29 at 11:37 +0800, ryan_chen wrote:
> In AST2600 there have a slow peripheral bus between CPU
> and i2c controller.
> Therefore GIC i2c interrupt status clear have delay timing,
> when CPU issue write clear i2c controller interrupt status.
> To avoid this issue, the driver need have read after write
> clear at i2c ISR.
>
> Signed-off-by: ryan_chen
Acked-by: Benjamin Herrenschmidt
--
> ---
> drivers/i2c/busses/i2c-aspeed.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> b/drivers/i2c/busses/i2c-aspeed.c
> index 07c1993274c5..f51702d86a90 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -603,6 +603,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq,
> void *dev_id)
> /* Ack all interrupts except for Rx done */
> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -645,9 +646,11 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq,
> void *dev_id)
> irq_received, irq_handled);
>
> /* Ack Rx done */
> - if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> writel(ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + }
> spin_unlock(>lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
> And is there maybe a Fixes: tag for it?
> [Ryan Chen] Yes it is a fix patch.
I meant this (from submitting-patches.rst):
===
If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
the SHA-1 ID, and the one line summary. Do not split the tag across multiple
lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
parsing scripts. For example::
Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the
number of pages it actually freed")
===
So, is it possible to identify a commit introducing the problem?
signature.asc
Description: PGP signature
RE: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
-Original Message-
From: Wolfram Sang [mailto:w...@the-dreams.de]
Sent: Wednesday, April 29, 2020 3:54 PM
To: Ryan Chen
Cc: Brendan Higgins ; Benjamin Herrenschmidt
; Joel Stanley ; Andrew Jeffery
; linux-...@vger.kernel.org; open...@lists.ozlabs.org;
linux-arm-ker...@lists.infradead.org; linux-asp...@lists.ozlabs.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status
clear race condition.
On Wed, Apr 29, 2020 at 11:37:37AM +0800, ryan_chen wrote:
> In AST2600 there have a slow peripheral bus between CPU and i2c
> controller.
> Therefore GIC i2c interrupt status clear have delay timing, when CPU
> issue write clear i2c controller interrupt status.
> To avoid this issue, the driver need have read after write clear at
> i2c ISR.
>
> Signed-off-by: ryan_chen
v0? is it a prototype?
[Ryan Chen] It is not prototype it is official patch.
And is there maybe a Fixes: tag for it?
[Ryan Chen] Yes it is a fix patch.
> ---
> drivers/i2c/busses/i2c-aspeed.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c
> b/drivers/i2c/busses/i2c-aspeed.c index 07c1993274c5..f51702d86a90
> 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -603,6 +603,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> /* Ack all interrupts except for Rx done */
> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -645,9 +646,11 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> irq_received, irq_handled);
>
> /* Ack Rx done */
> - if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> writel(ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + }
> spin_unlock(>lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }
> --
> 2.17.1
>
Re: [PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
On Wed, Apr 29, 2020 at 11:37:37AM +0800, ryan_chen wrote:
> In AST2600 there have a slow peripheral bus between CPU
> and i2c controller.
> Therefore GIC i2c interrupt status clear have delay timing,
> when CPU issue write clear i2c controller interrupt status.
> To avoid this issue, the driver need have read after write
> clear at i2c ISR.
>
> Signed-off-by: ryan_chen
v0? is it a prototype?
And is there maybe a Fixes: tag for it?
> ---
> drivers/i2c/busses/i2c-aspeed.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 07c1993274c5..f51702d86a90 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -603,6 +603,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> /* Ack all interrupts except for Rx done */
> writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> irq_remaining = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -645,9 +646,11 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
> *dev_id)
> irq_received, irq_handled);
>
> /* Ack Rx done */
> - if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> + if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
> writel(ASPEED_I2CD_INTR_RX_DONE,
> bus->base + ASPEED_I2C_INTR_STS_REG);
> + readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + }
> spin_unlock(>lock);
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
> --
> 2.17.1
>
signature.asc
Description: PGP signature
[PATCH v0 linux master] i2c/busses: Avoid i2c interrupt status clear race condition.
In AST2600 there have a slow peripheral bus between CPU
and i2c controller.
Therefore GIC i2c interrupt status clear have delay timing,
when CPU issue write clear i2c controller interrupt status.
To avoid this issue, the driver need have read after write
clear at i2c ISR.
Signed-off-by: ryan_chen
---
drivers/i2c/busses/i2c-aspeed.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 07c1993274c5..f51702d86a90 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -603,6 +603,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
/* Ack all interrupts except for Rx done */
writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
bus->base + ASPEED_I2C_INTR_STS_REG);
+ readl(bus->base + ASPEED_I2C_INTR_STS_REG);
irq_remaining = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -645,9 +646,11 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void
*dev_id)
irq_received, irq_handled);
/* Ack Rx done */
- if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
+ if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
writel(ASPEED_I2CD_INTR_RX_DONE,
bus->base + ASPEED_I2C_INTR_STS_REG);
+ readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ }
spin_unlock(>lock);
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
--
2.17.1
