Re: [PATCH] i2c: imx: disable autosuspend on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler accesses hardware registers without checking if the device is active, causing a potential system crash.
- [High] DMA channels are permanently leaked if `i2c_add_numbered_adapter()` fails during probe.
- [High] Race condition in `i2c_imx_unreg_slave()` allows `slave_timer` to be re-armed after cancellation, leading to a use-after-free.
--

commit 37a8cb58f3db8dddb256f2da1347377502c8b5b3
Author: Guangshuo Li <lgs201920130244@gmail.com>
Subject: i2c: imx: disable autosuspend on remove

This commit adds a missing pm_runtime_dont_use_autosuspend() call in the
remove path to properly clean up runtime PM state. This ensures that the
autosuspend flag is cleared when the driver is unbound.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 19ec056b00afe..4dc3df6a41a4f 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1920,6 +1920,7 @@ static void i2c_imx_remove(struct platform_device *pdev)
>  
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  }
>  
>  static int i2c_imx_runtime_suspend(struct device *dev)

[Severity: High]
This isn't a bug introduced by this patch, but does this runtime suspend
callback need to call synchronize_irq() before the clock is disabled?

Since the driver requests the IRQ with IRQF_SHARED, if another device on the
shared line triggers an interrupt while the I2C device is runtime suspended,
i2c_imx_isr() executes and reads the IMX_I2C_I2SR register:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr() {
    [ ... ]
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    [ ... ]
}

Could this unconditional read of hardware registers while the controller's
clock is disabled potentially cause a fatal external abort?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential DMA
channel resource leak in the probe function?

In i2c_imx_probe(), i2c_imx_dma_request() successfully requests TX and RX DMA
channels. If the subsequent call to i2c_add_numbered_adapter() fails:

drivers/i2c/busses/i2c-imx.c:i2c_imx_probe() {
    [ ... ]
    ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
    if (ret < 0)
        goto clk_notifier_unregister;
    [ ... ]
}

This jump appears to bypass calling i2c_imx_dma_free(). Does this leak the
channels permanently since they are not devm-managed?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential race
condition in i2c_imx_unreg_slave() that could lead to a use-after-free?

The function cancels the timer and clears the slave pointer without holding
the slave lock:

drivers/i2c/busses/i2c-imx.c:i2c_imx_unreg_slave() {
    [ ... ]
    hrtimer_cancel(&i2c_imx->slave_timer);
    i2c_imx->slave = NULL;
    [ ... ]
}

If a hardware interrupt executes concurrently between these operations, could
i2c_imx_isr() acquire the lock, see i2c_imx->slave as non-NULL, and invoke
i2c_imx_slave_handle() which restarts the timer via hrtimer_restart()? Since
the device is subsequently freed, would this leave a pending timer referencing
freed memory?

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