[PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[Wolfram Sang]

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(-)

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

Re: [PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[Niklas Söderlund]

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
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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

Re: [PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[Wolfram Sang]

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


> 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!


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

Re: [PATCH RFT] i2c: emev2: avoid race when unregistering slave client

[Wolfram Sang]

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

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!


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