[PATCH] i3c: master: svc: report timeout waiting for STOP idle

[PATCH] i3c: master: svc: report timeout waiting for STOP idle

[Pengpeng Hou]

svc_i3c_master_xfer() emits STOP or force-exit for the final transfer
and then waits for the controller state to become idle, but ignores
readl_poll_timeout(). The function can therefore return success while
the controller is still not idle.

Return the idle-wait error through the existing warning/FIFO cleanup
path so the caller observes the failed transfer without emitting a
second STOP after the final STOP or force-exit has already been sent.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/i3c/master/svc-i3c-master.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 93805df8a940..93ae6eb1a355 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1488,8 +1488,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 			svc_i3c_master_emit_force_exit(master);
 
 		/* Wait idle if stop is sent. */
-		readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
-				   SVC_I3C_MSTATUS_STATE_IDLE(reg), 0, 1000);
+		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+					 SVC_I3C_MSTATUS_STATE_IDLE(reg),
+					 0, 1000);
+		if (ret)
+			goto cleanup;
 	}
 
 	return 0;
@@ -1502,6 +1505,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	else
 		svc_i3c_master_emit_force_exit(master);
 
+cleanup:
 	svc_i3c_master_clear_merrwarn(master);
 	svc_i3c_master_flush_fifo(master);
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing synchronization between IBI processing and device detachment causes Use-After-Free.
- [Critical] Stack Buffer Overflow during Dynamic Address Assignment (DAA).
- [Critical] Heap Buffer Overflow in IBI payload reading loop.
- [High] Use of sleeping polling macro in atomic/hardirq context.
- [High] DDR write payload is written twice to the FIFO when SVC_I3C_QUIRK_FIFO_EMPTY is active.
- [High] Hard lockup (infinite loop) in hardirq handler if hardware reports RX pending without bytes.
- [High] Endianness Portability Bug (Mixed I/O accessors on FIFO).
--

commit f5ce1c99dd938b33fc88fc935174301f58c2a87a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

i3c: master: svc: report timeout waiting for STOP idle

The commit modifies svc_i3c_master_xfer() to capture the return value
of readl_poll_timeout() when waiting for the controller state to become
idle. It routes the error through the cleanup path to ensure the caller
observes the failed transfer.

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 93805df8a9406..ae38ceee5abf8 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1488,8 +1488,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  			svc_i3c_master_emit_force_exit(master);
>  
>  		/* Wait idle if stop is sent. */
> -		readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> -				   SVC_I3C_MSTATUS_STATE_IDLE(reg), 0, 1000);
> +		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +					 SVC_I3C_MSTATUS_STATE_IDLE(reg),
> +					 0, 1000);

[Severity: High]
This isn't a bug introduced by this patch, but does this code use a
sleeping polling macro in an atomic context?

The readl_poll_timeout macro uses ktime_get() for non-zero timeouts.
Calling this from an atomic context, such as while holding a spinlock
with interrupts disabled, can cause kernel deadlocks when
CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

This also appears in svc_i3c_master_handle_ibi() where
readl_relaxed_poll_timeout() is used inside a hardirq handler.

> +		if (ret)
> +			goto cleanup;
>  	}
>  
>  	return 0;
> @@ -1500,6 +1503,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	else
>  		svc_i3c_master_emit_force_exit(master);
>  
> +cleanup:
>  	svc_i3c_master_clear_merrwarn(master);
>  	svc_i3c_master_flush_fifo(master);

While reviewing this file, I noticed a few other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but does this code write the DDR payload to
the FIFO twice when the SVC_I3C_QUIRK_FIFO_EMPTY quirk is active?

In svc_i3c_master_xfer(), if the transfer is SVC_I3C_MCTRL_TYPE_DDR and
is a write, the full payload is pushed into the FIFO:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
    if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
        writel(rnw_cmd, master->regs + SVC_I3C_MWDATAB);
        if (!rnw) {
            ret = svc_i3c_master_write(master, out, xfer_len);
    ...
}

However, xfer_len is not modified. Later, the quirk workaround triggers
and pushes the same payload bytes into the FIFO again:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
        if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
            ...
            writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code mix stream and register
I/O accessors on the same FIFO, potentially causing data corruption on
big-endian architectures?

The subsystem architecture guidelines explicitly forbid mixing accessors
like writesb() and writel() on the same FIFO address because register
accessors perform implicit byte-swapping while stream accessors do not.

In svc_i3c_master_xfer(), both are mixed for bulk and remainder writes:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
            writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
            /* Mark END bit if this is the last byte */
            writel(out[len - 1] | end, master->regs + SVC_I3C_MWDATAB);
    ...
}

Similarly, readsb() is used in svc_i3c_master_handle_ibi() on the RX FIFO,
whereas svc_i3c_master_read() uses readl() on the same FIFO.

