* Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle
2026-06-23 6:08 [PATCH] i3c: master: svc: report timeout waiting for STOP idle Pengpeng Hou
@ 2026-06-23 6:25 ` sashiko-bot
2026-06-23 9:16 ` Miquel Raynal
2026-06-23 14:23 ` Frank Li
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-06-23 6:25 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: Frank.Li, imx, linux-i3c
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i3c: master: svc: report timeout waiting for STOP idle
2026-06-23 6:08 [PATCH] i3c: master: svc: report timeout waiting for STOP idle Pengpeng Hou
2026-06-23 6:25 ` sashiko-bot
2026-06-23 9:16 ` Miquel Raynal
@ 2026-06-23 14:23 ` Frank Li
2 siblings, 0 replies; 4+ messages in thread
From: Frank Li @ 2026-06-23 14:23 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Miquel Raynal, Frank Li, Alexandre Belloni, linux-i3c, imx,
linux-kernel
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)
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread