Re: [PATCH v2] i2c: imx: Fix slave registration error path and missing timer cleanup

[Frank Li]

On Fri, Jun 26, 2026 at 12:02:19AM +0800, Liem wrote:
>
> There are two issues that affect the i2c-imx slave handling:
>
> 1. 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.
>
> 2. In i2c_imx_unreg_slave(), the slave pointer is set to NULL after
>    disabling interrupts.  However, a pending interrupt might already
>    have started the hrtimer (i2c_imx_slave_timeout) before the pointer
>    was cleared.  If the hrtimer fires after i2c_imx->slave is set to
>    NULL, the timer callback i2c_imx_slave_finish_op() will call
>    i2c_imx_slave_event() with a NULL slave pointer, and the
>    last_slave_event check loop in i2c_imx_slave_finish_op() may cause
>    a system hang because last_slave_event is no longer updated.  Fix
>    by canceling the hrtimer and waiting for it to complete after
>    disabling interrupts, before clearing the slave pointer.

Please use two patches to fix these problem. One patch fix one problem.

Frank

>
> Both issues can trigger a kernel oops, system hang, or permanent
> slave registration failure under certain race conditions.  Add the
> missing NULL assignment and the missing hrtimer cleanup to harden
> the slave path.
>
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Liem <liem16213@xxxxxxxxx>
> ---
> v1 -> v2:
>   - Instead of adding a NULL check in i2c_imx_slave_event(), cancel
>     the hrtimer and wait for it to finish in i2c_imx_unreg_slave()
>     after disabling interrupts, as suggested by <Carlos Song>.
>     This avoids a potential hang in the last_slave_event loop in
>     i2c_imx_slave_finish_op().
> ---
>  drivers/i2c/busses/i2c-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 28313d0fad37..04ffb927aba9 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -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;
>         }
> @@ -957,7 +958,7 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>         imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
>
>         i2c_imx_reset_regs(i2c_imx);
> -
> +       hrtimer_cancel(&i2c_imx->slave_timer);
>         i2c_imx->slave = NULL;
>
>         /* Suspend */
> --
> 2.53.0
>
>