Re: [PATCH 2/3] i2c: imx: release DMA channels on probe error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without verifying if the device is runtime active, and the runtime suspend callback lacks `synchronize_irq()`. This combination causes system crashes or invalid reads on shared IRQ lines.
--

commit bd798c45679ae67bd4a370c03b96a207ed6bc4a9
Author: Shengzhuo Wei <me@cherr.cc>

i2c: imx: release DMA channels on probe error

i2c_imx_dma_request() acquires exclusive tx/rx DMA channels and is optional.
If i2c_add_numbered_adapter() then fails, probe returns without releasing
the channels. This commit ensures the channels are released on the probe
error path.

Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Cc: stable@vger.kernel.org

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 19ec056b00afe..c24d9201a60e8 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1880,6 +1880,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  
>  clk_notifier_unregister:
>  	clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
> +	if (i2c_imx->dma)
> +		i2c_imx_dma_free(i2c_imx);
>  	free_irq(irq, i2c_imx);

[Severity: High]
This isn't a bug introduced by this patch, but since we're modifying the
probe and IRQ cleanup paths, could this driver crash due to unclocked
register reads if the IRQ is shared?

Looking at the driver, the IRQ is registered as shared during probe:

drivers/i2c/busses/i2c-imx.c:i2c_imx_probe() {
    ...
    ret = request_irq(irq, i2c_imx_isr, IRQF_SHARED | IRQF_NO_SUSPEND,
                      pdev->name, i2c_imx);
    ...
}

However, the runtime suspend callback disables the clock without ensuring
no ISR is running:

drivers/i2c/busses/i2c-imx.c:i2c_imx_runtime_suspend() {
    ...
    clk_disable(i2c_imx->clk);
    ...
}

If another device sharing the same interrupt line fires while this device
is runtime suspended, i2c_imx_isr() is called and unconditionally reads from
the unclocked hardware:

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

Should the runtime suspend callback use synchronize_irq(), and the ISR use
pm_runtime_get_if_active() before accessing hardware registers to prevent
these invalid reads or system crashes?

>  rpm_disable:
>  	pm_runtime_put_noidle(&pdev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-i2c-dma-channel-leak-v1-0-271d4adc03a0@cherr.cc?part=2