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@xxxxxxxxx>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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
