Re: [PATCH v4 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] DMA channels allocated during probe are never released on probe failure or device removal.
- [High] The driver requests its IRQ before enabling peripheral clocks and initializing waitqueues, creating a race condition that can crash the system.
- [High] Clock prepare and enable counts are permanently leaked on driver unload.
- [High] The interrupt handler accesses hardware registers without verifying the device's PM runtime state.
- [High] The atomic transfer callback `lpi2c_imx_xfer_atomic` calls a sleeping Runtime PM function, violating atomic context constraints.
--

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

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

When probe fails after clk_bulk_prepare_enable() succeeds but before
runtime PM is initialized, the enabled clocks are never disabled.
Additionally, calling pm_runtime_put_sync() in the error path can
trigger the runtime suspend callback, which may attempt to disable
clocks that have not been fully set up, leading to potential issues
during error unwinding.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index a01c236964819..540dc6cb664eb 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1565,8 +1569,13 @@ 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);
> +free_irq:
> +	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);

[Severity: High]
This is a pre-existing issue, but does explicitly placing clk_disable before
free_irq expose a window where an interrupt can arrive and cause a fault
during cleanup?

If a pending interrupt fires after clocks are disabled but before the interrupt
is freed, lpi2c_imx_isr() in drivers/i2c/busses/i2c-imx-lpi2c.c will execute
and attempt to read hardware registers while clocks are disabled.

[Severity: High]
This isn't a regression introduced by this patch, but are dma channels leaked
if probe fails after lpi2c_dma_init() succeeds?

The driver requests channels using dma_request_chan() in lpi2c_dma_init().
If a later probe step fails (such as i2c_add_adapter()), the error unwinding
path skips releasing the channels.

Additionally, lpi2c_imx_remove() in drivers/i2c/busses/i2c-imx-lpi2c.c appears
to lack cleanup, which could permanently leak two channels every time the
driver unbinds.

[Severity: High]
This is a pre-existing issue, but are clock prepare and enable counts retained
on driver unload?

Because lpi2c_imx_remove() in drivers/i2c/busses/i2c-imx-lpi2c.c only calls
pm_runtime_disable() without explicit clock unprepare or disable operations,
the actual clock counts might never be decremented. Devres will drop the clock
handles, but the clocks could remain enabled preventing deep sleep states.

[Severity: High]
This is a pre-existing issue, but could the interrupt handler access hardware
registers while the device is in a suspended state?

According to subsystem guidelines, interrupt handlers must use
pm_runtime_get_if_active() before accessing registers. Because the interrupt
is not explicitly disabled at the controller during runtime suspend, a spurious
or delayed interrupt might fire in lpi2c_imx_isr() while clocks are gated.

[Severity: High]
This isn't a regression introduced by this patch, but is it possible for the
atomic transfer callback to sleep?

lpi2c_imx_xfer_atomic() in drivers/i2c/busses/i2c-imx-lpi2c.c delegates to
lpi2c_imx_xfer_common(), which then calls lpi2c_imx_master_enable(). This path
executes pm_runtime_resume_and_get(). If the device is suspended, the power
management runtime core will attempt to acquire a mutex and sleep in an atomic
context where interrupts are disabled.

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