[PATCH v5 0/5] i3c: Improve CCC reliability with actual_len, validation, and Direct GET retry

[PATCH v5 0/5] i3c: Improve CCC reliability with actual_len, validation, and Direct GET retry

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

This series improves I3C Common Command Code (CCC) handling in the I3C
core and master drivers. It separates requested and received GET payload
lengths, validates GET responses centrally, and retries failed Direct GET
CCCs once by default.

The DesignWare I3C master driver reports standard I3C M0/M2 error codes so
the core can decide when a retry is appropriate. Variable-length GET CCC
rules (GETMRL, GETMXDS) are described by callers via optional_bytes.

Patch 1/5 adds actual_len to struct i3c_ccc_cmd_payload.

Patch 2/5 has all I3C master drivers set actual_len on successful GET
transfers and updates core GET helpers to interpret results from actual_len
instead of len.

Patch 3/5 maps DesignWare CCC hardware errors to I3C M0/M2. M2 is reported
only for broadcast address-header NACK (7'h7E). Target address NACK
returns -EIO.

Patch 4/5 adds retries to struct i3c_ccc_cmd, validates GET payload length
in i3c_master_send_ccc_cmd_locked() after a successful transfer, and
retries failed Direct GET CCCs when the driver reports failure or an I3C
error. Validation failures are not retried.

Patch 5/5 adds optional_bytes so callers describe variable-length GET CCC
responses. GETMRL and GETMXDS set optional_bytes at the call site.

Changes in v5:
- Split the series into five patches for clearer review:
  actual_len API, driver/core adoption, DW error mapping, core validation/
  retry, and optional_bytes.
- Fix regression in v4 patch 1: keep actual_len assignment in
  dw_i3c_ccc_get(), not dw_i3c_ccc_set().
- Move actual_len adoption in drivers/i3c/master.c into patch 2 so patch 2
  matches its title and works on its own.
- Split optional_bytes into its own patch (5/5); patch 4/5 adds strict
  exact-length validation and retry only.
- Do not retry on validation failure; retry only on driver/I3C errors.
- Reset actual_len before each GET retry only when cmd->rnw is set.
- Add @retries kernel-doc for struct i3c_ccc_cmd.
- Simplify i3c_ccc_validate_payload_len() per review (drop redundant
  exact-length check when optional_bytes == 0).
- Clarify GETMXDS fallback comment: with optional_bytes = 3, a 2-byte
  success on the first attempt no longer needs the shorter retry path.
- Add Reviewed-by: Alexandre Mergnat on patch 3/5.

Changes in v4:
- Add actual_len to keep requested and received lengths separate.
- Map M2 only for broadcast-address NACK; not for target-address NACK.
- Replace CCC-ID-specific validation with generic actual_len /
  optional_bytes checks.
- Retry Direct GET CCCs on any error (default once), not only M0/M2.
- Add optional_bytes and cmd->retries to ccc.h; drop req_len and
  payload.len save/restore in the core.
- Update SVC, Cadence, ADI, Renesas, and MIPI HCI master drivers.

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 (5):
  i3c: ccc: Add actual_len to struct i3c_ccc_cmd_payload
  i3c: master: 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 Direct GET once
  i3c: master: Add optional_bytes for variable-length GET CCC validation

 drivers/i3c/master.c                   | 97 ++++++++++++++++++++++----
 drivers/i3c/master/adi-i3c-master.c    |  2 +
 drivers/i3c/master/dw-i3c-master.c     | 27 ++++++-
 drivers/i3c/master/i3c-master-cdns.c   |  2 +
 drivers/i3c/master/mipi-i3c-hci/core.c |  5 +-
 drivers/i3c/master/renesas-i3c.c       |  2 +
 drivers/i3c/master/svc-i3c-master.c    |  4 +-
 include/linux/i3c/ccc.h                | 11 ++-
 8 files changed, 128 insertions(+), 22 deletions(-)

-- 
2.43.7

[PATCH v5 1/5] i3c: ccc: Add actual_len to struct i3c_ccc_cmd_payload

[tze.yee.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

Add actual_len to struct i3c_ccc_cmd_payload so drivers can report how
many bytes were received on a GET CCC without overwriting the requested
buffer length in len.

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 v5:
- Split from v4 patch 1/3: ccc.h only; no driver changes in this patch.
---
 include/linux/i3c/ccc.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index ad59a4ae60d1..d8052949e57e 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -343,11 +343,13 @@ struct i3c_ccc_getxtime {
 /**
  * struct i3c_ccc_cmd_payload - CCC payload
  *
- * @len: payload length
+ * @len: requested payload length
+ * @actual_len: number of bytes received on a GET CCC (filled by the driver)
  * @data: payload data. This buffer must be DMA-able
  */
 struct i3c_ccc_cmd_payload {
 	u16 len;
+	u16 actual_len;
 	void *data;
 };
 
-- 
2.43.7

[PATCH v5 2/5] i3c: master: Report actual GET CCC payload length on success

[tze.yee.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

Set dests[].payload.actual_len on successful GET CCC transfers in I3C
master drivers so the core can distinguish requested and received
buffer lengths. Switch core GET helpers to use actual_len instead of
len when interpreting GET CCC results.

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 v5:
- New patch scope: all I3C master drivers (adi, dw, cdns, renesas, svc,
  mipi-hci) set actual_len on successful GET transfers.
- dw_i3c_ccc_get() sets actual_len from cmd->rx_len; dw_i3c_ccc_set()
  does not touch actual_len (fixes v4 regression).
- Initialise actual_len to 0 in i3c_ccc_cmd_dest_init().
- Switch core GET helpers (getmrl, getmwl, getmxds, gethdrcap) to use
  actual_len instead of len when interpreting GET results.

Changes in v4:
- Refactor dw_i3c_ccc_map_err() to return enum i3c_error_code; callers
  assign ccc->err.
- Map I3C_ERROR_M2 only for RESPONSE_ERROR_IBA_NACK (broadcast address
  7'h7E not ACKed). Do not report target-address NACK as M2.
- Keep RESPONSE_ERROR_ADDRESS_NACK returning -EIO in
  dw_i3c_master_end_xfer_locked().
---
 drivers/i3c/master.c                   | 11 ++++++-----
 drivers/i3c/master/adi-i3c-master.c    |  2 ++
 drivers/i3c/master/dw-i3c-master.c     |  5 ++++-
 drivers/i3c/master/i3c-master-cdns.c   |  2 ++
 drivers/i3c/master/mipi-i3c-hci/core.c |  5 +++--
 drivers/i3c/master/renesas-i3c.c       |  2 ++
 drivers/i3c/master/svc-i3c-master.c    |  4 ++--
 7 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5cd4e5da2233..037b3b980717 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -901,6 +901,7 @@ static void *i3c_ccc_cmd_dest_init(struct i3c_ccc_cmd_dest *dest, u8 addr,
 {
 	dest->addr = addr;
 	dest->payload.len = payloadlen;
+	dest->payload.actual_len = 0;
 	if (payloadlen)
 		dest->payload.data = kzalloc(payloadlen, GFP_KERNEL);
 	else
@@ -1302,7 +1303,7 @@ static int i3c_master_getmrl_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto out;
 
-	switch (dest.payload.len) {
+	switch (dest.payload.actual_len) {
 	case 3:
 		info->max_ibi_len = mrl->ibi_len;
 		fallthrough;
@@ -1337,7 +1338,7 @@ static int i3c_master_getmwl_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto out;
 
-	if (dest.payload.len != sizeof(*mwl)) {
+	if (dest.payload.actual_len != sizeof(*mwl)) {
 		ret = -EIO;
 		goto out;
 	}
@@ -1376,14 +1377,14 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
 			goto out;
 	}
 
-	if (dest.payload.len != 2 && dest.payload.len != 5) {
+	if (dest.payload.actual_len != 2 && dest.payload.actual_len != 5) {
 		ret = -EIO;
 		goto out;
 	}
 
 	info->max_read_ds = getmaxds->maxrd;
 	info->max_write_ds = getmaxds->maxwr;
-	if (dest.payload.len == 5)
+	if (dest.payload.actual_len == 5)
 		info->max_read_turnaround = getmaxds->maxrdturn[0] |
 					    ((u32)getmaxds->maxrdturn[1] << 8) |
 					    ((u32)getmaxds->maxrdturn[2] << 16);
@@ -1412,7 +1413,7 @@ static int i3c_master_gethdrcap_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto out;
 
-	if (dest.payload.len != 1) {
+	if (dest.payload.actual_len != 1) {
 		ret = -EIO;
 		goto out;
 	}
diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
index 047081c9f064..64735b488726 100644
--- a/drivers/i3c/master/adi-i3c-master.c
+++ b/drivers/i3c/master/adi-i3c-master.c
@@ -360,6 +360,8 @@ static int adi_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 		adi_i3c_master_unqueue_xfer(master, xfer);
 
 	cmd->err = adi_i3c_cmd_get_err(&xfer->cmds[0]);
+	if (!xfer->ret && cmd->rnw)
+		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;
 
 	return xfer->ret;
 }
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 655693a2187e..6998ffd15f5f 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -780,7 +780,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.actual_len = cmd->rx_len;
+	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
 		ccc->err = I3C_ERROR_M2;
 
 	return ret;
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index 5cfec6761494..803c27983852 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -715,6 +715,8 @@ static int cdns_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
 
 	ret = xfer->ret;
 	cmd->err = cdns_i3c_cmd_get_err(&xfer->cmds[0]);
+	if (!ret && cmd->rnw)
+		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;
 	cdns_i3c_master_free_xfer(xfer);
 
 	return ret;
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index b781dbed2165..2b215658e093 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -326,7 +326,7 @@ static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
 		goto out;
 	for (i = prefixed; i < nxfers; i++) {
 		if (ccc->rnw)
-			ccc->dests[i - prefixed].payload.len =
+			ccc->dests[i - prefixed].payload.actual_len =
 				RESP_DATA_LENGTH(xfer[i].response);
 		switch (RESP_STATUS(xfer[i].response)) {
 		case RESP_SUCCESS:
@@ -343,7 +343,8 @@ static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
 
 	if (ccc->rnw)
 		dev_dbg(&hci->master.dev, "got: %*ph",
-			ccc->dests[0].payload.len, ccc->dests[0].payload.data);
+			ccc->dests[0].payload.actual_len,
+			ccc->dests[0].payload.data);
 
 out:
 	hci_free_xfer(xfer, nxfers);
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f39c449922ca..fec614700843 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -805,6 +805,8 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 	ret = xfer->ret;
 	if (ret)
 		ccc->err = I3C_ERROR_M2;
+	else if (ccc->rnw)
+		ccc->dests[0].payload.actual_len = cmd->rx_count;
 
 	return ret;
 }
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e2d99a3ac07d..c92d96e6b4d8 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1706,8 +1706,8 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 		svc_i3c_master_dequeue_xfer(master, xfer);
 	mutex_unlock(&master->lock);
 
-	if (cmd->actual_len != xfer_len)
-		ccc->dests[0].payload.len = cmd->actual_len;
+	if (ccc->rnw)
+		ccc->dests[0].payload.actual_len = cmd->actual_len;
 
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
-- 
2.43.7

Re: [PATCH v5 2/5] i3c: master: Report actual GET CCC payload length on success

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ADI and Cadence I3C master drivers spoof actual_len, hiding short reads and bypassing core validation.
- [High] Renesas I3C master driver leaves rx_count at 0 for small transfers, breaking device enumeration.
--

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

i3c: master: Report actual GET CCC payload length on success
    
Set dests[].payload.actual_len on successful GET CCC transfers in I3C
master drivers so the core can distinguish requested and received
buffer lengths. Switch core GET helpers to use actual_len instead of
len when interpreting GET CCC results.

[ ... ]

> diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
> index 047081c9f0643..64735b4887261 100644
> --- a/drivers/i3c/master/adi-i3c-master.c
> +++ b/drivers/i3c/master/adi-i3c-master.c
[ ... ]
> @@ -360,6 +360,8 @@ static int adi_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>  		adi_i3c_master_unqueue_xfer(master, xfer);
>  
>  	cmd->err = adi_i3c_cmd_get_err(&xfer->cmds[0]);
> +	if (!xfer->ret && cmd->rnw)
> +		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;

[Severity: High]
Does this unconditionally set actual_len to the requested length, discarding
the true hardware transfer size?

By blindly copying the requested length here, it seems this defeats the new GET
CCC validation logic. If a device returns 0 bytes or a short read, the core
validation could falsely pass, potentially causing the kernel to parse
uninitialized or zero-padded memory as valid device capabilities.

>  
>  	return xfer->ret;
>  }
[ ... ]
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 6d221596ea357..36998ee4073d7 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
[ ... ]
> @@ -714,6 +714,8 @@ static int cdns_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>  
>  	ret = xfer->ret;
>  	cmd->err = cdns_i3c_cmd_get_err(&xfer->cmds[0]);
> +	if (!ret && cmd->rnw)
> +		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;

[Severity: High]
Similar to the ADI driver, does this bypass the core validation by
unconditionally copying the requested length?

If a short read occurs, the driver will incorrectly report a full read instead
of the actual hardware byte count.

>  	cdns_i3c_master_free_xfer(xfer);
>  
>  	return ret;
[ ... ]
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..fec614700843f 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -805,6 +805,8 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
>  	ret = xfer->ret;
>  	if (ret)
>  		ccc->err = I3C_ERROR_M2;
> +	else if (ccc->rnw)
> +		ccc->dests[0].payload.actual_len = cmd->rx_count;

[Severity: High]
Does this leave actual_len at 0 for small transfers, breaking device
enumeration?

For small GET CCC reads, if the read buffer full interrupt doesn't
trigger, the transfer completes in renesas_i3c_resp_isr() where it drains the
remaining bytes from the FIFO but doesn't accumulate bytes_remaining into
cmd->rx_count:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
    ...
    bytes_remaining = data_len - cmd->rx_count;
    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
    ...
}

Since cmd->rx_count remains 0, won't this propagate an actual_len of 0 to the
core, causing the core validation to reject the transfer with -EIO?

>  
>  	return ret;
>  }

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

[PATCH v5 3/5] 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 I3C master CCC hardware errors to I3C M0/M2 error codes.
I3C_ERROR_M2 is reported only for broadcast address-header NACK
(RESPONSE_ERROR_IBA_NACK). Target address NACK remains -EIO.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
---
Changes in v5:
- Unchanged from v4 patch 2/3.
- Add Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>.
---
 drivers/i3c/master/dw-i3c-master.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 6998ffd15f5f..974b9fc79356 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,29 @@ static void dw_i3c_master_bus_cleanup(struct i3c_master_controller *m)
 	dw_i3c_master_disable(master);
 }
 
+static enum i3c_error_code dw_i3c_ccc_map_err(u8 dw_err)
+{
+	switch (dw_err) {
+	case RESPONSE_ERROR_IBA_NACK:
+		return I3C_ERROR_M2;
+	case RESPONSE_ERROR_CRC:
+	case RESPONSE_ERROR_PARITY:
+	case RESPONSE_ERROR_FRAME:
+	case RESPONSE_ERROR_TRANSF_ABORT:
+		return I3C_ERROR_M0;
+	default:
+		return I3C_ERROR_UNKNOWN;
+	}
+}
+
 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)
@@ -753,6 +770,8 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
 	struct dw_i3c_cmd *cmd;
 	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;
@@ -781,10 +800,9 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
 
 	ret = xfer->ret;
 	cmd = &xfer->cmds[0];
+	ccc->err = dw_i3c_ccc_map_err(cmd->error);
 	if (!ret)
 		ccc->dests[0].payload.actual_len = cmd->rx_len;
-	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
-		ccc->err = I3C_ERROR_M2;
 
 	return ret;
 }
-- 
2.43.7

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dw_i3c_ccc_set() fails to use dw_i3c_ccc_map_err(), resulting in inconsistent M0 error mapping for CCC SET commands
--

commit 0ce229b1e86f48f8bc79433bee08c7da0909bad8
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 I3C master CCC hardware errors to I3C M0/M2 error
codes. It introduces a new mapping function to handle this translation and
updates CCC commands to use it.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -781,10 +800,9 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
>  
>  	ret = xfer->ret;
>  	cmd = &xfer->cmds[0];
> +	ccc->err = dw_i3c_ccc_map_err(cmd->error);
>  	if (!ret)
>  		ccc->dests[0].payload.actual_len = cmd->rx_len;
> -	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
> -		ccc->err = I3C_ERROR_M2;

[Severity: Medium]
Should dw_i3c_ccc_set() also be updated to use the new dw_i3c_ccc_map_err()
function?

It looks like dw_i3c_ccc_get() was updated to map M0/M2 errors, but
dw_i3c_ccc_set() retains the legacy hardcoded check:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_ccc_set() {
...
	ret = xfer->ret;
	if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
		ccc->err = I3C_ERROR_M2;
...
}

Without using the new mapping function here, hardware errors (like CRC,
Parity, or Frame errors) during CCC SET commands will not be mapped to
I3C_ERROR_M0, resulting in inconsistent error handling compared to GET
commands.

>  
>  	return ret;
>  }

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

[PATCH v5 4/5] i3c: master: Validate GET CCC payload length and retry Direct GET once

[tze.yee.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

Add retries to struct i3c_ccc_cmd. Validate GET payload length in
i3c_master_send_ccc_cmd_locked() after a successful transfer.

Retry failed Direct GET CCCs up to cmd->retries times when the driver
reports failure or an I3C error; validation failures are not retried.
SET CCCs are not retried by default.

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 v5:
- Split from v4 patch 3/3: retry and strict validation only; optional_bytes
  moved to patch 5/5.
- Validate GET payload length after a successful transfer.
- Retry failed Direct GET CCCs when the driver reports failure or an I3C
  error; validation failures are not retried.

Changes in v4:
- Add optional_bytes to struct i3c_ccc_cmd_payload and retries to
  struct i3c_ccc_cmd (default I3C_CCC_RETRIES for GET, 0 for SET).
- Replace CCC-ID-specific payload checks with generic validation using
  actual_len, len, and optional_bytes.
- GETMRL and GETMXDS set optional_bytes at the call site instead of
  hardcoding lengths in the core.
- Retry failed Direct GET CCCs on any error up to cmd->retries times,
  per I3C spec Direct GET single-retry model (§5.1.9.2.3); drop M0/M2-
  gated retry logic.
- Drop req_lens save/restore and the stack/kmalloc bookkeeping for
  payload.len.
- Update I3C master drivers (SVC, Cadence, ADI, Renesas, MIPI HCI) to
  populate actual_len on successful GET transfers.
- Use actual_len in getmrl_locked(), getmwl_locked(), getmxds_locked(),
  and gethdrcap_locked().
---
 drivers/i3c/master.c    | 59 ++++++++++++++++++++++++++++++++++++++---
 include/linux/i3c/ccc.h |  5 ++++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 037b3b980717..3b6d3ecd9d12 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -915,17 +915,46 @@ static void i3c_ccc_cmd_dest_cleanup(struct i3c_ccc_cmd_dest *dest)
 	kfree(dest->payload.data);
 }
 
-static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
-			     struct i3c_ccc_cmd_dest *dests,
-			     unsigned int ndests)
+static void i3c_ccc_cmd_init_retries(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
+				     struct i3c_ccc_cmd_dest *dests,
+				     unsigned int ndests, unsigned int retries)
 {
 	cmd->rnw = rnw ? 1 : 0;
 	cmd->id = id;
 	cmd->dests = dests;
 	cmd->ndests = ndests;
+	cmd->retries = retries;
 	cmd->err = I3C_ERROR_UNKNOWN;
 }
 
+static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
+			     struct i3c_ccc_cmd_dest *dests,
+			     unsigned int ndests)
+{
+	i3c_ccc_cmd_init_retries(cmd, rnw, id, dests, ndests,
+				 rnw ? I3C_CCC_RETRIES : 0);
+}
+
+static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd)
+{
+	unsigned int i;
+
+	if (!cmd->rnw)
+		return 0;
+
+	for (i = 0; i < cmd->ndests; i++) {
+		struct i3c_ccc_cmd_payload *p = &cmd->dests[i].payload;
+
+		if (p->actual_len > p->len)
+			return -EIO;
+
+		if (p->len && p->actual_len != p->len)
+			return -EIO;
+	}
+
+	return 0;
+}
+
 /**
  * i3c_master_send_ccc_cmd_locked() - send a CCC (Common Command Codes)
  * @master: master used to send frames on the bus
@@ -937,6 +966,9 @@ 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)
 {
+	unsigned int attempt, max_attempts;
+	int ret;
+
 	if (!cmd || !master)
 		return -EINVAL;
 
@@ -954,7 +986,25 @@ 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);
+	max_attempts = cmd->retries + 1;
+	ret = -EIO;
+	for (attempt = 0; attempt < max_attempts; attempt++) {
+		unsigned int i;
+
+		if (cmd->rnw)
+			for (i = 0; i < cmd->ndests; i++)
+				cmd->dests[i].payload.actual_len = 0;
+
+		cmd->err = I3C_ERROR_UNKNOWN;
+		ret = master->ops->send_ccc_cmd(master, cmd);
+		if (!ret && cmd->err == I3C_ERROR_UNKNOWN)
+			break;
+	}
+
+	if (!ret)
+		ret = i3c_ccc_validate_payload_len(cmd);
+
+	return ret;
 }
 
 static struct i2c_dev_desc *
@@ -1372,6 +1422,7 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
 		 * while expecting shorter length from this CCC command.
 		 */
 		dest.payload.len -= 3;