[Severity: Critical]
This is a pre-existing issue, but is there missing synchronization between
IBI processing and device detachment that could cause a use-after-free?

In svc_i3c_master_ibi_isr(), the device descriptor is fetched under
master->ibi.lock, but the lock is dropped before handling the IBI:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_ibi_isr() {
    ...
    spin_lock(&master->ibi.lock);
    dev = master->ibi.slots[ibiaddr];
    spin_unlock(&master->ibi.lock);
    ...
    svc_i3c_master_handle_ibi(master, dev);
    ...
}

If the I3C core detaches the device concurrently,
svc_i3c_master_detach_i3c_dev() can free the device data without
synchronizing against the IRQ handler:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_detach_i3c_dev() {
    ...
    kfree(data);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this code lack bounds checking on
dev_nb, potentially causing a stack buffer overflow?

The svc_i3c_master_do_daa_locked() function defines fixed-size arrays on
the stack for up to SVC_I3C_MAX_DEVS (32) devices:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_do_daa_locked() {
    u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
    unsigned int dev_nb = 0, last_addr = 0, dyn_addr = 0;
    ...
}

The loop assigns addresses to responding devices and writes to these arrays
using dev_nb without verifying it hasn't exceeded the array bounds:

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_do_daa_locked() {
    ...
    addrs[dev_nb] = dyn_addr;
    ...
    dev_nb++;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does the loop draining the hardware
RX FIFO use a hardcoded bound that could exceed the dynamically allocated
buffer size?

The slot->data buffer is allocated based on the device's max_payload_len,
which can be small. However, the loop condition checks against
SVC_I3C_FIFO_SIZE (16 bytes):

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_handle_ibi() {
    ...
    while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
           slot->len < SVC_I3C_FIFO_SIZE) {
        ...
        count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
        readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
        slot->len += count;
        ...
}

Because the check evaluates slot->len < 16 before adding count (which
can also be up to 16 bytes), up to 31 bytes could be written to a buffer
that might only be 1 or 2 bytes long.

[Severity: High]
This is a pre-existing issue, but could this loop cause a hard lockup in
the hardirq handler if the hardware reports RXPEND is true but RXCOUNT
is 0?

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_handle_ibi() {
    ...
    while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) &&
           slot->len < SVC_I3C_FIFO_SIZE) {
        ...
        count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
        readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
        slot->len += count;
        buf += count;
    }
    ...
}

The loop relies entirely on the hardware state to terminate. If RXCOUNT
is 0, count will be 0, slot->len will not increase, and the loop will
spin indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623060821.23238-1-pengpeng@iscas.ac.cn?part=1

Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle

[Miquel Raynal]

Hello,

On 23/06/2026 at 14:08:21 +08, Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> svc_i3c_master_xfer() emits STOP or force-exit for the final transfer
> and then waits for the controller state to become idle, but ignores
> readl_poll_timeout(). The function can therefore return success while
> the controller is still not idle.
>
> Return the idle-wait error through the existing warning/FIFO cleanup
> path so the caller observes the failed transfer without emitting a
> second STOP after the final STOP or force-exit has already been sent.
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle

[Frank Li]

On Tue, Jun 23, 2026 at 02:08:21PM +0800, Pengpeng Hou wrote:
>
> svc_i3c_master_xfer() emits STOP or force-exit for the final transfer
> and then waits for the controller state to become idle, but ignores
> readl_poll_timeout(). The function can therefore return success while
> the controller is still not idle.
>
> Return the idle-wait error through the existing warning/FIFO cleanup
> path so the caller observes the failed transfer without emitting a
> second STOP after the final STOP or force-exit has already been sent.
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---

Dose you really met timeout? if yes, what step to reproduce it. Suppose
only few FCLK to emit STOP or force-exit unconditional.

Any way not harmful to add check it.

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

>  drivers/i3c/master/svc-i3c-master.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 93805df8a940..93ae6eb1a355 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1488,8 +1488,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>                         svc_i3c_master_emit_force_exit(master);
>
>                 /* Wait idle if stop is sent. */
> -               readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> -                                  SVC_I3C_MSTATUS_STATE_IDLE(reg), 0, 1000);
> +               ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +                                        SVC_I3C_MSTATUS_STATE_IDLE(reg),
> +                                        0, 1000);
> +               if (ret)
> +                       goto cleanup;
>         }
>
>         return 0;
> @@ -1502,6 +1505,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>         else
>                 svc_i3c_master_emit_force_exit(master);
>
> +cleanup:
>         svc_i3c_master_clear_merrwarn(master);
>         svc_i3c_master_flush_fifo(master);
>
> --
> 2.50.1 (Apple Git-155)
>
>