Re: [PATCH v4 1/2] i2c: imx: Fix slave registration race and error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Shared interrupt handler accesses hardware registers while the device may be runtime suspended, and the suspend callback lacks synchronize_irq().
- [High] Lockless nullification of `i2c_imx->slave` in `i2c_imx_unreg_slave()` creates a race condition with the interrupt handler, risking a NULL pointer dereference.
- [High] Inconsistent handling of `pm_runtime_put_sync()` return value in `i2c_imx_unreg_slave()` can cause spurious unregistration failures in the I2C core.
- [High] `i2c_imx_slave_init()` is executed outside of the `slave_lock` in `i2c_imx_reg_slave()`, exposing partially initialized hardware state to concurrent ISR execution.
- [High] DMA mappings are permanently leaked on the timeout error paths during DMA transfers.
--

commit 04be58cb17fc93e06e4dbace3d95e195d097878a
Author: Liem <liem16213@gmail.com>

i2c: imx: Fix slave registration race and error handling

This patch fixes a race condition and error handling bug in slave
registration by moving the assignment of the slave pointer after a
successful resume and inside a lock.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cf5bacf3a4884..136e179d18b2d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -865,9 +865,6 @@ static int i2c_imx_reg_slave(struct i2c_client *client)
>  	if (i2c_imx->slave)
>  		return -EBUSY;
>  
> -	i2c_imx->slave = client;
> -	i2c_imx->last_slave_event = I2C_SLAVE_STOP;
> -
>  	/* Resume */
>  	ret = pm_runtime_resume_and_get(i2c_imx->adapter.dev.parent);
>  	if (ret < 0) {
> @@ -875,6 +872,11 @@ static int i2c_imx_reg_slave(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	scoped_guard(spinlock_irqsave, &i2c_imx->slave_lock) {
> +		i2c_imx->slave = client;
> +		i2c_imx->last_slave_event = I2C_SLAVE_STOP;
> +	}
> +
>  	i2c_imx_slave_init(i2c_imx);

[Severity: High]
This is a pre-existing issue, but does executing i2c_imx_slave_init()
outside of the slave_lock expose partially initialized hardware state to
concurrent ISR execution?

If a shared interrupt fires between the lock release and the register reset
in i2c_imx_slave_init(), the ISR could acquire the lock, see i2c_imx->slave
is non-NULL, and act on stale I2SR_IIF flags left over from prior
operations.

Could this invoke i2c_imx_slave_handle() with garbage hardware state?

>  
>  	return 0;
>  }

While reviewing these locking changes, I noticed a few other patterns in the
driver that might need attention.

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

[Severity: High]
This is a pre-existing issue, but does the lockless nullification of
i2c_imx->slave in i2c_imx_unreg_slave() create a race condition with the
interrupt handler?

If the shared interrupt handler i2c_imx_isr() executes concurrently, it may
acquire slave_lock, verify i2c_imx->slave is non-NULL, and proceed. If
i2c_imx_unreg_slave() clears the pointer immediately after this check,
would the ISR pass a NULL pointer to i2c_imx_slave_handle(), causing a
direct NULL pointer dereference?


Also in the removal path:

drivers/i2c/busses/i2c-imx.c:i2c_imx_unreg_slave() {
    ...
    ret = pm_runtime_put_sync(i2c_imx->adapter.dev.parent);
    if (ret < 0)
        dev_err(&i2c_imx->adapter.dev, "failed to suspend i2c controller");

    return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but can this inconsistent
handling of pm_runtime_put_sync() cause spurious unregistration failures?

According to the Runtime PM documentation, pm_runtime_put_sync() returns 1
if the device was already suspended. However, i2c_slave_unregister() expects
exactly 0 for success.

Since unreg_slave() has already successfully detached the slave, does
returning 1 leave the I2C core thinking the slave is still active, failing
to clear client->slave_cb?


Regarding the ISR and power management:

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

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

[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler access
hardware registers while the device might be runtime suspended?

If another device on the same IRQ line triggers an interrupt while the I2C
controller is runtime-suspended, i2c_imx_isr() unconditionally reads
IMX_I2C_I2SR without calling pm_runtime_get_if_active().

Additionally, the suspend callback disables the clock without calling
synchronize_irq() to ensure no ISR is mid-flight.

Could reading memory-mapped registers of a clock-gated device cause a
synchronous external abort or return garbage values?


Finally, looking at the DMA transfer error paths:

drivers/i2c/busses/i2c-imx.c:i2c_imx_dma_write() {
    ...
    if (time_left == 0) {
        dmaengine_terminate_sync(dma->chan_using);
        return -ETIMEDOUT;
    }
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but are the DMA mappings
permanently leaked on these timeout error paths?

Because the transfer is forcefully aborted, the async DMA completion
callback i2c_imx_dma_callback() is never executed. Since
dma_unmap_single() is only called inside that callback, does this result in
exhausting IOMMU space if timeouts occur?

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