+		i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
 		ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
 		if (ret)
 			goto out;
diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index d8052949e57e..2506d83b8255 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -12,6 +12,8 @@
 #include <linux/i3c/device.h>
 
 /* I3C CCC (Common Command Codes) related definitions */
+#define I3C_CCC_RETRIES	1
+
 #define I3C_CCC_DIRECT			BIT(7)
 
 #define I3C_CCC_ID(id, broadcast)	\
@@ -374,12 +376,15 @@ struct i3c_ccc_cmd_dest {
  * @ndests: number of destinations. Should always be one for broadcast commands
  * @dests: array of destinations and associated payload for this CCC. Most of
  *	   the time, only one destination is provided
+ * @retries: number of times to retry a failed Direct GET CCC (see
+ *	     &I3C_CCC_RETRIES)
  * @err: I3C error code
  */
 struct i3c_ccc_cmd {
 	u8 rnw;
 	u8 id;
 	unsigned int ndests;
+	unsigned int retries;
 	struct i3c_ccc_cmd_dest *dests;
 	enum i3c_error_code err;
 };
-- 
2.43.7

[PATCH v5 5/5] i3c: master: Add optional_bytes for variable-length GET CCC validation

[tze.yee.ng]

From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

Add optional_bytes to struct i3c_ccc_cmd_payload so callers describe
variable-length GET CCC responses. GETMRL and GETMXDS set optional_bytes
at the call site. Extend i3c_ccc_validate_payload_len() to honour it.

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 v5:
- Split from v4 patch 3/3: optional_bytes and variable-length GET rules only.
- Simplify validation: drop redundant exact-length check when
  optional_bytes == 0 (per review).
- Clarify GETMXDS fallback: with optional_bytes = 3, a 2-byte success on
  the first attempt no longer needs the fallback; it remains for when the
  5-byte request fails rather than short-reading.
---
 drivers/i3c/master.c    | 29 +++++++++++++++++++++++------
 include/linux/i3c/ccc.h |  2 ++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 3b6d3ecd9d12..12ca7c929ee5 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -902,6 +902,7 @@ static void *i3c_ccc_cmd_dest_init(struct i3c_ccc_cmd_dest *dest, u8 addr,
 	dest->addr = addr;
 	dest->payload.len = payloadlen;
 	dest->payload.actual_len = 0;
+	dest->payload.optional_bytes = 0;
 	if (payloadlen)
 		dest->payload.data = kzalloc(payloadlen, GFP_KERNEL);
 	else
@@ -944,11 +945,19 @@ static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd)
 
 	for (i = 0; i < cmd->ndests; i++) {
 		struct i3c_ccc_cmd_payload *p = &cmd->dests[i].payload;
+		u16 min_len;
+
+		if (p->optional_bytes > p->len)
+			return -EINVAL;
 
 		if (p->actual_len > p->len)
 			return -EIO;
 
-		if (p->len && p->actual_len != p->len)
+		if (!p->len)
+			continue;
+
+		min_len = p->len - p->optional_bytes;
+		if (p->actual_len < min_len)
 			return -EIO;
 	}
 
@@ -1342,10 +1351,14 @@ static int i3c_master_getmrl_locked(struct i3c_master_controller *master,
 		return -ENOMEM;
 
 	/*
-	 * When the device does not have IBI payload GETMRL only returns 2
-	 * bytes of data.
+	 * GETMRL returns 2 bytes (max read length) when the device does not
+	 * advertise IBI payload, or 2 or 3 bytes when it does (the optional
+	 * third byte is max IBI length). Use optional_bytes to allow either
+	 * length when IBI payload is supported.
 	 */
-	if (!(info->bcr & I3C_BCR_IBI_PAYLOAD))
+	if (info->bcr & I3C_BCR_IBI_PAYLOAD)
+		dest.payload.optional_bytes = 1;
+	else
 		dest.payload.len -= 1;
 
 	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMRL, &dest, 1);
@@ -1414,14 +1427,18 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
 	if (!getmaxds)
 		return -ENOMEM;
 
+	dest.payload.optional_bytes = 3;
+
 	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
 	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
 	if (ret) {
 		/*
-		 * Retry when the device does not support max read turnaround
-		 * while expecting shorter length from this CCC command.
+		 * optional_bytes = 3 accepts a 2-byte response on the first
+		 * attempt, so this fallback runs only when the 5-byte request
+		 * fails rather than returning a short read.
 		 */
 		dest.payload.len -= 3;
+		dest.payload.optional_bytes = 0;
 		i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
 		ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
 		if (ret)
diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index 2506d83b8255..7ad677baf761 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -347,11 +347,13 @@ struct i3c_ccc_getxtime {
  *
  * @len: requested payload length
  * @actual_len: number of bytes received on a GET CCC (filled by the driver)
+ * @optional_bytes: GET CCCs may return up to this many fewer bytes than @len
  * @data: payload data. This buffer must be DMA-able
  */
 struct i3c_ccc_cmd_payload {
 	u16 len;
 	u16 actual_len;
+	u16 optional_bytes;
 	void *data;
 };
 
-- 
2.43.7