[PATCH 0/2] i2c: rcar: improve concurrency

[PATCH 0/2] i2c: rcar: improve concurrency

[Wolfram Sang]

Here is a small series which mainly upports a patch from the BSP, reasonably
fixing an issue. Thanks, Yamasaki-san. I also noticed during review that there
is no explanation of the special lockless design of this driver, so I added a
comment for that. It is really noteworthy, I think.


Hiromitsu Yamasaki (1):
  i2c: rcar: fix concurrency issue related to ICDMAER

Wolfram Sang (1):
  i2c: rcar: explain the lockless design

 drivers/i2c/busses/i2c-rcar.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH 1/2] i2c: rcar: fix concurrency issue related to ICDMAER

[Wolfram Sang]

From: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>

This patch fixes the problem that an interrupt may set up a new I2C
message and the DMA callback overwrites this setup.

By disabling the DMA Enable Register(ICDMAER), rcar_i2c_dma_unmap()
enables interrupts for register settings (such as Master Control
Register(ICMCR)) and advances the I2C transfer sequence.

If an interrupt occurs immediately after ICDMAER is disabled, the
callback handler later continues and overwrites the previous settings
from the interrupt. So, disable ICDMAER at the end of the callback to
ensure other interrupts are masked until then.

Note that this driver needs to work lock-free because there are IP cores
with a HW race condition which prevent us from using a spinlock in the
interrupt handler.

Reproduction test:
1. Add udelay(1) after disabling ICDMAER. (It is expected to
generate an interrupt of rcar_i2c_irq())

    void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
    {
        ...
        rcar_i2c_write(priv, ICDMAER, 0);
        udelay(1);                        <-- Add this line
        ...
        priv->dma_direction = DMA_NONE;
    }

2. Execute DMA transfer.
Performs DMA transfer of read or write. In the sample, write was used.

 $ i2cset -y -f 4 0x6a 0x01 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 i

3. A log message of BUG_ON() will be displayed.

Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index dd52a068b140..6410896f717b 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -363,9 +363,6 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 	struct dma_chan *chan = priv->dma_direction == DMA_FROM_DEVICE
 		? priv->dma_rx : priv->dma_tx;
 
-	/* Disable DMA Master Received/Transmitted */
-	rcar_i2c_write(priv, ICDMAER, 0);
-
 	dma_unmap_single(chan->device->dev, sg_dma_address(&priv->sg),
 			 sg_dma_len(&priv->sg), priv->dma_direction);
 
@@ -375,6 +372,9 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
 		priv->flags |= ID_P_NO_RXDMA;
 
 	priv->dma_direction = DMA_NONE;
+
+	/* Disable DMA Master Received/Transmitted */
+	rcar_i2c_write(priv, ICDMAER, 0);
 }
 
 static void rcar_i2c_cleanup_dma(struct rcar_i2c_priv *priv)
-- 
2.11.0

Re: [PATCH 1/2] i2c: rcar: fix concurrency issue related to ICDMAER

[Wolfram Sang]

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


> Reproduction test:
> 1. Add udelay(1) after disabling ICDMAER. (It is expected to
> generate an interrupt of rcar_i2c_irq())
> 
>     void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
>     {
>         ...
>         rcar_i2c_write(priv, ICDMAER, 0);
>         udelay(1);                        <-- Add this line

For the record (and self-documentation): I needed to increase the delay
and was using usleep_range(500, 800) in the end.

>         ...
>         priv->dma_direction = DMA_NONE;
>     }
> 
> 2. Execute DMA transfer.
> Performs DMA transfer of read or write. In the sample, write was used.
> 
>  $ i2cset -y -f 4 0x6a 0x01 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 i

And I used this command instead to make sure another message is waiting:

$ i2ctransfer -y 4 w9@0x6a 1 1+ r16

Yet, in general, I could reproduce that this patch fixes the issue, so
all is fine, I think.


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

Re: [PATCH 1/2] i2c: rcar: fix concurrency issue related to ICDMAER

[Simon Horman]

On Sun, Mar 03, 2019 at 04:03:13PM +0100, Wolfram Sang wrote:
> From: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> 
> This patch fixes the problem that an interrupt may set up a new I2C
> message and the DMA callback overwrites this setup.
> 
> By disabling the DMA Enable Register(ICDMAER), rcar_i2c_dma_unmap()
> enables interrupts for register settings (such as Master Control
> Register(ICMCR)) and advances the I2C transfer sequence.
> 
> If an interrupt occurs immediately after ICDMAER is disabled, the
> callback handler later continues and overwrites the previous settings
> from the interrupt. So, disable ICDMAER at the end of the callback to
> ensure other interrupts are masked until then.
> 
> Note that this driver needs to work lock-free because there are IP cores
> with a HW race condition which prevent us from using a spinlock in the
> interrupt handler.
> 
> Reproduction test:
> 1. Add udelay(1) after disabling ICDMAER. (It is expected to
> generate an interrupt of rcar_i2c_irq())
> 
>     void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
>     {
>         ...
>         rcar_i2c_write(priv, ICDMAER, 0);
>         udelay(1);                        <-- Add this line
>         ...
>         priv->dma_direction = DMA_NONE;
>     }
> 
> 2. Execute DMA transfer.
> Performs DMA transfer of read or write. In the sample, write was used.
> 
>  $ i2cset -y -f 4 0x6a 0x01 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 i
> 
> 3. A log message of BUG_ON() will be displayed.
> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/i2c/busses/i2c-rcar.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index dd52a068b140..6410896f717b 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -363,9 +363,6 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
>  	struct dma_chan *chan = priv->dma_direction == DMA_FROM_DEVICE
>  		? priv->dma_rx : priv->dma_tx;
>  
> -	/* Disable DMA Master Received/Transmitted */
> -	rcar_i2c_write(priv, ICDMAER, 0);
> -
>  	dma_unmap_single(chan->device->dev, sg_dma_address(&priv->sg),
>  			 sg_dma_len(&priv->sg), priv->dma_direction);
>  
> @@ -375,6 +372,9 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
>  		priv->flags |= ID_P_NO_RXDMA;
>  
>  	priv->dma_direction = DMA_NONE;
> +
> +	/* Disable DMA Master Received/Transmitted */
> +	rcar_i2c_write(priv, ICDMAER, 0);
>  }
>  
>  static void rcar_i2c_cleanup_dma(struct rcar_i2c_priv *priv)
> -- 
> 2.11.0
> 

