| Message ID | 20200429033737.2781-1-ryan_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c926c87b8e36dcc0ea5c2a0a0227ed4f32d0516a |
| Headers | show |
| Series | [v0,linux,master] i2c/busses: Avoid i2c interrupt status clear race condition. | expand |
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 <ryan_chen@aspeedtech.com> 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(&bus->lock); > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > } > -- > 2.17.1 >
-----Original Message----- From: Wolfram Sang [mailto:wsa@the-dreams.de] Sent: Wednesday, April 29, 2020 3:54 PM To: Ryan Chen <ryan_chen@aspeedtech.com> Cc: Brendan Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt <benh@kernel.crashing.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; linux-aspeed@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 <ryan_chen@aspeedtech.com> 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(&bus->lock); > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; } > -- > 2.17.1 >
> 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?
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 <ryan_chen@aspeedtech.com> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> -- > --- > 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(&bus->lock); > return irq_remaining ? IRQ_NONE : IRQ_HANDLED; > }
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.
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 <ryan_chen@aspeedtech.com> 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.
> 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 <ryan_chen@aspeedtech.com> >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.
> > 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.
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(&bus->lock); return irq_remaining ? IRQ_NONE : IRQ_HANDLED; }
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 <ryan_chen@aspeedtech.com> --- drivers/i2c/busses/i2c-aspeed.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)