[RFT] i2c: emev2: avoid race when unregistering slave client
diff mbox series

Message ID 20190808195417.13482-1-wsa+renesas@sang-engineering.com
State Accepted
Delegated to: Geert Uytterhoeven
Headers show
Series
  • [RFT] i2c: emev2: avoid race when unregistering slave client
Related show

Commit Message

Wolfram Sang Aug. 8, 2019, 7:54 p.m. UTC
After we disabled interrupts, there might still be an active one
running. Sync before clearing the pointer to the slave device.

Fixes: c31d0a00021d ("i2c: emev2: add slave support")
Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Not tested on hardware yet. If someone has the board set up, testing if
standard I2C communication works would be nice. That would mean irq
setup did not regress. The actual race is more complicated to trigger.
If noone has the board, I will fetch it from my repository. It is packed
away currently.

 drivers/i2c/busses/i2c-emev2.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Niklas Söderlund Aug. 8, 2019, 9:51 p.m. UTC | #1
Hi Wolfram,

On 2019-08-08 21:54:17 +0200, Wolfram Sang wrote:
> After we disabled interrupts, there might still be an active one
> running. Sync before clearing the pointer to the slave device.
> 
> Fixes: c31d0a00021d ("i2c: emev2: add slave support")
> Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Not tested on hardware yet. If someone has the board set up, testing if
> standard I2C communication works would be nice. That would mean irq
> setup did not regress. The actual race is more complicated to trigger.
> If noone has the board, I will fetch it from my repository. It is packed
> away currently.

I do have the hardware but similar situation as you, packed away in its 
box. But the box is also packed in a larger box as I'm preparing for the 
move. I'm not sure in what box I put the box in, and I'm not super keen 
to look thru all of them before I unpack them on the other end.

> 
>  drivers/i2c/busses/i2c-emev2.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
> index 35b302d983e0..959d4912ec0d 100644
> --- a/drivers/i2c/busses/i2c-emev2.c
> +++ b/drivers/i2c/busses/i2c-emev2.c
> @@ -69,6 +69,7 @@ struct em_i2c_device {
>  	struct completion msg_done;
>  	struct clk *sclk;
>  	struct i2c_client *slave;
> +	int irq;
>  };
>  
>  static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear, u8 set, u8 reg)
> @@ -339,6 +340,12 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
>  
>  	writeb(0, priv->base + I2C_OFS_SVA0);
>  
> +	/*
> +	 * Wait for interrupt to finish. New slave irqs cannot happen because we
> +	 * cleared the slave address and, thus, only extension codes will be
> +	 * detected which do not use the slave ptr.
> +	 */
> +	synchronize_irq(priv->irq);
>  	priv->slave = NULL;
>  
>  	return 0;
> @@ -355,7 +362,7 @@ static int em_i2c_probe(struct platform_device *pdev)
>  {
>  	struct em_i2c_device *priv;
>  	struct resource *r;
> -	int irq, ret;
> +	int ret;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -390,8 +397,8 @@ static int em_i2c_probe(struct platform_device *pdev)
>  
>  	em_i2c_reset(&priv->adap);
>  
> -	irq = platform_get_irq(pdev, 0);
> -	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
> +	priv->irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_irq(&pdev->dev, priv->irq, em_i2c_irq_handler, 0,
>  				"em_i2c", priv);
>  	if (ret)
>  		goto err_clk;
> @@ -401,7 +408,8 @@ static int em_i2c_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_clk;
>  
> -	dev_info(&pdev->dev, "Added i2c controller %d, irq %d\n", priv->adap.nr, irq);
> +	dev_info(&pdev->dev, "Added i2c controller %d, irq %d\n", priv->adap.nr,
> +		 priv->irq);
>  
>  	return 0;
>  
> -- 
> 2.20.1
>
Adamski, Krzysztof (Nokia - PL/Wroclaw) Aug. 9, 2019, 10:40 a.m. UTC | #2
On Thu, Aug 08, 2019 at 09:54:17PM +0200, Wolfram Sang wrote:
>After we disabled interrupts, there might still be an active one
>running. Sync before clearing the pointer to the slave device.
>
>Fixes: c31d0a00021d ("i2c: emev2: add slave support")
>Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>---
>
>Not tested on hardware yet. If someone has the board set up, testing if
>standard I2C communication works would be nice. That would mean irq
>setup did not regress. The actual race is more complicated to trigger.
>If noone has the board, I will fetch it from my repository. It is packed
>away currently.

I don't see how this could influence the standard I2C communication at
all. If change in em_i2c_unreg_slave() is excluded, all that was changed
is moving irq number from local variable to the em_i2c_device struct
which is also not used outside of the em_i2c_unreg_slave() appart from
logging :)

