Re: [PATCH] i2c: imx: Fix slave registration error path and missing NULL check

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The newly added NULL check in `i2c_imx_slave_event()` conditionally prevents state updates, creating an infinite loop in `i2c_imx_slave_finish_op()`.
- [High] The lockless NULL check of `i2c_imx->slave` in `i2c_imx_slave_event()` introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] The background `slave_timer` is not synchronously cancelled in `i2c_imx_unreg_slave()`, allowing a suspended device to be accessed.
- [High] The shared IRQ handler accesses hardware registers unconditionally without verifying the runtime PM state, violating PM Subsystem guidelines.
- [High] The runtime suspend callback powers down hardware without calling `synchronize_irq()`, violating PM Subsystem Concurrency rules for shared IRQs.
--

commit acf987bc13c7f8a8bbb96ab3004b61c139d9bb11
Author: Liem <liem16213@gmail.com>

i2c: imx: Fix slave registration error path and missing NULL check

This patch fixes an issue in i2c_imx_reg_slave() by clearing the slave
pointer when pm_runtime_resume_and_get() fails. It also adds a NULL check
in i2c_imx_slave_event() to prevent dereferencing a NULL slave pointer
during slave events.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cf5bacf3a4884..8c44f6bf618c1 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -715,8 +715,10 @@ static void i2c_imx_enable_bus_idle(struct imx_i2c_struct *i2c_imx)
>  static void i2c_imx_slave_event(struct imx_i2c_struct *i2c_imx,
>  				enum i2c_slave_event event, u8 *val)
>  {
> -	i2c_slave_event(i2c_imx->slave, event, val);
> -	i2c_imx->last_slave_event = event;
> +	if (i2c_imx->slave) {
> +		i2c_slave_event(i2c_imx->slave, event, val);

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

The slave pointer is read locklessly here, and the compiler could read
i2c_imx->slave twice: once for the if-condition and once to pass as an
argument to i2c_slave_event().

If i2c_imx_unreg_slave() clears the pointer to NULL concurrently from process
context without holding slave_lock:

i2c_imx_unreg_slave() {
    ...
    i2c_imx->slave = NULL;
    ...
}

Could i2c_slave_event() receive and dereference a NULL pointer?

> +		i2c_imx->last_slave_event = event;
> +	}

[Severity: Critical]
Does skipping this state update when the slave is NULL cause an infinite loop
in i2c_imx_slave_finish_op()?

If i2c_imx->slave is cleared concurrently, last_slave_event is never advanced
to I2C_SLAVE_STOP.

i2c_imx_slave_finish_op() iterates based on this state:

i2c_imx_slave_finish_op() {
    ...
    while (i2c_imx->last_slave_event != I2C_SLAVE_STOP) {
        ...
        case I2C_SLAVE_WRITE_REQUESTED:
            ...
            i2c_imx_slave_event(i2c_imx, I2C_SLAVE_STOP, &val);
            break;
    }
}

Will this cause the while loop to spin indefinitely?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does i2c_imx_unreg_slave()
fail to synchronously cancel the background timer?

It drops the runtime PM reference and disables the clock, but does not cancel
i2c_imx->slave_timer.

If the timer executes asynchronously after the clock is off:

i2c_imx_slave_timeout() {
    ...
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    ...
}

Will reading the hardware register trigger an external abort?

[Severity: High]
This isn't a bug introduced by this patch, but does the shared IRQ handler
access hardware registers unconditionally without verifying the runtime PM
state?

The driver requests a shared IRQ line:

i2c_imx_probe() {
    ...
    ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED,
                               pdev->name, i2c_imx);
    ...
}

If another device on the shared IRQ line triggers an interrupt while the I2C
controller is runtime-suspended, the ISR reads from a powered-off IP block:

i2c_imx_isr() {
    ...
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    ...
}

Should the ISR verify the device is active using pm_runtime_get_if_active()?

[Severity: High]
This isn't a bug introduced by this patch, but does the runtime suspend
callback power down hardware without calling synchronize_irq()?

i2c_imx_runtime_suspend() {
    ...
    clk_disable(i2c_imx->clk);
    ...
}

Because the driver requests an IRQ with the IRQF_SHARED flag, PM subsystem
guidelines mandate that synchronize_irq() must be called in the runtime
suspend callback before powering down hardware. This prevents the IRQ
handler from executing mid-flight on another CPU while the clock is being
disabled.

Is a call to synchronize_irq() needed here?

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