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

[Frank Li]

On Mon, Jun 29, 2026 at 10:38:28AM +0800, Liem wrote:
> In i2c_imx_reg_slave(), the slave pointer was assigned before
> pm_runtime_resume_and_get().  If pm_runtime_resume_and_get() failed,
> the error path returned without clearing i2c_imx->slave, leaving it
> non-NULL and causing all subsequent registration attempts to fail
> with -EBUSY.
>
> Additionally, because this driver uses a shared IRQ, the interrupt
> handler i2c_imx_isr() can execute concurrently and, after acquiring
> slave_lock, dereference i2c_imx->slave.  The previous fix attempt
> added a lockless i2c_imx->slave = NULL on the error path, but that
> could race with the ISR under the lock and still cause a NULL pointer
> dereference.
>
> Fix both issues by deferring the assignment of i2c_imx->slave and
> i2c_imx->last_slave_event to after a successful resume, and by
> performing the assignment inside the slave_lock critical section.
> This guarantees that the slave pointer is never left stale on the
> error path and is always valid when observed by the interrupt handler.
>
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Liem <liem16213@xxxxxxxxx>
> ---

anthor question, why v1..v3 use the thread? Suppose each new version should
start new thread. Do you use --in-reply-to in send patch?

Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

> v3 -> v4:
>   - Instead of clearing the slave pointer on error, defer the
>     assignment until after pm_runtime_resume_and_get() succeeds,
>     and take slave_lock to avoid racing with the shared IRQ handler.
>     Suggested by Sashiko and Carlos Song
> ---
>  drivers/i2c/busses/i2c-imx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..2398c406e913 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -930,9 +930,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) {
> @@ -940,6 +937,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);
>
>  	return 0;
> --
> 2.34.1
>