[PATCH v2 0/3] i3c: Improve CCC reliability for DesignWare master

[PATCH v2 0/3] i3c: Improve CCC reliability for DesignWare master

[tze.yee.ng]

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 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 |  39 ++++++++--
 2 files changed, 143 insertions(+), 7 deletions(-)

-- 
2.43.7


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v2 1/3] i3c: master: dw: Report actual GET CCC payload length on success

[tze.yee.ng]

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

Re: [PATCH v2 1/3] i3c: master: dw: Report actual GET CCC payload length on success

[sashiko-bot]

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

Pre-existing issues:
- [High] Timeout of an active transfer stalls the hardware queue indefinitely for pending transfers, causing out-of-order execution.
- [High] The Dynamic Address Assignment (DAA) function ignores transfer errors and registers bogus phantom devices on timeout or NACK.
- [High] Premature completion of a newly enqueued transfer after a timeout due to uncleared IRQ status.
--

commit 617688804be1317c33b5ca391a037295f2427546
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 isn't a bug introduced by this patch, but does
dw_i3c_master_dequeue_xfer_locked() stall the hardware queue for pending
transfers?

When a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware queues and sets master->xferqueue.cur = NULL:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_dequeue_xfer_locked() {
    if (master->xferqueue.cur == xfer) {
        ...
        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 appear to advance the queue by popping the next transfer
from master->xferqueue.list and calling dw_i3c_master_start_xfer_locked().
Could this leave any pending transfers stranded indefinitely? A subsequently
enqueued transfer might bypass the stranded items because cur == NULL triggers
the immediate start of the new transfer, leading to out-of-order execution
and eventual failure of the stranded transfers via their own timeouts.

[Severity: High]
This isn't a bug introduced by this patch, but can an uncleared IRQ status
cause a premature completion of a newly enqueued transfer after a timeout?

If a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the queues
via RESET_CTRL but does not seem to clear pending interrupt flags in the
INTR_STATUS register.

If the interrupt from the timed-out transfer is still pending and a new
transfer is enqueued, making it master->xferqueue.cur, would the IRQ handler
falsely complete the current transfer?

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
    ...
    dw_i3c_master_end_xfer_locked(master, status);
}

It seems the handler might read nresp = 0 and then erroneously call
complete(&xfer->comp) with ret = 0, signaling that the newly enqueued transfer
has completed successfully before it has actually finished.

[Severity: High]
This isn't a bug introduced by this patch, but does dw_i3c_master_daa()
ignore transfer errors and register phantom devices on timeout or NACK?

In dw_i3c_master_daa(), the result of the ENTDAA transfer is processed without
checking the return value of xfer->ret:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_daa() {
    ...
    if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
        dw_i3c_master_dequeue_xfer(master, xfer);

    newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);
    newdevs &= ~olddevs;
}

If the transfer times out, xfer->ret is set to -ETIMEDOUT and cmd->rx_len
remains 0. The mask then falsely encompasses all available unassigned device
slots, which could deceive the driver into thinking all devices were
successfully assigned. Does this flood the I3C subsystem with non-existent
phantom devices?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780999511.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

[PATCH v2 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2

[tze.yee.ng]

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

Re: [PATCH v2 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2

[sashiko-bot]

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

Pre-existing issues:
- [High] dw_i3c_master_daa() ignores the transfer error status and uses an uninitialized receive length, leading to spurious registration of dummy devices on failure or timeout.
- [High] The driver blindly trusts hardware response counts and Transaction IDs (TID) from the I3C controller, leading to out-of-bounds array access and arbitrary memory corruption.
- [High] The driver blindly trusts the response data length from the hardware and copies it into the receive buffer, risking a buffer overflow.
- [High] The IRQ handler prematurely completes ongoing I3C transfers when unrelated interrupts (such as In-Band Interrupts) occur, bypassing hardware state verification.
- [High] If a transfer times out, subsequent queued transfers are left indefinitely stalled, and any new transfer will execute out-of-order.
--

commit 4df4fe8c6ced05d7c1108a9f52a2ac16c9feeede
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

i3c: master: dw: Map CCC hardware errors to I3C M0/M2

This commit maps DesignWare response-queue status to standard I3C error
codes in ccc->err and returns -EIO for bus NACKs so they can be retried by
the core.

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

[Severity: High]
This isn't a regression introduced by this patch, but while reviewing
dw_i3c_master_end_xfer_locked(), could the hardware response TID cause an
out-of-bounds access?

Looking at the loop earlier in this function:

dw_i3c_master_end_xfer_locked()
    ...
    resp = readl(master->regs + RESPONSE_QUEUE_PORT);
    cmd = &xfer->cmds[RESPONSE_PORT_TID(resp)];

Since RESPONSE_PORT_TID(resp) is used directly to index the xfer->cmds
array without bounds checking, could a hardware fault or arbitrary response
trigger an out-of-bounds heap access?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential buffer
overflow in the same loop:

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

Since cmd->rx_len is overwritten with the hardware response length and
passed to dw_i3c_master_read_rx_fifo() without validation, could a response
length larger than the allocated cmd->rx_buf cause a heap buffer overflow?

[Severity: High]
This isn't a regression introduced by this patch, but does the IRQ handler
prematurely complete transfers?

If an In-Band Interrupt (IBI) triggers while a transfer is active:

dw_i3c_master_irq_handler()
    ...
    spin_lock(&master->xferqueue.lock);
    dw_i3c_master_end_xfer_locked(master, status);

dw_i3c_master_end_xfer_locked() doesn't verify if the interrupt was a
response completion. If nresp is 0, the function still proceeds to
unconditionally call complete(&xfer->comp) and start the next transfer.

Does this prematurely abort hardware synchronization for the active
transfer?

[ ... ]

[Severity: High]
This is a pre-existing issue in dw_i3c_master_daa(), but does it register
phantom devices on timeout?

If wait_for_completion_timeout() fails, cmd->rx_len remains uninitialized.
Then the calculation:

dw_i3c_master_daa()
    ...
    newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);
    newdevs &= ~olddevs;

    for (pos = 0; pos < master->maxdevs; pos++) {
        if (newdevs & BIT(pos))
            i3c_master_add_i3c_dev_locked(m, master->devs[pos].addr);
    }

Since cmd->rx_len is 0, GENMASK evaluates to setting all remaining slots in
newdevs, spuriously registering non-existent devices. Is there a missing
check for the transfer error status here?

[Severity: High]
This isn't a regression introduced by this patch, but what happens to
queued transfers if one times out?

Looking at dw_i3c_master_dequeue_xfer_locked():

dw_i3c_master_dequeue_xfer_locked()
    if (master->xferqueue.cur == xfer) {
        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);
    ...
    } else {
        list_del_init(&xfer->node);
    }

