[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

[PATCH v4 0/2] i2c: imx: Fix slave mode corner issues

[Liem]

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

Patch 1 defers slave pointer assignment to after a successful resume
and protects it with the slave_lock to prevent races with the shared
IRQ handler.

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

Changes in v4:
- Patch 1: reworked to avoid race with shared IRQ handler, as
  suggested by Sashiko.
- Patch 2: unchanged.

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: Fix slave registration race and error handling
  i2c: imx: Cancel hrtimer before clearing slave pointer

 drivers/i2c/busses/i2c-imx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.34.1

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

[Liem]

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

[PATCH v4 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
Acked-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Liem <liem16213@gmail.com>
---
v3 -> v4: No changes, added Acked-by from Carlos Song.
---
 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 2398c406e913..b1c6581db774 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -960,6 +960,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 v4 2/2] i2c: imx: Cancel hrtimer before clearing slave pointer

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch fails to completely resolve the NULL pointer dereference and timer re-arming race because it does not synchronize with the hardware interrupt handler (`i2c_imx_isr`).
- [High] Unsafe register access in shared interrupt handler while device is runtime suspended.
--

commit c2a34f9b292f8911be5d2996abfed1cdbdad49e7
Author: Liem <liem16213@gmail.com>

i2c: imx: Cancel hrtimer before clearing slave pointer

This commit attempts to fix a use-after-free and NULL pointer dereference
by canceling the slave hrtimer before setting the slave pointer to NULL in
i2c_imx_unreg_slave(). The goal is to prevent the timer callback from firing
and dereferencing a cleared pointer.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 136e179d18b2d..4a6348f03fc5d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -895,6 +895,7 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>  
>  	i2c_imx_reset_regs(i2c_imx);
>  
> +	hrtimer_cancel(&i2c_imx->slave_timer);

[Severity: High]
This is a pre-existing issue, but does this patch fully close the race
condition it attempts to fix?

Since teardown happens without holding slave_lock or calling
synchronize_irq(), the hardware interrupt handler (i2c_imx_isr) can
execute concurrently.

If the ISR evaluates i2c_imx->slave before it is set to NULL here, it will
proceed to i2c_imx_slave_handle() and unconditionally re-arm the timer:

drivers/i2c/busses/i2c-imx.c:i2c_imx_slave_handle() {
    ...
    hrtimer_restart(&i2c_imx->slave_timer);
    ...
}

Because i2c_imx_isr() is a hardware interrupt, hrtimer_cancel() here will
return immediately without waiting for it. The concurrent ISR will re-arm
the timer, which can then fire after i2c_imx->slave is set to NULL, still
causing the NULL pointer dereference.

Should synchronize_irq() be called before clearing the pointer and
canceling the timer, or should the slave_lock be held?

>  	i2c_imx->slave = NULL;
>  
>  	/* Suspend */

[Severity: High]
This is a pre-existing issue, but can the shared interrupt handler crash the
system while the device is suspended?

The interrupt is requested as IRQF_SHARED in i2c_imx_probe():

    ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED,
                               pdev->name, i2c_imx);

When the device is suspended, i2c_imx_runtime_suspend() disables the clock
via clk_disable(i2c_imx->clk) without masking the shared IRQ line.

If another device on the same IRQ line triggers an interrupt while the
controller is suspended, i2c_imx_isr() will execute and unconditionally
read hardware registers:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr() {
    ...
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    ...
}

On i.MX hardware, accessing registers of an unclocked peripheral results
in a synchronous bus fault. Does the ISR need a check to ensure the device
is not runtime-suspended before reading registers?

[ ... ]

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Shared interrupt handler accesses hardware registers while the device may be runtime suspended, and the suspend callback lacks synchronize_irq().
- [High] Lockless nullification of `i2c_imx->slave` in `i2c_imx_unreg_slave()` creates a race condition with the interrupt handler, risking a NULL pointer dereference.
- [High] Inconsistent handling of `pm_runtime_put_sync()` return value in `i2c_imx_unreg_slave()` can cause spurious unregistration failures in the I2C core.
- [High] `i2c_imx_slave_init()` is executed outside of the `slave_lock` in `i2c_imx_reg_slave()`, exposing partially initialized hardware state to concurrent ISR execution.
- [High] DMA mappings are permanently leaked on the timeout error paths during DMA transfers.
--

commit 04be58cb17fc93e06e4dbace3d95e195d097878a
Author: Liem <liem16213@gmail.com>

i2c: imx: Fix slave registration race and error handling

This patch fixes a race condition and error handling bug in slave
registration by moving the assignment of the slave pointer after a
successful resume and inside a lock.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cf5bacf3a4884..136e179d18b2d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -865,9 +865,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) {
> @@ -875,6 +872,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);

