Re: [PATCH v6 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `lpi2c_imx_remove()` function leaks clock prepare and enable references when unbinding the driver.
- [High] NULL pointer dereference when `devm_clk_bulk_get_all()` returns 0.
- [High] Clock references are permanently leaked in the error path of `lpi2c_runtime_resume()`.
- [Medium] Unprotected hardware register access in the interrupt handler causes a bus fault on spurious interrupts.
- [Low] Unconditional `pm` assignment produces dead code when PM is disabled.
--

--- Patch [1]: [PATCH v6 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure ---
commit 2175c9d5c422256763ff98e4c5aedc0fe0e29c4d
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: properly unwind resources on probe failure

The commit introduces a new error label to explicitly invoke
clk_bulk_disable_unprepare() on probe failure. It replaces
pm_runtime_put_sync() with pm_runtime_disable(), pm_runtime_set_suspended()
and pm_runtime_put_noidle() to bypass the runtime suspend callback during
error recovery.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e6c24a9d934d7..615d9f7f11471 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1527,14 +1527,18 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	 * each transfer
>  	 */
>  	ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);

[Severity: High]
This is a pre-existing issue, but if devm_clk_bulk_get_all() returns 0,
lpi2c_imx->clks remains NULL. Does the array access lpi2c_imx->clks[0].clk
cause a NULL pointer dereference in that case?

> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "can't lock I2C peripheral clock rate\n");
> +	if (ret) {
> +		ret = dev_err_probe(&pdev->dev, ret,
> +				    "can't lock I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>  
>  	lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
> -	if (!lpi2c_imx->rate_per)
> -		return dev_err_probe(&pdev->dev, -EINVAL,
> -				     "can't get I2C peripheral clock rate\n");
> +	if (!lpi2c_imx->rate_per) {
> +		ret = dev_err_probe(&pdev->dev, -EINVAL,
> +				    "can't get I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>  
>  	if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
>  		pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
> @@ -1576,8 +1580,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  
>  rpm_disable:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> -	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +clk_disable:
> +	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does lpi2c_imx_remove() safely
clean up the prepared clocks?

static void lpi2c_imx_remove(struct platform_device *pdev)
{
	struct lpi2c_imx_struct *lpi2c_imx = platform_get_drvdata(pdev);

	i2c_del_adapter(&lpi2c_imx->adapter);

	pm_runtime_disable(&pdev->dev);
	pm_runtime_dont_use_autosuspend(&pdev->dev);
}

Calling pm_runtime_disable() without a prior pm_runtime_get_sync() leaves
the device in its current state. If the device was active, the clocks could be
permanently leaked since lpi2c_runtime_suspend() won't execute, and there is
no fallback clk_bulk_disable_unprepare() call here.

[Severity: High]
This is a pre-existing issue, but could a devm_request_irq() failure in
lpi2c_runtime_resume() cause clock references to be leaked?

	if (need_request_free_irq) {
		ret = devm_request_irq(dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
				       dev_name(dev), lpi2c_imx);
		if (ret) {
			dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
			return ret;
		}
	}

If the IRQ request fails, it appears the function returns the error without
reverting the prior clk_bulk_prepare_enable() operations. Since the PM core
would mark the device as suspended on failure, would the clocks remain enabled
indefinitely?

[Severity: Medium]
This isn't a bug introduced by this patch, but is it possible for the
interrupt handler to access unclocked registers during a spurious interrupt?

static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
{
	struct lpi2c_imx_struct *lpi2c_imx = dev_id;

	if (lpi2c_imx->target) {
		u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
		u32 ssr = readl(lpi2c_imx->base + LPI2C_SSR);

The handler unconditionally reads hardware registers without verifying the
device PM status is active via pm_runtime_get_if_active(). Could this
trigger a bus fault if the device is currently suspended and its clocks are
disabled?

[Severity: Low]
This is a pre-existing issue, but can the PM callbacks become dead code
when power management is disabled?

static struct platform_driver lpi2c_imx_driver = {
	.probe = lpi2c_imx_probe,
	.remove = lpi2c_imx_remove,
	.driver = {
		.name = DRIVER_NAME,
		.of_match_table = lpi2c_imx_of_match,
		.pm = &lpi2c_pm_ops,
	},
};

The .pm assignment does not use the pm_ptr() wrapper, which may prevent the
compiler from safely discarding the PM callbacks when CONFIG_PM is not set.

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