If the current transfer is cancelled, the hardware queue is reset and
master->xferqueue.cur is set to NULL, but it fails to pop the next pending
transfer from the wait queue and restart execution.

Doesn't this stall subsequent queued transfers indefinitely, and allow new
transfers to jump the line since cur is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780999511.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

[PATCH v2 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once

[tze.yee.ng]

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>
---
 drivers/i3c/master.c               | 111 ++++++++++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c |   2 +-
 2 files changed, 111 insertions(+), 2 deletions(-)

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 *
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 45bde92d0342..fbd1c899e728 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -493,7 +493,6 @@ 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:
@@ -503,6 +502,7 @@ 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;
-- 
2.43.7


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Address NACKs are incorrectly mapped to -EINVAL instead of a hardware error code (-EIO or -ENXIO), breaking standard subsystem error reporting conventions for standard transfers.
--

commit f0f1354839be36f55ac51ec9d8d9c182653a6497
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

i3c: master: Validate GET CCC payload length and retry M0/M2 once

Validate GET CCC payload length after a successful transfer. Treat a
short read as I3C_ERROR_M0 and return -EIO. Retry GET CCCs once on
retriable errors.

[ ... ]
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index c4004f87c6857..a8b5ebabd3508 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -493,7 +493,6 @@ 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:
> @@ -503,6 +502,7 @@ 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;

[Severity: High]
Does moving RESPONSE_ERROR_ADDRESS_NACK to the default case incorrectly map
address NACKs to -EINVAL instead of a hardware error code like -EIO or
-ENXIO?

This function serves as the generic completion handler for all transfers,
including standard I3C and I2C private reads and writes. An address NACK
usually indicates an absent, disconnected, or busy device.

Returning -EINVAL for a hardware NACK during a data transfer diverges from
subsystem conventions. Can this cause userspace tools like i2cdetect or
core probing logic to misinterpret an absent device as a fatal invalid
parameter error, rather than allowing graceful fallbacks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780999511.git.tze.yee.ng@altera.com?part=3

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2 3/3] i3c: master: Validate GET CCC payload length and retry M0/M2 once

[NG, TZE YEE]

On 9/6/2026 6:30 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Address NACKs are incorrectly mapped to -EINVAL instead of a hardware error code (-EIO or -ENXIO), breaking standard subsystem error reporting conventions for standard transfers.
> --
> 
> commit f0f1354839be36f55ac51ec9d8d9c182653a6497
> Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
> 
> i3c: master: Validate GET CCC payload length and retry M0/M2 once
> 
> Validate GET CCC payload length after a successful transfer. Treat a
> short read as I3C_ERROR_M0 and return -EIO. Retry GET CCCs once on
> retriable errors.
> 
> [ ... ]
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index c4004f87c6857..a8b5ebabd3508 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -493,7 +493,6 @@ 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:
>> @@ -503,6 +502,7 @@ 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;
> 
> [Severity: High]
> Does moving RESPONSE_ERROR_ADDRESS_NACK to the default case incorrectly map
> address NACKs to -EINVAL instead of a hardware error code like -EIO or
> -ENXIO?
> 
> This function serves as the generic completion handler for all transfers,
> including standard I3C and I2C private reads and writes. An address NACK
> usually indicates an absent, disconnected, or busy device.
> 
> Returning -EINVAL for a hardware NACK during a data transfer diverges from
> subsystem conventions. Can this cause userspace tools like i2cdetect or
> core probing logic to misinterpret an absent device as a fatal invalid
> parameter error, rather than allowing graceful fallbacks?
> 
I will submit v3 to move RESPONSE_ERROR_ADDRESS_NACK to return -EIO 
(alongside IBA_NACK) in patch 2/3 and drop the accidental revert in 
patch 3/3.

Regards,
Tze Yee
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c