RE: [PATCH v3 1/2] i2c: imx: Clear slave pointer on registration error

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@xxxxxxxxx>
> Sent: Friday, June 26, 2026 10:59 AM
> To: Frank Li (OSS) <frank.li@xxxxxxxxxxx>
> Cc: Frank Li <frank.li@xxxxxxx>; andi.shyti@xxxxxxxxxx; Biwen Li
> <biwen.li@xxxxxxx>; festevam@xxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; liem16213@xxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; o.rempel@xxxxxxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; wsa@xxxxxxxxxx
> Subject: [PATCH v3 1/2] i2c: imx: Clear slave pointer on registration error
> 
> In i2c_imx_reg_slave(), i2c_imx->slave is checked at the beginning and the
> function returns -EBUSY if it is non-NULL.  If
> pm_runtime_resume_and_get() fails later, the error path returns without clearing
> i2c_imx->slave, leaving it non-NULL.  Subsequent attempts to register a slave will
> then immediately fail with -EBUSY, making it impossible to register the slave
> again.
> 
> Fix by setting i2c_imx->slave = NULL on the error path.
> 
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Liem <liem16213@xxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-imx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> 28313d0fad37..17defb470776 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c

Hi, Liem

LGTM. But I notice Sashiko give a worth-considering topic: 

Can this assignment race with the interrupt handler?
Because the driver uses a shared IRQ, i2c_imx_isr() could execute
concurrently if another device triggers the interrupt line.
If the ISR acquires slave_lock and evaluates i2c_imx->slave as valid, and
this error path locklessly sets it to NULL, wouldn't subsequent accesses
in the ISR dereference a NULL pointer?

> @@ -936,6 +936,7 @@ static int i2c_imx_reg_slave(struct i2c_client *client)
>  	/* Resume */
>  	ret = pm_runtime_resume_and_get(i2c_imx->adapter.dev.parent);
>  	if (ret < 0) {
> +		i2c_imx->slave = NULL;

>  		dev_err(&i2c_imx->adapter.dev, "failed to resume i2c controller");
>  		return ret;
>  	}

Is it helpful?

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 73317ddd5f02..e50058fd39ee 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -930,14 +930,16 @@ 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) {
                dev_err(&i2c_imx->adapter.dev, "failed to resume i2c controller");
                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);

> --
> 2.34.1
>