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

[Carlos Song]

> -----Original Message-----
> From: Liem <liem16213@xxxxxxxxx>
> Sent: Monday, June 29, 2026 10:38 AM
> To: Carlos Song (OSS) <carlos.song@xxxxxxxxxxx>
> Cc: andi.shyti@xxxxxxxxxx; Biwen Li <biwen.li@xxxxxxx>; festevam@xxxxxxxxx;
> Frank Li <frank.li@xxxxxxx>; Frank Li (OSS) <frank.li@xxxxxxxxxxx>;
> 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: [EXT] [PATCH v4 1/2] i2c: imx: Fix slave registration race and error
> handling
> 
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
> 
> 
> 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>

Hi, Liem

LGTM. Thank you very much.

Acked-by: Carlos Song <carlos.song@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
> 
>