On Mon, Mar 04, 2024 at 04:04:08PM +0100, Andi Shyti wrote: > Hi Jesper, Hi Andi, > On Mon, Mar 04, 2024 at 12:01:14PM +0100, Jesper Nilsson wrote: > > devm_request_irq() is called before we initialize the "variant" > > member variable from of_device_get_match_data(), so if an interrupt > > is triggered inbetween, we can end up following a NULL pointer > > in the interrupt handler. > > > > This problem was exposed when the I2C controller in question was > > (mis)configured to be used in both secure world and Linux. > > > > That this can happen is also reflected by the existing code that > > clears any pending interrupts from "u-boot or misc causes". > > > > Move the clearing of pending interrupts and the call to > > devm_request_irq() to the end of probe. > > I'm OK with moving the irq request at the end and I'm going to > give my r-b anyway. There is still one comment below. Thanks. > > Additionally, return failure if we can't find a match in devicetree. > > > > Signed-off-by: Jesper Nilsson <jesper.nilsson@xxxxxxxx> > > The way you are describing it you would need the Fixes tag here > and this patch should be treated as a fix. Ok, the variant member was introduced in: 218e1496135e ("i2c: exynos5: add support for HSI2C on Exynos5260 SoC") Before that, the ordering didn't matter. > Nevertheless, I think that it's odd that the device is sending > interrupts at this phase and the real fix should be preventing > the controller to send interrupts here. I found this bug when we moved control of an I2C bus into secure world (OP-TEE) and hadn't removed it from devicetree config. Since we hadn't enabled the secure parts yet, the kernel could still get the interrupt, even when OP-TEE was performing I2C transfers. > How have you tested this patch? This was tested in our ARTPEC-8 SoC, which includes the same IP the exynos SoCs, both with and without the problematic configuration described above. > > --- > > drivers/i2c/busses/i2c-exynos5.c | 32 ++++++++++++++++++-------------- > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > > index 385ef9d9e4d4..eba717e5cad7 100644 > > --- a/drivers/i2c/busses/i2c-exynos5.c > > +++ b/drivers/i2c/busses/i2c-exynos5.c > > @@ -906,24 +906,14 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > > i2c->adap.algo_data = i2c; > > i2c->adap.dev.parent = &pdev->dev; > > > > - /* Clear pending interrupts from u-boot or misc causes */ > > - exynos5_i2c_clr_pend_irq(i2c); > > - > > spin_lock_init(&i2c->lock); > > init_completion(&i2c->msg_complete); > > > > - i2c->irq = ret = platform_get_irq(pdev, 0); > > - if (ret < 0) > > - goto err_clk; > > - > > - ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq, > > - IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c); > > - if (ret != 0) { > > - dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq); > > - goto err_clk; > > - } > > - > > i2c->variant = of_device_get_match_data(&pdev->dev); > > + if (!i2c->variant) { > > + dev_err(&pdev->dev, "can't match device variant\n"); > > + return -ENODEV; > > return dev_err_probe(), please. Will do. > Andi /^JN - Jesper Nilsson -- Jesper Nilsson -- jesper.nilsson@xxxxxxxx