Received-SPF: pass (google.com: domain of linux-kernel+bounces-90773-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 4 Mar 2024 16:04:08 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, 
	Alim Akhtar <alim.akhtar@samsung.com>, linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@axis.com, 
	Andi Shyti <andi.shyti@kernel.org>
Subject: Re: [PATCH] i2c: exynos5: Init data before registering interrupt
 handler
Message-ID: <vpe5jvnhz3r5cpfiofwrelp62awe74knbxrz47i2deflczx276@yahhrshr355r>
References: <20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240304-i2c_exynos5-v1-1-e91c889d2025@axis.com>

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