| Message ID | 20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: exynos5: Init data before registering interrupt handler | expand |
On 04/03/2024 16:04, Andi Shyti wrote: >> - >> 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. How this condition even possibly happen? And how is this related to the problem described here? No, don't mix two different issues. Best regards, Krzysztof
On Mon, Mar 04, 2024 at 04:08:00PM +0100, Krzysztof Kozlowski wrote: > On 04/03/2024 16:04, Andi Shyti wrote: > >> - > >> 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. > > How this condition even possibly happen? And how is this related to the > problem described here? No, it was not. That was part of the debugging before we understood the real problem. > No, don't mix two different issues. No problem, I can drop this part. > Best regards, > Krzysztof /^JN - Jesper Nilsson
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; + } ret = exynos5_hsi2c_clock_setup(i2c); if (ret) @@ -940,6 +930,20 @@ static int exynos5_i2c_probe(struct platform_device *pdev) clk_disable(i2c->clk); clk_disable(i2c->pclk); + /* Clear pending interrupts from u-boot or misc causes */ + exynos5_i2c_clr_pend_irq(i2c); + + 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; + } + return 0; err_clk:
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. Additionally, return failure if we can't find a match in devicetree. Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com> --- drivers/i2c/busses/i2c-exynos5.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) --- base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a change-id: 20240228-i2c_exynos5-db13a72eec8b Best regards,