[Severity: High]
This is a pre-existing issue, but does executing i2c_imx_slave_init()
outside of the slave_lock expose partially initialized hardware state to
concurrent ISR execution?

If a shared interrupt fires between the lock release and the register reset
in i2c_imx_slave_init(), the ISR could acquire the lock, see i2c_imx->slave
is non-NULL, and act on stale I2SR_IIF flags left over from prior
operations.

Could this invoke i2c_imx_slave_handle() with garbage hardware state?

>  
>  	return 0;
>  }

While reviewing these locking changes, I noticed a few other patterns in the
driver that might need attention.

drivers/i2c/busses/i2c-imx.c:i2c_imx_unreg_slave() {
    ...
    i2c_imx->slave = NULL;
    ...
}

[Severity: High]
This is a pre-existing issue, but does the lockless nullification of
i2c_imx->slave in i2c_imx_unreg_slave() create a race condition with the
interrupt handler?

If the shared interrupt handler i2c_imx_isr() executes concurrently, it may
acquire slave_lock, verify i2c_imx->slave is non-NULL, and proceed. If
i2c_imx_unreg_slave() clears the pointer immediately after this check,
would the ISR pass a NULL pointer to i2c_imx_slave_handle(), causing a
direct NULL pointer dereference?


Also in the removal path:

drivers/i2c/busses/i2c-imx.c:i2c_imx_unreg_slave() {
    ...
    ret = pm_runtime_put_sync(i2c_imx->adapter.dev.parent);
    if (ret < 0)
        dev_err(&i2c_imx->adapter.dev, "failed to suspend i2c controller");

    return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but can this inconsistent
handling of pm_runtime_put_sync() cause spurious unregistration failures?

According to the Runtime PM documentation, pm_runtime_put_sync() returns 1
if the device was already suspended. However, i2c_slave_unregister() expects
exactly 0 for success.

Since unreg_slave() has already successfully detached the slave, does
returning 1 leave the I2C core thinking the slave is still active, failing
to clear client->slave_cb?


Regarding the ISR and power management:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr() {
    ...
    status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
    ...
}

drivers/i2c/busses/i2c-imx.c:i2c_imx_runtime_suspend() {
    ...
    clk_disable(i2c_imx->clk);
    ...
}

[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler access
hardware registers while the device might be runtime suspended?

If another device on the same IRQ line triggers an interrupt while the I2C
controller is runtime-suspended, i2c_imx_isr() unconditionally reads
IMX_I2C_I2SR without calling pm_runtime_get_if_active().

Additionally, the suspend callback disables the clock without calling
synchronize_irq() to ensure no ISR is mid-flight.

Could reading memory-mapped registers of a clock-gated device cause a
synchronous external abort or return garbage values?


Finally, looking at the DMA transfer error paths:

drivers/i2c/busses/i2c-imx.c:i2c_imx_dma_write() {
    ...
    if (time_left == 0) {
        dmaengine_terminate_sync(dma->chan_using);
        return -ETIMEDOUT;
    }
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but are the DMA mappings
permanently leaked on these timeout error paths?

Because the transfer is forcefully aborted, the async DMA completion
callback i2c_imx_dma_callback() is never executed. Since
dma_unmap_single() is only called inside that callback, does this result in
exhausting IOMMU space if timeouts occur?

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

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

[Carlos Song]

> -----Original Message-----
> From: Liem <liem16213@gmail.com>
> Sent: Monday, June 29, 2026 10:38 AM
> To: Carlos Song (OSS) <carlos.song@oss.nxp.com>
> Cc: andi.shyti@kernel.org; Biwen Li <biwen.li@nxp.com>; festevam@gmail.com;
> Frank Li <frank.li@nxp.com>; Frank Li (OSS) <frank.li@oss.nxp.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: [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@vger.kernel.org
> Signed-off-by: Liem <liem16213@gmail.com>

Hi, Liem

LGTM. Thank you very much.

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

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

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

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@nxp.com>

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

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

[Frank Li]

On Mon, Jun 29, 2026 at 10:38:29AM +0800, Liem wrote:
> 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
> Acked-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Liem <liem16213@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> v3 -> v4: No changes, added Acked-by from Carlos Song.
> ---
>  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 2398c406e913..b1c6581db774 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -960,6 +960,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 v4 1/2] i2c: imx: Fix slave registration race and error handling

[liem]

Hi,Frank Li!

I apologize for this. I didn't know that different versions required 
different threads. I did use `--in-reply-to` in the patch email. Do I 
need to resubmit the patch?

Regards,
Liem