Re: [PATCH 1/2] i2c: rcar: fix concurrency issue related to ICDMAER

[Wolfram Sang]

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

On Sun, Mar 03, 2019 at 04:03:13PM +0100, Wolfram Sang wrote:
> From: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> 
> This patch fixes the problem that an interrupt may set up a new I2C
> message and the DMA callback overwrites this setup.
> 
> By disabling the DMA Enable Register(ICDMAER), rcar_i2c_dma_unmap()
> enables interrupts for register settings (such as Master Control
> Register(ICMCR)) and advances the I2C transfer sequence.
> 
> If an interrupt occurs immediately after ICDMAER is disabled, the
> callback handler later continues and overwrites the previous settings
> from the interrupt. So, disable ICDMAER at the end of the callback to
> ensure other interrupts are masked until then.
> 
> Note that this driver needs to work lock-free because there are IP cores
> with a HW race condition which prevent us from using a spinlock in the
> interrupt handler.
> 
> Reproduction test:
> 1. Add udelay(1) after disabling ICDMAER. (It is expected to
> generate an interrupt of rcar_i2c_irq())
> 
>     void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv)
>     {
>         ...
>         rcar_i2c_write(priv, ICDMAER, 0);
>         udelay(1);                        <-- Add this line
>         ...
>         priv->dma_direction = DMA_NONE;
>     }
> 
> 2. Execute DMA transfer.
> Performs DMA transfer of read or write. In the sample, write was used.
> 
>  $ i2cset -y -f 4 0x6a 0x01 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 i
> 
> 3. A log message of BUG_ON() will be displayed.
> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Updated the test case, added a note about all this to the comment and
applied to for-current, thanks!


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

[PATCH 2/2] i2c: rcar: explain the lockless design

[Wolfram Sang]

To make sure people can understand the lockless design of this driver
without the need to dive into git history, add a comment giving an
overview of the situation.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 6410896f717b..3ce74edcd70c 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -611,6 +611,15 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 	return true;
 }
 
+/*
+ * This driver has a lock-free design because there are IP cores (at least
+ * R-Car Gen2) which have an inherent race condition in their hardware design.
+ * There, we need to clear RCAR_BUS_MASK_DATA bits as soon as possible after
+ * the interrupt was generated, otherwise an unwanted repeated message gets
+ * generated. It turned out that taking a spinlock at the beginning of the ISR
+ * was already causing repeated messages. Thus, this driver was converted to
+ * the now lockless behaviour. Please keep this in mind when hacking the driver.
+ */
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-- 
2.11.0

Re: [PATCH 2/2] i2c: rcar: explain the lockless design

[Simon Horman]

On Sun, Mar 03, 2019 at 04:03:14PM +0100, Wolfram Sang wrote:
> To make sure people can understand the lockless design of this driver
> without the need to dive into git history, add a comment giving an
> overview of the situation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/i2c/busses/i2c-rcar.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 6410896f717b..3ce74edcd70c 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -611,6 +611,15 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
>  	return true;
>  }
>  
> +/*
> + * This driver has a lock-free design because there are IP cores (at least
> + * R-Car Gen2) which have an inherent race condition in their hardware design.
> + * There, we need to clear RCAR_BUS_MASK_DATA bits as soon as possible after
> + * the interrupt was generated, otherwise an unwanted repeated message gets
> + * generated. It turned out that taking a spinlock at the beginning of the ISR
> + * was already causing repeated messages. Thus, this driver was converted to
> + * the now lockless behaviour. Please keep this in mind when hacking the driver.
> + */
>  static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
>  {
>  	struct rcar_i2c_priv *priv = ptr;
> -- 
> 2.11.0
> 

Re: [PATCH 2/2] i2c: rcar: explain the lockless design

[Wolfram Sang]

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

On Sun, Mar 03, 2019 at 04:03:14PM +0100, Wolfram Sang wrote:
> To make sure people can understand the lockless design of this driver
> without the need to dive into git history, add a comment giving an
> overview of the situation.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks!


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