Re: [PATCH] i2c: imx-lpi2c: release clocks on remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Manually disabling hardware clocks while the devm-managed interrupt is still active creates a race condition leading to a system crash on spurious interrupts.
- [Medium] Ignored return value of pm_runtime_get_sync() leads to unbalanced clock disable and reference count underflow if device resume fails.

Pre-existing issues:
- [High] ISR unconditionally accesses hardware registers without ensuring the device is runtime active.
--

commit aa70bea937f841715508dc37222d676d4d04042f
Author: Guangshuo Li <lgs201920130244@gmail.com>
i2c: imx-lpi2c: release clocks on remove

This commit modifies lpi2c_imx_remove() to resume the device before disabling
runtime PM, ensuring the clocks are in a known enabled and prepared state. It
then drops the temporary runtime PM reference and explicitly disables and
unprepares the clocks during final teardown.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 49aabef7f115c..7f4bc98325853 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1613,8 +1613,13 @@ static void lpi2c_imx_remove(struct platform_device *pdev)
>  
>  	i2c_del_adapter(&lpi2c_imx->adapter);
>  
> +	pm_runtime_get_sync(&pdev->dev);

[Severity: Medium]
If this fails and returns a negative error code, is it safe to proceed? The
clocks would remain disabled, which might cause an enable count underflow
when clk_bulk_disable_unprepare() is unconditionally called later in this
function.

>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +
> +	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);

[Severity: High]
Does disabling the clocks here create a race condition with the devm managed
interrupt? Because the interrupt handler was registered with
devm_request_irq(), it is only automatically freed after lpi2c_imx_remove()
returns. If a spurious or delayed interrupt fires during this window,
lpi2c_imx_isr() will attempt to read hardware registers with clocks disabled,
which can cause a fatal system crash.

[Severity: High]
This is a pre-existing issue, but does lpi2c_imx_isr() also need to check if
the device is runtime active? The ISR unconditionally reads hardware registers:

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

If the device is suspended and a spurious interrupt fires, this could also
lead to a system crash. Should the ISR use pm_runtime_get_if_active()
before accessing hardware registers?

> +	pm_runtime_set_suspended(&pdev->dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914090718.1666333-1-lgs201920130244@gmail.com?part=1