Re: [PATCH v6 2/2] i2c: imx-lpi2c: reset controller in probe stage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The probe error path explicitly disables device clocks while the devres-managed interrupt remains active, risking an external abort.

Pre-existing issues:
- [High] The interrupt handler unconditionally accesses hardware registers without verifying the device's runtime PM state, risking a synchronous external abort.
--

--- Patch [2]: [PATCH v6 2/2] i2c: imx-lpi2c: reset controller in probe stage ---
commit a2b44c86e6f00595e1b33e7174c800f8fdb5e7ca
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: reset controller in probe stage

This commit resets the I2C controller during the probe stage to prevent
unexpected states left from prior boot stages. It asserts and deasserts
software reset bits in the MCR and SCR registers. It also moves the
IRQ allocation to occur after this reset initialization.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 615d9f7f11471..cfc0d108d3c4a 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

[ ... ]

> @@ -1550,6 +1545,22 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	/*
> +	 * Reset all internal controller registers of both Master and Target
> +	 * to avoid effects of previous status.
> +	 */
> +	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
> +	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> +	writel(0, lpi2c_imx->base + LPI2C_MCR);
> +	writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> +	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
> +			       pdev->name, lpi2c_imx);

[Severity: High]
If an error occurs later in lpi2c_imx_probe() (such as in
lpi2c_imx_init_recovery_info() or i2c_add_adapter()), the error path jumps
to clk_disable and calls clk_bulk_disable_unprepare() before returning.

Since devm_request_irq() is used, the interrupt isn't freed until after the
probe function returns.

Does this create a window where the devres-managed IRQ remains active while
the device clocks are explicitly disabled? If a spurious interrupt arrives
during this window, could it cause an external abort?

[Severity: High]
This is a pre-existing issue, but lpi2c_imx_isr() unconditionally reads
hardware registers without verifying the runtime PM state:

drivers/i2c/busses/i2c-imx-lpi2c.c:lpi2c_imx_isr() {
    ...
    u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
    ...
}

Because IRQF_NO_SUSPEND is used, the interrupt could fire when the device
is in the RPM_SUSPENDED state with clocks disabled, especially for SoCs
where need_request_free_irq is false.

Would it be safer to check the PM state with pm_runtime_get_if_active()
in the interrupt handler before accessing registers, returning IRQ_NONE
if the device is suspended?

> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
> +		goto rpm_disable;
> +	}
> +
>  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716071957.2670263-1-carlos.song@oss.nxp.com?part=2