linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
@ 2018-09-14  3:30 Guenter Roeck
  2018-09-14 16:28 ` Jae Hyun Yoo
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Guenter Roeck @ 2018-09-14  3:30 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: Benjamin Herrenschmidt, Joel Stanley, Andrew Jeffery, linux-i2c,
	openbmc, linux-aspeed, linux-kernel, Cédric Le Goater,
	Guenter Roeck, Jae Hyun Yoo

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 <jae.hyun.yoo@linux.intel.com>
Cc: Joel Stanley <joel@jms.id.au>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-14  3:30 [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler Guenter Roeck
@ 2018-09-14 16:28 ` Jae Hyun Yoo
  2018-09-17 16:34   ` Wolfram Sang
  2018-09-18  1:11 ` Joel Stanley
  2018-09-24 21:45 ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Jae Hyun Yoo @ 2018-09-14 16:28 UTC (permalink / raw)
  To: Guenter Roeck, Brendan Higgins
  Cc: linux-aspeed, Andrew Jeffery, openbmc, linux-kernel,
	Cédric Le Goater, linux-i2c

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 <jae.hyun.yoo@linux.intel.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   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 <jae.hyun.yoo@linux.intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-14 16:28 ` Jae Hyun Yoo
@ 2018-09-17 16:34   ` Wolfram Sang
  2018-09-17 17:16     ` Jae Hyun Yoo
  0 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2018-09-17 16:34 UTC (permalink / raw)
  To: Jae Hyun Yoo
  Cc: Guenter Roeck, Brendan Higgins, linux-aspeed, Andrew Jeffery,
	openbmc, linux-kernel, Cédric Le Goater, linux-i2c

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]


> Looks good to me. Thanks! :)
> 
> Acked-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>

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)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-17 16:34   ` Wolfram Sang
@ 2018-09-17 17:16     ` Jae Hyun Yoo
  2018-09-17 18:48       ` Guenter Roeck
  0 siblings, 1 reply; 8+ messages in thread
From: Jae Hyun Yoo @ 2018-09-17 17:16 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Guenter Roeck, Brendan Higgins, linux-aspeed, Andrew Jeffery,
	openbmc, linux-kernel, Cédric Le Goater, linux-i2c

On 9/17/2018 9:34 AM, Wolfram Sang wrote:
> 
>> Looks good to me. Thanks! :)
>>
>> Acked-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> 
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-17 17:16     ` Jae Hyun Yoo
@ 2018-09-17 18:48       ` Guenter Roeck
  0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2018-09-17 18:48 UTC (permalink / raw)
  To: Jae Hyun Yoo
  Cc: Wolfram Sang, Brendan Higgins, linux-aspeed, Andrew Jeffery,
	openbmc, linux-kernel, Cédric Le Goater, linux-i2c

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 <jae.hyun.yoo@linux.intel.com>
> >
> >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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-14  3:30 [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler Guenter Roeck
  2018-09-14 16:28 ` Jae Hyun Yoo
@ 2018-09-18  1:11 ` Joel Stanley
  2018-09-18  1:28   ` Brendan Higgins
  2018-09-24 21:45 ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Joel Stanley @ 2018-09-18  1:11 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Brendan Higgins, Benjamin Herrenschmidt, Andrew Jeffery,
	linux-i2c, OpenBMC Maillist, linux-aspeed,
	Linux Kernel Mailing List, Cédric Le Goater, Jae Hyun Yoo

On Fri, 14 Sep 2018 at 13:00, Guenter Roeck <linux@roeck-us.net> 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 <jae.hyun.yoo@linux.intel.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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 <joel@jms.id.au>

Cheers,

Joel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-18  1:11 ` Joel Stanley
@ 2018-09-18  1:28   ` Brendan Higgins
  0 siblings, 0 replies; 8+ messages in thread
From: Brendan Higgins @ 2018-09-18  1:28 UTC (permalink / raw)
  To: Joel Stanley
  Cc: linux, Benjamin Herrenschmidt, Andrew Jeffery, linux-i2c,
	OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
	Cédric Le Goater, Jae Hyun Yoo

On Mon, Sep 17, 2018 at 6:11 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Fri, 14 Sep 2018 at 13:00, Guenter Roeck <linux@roeck-us.net> 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 <jae.hyun.yoo@linux.intel.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> 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 <joel@jms.id.au>
>

Nice work! Thanks!

Acked-by: Brendan Higgins <brendanhiggins@google.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler
  2018-09-14  3:30 [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler Guenter Roeck
  2018-09-14 16:28 ` Jae Hyun Yoo
  2018-09-18  1:11 ` Joel Stanley
@ 2018-09-24 21:45 ` Wolfram Sang
  2 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2018-09-24 21:45 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
	Andrew Jeffery, linux-i2c, openbmc, linux-aspeed, linux-kernel,
	Cédric Le Goater, Jae Hyun Yoo

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

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 <jae.hyun.yoo@linux.intel.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-24 21:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14  3:30 [PATCH] i2c: aspeed: Acknowledge most interrupts early in interrupt handler Guenter Roeck
2018-09-14 16:28 ` Jae Hyun Yoo
2018-09-17 16:34   ` Wolfram Sang
2018-09-17 17:16     ` Jae Hyun Yoo
2018-09-17 18:48       ` Guenter Roeck
2018-09-18  1:11 ` Joel Stanley
2018-09-18  1:28   ` Brendan Higgins
2018-09-24 21:45 ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).