Re: [PATCH v4 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:

Pre-existing issues:
- [High] DMA channels requested via `dma_request_chan()` are never released, leading to a permanent resource leak upon driver unbind or probe failure.
- [High] Device removal cancels pending async suspend without disabling hardware clocks, leaving clocks permanently enabled.
--

commit a0b11faf7796dce688312611f6dc5e0060ca2cbb
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: reset controller in probe stage

Reset I2C controller in probe stage to avoid unexpected LPI2C controller
state left from previous stages and hang system boot.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 540dc6cb664eb..ccc7384323e03 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1547,19 +1558,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
>  	/* Give it another chance if pinctrl used is not ready yet */
>  	if (ret == -EPROBE_DEFER)
> -		goto rpm_disable;
> +		goto free_irq;
>  
>  	/* Init DMA */
>  	ret = lpi2c_dma_init(&pdev->dev, phy_addr);
>  	if (ret) {
>  		if (ret == -EPROBE_DEFER)
> -			goto rpm_disable;
> +			goto free_irq;
>  		dev_info(&pdev->dev, "use pio mode\n");
>  	}
>  
>  	ret = i2c_add_adapter(&lpi2c_imx->adapter);
>  	if (ret)
> -		goto rpm_disable;
> +		goto free_irq;

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak
the DMA channels?

If i2c_add_adapter() fails, we jump to the free_irq label, but nothing
releases the TX and RX channels allocated via dma_request_chan() in
lpi2c_dma_init().

It looks like lpi2c_imx_remove() also entirely omits dma_release_channel()
cleanup during driver unbind:

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);
}

Will this eventually exhaust the system's DMA channels after repeated probe
failures or driver unbinds?

>  
>  	pm_runtime_put_autosuspend(&pdev->dev);

[Severity: High]
This is also a pre-existing issue, but does lpi2c_imx_remove() leave
hardware clocks permanently enabled if the device is active when unbound?

When lpi2c_imx_remove() calls pm_runtime_disable(), it synchronously cancels
any pending asynchronous idle/suspend requests (like the autosuspend timer
started here at the end of probe), but leaves the device in its current
power state.

If the device is in RPM_ACTIVE during unbind, the clocks remain enabled. Is
a manual check or a call to pm_runtime_put_sync() needed before disabling
PM runtime to avoid a silent resource leak?

>  
> @@ -1567,6 +1578,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +free_irq:
> +	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
>  rpm_disable:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

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