Then, if we consider em_i2c_unreg_slave() I also don't see how this
could regres as no locks are being held in this function so calling
synchronize_irq() should be safe.

So from my point of view:
Reviewed-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Krzysztof
Wolfram Sang Aug. 9, 2019, 11:11 a.m. UTC | #3
> I don't see how this could influence the standard I2C communication at
> all. If change in em_i2c_unreg_slave() is excluded, all that was changed
> is moving irq number from local variable to the em_i2c_device struct
> which is also not used outside of the em_i2c_unreg_slave() appart from
> logging :)

I agree. Still, I do have brown-paper-bag experiences caused by wrong
assumptions like "this cannot fail". And we are changing the way
interrupts are acquired. So, if it is not too hard, I'd prefer to have
patches tested, too. I'd still apply the patch if it turns out to be too
complicated to test (given the reviews raise the trust). Yet, also on
the pro-side, it doesn't hurt to test a newer kernel on a packed-away
system once in a while.

Thanks for the reviews!
Wolfram Sang Aug. 14, 2019, 12:48 p.m. UTC | #4
On Thu, Aug 08, 2019 at 09:54:17PM +0200, Wolfram Sang wrote:
> After we disabled interrupts, there might still be an active one
> running. Sync before clearing the pointer to the slave device.
> 
> Fixes: c31d0a00021d ("i2c: emev2: add slave support")
> Reported-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks!

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 35b302d983e0..959d4912ec0d 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -69,6 +69,7 @@  struct em_i2c_device {
 	struct completion msg_done;
 	struct clk *sclk;
 	struct i2c_client *slave;
+	int irq;
 };
 
 static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear, u8 set, u8 reg)
@@ -339,6 +340,12 @@  static int em_i2c_unreg_slave(struct i2c_client *slave)
 
 	writeb(0, priv->base + I2C_OFS_SVA0);
 
+	/*
+	 * Wait for interrupt to finish. New slave irqs cannot happen because we
+	 * cleared the slave address and, thus, only extension codes will be
+	 * detected which do not use the slave ptr.
+	 */
+	synchronize_irq(priv->irq);
 	priv->slave = NULL;
 
 	return 0;
@@ -355,7 +362,7 @@  static int em_i2c_probe(struct platform_device *pdev)
 {
 	struct em_i2c_device *priv;
 	struct resource *r;
-	int irq, ret;
+	int ret;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -390,8 +397,8 @@  static int em_i2c_probe(struct platform_device *pdev)
 
 	em_i2c_reset(&priv->adap);
 
-	irq = platform_get_irq(pdev, 0);
-	ret = devm_request_irq(&pdev->dev, irq, em_i2c_irq_handler, 0,
+	priv->irq = platform_get_irq(pdev, 0);
+	ret = devm_request_irq(&pdev->dev, priv->irq, em_i2c_irq_handler, 0,
 				"em_i2c", priv);
 	if (ret)
 		goto err_clk;
@@ -401,7 +408,8 @@  static int em_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk;
 
-	dev_info(&pdev->dev, "Added i2c controller %d, irq %d\n", priv->adap.nr, irq);
+	dev_info(&pdev->dev, "Added i2c controller %d, irq %d\n", priv->adap.nr,
+		 priv->irq);
 
 	return 0;