* [PATCH v3 0/3] i3c: Improve CCC reliability for DesignWare master
@ 2026-06-11 1:54 ` tze.yee.ng
0 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Tze Yee Ng <tze.yee.ng@altera.com>
Improve I3C CCC handling on the DesignWare master used on SoCFPGA
platforms: report the actual GET payload length, map hardware errors
to I3C M0/M2, and validate GET responses with a single retry for
transient failures.
Patch 1/3 fixes a DW driver bug: on successful GET CCC, set
dests[0].payload.len from RESPONSE_PORT_DATA_LEN.
Patch 2/3 maps DesignWare response-queue errors to ccc->err (M2 for
IBA/address NACK; M0 for CRC/parity/frame/transfer-abort).
Patch 3/3 moves protocol handling into the I3C core: validate GET
payload length (GETMRL: 2 or 3 bytes; GETMXDS: 2 or 5 bytes), retry
GET CCCs once on M0/M2, restore requested payload.len on retry/error,
and use a stack buffer for the common single-destination GET case.
Changes in v3:
- In dw_i3c_master_end_xfer_locked(), move RESPONSE_ERROR_ADDRESS_NACK to
return -EIO.
Changes in v2:
- Split the monolithic patch into three patches (per review feedback).
- Move GET payload validation and CCC retry from the DW driver to
drivers/i3c/master.c.
- Validate GET CCCs only; drop SET payload-length checks (DW
RESPONSE_PORT_DATA_LEN is 0 on SET).
- Retry GET CCCs only; do not repeat side-effecting SET CCCs.
- Tighten GETMRL validation to exactly 2 or 3 bytes; add GETMXDS
2/5-byte handling.
- Expand M0 mapping to CRC/parity/transfer-abort, not only frame
errors.
- Restore dests[].payload.len before retry and on error return.
- Avoid kmalloc on the common single-destination GET path.
Adrian Ng Ho Yin (3):
i3c: master: dw: Report actual GET CCC payload length on success
i3c: master: dw: Map CCC hardware errors to I3C M0/M2
i3c: master: Validate GET CCC payload length and retry M0/M2 once
drivers/i3c/master.c | 111 ++++++++++++++++++++++++++++-
drivers/i3c/master/dw-i3c-master.c | 41 +++++++++--
2 files changed, 144 insertions(+), 8 deletions(-)
--
2.43.7
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] i3c: master: dw: Report actual GET CCC payload length on success
2026-06-11 1:54 ` tze.yee.ng
@ 2026-06-11 1:54 ` tze.yee.ng
-1 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
On successful GET CCC transfers, set dests[0].payload.len from
RESPONSE_PORT_DATA_LEN so the I3C core receives the number of bytes
actually read. Core helpers such as i3c_master_getmrl_locked() use
dest.payload.len after the transfer to interpret the response.
Save the requested length in a local variable before programming the
hardware so the caller's buffer size is not conflated with the bytes
received.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 655693a2187e..06fdf8857b9c 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -751,21 +751,24 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
{
struct dw_i3c_cmd *cmd;
+ u16 req_len;
int ret, pos;
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
return pos;
+ req_len = ccc->dests[0].payload.len;
+
struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, 1);
if (!xfer)
return -ENOMEM;
cmd = xfer->cmds;
cmd->rx_buf = ccc->dests[0].payload.data;
- cmd->rx_len = ccc->dests[0].payload.len;
+ cmd->rx_len = req_len;
- cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(ccc->dests[0].payload.len) |
+ cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(req_len) |
COMMAND_PORT_TRANSFER_ARG;
cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER |
@@ -780,7 +783,10 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
dw_i3c_master_dequeue_xfer(master, xfer);
ret = xfer->ret;
- if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
+ cmd = &xfer->cmds[0];
+ if (!ret)
+ ccc->dests[0].payload.len = cmd->rx_len;
+ if (cmd->error == RESPONSE_ERROR_IBA_NACK)
ccc->err = I3C_ERROR_M2;
return ret;
--
2.43.7
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 1/3] i3c: master: dw: Report actual GET CCC payload length on success
@ 2026-06-11 1:54 ` tze.yee.ng
0 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
On successful GET CCC transfers, set dests[0].payload.len from
RESPONSE_PORT_DATA_LEN so the I3C core receives the number of bytes
actually read. Core helpers such as i3c_master_getmrl_locked() use
dest.payload.len after the transfer to interpret the response.
Save the requested length in a local variable before programming the
hardware so the caller's buffer size is not conflated with the bytes
received.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
drivers/i3c/master/dw-i3c-master.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 655693a2187e..06fdf8857b9c 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -751,21 +751,24 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
{
struct dw_i3c_cmd *cmd;
+ u16 req_len;
int ret, pos;
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
return pos;
+ req_len = ccc->dests[0].payload.len;
+
struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, 1);
if (!xfer)
return -ENOMEM;
cmd = xfer->cmds;
cmd->rx_buf = ccc->dests[0].payload.data;
- cmd->rx_len = ccc->dests[0].payload.len;
+ cmd->rx_len = req_len;
- cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(ccc->dests[0].payload.len) |
+ cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(req_len) |
COMMAND_PORT_TRANSFER_ARG;
cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER |
@@ -780,7 +783,10 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
dw_i3c_master_dequeue_xfer(master, xfer);
ret = xfer->ret;
- if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
+ cmd = &xfer->cmds[0];
+ if (!ret)
+ ccc->dests[0].payload.len = cmd->rx_len;
+ if (cmd->error == RESPONSE_ERROR_IBA_NACK)
ccc->err = I3C_ERROR_M2;
return ret;
--
2.43.7
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/3] i3c: master: dw: Report actual GET CCC payload length on success
2026-06-11 1:54 ` tze.yee.ng
(?)
@ 2026-06-11 2:08 ` sashiko-bot
-1 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-11 2:08 UTC (permalink / raw)
To: tze.yee.ng; +Cc: linux-i3c, Frank.Li
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Heap/stack buffer overflow when reading from the hardware RX FIFO.
- [High] Transfer queue stall and cascading timeouts after a single transfer timeout.
- [High] Spurious completion of new transfers due to unhandled late interrupts from previous timeouts.
--
commit 097603b8b0f1e852cd40f174d2e0bd01af652c87
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
i3c: master: dw: Report actual GET CCC payload length on success
On successful GET CCC transfers, set dests[0].payload.len from
RESPONSE_PORT_DATA_LEN so the I3C core receives the number of bytes
actually read. Core helpers such as i3c_master_getmrl_locked() use
dest.payload.len after the transfer to interpret the response.
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..e42cf3ddcec8f 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -780,7 +783,10 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
> dw_i3c_master_dequeue_xfer(master, xfer);
[Severity: High]
This is a pre-existing issue, but does dw_i3c_master_dequeue_xfer_locked()
properly restart the queue after a timeout?
When a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware and clears master->xferqueue.cur:
drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_dequeue_xfer_locked() {
...
master->xferqueue.cur = NULL;
writel(RESET_CTRL_RX_FIFO | RESET_CTRL_TX_FIFO |
RESET_CTRL_RESP_QUEUE | RESET_CTRL_CMD_QUEUE,
master->regs + RESET_CTRL);
...
}
However, it doesn't seem to dequeue the next transfer from
master->xferqueue.list and start it.
Because cur is NULL and no code advances the queue, will all pending transfers
remain stranded and individually time out unless a new transfer is enqueued?
[Severity: High]
This is a pre-existing issue, but can unhandled late interrupts from a timeout
cause spurious completions for new transfers?
If a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware queues, dropping nresp to 0, but it does not clear latched interrupt
statuses like INTR_TRANSFER_ERR_STAT.
If a new transfer is enqueued before the delayed IRQ handler acquires the
lock, the ISR will fire based on the old interrupt.
drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_end_xfer_locked() {
...
nresp = readl(master->regs + QUEUE_STATUS_LEVEL);
nresp = QUEUE_STATUS_LEVEL_RESP(nresp);
for (i = 0; i < nresp; i++) {
// ... skipped if nresp == 0
...
}
dw_i3c_master_end_xfer_locked() reads nresp = 0 and skips the error parsing
loop, leaving ret = 0.
Will this erroneously complete the newly enqueued transfer with a success
status without actually executing it on the bus?
>
> ret = xfer->ret;
> - if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
> + cmd = &xfer->cmds[0];
> + if (!ret)
> + ccc->dests[0].payload.len = cmd->rx_len;
[Severity: Critical]
This is a pre-existing issue, but is there a risk of a buffer overflow when
reading from the hardware RX FIFO?
In dw_i3c_master_end_xfer_locked(), the driver reads the response length from
the hardware and directly overwrites cmd->rx_len:
drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_end_xfer_locked() {
...
cmd->rx_len = RESPONSE_PORT_DATA_LEN(resp);
cmd->error = RESPONSE_PORT_ERR_STATUS(resp);
if (cmd->rx_len && !cmd->error)
dw_i3c_master_read_rx_fifo(master, cmd->rx_buf,
cmd->rx_len);
...
}
The length from the hardware is not validated against the originally requested
buffer size.
If a peripheral sends more data than requested, could this directly overflow
the cmd->rx_buf into adjacent memory?
It seems the payload length validation introduced in subsequent patches of this
series only checks the length after the buffer has already been overwritten.
> + if (cmd->error == RESPONSE_ERROR_IBA_NACK)
> ccc->err = I3C_ERROR_M2;
>
> return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781142158.git.tze.yee.ng@altera.com?part=1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2
2026-06-11 1:54 ` tze.yee.ng
@ 2026-06-11 1:54 ` tze.yee.ng
-1 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Map DesignWare response-queue status to standard I3C error codes in
ccc->err:
- RESPONSE_ERROR_IBA_NACK and RESPONSE_ERROR_ADDRESS_NACK -> I3C_ERROR_M2
- RESPONSE_ERROR_CRC, RESPONSE_ERROR_PARITY, RESPONSE_ERROR_FRAME and
RESPONSE_ERROR_TRANSF_ABORT -> I3C_ERROR_M0
Return -EIO for RESPONSE_ERROR_ADDRESS_NACK so bus NACKs are not reported
as -EINVAL alongside I3C_ERROR_M2, consistent with IBA_NACK handling and
other I3C master drivers.
The M0 mappings match the generic I/O failures already reported by
dw_i3c_master_end_xfer_locked() so the core can retry transient bus
integrity errors on CCC transfers.
Reset ccc->err to I3C_ERROR_UNKNOWN before each transfer.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
drivers/i3c/master/dw-i3c-master.c | 31 +++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 06fdf8857b9c..45bde92d0342 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -493,6 +493,7 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
break;
case RESPONSE_ERROR_PARITY:
case RESPONSE_ERROR_IBA_NACK:
+ case RESPONSE_ERROR_ADDRESS_NACK:
case RESPONSE_ERROR_TRANSF_ABORT:
case RESPONSE_ERROR_CRC:
case RESPONSE_ERROR_FRAME:
@@ -502,7 +503,6 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
ret = -ENOSPC;
break;
case RESPONSE_ERROR_I2C_W_NACK_ERR:
- case RESPONSE_ERROR_ADDRESS_NACK:
default:
ret = -EINVAL;
break;
@@ -708,12 +708,32 @@ static void dw_i3c_master_bus_cleanup(struct i3c_master_controller *m)
dw_i3c_master_disable(master);
}
+static void dw_i3c_ccc_map_err(struct i3c_ccc_cmd *ccc, struct dw_i3c_cmd *cmd)
+{
+ switch (cmd->error) {
+ case RESPONSE_ERROR_IBA_NACK:
+ case RESPONSE_ERROR_ADDRESS_NACK:
+ ccc->err = I3C_ERROR_M2;
+ break;
+ case RESPONSE_ERROR_CRC:
+ case RESPONSE_ERROR_PARITY:
+ case RESPONSE_ERROR_FRAME:
+ case RESPONSE_ERROR_TRANSF_ABORT:
+ ccc->err = I3C_ERROR_M0;
+ break;
+ default:
+ break;
+ }
+}
+
static int dw_i3c_ccc_set(struct dw_i3c_master *master,
struct i3c_ccc_cmd *ccc)
{
struct dw_i3c_cmd *cmd;
int ret, pos = 0;
+ ccc->err = I3C_ERROR_UNKNOWN;
+
if (ccc->id & I3C_CCC_DIRECT) {
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
@@ -742,8 +762,8 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
dw_i3c_master_dequeue_xfer(master, xfer);
ret = xfer->ret;
- if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
- ccc->err = I3C_ERROR_M2;
+ cmd = &xfer->cmds[0];
+ dw_i3c_ccc_map_err(ccc, cmd);
return ret;
}
@@ -754,6 +774,8 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
u16 req_len;
int ret, pos;
+ ccc->err = I3C_ERROR_UNKNOWN;
+
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
return pos;
@@ -784,10 +806,9 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
ret = xfer->ret;
cmd = &xfer->cmds[0];
+ dw_i3c_ccc_map_err(ccc, cmd);
if (!ret)
ccc->dests[0].payload.len = cmd->rx_len;
- if (cmd->error == RESPONSE_ERROR_IBA_NACK)
- ccc->err = I3C_ERROR_M2;
return ret;
}
--
2.43.7
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2
@ 2026-06-11 1:54 ` tze.yee.ng
0 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Map DesignWare response-queue status to standard I3C error codes in
ccc->err:
- RESPONSE_ERROR_IBA_NACK and RESPONSE_ERROR_ADDRESS_NACK -> I3C_ERROR_M2
- RESPONSE_ERROR_CRC, RESPONSE_ERROR_PARITY, RESPONSE_ERROR_FRAME and
RESPONSE_ERROR_TRANSF_ABORT -> I3C_ERROR_M0
Return -EIO for RESPONSE_ERROR_ADDRESS_NACK so bus NACKs are not reported
as -EINVAL alongside I3C_ERROR_M2, consistent with IBA_NACK handling and
other I3C master drivers.
The M0 mappings match the generic I/O failures already reported by
dw_i3c_master_end_xfer_locked() so the core can retry transient bus
integrity errors on CCC transfers.
Reset ccc->err to I3C_ERROR_UNKNOWN before each transfer.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
drivers/i3c/master/dw-i3c-master.c | 31 +++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 06fdf8857b9c..45bde92d0342 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -493,6 +493,7 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
break;
case RESPONSE_ERROR_PARITY:
case RESPONSE_ERROR_IBA_NACK:
+ case RESPONSE_ERROR_ADDRESS_NACK:
case RESPONSE_ERROR_TRANSF_ABORT:
case RESPONSE_ERROR_CRC:
case RESPONSE_ERROR_FRAME:
@@ -502,7 +503,6 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
ret = -ENOSPC;
break;
case RESPONSE_ERROR_I2C_W_NACK_ERR:
- case RESPONSE_ERROR_ADDRESS_NACK:
default:
ret = -EINVAL;
break;
@@ -708,12 +708,32 @@ static void dw_i3c_master_bus_cleanup(struct i3c_master_controller *m)
dw_i3c_master_disable(master);
}
+static void dw_i3c_ccc_map_err(struct i3c_ccc_cmd *ccc, struct dw_i3c_cmd *cmd)
+{
+ switch (cmd->error) {
+ case RESPONSE_ERROR_IBA_NACK:
+ case RESPONSE_ERROR_ADDRESS_NACK:
+ ccc->err = I3C_ERROR_M2;
+ break;
+ case RESPONSE_ERROR_CRC:
+ case RESPONSE_ERROR_PARITY:
+ case RESPONSE_ERROR_FRAME:
+ case RESPONSE_ERROR_TRANSF_ABORT:
+ ccc->err = I3C_ERROR_M0;
+ break;
+ default:
+ break;
+ }
+}
+
static int dw_i3c_ccc_set(struct dw_i3c_master *master,
struct i3c_ccc_cmd *ccc)
{
struct dw_i3c_cmd *cmd;
int ret, pos = 0;
+ ccc->err = I3C_ERROR_UNKNOWN;
+
if (ccc->id & I3C_CCC_DIRECT) {
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
@@ -742,8 +762,8 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
dw_i3c_master_dequeue_xfer(master, xfer);
ret = xfer->ret;
- if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
- ccc->err = I3C_ERROR_M2;
+ cmd = &xfer->cmds[0];
+ dw_i3c_ccc_map_err(ccc, cmd);
return ret;
}
@@ -754,6 +774,8 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
u16 req_len;
int ret, pos;
+ ccc->err = I3C_ERROR_UNKNOWN;
+
pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
if (pos < 0)
return pos;
@@ -784,10 +806,9 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
ret = xfer->ret;
cmd = &xfer->cmds[0];
+ dw_i3c_ccc_map_err(ccc, cmd);
if (!ret)
ccc->dests[0].payload.len = cmd->rx_len;
- if (cmd->error == RESPONSE_ERROR_IBA_NACK)
- ccc->err = I3C_ERROR_M2;
return ret;
}
--
2.43.7
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2
2026-06-11 1:54 ` tze.yee.ng
(?)
@ 2026-06-11 2:08 ` sashiko-bot
-1 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-11 2:08 UTC (permalink / raw)
To: tze.yee.ng; +Cc: Frank.Li, linux-i3c
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Premature transfer completion and queue desynchronization due to missing completion checks in the ISR.
- [High] When a transfer times out, the dequeue function does not start the next pending transfer, causing all queued transfers to stall and timeout.
- [High] The `dw_i3c_master_daa` function ignores the result of the ENTDAA transfer and `xfer->ret`. If the transfer times out, it misinterprets this as a successful assignment and instantiates phantom I3C devices.
- [High] A timed-out transfer leaves uncleared error interrupt status in the hardware, which can cause a subsequent transfer to be falsely completed with success.
--
commit 5f1a48847f4d3d2352ca97a610c727bc9bbd9942
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
i3c: master: dw: Map CCC hardware errors to I3C M0/M2
This patch maps DesignWare response-queue status errors to standard I3C
error codes in ccc->err. It also ensures ccc->err is reset to
I3C_ERROR_UNKNOWN before each transfer.
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index e42cf3ddcec8f..c4004f87c6857 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -493,6 +493,7 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)
[Severity: Critical]
This is a pre-existing issue, but I have a question about how this function
interacts with the interrupt handler.
If dw_i3c_master_irq_handler() unconditionally calls
dw_i3c_master_end_xfer_locked() for any unmasked interrupt, including
INTR_IBI_THLD_STAT (In-Band Interrupt), could this lead to an issue when
a transfer is still executing?
When nresp is 0, the response loop finishes without errors, ret remains 0,
and it calls complete(&xfer->comp).
Could this prematurely abort the current transfer on the software side,
reporting it as completely successful while the hardware is still performing
DMA, potentially leading to data corruption or use-after-free on transfer
buffers?
> break;
> case RESPONSE_ERROR_PARITY:
> case RESPONSE_ERROR_IBA_NACK:
> + case RESPONSE_ERROR_ADDRESS_NACK:
> case RESPONSE_ERROR_TRANSF_ABORT:
> case RESPONSE_ERROR_CRC:
> case RESPONSE_ERROR_FRAME:
[ ... ]
> @@ -742,8 +762,8 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
> dw_i3c_master_dequeue_xfer(master, xfer);
>
[Severity: High]
This is a pre-existing issue, but I am curious about what happens when this
timeout occurs and dw_i3c_master_dequeue_xfer() is called.
In dw_i3c_master_dequeue_xfer_locked(), when a timed-out transfer is
cancelled, it nullifies master->xferqueue.cur and resets the hardware
queues. However, it does not pop the next transfer from
master->xferqueue.list or issue a DEV_CTRL_RESUME command.
Does this strand all previously queued transfers so they eventually time
out? And if a new transfer is enqueued, will it see cur == NULL, bypass
the stranded transfers, and cause out-of-order execution?
[Severity: High]
Additionally, this is another pre-existing issue on the same timeout path:
when the hardware queues are reset, it appears INTR_TRANSFER_ERR_STAT is
not cleared from the hardware's interrupt status register.
When a new transfer is enqueued and started, could the pending error
interrupt immediately trigger dw_i3c_master_irq_handler()?
Since the queue was reset, nresp would be 0. The loop over responses would
execute 0 times, leaving ret = 0. This seems like it would falsely signal
that the new transfer completed successfully. Is this sequence possible?
> ret = xfer->ret;
> - if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
> - ccc->err = I3C_ERROR_M2;
> + cmd = &xfer->cmds[0];
> + dw_i3c_ccc_map_err(ccc, cmd);
>
> return ret;
> }
[ ... ]
[Severity: High]
This is a pre-existing issue, but observing the timeout handling here brings
up a question about dw_i3c_master_daa() in the same driver.
If wait_for_completion_timeout() fails during an ENTDAA transfer in
dw_i3c_master_daa(), cmd->rx_len remains 0. The function appears to ignore
xfer->ret and computes:
newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);
With rx_len == 0, the mask covers all maxdevs slots. Intersecting this with
unassigned slots seems to register a phantom device for every empty slot on
the bus, and then returns 0 to falsely signal a successful DAA procedure.
Should dw_i3c_master_daa() check the result of the transfer to avoid
instantiating phantom I3C devices on a timeout?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781142158.git.tze.yee.ng@altera.com?part=2
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once
2026-06-11 1:54 ` tze.yee.ng
@ 2026-06-11 1:54 ` tze.yee.ng
-1 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Validate GET CCC payload length after a successful transfer. Treat a
short read as I3C_ERROR_M0 and return -EIO. GETMRL accepts exactly 2 or
3 bytes per the I3C spec defined formats. GETMXDS may return 2 bytes
(format 1) or 5 bytes (format 2) per I3C spec.
Retry GET CCCs once on retriable errors: I3C_ERROR_M0 (frame error) and
I3C_ERROR_M2 (address-header NACK, e.g. IBI or Controller Role Request
arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
to avoid repeating side-effecting commands. Restore dests[].payload.len
to the originally requested length before each attempt and again before
returning an error, so callers that adjust the length on failure (e.g.
i3c_master_getmxds_locked()) do not underflow a shortened value.
Use a stack buffer for the common single-destination GET case and only
kmalloc when ndests > 1.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v3:
- Drop the change that moves RESPONSE_ERROR_ADDRESS_NACK to default case
in dw_i3c_master_end_xfer_locked(). Now dw_i3c_master_end_xfer_locked()
returns -EIO for RESPONSE_ERROR_ADDRESS_NACK.
---
drivers/i3c/master.c | 111 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 110 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5cd4e5da2233..c94d37cd8b3f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -26,6 +26,12 @@ static DEFINE_MUTEX(i3c_core_lock);
static int __i3c_first_dynamic_bus_num;
static BLOCKING_NOTIFIER_HEAD(i3c_bus_notifier);
+#define I3C_CCC_GETMRL_LEN_SHORT 2
+#define I3C_CCC_GETMRL_LEN_FULL 3
+#define I3C_CCC_GETMXDS_LEN_SHORT 2
+#define I3C_CCC_GETMXDS_LEN_FULL 5
+#define I3C_CCC_MAX_RETRIES 2
+
/**
* i3c_bus_maintenance_lock - Lock the bus for a maintenance operation
* @bus: I3C bus to take the lock on
@@ -925,6 +931,61 @@ static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
cmd->err = I3C_ERROR_UNKNOWN;
}
+static bool i3c_ccc_get_payload_ok(u8 id, u16 req_len, u16 actual_len)
+{
+ if (actual_len > req_len)
+ return false;
+
+ if (!req_len)
+ return actual_len == 0;
+
+ if (id == I3C_CCC_GETMRL)
+ return actual_len == I3C_CCC_GETMRL_LEN_SHORT ||
+ actual_len == I3C_CCC_GETMRL_LEN_FULL;
+
+ if (id == I3C_CCC_GETMXDS)
+ return actual_len == I3C_CCC_GETMXDS_LEN_SHORT ||
+ actual_len == I3C_CCC_GETMXDS_LEN_FULL;
+
+ return actual_len == req_len;
+}
+
+static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd,
+ const u16 *req_lens)
+{
+ unsigned int i;
+
+ if (!cmd->rnw)
+ return 0;
+
+ for (i = 0; i < cmd->ndests; i++) {
+ u16 actual = cmd->dests[i].payload.len;
+ u16 req = req_lens[i];
+
+ if (!i3c_ccc_get_payload_ok(cmd->id, req, actual)) {
+ cmd->err = I3C_ERROR_M0;
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * M0: transient frame errors.
+ * M2: address-header NACK (I3C spec section 5.1.2.2.3), e.g. when a target
+ * simultaneously asserts an IBI or Controller Role Request and neither
+ * side ACKs. Software should re-issue the transfer; the controller wins
+ * arbitration after Repeated START.
+ *
+ * Retries apply to GET CCCs only; SET CCCs are not retried to avoid
+ * repeating side-effecting commands.
+ */
+static bool i3c_ccc_err_retriable(enum i3c_error_code err)
+{
+ return err == I3C_ERROR_M0 || err == I3C_ERROR_M2;
+}
+
/**
* i3c_master_send_ccc_cmd_locked() - send a CCC (Common Command Codes)
* @master: master used to send frames on the bus
@@ -936,9 +997,17 @@ static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
struct i3c_ccc_cmd *cmd)
{
+ u16 req_len;
+ u16 *req_lens = NULL;
+ u16 *req_lens_alloc = NULL;
+ unsigned int i;
+ int ret, retries;
+
if (!cmd || !master)
return -EINVAL;
+ retries = cmd->rnw ? I3C_CCC_MAX_RETRIES : 1;
+
if (WARN_ON(master->init_done &&
!rwsem_is_locked(&master->bus.lock)))
return -EINVAL;
@@ -953,7 +1022,47 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
!master->ops->supports_ccc_cmd(master, cmd))
return -EOPNOTSUPP;
- return master->ops->send_ccc_cmd(master, cmd);
+ if (cmd->rnw && cmd->dests && cmd->ndests) {
+ if (cmd->ndests == 1) {
+ req_len = cmd->dests[0].payload.len;
+ req_lens = &req_len;
+ } else {
+ req_lens_alloc = kmalloc_array(cmd->ndests,
+ sizeof(*req_lens_alloc),
+ GFP_KERNEL);
+ if (!req_lens_alloc)
+ return -ENOMEM;
+
+ req_lens = req_lens_alloc;
+ for (i = 0; i < cmd->ndests; i++)
+ req_lens[i] = cmd->dests[i].payload.len;
+ }
+ }
+
+ do {
+ cmd->err = I3C_ERROR_UNKNOWN;
+ if (req_lens) {
+ for (i = 0; i < cmd->ndests; i++)
+ cmd->dests[i].payload.len = req_lens[i];
+ }
+ ret = master->ops->send_ccc_cmd(master, cmd);
+ if (!ret && req_lens)
+ ret = i3c_ccc_validate_payload_len(cmd, req_lens);
+ } while (--retries && ret && i3c_ccc_err_retriable(cmd->err));
+
+ if (ret && req_lens) {
+ /*
+ * Drivers may update payload.len to the actual RX count;
+ * restore the requested length so callers can safely adjust
+ * it on error (e.g. i3c_master_getmxds_locked()).
+ */
+ for (i = 0; i < cmd->ndests; i++)
+ cmd->dests[i].payload.len = req_lens[i];
+ }
+
+ kfree(req_lens_alloc);
+
+ return ret;
}
static struct i2c_dev_desc *
--
2.43.7
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once
@ 2026-06-11 1:54 ` tze.yee.ng
0 siblings, 0 replies; 10+ messages in thread
From: tze.yee.ng @ 2026-06-11 1:54 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Adrian Ng Ho Yin, Felix Gu,
Wolfram Sang, Manikanta Guntupalli, Jorge Marques, Sakari Ailus,
linux-i3c, linux-kernel
From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Validate GET CCC payload length after a successful transfer. Treat a
short read as I3C_ERROR_M0 and return -EIO. GETMRL accepts exactly 2 or
3 bytes per the I3C spec defined formats. GETMXDS may return 2 bytes
(format 1) or 5 bytes (format 2) per I3C spec.
Retry GET CCCs once on retriable errors: I3C_ERROR_M0 (frame error) and
I3C_ERROR_M2 (address-header NACK, e.g. IBI or Controller Role Request
arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
to avoid repeating side-effecting commands. Restore dests[].payload.len
to the originally requested length before each attempt and again before
returning an error, so callers that adjust the length on failure (e.g.
i3c_master_getmxds_locked()) do not underflow a shortened value.
Use a stack buffer for the common single-destination GET case and only
kmalloc when ndests > 1.
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v3:
- Drop the change that moves RESPONSE_ERROR_ADDRESS_NACK to default case
in dw_i3c_master_end_xfer_locked(). Now dw_i3c_master_end_xfer_locked()
returns -EIO for RESPONSE_ERROR_ADDRESS_NACK.
---
drivers/i3c/master.c | 111 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 110 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5cd4e5da2233..c94d37cd8b3f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -26,6 +26,12 @@ static DEFINE_MUTEX(i3c_core_lock);
static int __i3c_first_dynamic_bus_num;
static BLOCKING_NOTIFIER_HEAD(i3c_bus_notifier);
+#define I3C_CCC_GETMRL_LEN_SHORT 2
+#define I3C_CCC_GETMRL_LEN_FULL 3
+#define I3C_CCC_GETMXDS_LEN_SHORT 2
+#define I3C_CCC_GETMXDS_LEN_FULL 5
+#define I3C_CCC_MAX_RETRIES 2
+
/**
* i3c_bus_maintenance_lock - Lock the bus for a maintenance operation
* @bus: I3C bus to take the lock on
@@ -925,6 +931,61 @@ static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
cmd->err = I3C_ERROR_UNKNOWN;
}
+static bool i3c_ccc_get_payload_ok(u8 id, u16 req_len, u16 actual_len)
+{
+ if (actual_len > req_len)
+ return false;
+
+ if (!req_len)
+ return actual_len == 0;
+
+ if (id == I3C_CCC_GETMRL)
+ return actual_len == I3C_CCC_GETMRL_LEN_SHORT ||
+ actual_len == I3C_CCC_GETMRL_LEN_FULL;
+
+ if (id == I3C_CCC_GETMXDS)
+ return actual_len == I3C_CCC_GETMXDS_LEN_SHORT ||
+ actual_len == I3C_CCC_GETMXDS_LEN_FULL;
+
+ return actual_len == req_len;
+}
+
+static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd,
+ const u16 *req_lens)
+{
+ unsigned int i;
+
+ if (!cmd->rnw)
+ return 0;
+
+ for (i = 0; i < cmd->ndests; i++) {
+ u16 actual = cmd->dests[i].payload.len;
+ u16 req = req_lens[i];
+
+ if (!i3c_ccc_get_payload_ok(cmd->id, req, actual)) {
+ cmd->err = I3C_ERROR_M0;
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * M0: transient frame errors.
+ * M2: address-header NACK (I3C spec section 5.1.2.2.3), e.g. when a target
+ * simultaneously asserts an IBI or Controller Role Request and neither
+ * side ACKs. Software should re-issue the transfer; the controller wins
+ * arbitration after Repeated START.
+ *
+ * Retries apply to GET CCCs only; SET CCCs are not retried to avoid
+ * repeating side-effecting commands.
+ */
+static bool i3c_ccc_err_retriable(enum i3c_error_code err)
+{
+ return err == I3C_ERROR_M0 || err == I3C_ERROR_M2;
+}
+
/**
* i3c_master_send_ccc_cmd_locked() - send a CCC (Common Command Codes)
* @master: master used to send frames on the bus
@@ -936,9 +997,17 @@ static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
struct i3c_ccc_cmd *cmd)
{
+ u16 req_len;
+ u16 *req_lens = NULL;
+ u16 *req_lens_alloc = NULL;
+ unsigned int i;
+ int ret, retries;
+
if (!cmd || !master)
return -EINVAL;
+ retries = cmd->rnw ? I3C_CCC_MAX_RETRIES : 1;
+
if (WARN_ON(master->init_done &&
!rwsem_is_locked(&master->bus.lock)))
return -EINVAL;
@@ -953,7 +1022,47 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
!master->ops->supports_ccc_cmd(master, cmd))
return -EOPNOTSUPP;
- return master->ops->send_ccc_cmd(master, cmd);
+ if (cmd->rnw && cmd->dests && cmd->ndests) {
+ if (cmd->ndests == 1) {
+ req_len = cmd->dests[0].payload.len;
+ req_lens = &req_len;
+ } else {
+ req_lens_alloc = kmalloc_array(cmd->ndests,
+ sizeof(*req_lens_alloc),
+ GFP_KERNEL);
+ if (!req_lens_alloc)
+ return -ENOMEM;
+
+ req_lens = req_lens_alloc;
+ for (i = 0; i < cmd->ndests; i++)
+ req_lens[i] = cmd->dests[i].payload.len;
+ }
+ }
+
+ do {
+ cmd->err = I3C_ERROR_UNKNOWN;
+ if (req_lens) {
+ for (i = 0; i < cmd->ndests; i++)
+ cmd->dests[i].payload.len = req_lens[i];
+ }
+ ret = master->ops->send_ccc_cmd(master, cmd);
+ if (!ret && req_lens)
+ ret = i3c_ccc_validate_payload_len(cmd, req_lens);
+ } while (--retries && ret && i3c_ccc_err_retriable(cmd->err));
+
+ if (ret && req_lens) {
+ /*
+ * Drivers may update payload.len to the actual RX count;
+ * restore the requested length so callers can safely adjust
+ * it on error (e.g. i3c_master_getmxds_locked()).
+ */
+ for (i = 0; i < cmd->ndests; i++)
+ cmd->dests[i].payload.len = req_lens[i];
+ }
+
+ kfree(req_lens_alloc);
+
+ return ret;
}
static struct i2c_dev_desc *
--
2.43.7
^ permalink raw reply related [flat|nested] 10+ messages in thread