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

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

[Liem]

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 a timer (e.g. for slave event processing) before
   the pointer was cleared.  The timer callback
   i2c_imx_slave_event() dereferences i2c_imx->slave without a
   NULL check, which results in a use-after-free / NULL pointer
   dereference.  Prevent this by checking that i2c_imx->slave is
   valid before calling i2c_slave_event() and updating the
   last_slave_event field.

Both issues can trigger a kernel oops or permanent slave
registration failure under certain race conditions.  Add the
missing NULL assignment and the missing NULL check to harden
the slave path.

Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
Cc: stable@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
 drivers/i2c/busses/i2c-imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 28313d0fad37..4f7bcbeecfd0 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -775,8 +775,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);
+		i2c_imx->last_slave_event = event;
+	}
 }
 
 static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
@@ -936,6 +938,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;
 	}
-- 
2.34.1

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

[sashiko-bot]

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

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

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@gmail.com>
> Sent: Thursday, June 25, 2026 3:12 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Andi Shyti <andi.shyti@kernel.org>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Frank Li <frank.li@nxp.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Biwen Li
> <biwen.li@nxp.com>; Wolfram Sang <wsa@kernel.org>;
> linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; Liem <liem16213@gmail.com>
> Subject: [PATCH] i2c: imx: Fix slave registration error path and missing NULL check
> 
> [You don't often get email from liem16213@gmail.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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 a timer (e.g. for slave event processing) before
>    the pointer was cleared.  The timer callback
>    i2c_imx_slave_event() dereferences i2c_imx->slave without a
>    NULL check, which results in a use-after-free / NULL pointer
>    dereference.  Prevent this by checking that i2c_imx->slave is
>    valid before calling i2c_slave_event() and updating the
>    last_slave_event field.
> 
> Both issues can trigger a kernel oops or permanent slave registration failure under
> certain race conditions.  Add the missing NULL assignment and the missing NULL
> check to harden the slave path.
> 
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> 28313d0fad37..4f7bcbeecfd0 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c


Hi, Liem 

Thank you very much for this fix!
Looks like you are catching a corner bug and try to fix this for i2c-imx target mode.

Have you meet the issue on one real platform?

> @@ -775,8 +775,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);
> +               i2c_imx->last_slave_event = event;
> +       }

From i2c-imx.c driver, I notice this call trace is:

1. IRQ-> i2c_imx_slave_handle-> i2c_imx_slave_timeout(if in progress)-> i2c_imx_slave_finish_op
2. IRQ->i2c_imx_slave_finish_op

'''
In i2c_imx_unreg_slave(), the slave pointer is set to NULL after
disabling interrupts. However, a pending interrupt might already
have started a timer (e.g. for slave event processing) before
the pointer was cleared.
...

Yes, this may happen, then trigger slave hrtimer running. 

Go into i2c_imx_slave_finish_op().

you add a judgement in i2c_slave_event():

if (i2c_imx->slave) {
	i2c_slave_event(i2c_imx->slave, event, val);
	i2c_imx->last_slave_event = event;
}

At this time i2c_imx->slave= NULL, 
so i2c_slave_event(i2c_imx->slave, event, val) and i2c_imx->last_slave_event = event; won't run again.

in i2c_imx_slave_finish_op(),

Fall into" while (i2c_imx->last_slave_event != I2C_SLAVE_STOP)" the loop. So system maybe hang.

My idea is just cancel the slave timer and wait it finished after disabled IRQ.
If I am wrong please correct me. Thank you again.

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;
}

Carlos

> 
>  static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx) @@ -936,6
> +938,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;

This a good fix. I agree with this.

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

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

[Liem]

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.

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@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
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

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

[sashiko-bot]

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

New issues:
- [High] Lockless modification of `i2c_imx->slave` in `i2c_imx_reg_slave()` introduces a data race and potential NULL pointer dereference.

Pre-existing issues:
- [High] Incomplete timer cancellation race in `i2c_imx_unreg_slave()` allows the ISR to restart the timer, leading to a kernel panic.
- [High] The shared interrupt handler `i2c_imx_isr()` accesses hardware registers without checking if the device is active, violating PM guidelines and causing bus faults.
- [High] Missing IRQ synchronization in `i2c_imx_runtime_suspend()` allows the hardware to be powered down while the shared interrupt handler is executing.
--

commit 0e234f45db94de278630074d43eb9ffb0e39884e
Author: Liem <liem16213@gmail.com>

i2c: imx: Fix slave registration error path and missing timer cleanup

This commit fixes a potential -EBUSY deadlock when slave registration fails
by properly clearing the slave pointer. It also cancels the slave hrtimer
during unregistration to prevent a system hang caused by the timer firing
after the slave pointer is cleared.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cf5bacf3a4884..17b54d1648441 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -871,6 +871,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;

[Severity: High]
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?

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

[ ... ]

> @@ -892,7 +893,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;

[Severity: High]
This is a pre-existing issue, but does canceling the timer here completely
prevent it from running?

Since i2c_imx_unreg_slave() doesn't synchronize with the shared interrupt
handler, i2c_imx_isr() could be actively executing on another CPU. If the
ISR finishes after hrtimer_cancel() is called, it will reach
i2c_imx_slave_handle() and unconditionally call
hrtimer_restart(&i2c_imx->slave_timer).

When the re-armed timer expires, i2c_imx_slave_timeout() will attempt to
read hardware registers that have already been powered down by
pm_runtime_put_sync(), which causes a bus fault.

[Severity: High]
This is also a pre-existing issue, but does the shared interrupt handler
safely access hardware registers?

i2c_imx_isr() unconditionally reads IMX_I2C_I2SR and IMX_I2C_I2CR. Because
it handles a shared interrupt line, it could be triggered by another device
while the I2C controller is runtime-suspended and its clock is disabled.
Should this use pm_runtime_get_if_active() before accessing the registers
to prevent a synchronous external abort?

[Severity: High]
This is another pre-existing issue, but does the runtime suspend callback
safely disable the hardware?

i2c_imx_runtime_suspend() disables the hardware clock via clk_disable()
without calling synchronize_irq(). If i2c_imx_isr() is mid-execution on
another CPU when the clock is turned off, wouldn't subsequent register
accesses within the handler fault?

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

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@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>
> ---
> 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
>
>

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

[liem]

Hi Frank,

Thanks for the review. I'll split this into two separate patches and
send a v3 series.

Regards,
Liem

On Fri, Jun 26, 2026 at 12:16 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> 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@vger.kernel.org
> > Signed-off-by: Liem <liem16213@gmail.com>
> > ---
> > 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
> >
> >

[PATCH v3 0/2] Fix slave mode corner issues

[Liem]

This series fixes two issues in the i2c-imx slave mode.

Patch 1 clears the slave pointer on registration failure to allow
subsequent re-registration.

Patch 2 cancels the hrtimer before clearing the slave pointer during
unregistration, preventing a potential use-after-free / NULL pointer
dereference.

Changes in v3:
- Split the original patch into two separate patches as suggested by
  Frank Li.
- v2: https://lore.kernel.org/imx/
  20260625160219.55116-1-liem16213@gmail.com/

Liem (2):
  i2c: imx: Clear slave pointer on registration error
  i2c: imx: Cancel hrtimer before clearing slave pointer

 drivers/i2c/busses/i2c-imx.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.34.1

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

[Liem]

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@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
 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
@@ -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;
 	}
-- 
2.34.1

[PATCH v3 2/2] i2c: imx: Cancel hrtimer before clearing slave pointer

[Liem]

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,which results in a
use-after-free / NULL pointer dereference.

Fix by canceling the hrtimer and waiting for it to complete after
disabling interrupts, before clearing the slave pointer.

Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
Cc: stable@vger.kernel.org
Signed-off-by: Liem <liem16213@gmail.com>
---
 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 17defb470776..f02c216ba299 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -959,6 +959,7 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
 
 	i2c_imx_reset_regs(i2c_imx);
 
+	hrtimer_cancel(&i2c_imx->slave_timer);
 	i2c_imx->slave = NULL;
 
 	/* Suspend */
-- 
2.34.1

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

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@gmail.com>
> Sent: Friday, June 26, 2026 10:59 AM
> To: Frank Li (OSS) <frank.li@oss.nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; andi.shyti@kernel.org; Biwen Li
> <biwen.li@nxp.com>; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; liem16213@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org;
> linux-kernel@vger.kernel.org; o.rempel@pengutronix.de;
> s.hauer@pengutronix.de; stable@vger.kernel.org; wsa@kernel.org
> 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@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>
> ---
>  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
> 

RE: [PATCH v3 2/2] i2c: imx: Cancel hrtimer before clearing slave pointer

[Carlos Song (OSS)]

> -----Original Message-----
> From: Liem <liem16213@gmail.com>
> Sent: Friday, June 26, 2026 10:59 AM
> To: Frank Li (OSS) <frank.li@oss.nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; andi.shyti@kernel.org; Biwen Li
> <biwen.li@nxp.com>; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; liem16213@gmail.com;
> linux-arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org;
> linux-kernel@vger.kernel.org; o.rempel@pengutronix.de;
> s.hauer@pengutronix.de; stable@vger.kernel.org; wsa@kernel.org
> Subject: [PATCH v3 2/2] i2c: imx: Cancel hrtimer before clearing slave pointer
> 
> 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,which results in a use-after-free /
> NULL pointer dereference.
> 
> Fix by canceling the hrtimer and waiting for it to complete after disabling
> interrupts, before clearing the slave pointer.
> 
> Fixes: f7414cd6923f ("i2c: imx: support slave mode for imx I2C driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>

Hi,

LGTM, thank you very much!

Acked-by: Carlos Song <carlos.song@nxp.com>

> ---
>  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
> 17defb470776..f02c216ba299 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -959,6 +959,7 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
> 
>  	i2c_imx_reset_regs(i2c_imx);
> 
> +	hrtimer_cancel(&i2c_imx->slave_timer);
>  	i2c_imx->slave = NULL;
> 
>  	/* Suspend */
> --
> 2.34.1
> 

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

[liem]

Hi, carlos!

Thanks for the review.
This is a good idea; this is a better way to fix it.
I'll fix Patch 1 as suggested and send a v4.

Regards,

Liem