| Message ID | 20190808195417.13482-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFT] i2c: emev2: avoid race when unregistering slave client | expand |
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 >
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
> 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!
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!
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;
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(-)