diff mbox series

[v2] i2c: exynos5: Init data before registering interrupt handler

Message ID 20240304-i2c_exynos5-v2-1-7b9c312be719@axis.com (mailing list archive)
State New, archived
Headers show
Series [v2] i2c: exynos5: Init data before registering interrupt handler | expand

Commit Message

Jesper Nilsson March 4, 2024, 4:30 p.m. UTC 
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.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Fixes: 218e1496135e ("i2c: exynos5: add support for HSI2C on Exynos5260 SoC")
Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
---
Changes in v2:
- Use dev_err_probe() instead of open coding it
- Dropped the return failure if we can't find a match in devicetree
- Link to v1: https://lore.kernel.org/r/20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com
---
 drivers/i2c/busses/i2c-exynos5.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)


---
base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
change-id: 20240228-i2c_exynos5-db13a72eec8b

Best regards,

Comments

Andi Shyti March 5, 2024, 9:53 a.m. UTC | #1 
Hi Jesper,

I have taken your patch in my testing branch, but...

> +	/* Clear pending interrupts from u-boot or misc causes */
> +	exynos5_i2c_clr_pend_irq(i2c);
> +
> +	i2c->irq = ret = platform_get_irq(pdev, 0);

... I get this checkpatch note:

  CHECK: multiple assignments should be avoided
  #60: FILE: drivers/i2c/busses/i2c-exynos5.c:932:
  +       i2c->irq = ret = platform_get_irq(pdev, 0);

You are still in time to fix it, or you can fix it later or you
can leave it as for me it's not a big deal.

But, please, next time, make sure to run checkpatch before
sending a patch.

Thanks,
Andi
Jesper Nilsson March 5, 2024, 10:42 a.m. UTC | #2 
On Tue, Mar 05, 2024 at 10:53:13AM +0100, Andi Shyti wrote:
> Hi Jesper,

Hi Andi,

> I have taken your patch in my testing branch, but...
> 
> > +	/* Clear pending interrupts from u-boot or misc causes */
> > +	exynos5_i2c_clr_pend_irq(i2c);
> > +
> > +	i2c->irq = ret = platform_get_irq(pdev, 0);
> 
> ... I get this checkpatch note:
> 
>   CHECK: multiple assignments should be avoided
>   #60: FILE: drivers/i2c/busses/i2c-exynos5.c:932:
>   +       i2c->irq = ret = platform_get_irq(pdev, 0);

Hm, that's weird, I did run checkpatch and got no notices about that.
Ah, it looks like you're running with --strict to checkpatch.

> You are still in time to fix it, or you can fix it later or you
> can leave it as for me it's not a big deal.

No problem, v3 coming soon.

> But, please, next time, make sure to run checkpatch before
> sending a patch.

Yup. I'll run with --strict also in the future.

> Thanks,
> Andi

/^JN - Jesper Nilsson
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 385ef9d9e4d4..29d7cf158612 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -906,23 +906,9 @@  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);
 
 	ret = exynos5_hsi2c_clock_setup(i2c);
@@ -940,6 +926,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: