diff mbox series

i2c: exynos5: Init data before registering interrupt handler

Message ID 20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com
State New
Headers
Series i2c: exynos5: Init data before registering interrupt handler |

Commit Message

Jesper Nilsson March 4, 2024, 11:01 a.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.
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,

Comments

Andi Shyti March 4, 2024, 3:04 p.m. UTC | #1 
Hi Jesper,

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.

> Additionally, return failure if we can't find a match in devicetree.
> 
> Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>

The way you are describing it you would need the Fixes tag here
and this patch should be treated as a fix.

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.

How have you tested this patch?

> ---
>  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.

Andi
Krzysztof Kozlowski March 4, 2024, 3:08 p.m. UTC | #2 
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
Jesper Nilsson March 4, 2024, 4:15 p.m. UTC | #3 
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@axis.com>
> 
> 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 March 4, 2024, 4:17 p.m. UTC | #4 
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 mbox series

Patch

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: