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

[PATCH v3 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 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

[PATCH v3 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

[PATCH v3 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

[PATCH v3 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>
---
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

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

[Adrian Hunter]

On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
> 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
Some controller drivers do not set I3C_ERROR_M2 correctly:
  svc-i3c-master seems to set I3C_ERROR_M2 on all errors
  mipi-i3c-hci sets it also on error status 0x5: NACK: Address was NACK’ed

Others need to be checked

> arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried

What has section 5.1.2.2.3 got to do with I3C_ERROR_M2?

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

It would be better to contain individual CCC information in
the caller of i3c_master_send_ccc_cmd_locked().  Perhaps
add optional_bytes to struct i3c_ccc_cmd_payload:
	For I3C_CCC_GETMRL, optional_bytes = 1
	For I3C_CCC_GETMXDS, optional_bytes = 3

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

Simpler to add actual_len to struct i3c_ccc_cmd_payload and
amend controller drivers to use that.

> +			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 *

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

[NG, TZE YEE]

On 16/6/2026 3:41 pm, Adrian Hunter wrote:
> [You don't often get email from adrian.hunter@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
>> 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
> Some controller drivers do not set I3C_ERROR_M2 correctly:
>    svc-i3c-master seems to set I3C_ERROR_M2 on all errors
>    mipi-i3c-hci sets it also on error status 0x5: NACK: Address was NACK’ed
> 
> Others need to be checked
> 
I checked the same paths:
- svc-i3c-master sets I3C_ERROR_M2 on any CCC failure.
- mipi-i3c-hci maps RESP_ERR_NACK (0x5) to I3C_ERROR_M2.
So a generic core retry on I3C_ERROR_M2 is only as good as each driver's 
error reporting. Our DW series maps specific hardware status bits to 
M0/M2, but we agree the wider driver behaviour should be reviewed before 
relying on M2 retry across all masters.

We can either narrow this series to DW-only behaviour for now, or follow 
up with a separate audit/fix of M2 reporting in other drivers. Happy to 
take your preference.
>> arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
> 
> What has section 5.1.2.2.3 got to do with I3C_ERROR_M2?
> 
Section 5.1.2.2.3 describes the recovery behaviour for a transient 
address-header NACK (e.g. IBI/CRR arbitration), not the definition of 
I3C_ERROR_M2 itself.

M2 is the address-header NACK error code; §5.1.2.2.3 is one case where 
the spec says software should re-issue the transfer. We will reword the 
commit message to make that distinction clear and drop the implication 
that M2 is defined in that section.
>> 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;
> 
> It would be better to contain individual CCC information in
> the caller of i3c_master_send_ccc_cmd_locked().  Perhaps
> add optional_bytes to struct i3c_ccc_cmd_payload:
>          For I3C_CCC_GETMRL, optional_bytes = 1
>          For I3C_CCC_GETMXDS, optional_bytes = 3
> 
Agreed. The GETMRL/GETMXDS length rules are caller-specific and fit 
better at the call site than as hardcoded CCC IDs in 
i3c_master_send_ccc_cmd_locked().

Setting optional_bytes in the caller (e.g. getmrl_locked() → 1, 
getmxds_locked() → 3). We can respin v4 with that approach.>> +
>> +     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);
> 
> Simpler to add actual_len to struct i3c_ccc_cmd_payload and
> amend controller drivers to use that.
> 
Agreed. We will add actual_len to struct i3c_ccc_cmd_payload in v4.>> + 
                    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 *
> 

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

[Frank Li]

On Wed, Jun 10, 2026 at 06:54:06PM -0700, tze.yee.ng@altera.com wrote:
> 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;

not sure how req_len help it. Prevously everything copy into cmd.  now copy
req_len then copy to cmd,

No difference?

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

Only this line is validate.

Frank
> +	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
>  		ccc->err = I3C_ERROR_M2;
>
>  	return ret;
> --
> 2.43.7
>

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

[Frank Li]

On Wed, Jun 10, 2026 at 06:54:07PM -0700, tze.yee.ng@altera.com wrote:
> 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;
> +	}
> +}
> +

generaly this type map function like

enum i3c_error_code dw_i3c_ccc_map_err(int dw_err)
{
	....
}

ccc->err = dw_i3c_ccc_map_err(cmd->error);

Frank
>  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
>

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

[Frank Li]

On Wed, Jun 10, 2026 at 06:54:08PM -0700, tze.yee.ng@altera.com wrote:
> 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);

why need retry here?

Frank

> +		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
>

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

[Adrian Hunter]

On 19/06/2026 11:09, NG, TZE YEE wrote:
> On 16/6/2026 3:41 pm, Adrian Hunter wrote:
>> [You don't often get email from adrian.hunter@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
>>> 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
>> Some controller drivers do not set I3C_ERROR_M2 correctly:
>>    svc-i3c-master seems to set I3C_ERROR_M2 on all errors
>>    mipi-i3c-hci sets it also on error status 0x5: NACK: Address was NACK’ed
>>
>> Others need to be checked
>>
> I checked the same paths:
> - svc-i3c-master sets I3C_ERROR_M2 on any CCC failure.
> - mipi-i3c-hci maps RESP_ERR_NACK (0x5) to I3C_ERROR_M2.
> So a generic core retry on I3C_ERROR_M2 is only as good as each driver's 
> error reporting. Our DW series maps specific hardware status bits to 
> M0/M2, but we agree the wider driver behaviour should be reviewed before 
> relying on M2 retry across all masters.
> 
> We can either narrow this series to DW-only behaviour for now, or follow 
> up with a separate audit/fix of M2 reporting in other drivers. Happy to 
> take your preference.

I am wondering if we should ignore the nature of the error and just
retry after any error.

Also it seems like the number of retries should be passed by the caller,
so that different commands could be treated differently.  There is now
i3c_master_i3c_dev_present() which does its own retries.

>>> arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
>>
>> What has section 5.1.2.2.3 got to do with I3C_ERROR_M2?
>>
> Section 5.1.2.2.3 describes the recovery behaviour for a transient 
> address-header NACK (e.g. IBI/CRR arbitration), not the definition of 
> I3C_ERROR_M2 itself.
> 
> M2 is the address-header NACK error code; §5.1.2.2.3 is one case where 
> the spec says software should re-issue the transfer. We will reword the 
> commit message to make that distinction clear and drop the implication 
> that M2 is defined in that section.

The spec. has a different definition of M2:

"If the Master does not receive an ACK of a transmitted Broadcast Address (7’h7E),"

Whereas section 5.1.2.2.3 is about Frames starting with the target address.
That could be covered by DEV_NACK_RETRY_CNT (which we should change to
default to 1 not 0 by the way)

>>> 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;
>>
>> It would be better to contain individual CCC information in
>> the caller of i3c_master_send_ccc_cmd_locked().  Perhaps
>> add optional_bytes to struct i3c_ccc_cmd_payload:
>>          For I3C_CCC_GETMRL, optional_bytes = 1
>>          For I3C_CCC_GETMXDS, optional_bytes = 3
>>
> Agreed. The GETMRL/GETMXDS length rules are caller-specific and fit 
> better at the call site than as hardcoded CCC IDs in 
> i3c_master_send_ccc_cmd_locked().
> 
> Setting optional_bytes in the caller (e.g. getmrl_locked() → 1, 
> getmxds_locked() → 3). We can respin v4 with that approach.>> +
>>> +     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);
>>
>> Simpler to add actual_len to struct i3c_ccc_cmd_payload and
>> amend controller drivers to use that.
>>
> Agreed. We will add actual_len to struct i3c_ccc_cmd_payload in v4.>> + 
>                     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 *
>>
> 

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

[NG, TZE YEE]

On 20/6/2026 3:26 am, Frank Li wrote:
> On Wed, Jun 10, 2026 at 06:54:07PM -0700, tze.yee.ng@altera.com wrote:
>> 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;
>> +	}
>> +}
>> +
> 
> generaly this type map function like
> 
> enum i3c_error_code dw_i3c_ccc_map_err(int dw_err)
> {
> 	....
> }
> 
> ccc->err = dw_i3c_ccc_map_err(cmd->error);
> 
> Frank
Agree, in v4, we will change it to return enum i3c_error_code from the 
DW error code.>>   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
>>

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

[NG, TZE YEE]

On 20/6/2026 3:21 am, Frank Li wrote:
> On Wed, Jun 10, 2026 at 06:54:06PM -0700, tze.yee.ng@altera.com wrote:
>> 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;
> 
> not sure how req_len help it. Prevously everything copy into cmd.  now copy
> req_len then copy to cmd,
> 
> No difference?
> 
You are correct. Saving req_len here does nothing useful as payload.len 
is never getting overwritten. I will drop req_len in v4; keep using 
ccc->dests[0].payload.len when setting up cmd->rx_len / 
COMMAND_PORT_ARG_DATA_LEN.>>
>>   	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;
> 
> Only this line is validate.
> 
> Frank
>> +	if (cmd->error == RESPONSE_ERROR_IBA_NACK)
>>   		ccc->err = I3C_ERROR_M2;
>>
>>   	return ret;
>> --
>> 2.43.7
>>

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

[NG, TZE YEE]

On 23/6/2026 8:29 pm, Adrian Hunter wrote:
> On 19/06/2026 11:09, NG, TZE YEE wrote:
>> On 16/6/2026 3:41 pm, Adrian Hunter wrote:
>>> [You don't often get email from adrian.hunter@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
>>>> 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
>>> Some controller drivers do not set I3C_ERROR_M2 correctly:
>>>     svc-i3c-master seems to set I3C_ERROR_M2 on all errors
>>>     mipi-i3c-hci sets it also on error status 0x5: NACK: Address was NACK’ed
>>>
>>> Others need to be checked
>>>
>> I checked the same paths:
>> - svc-i3c-master sets I3C_ERROR_M2 on any CCC failure.
>> - mipi-i3c-hci maps RESP_ERR_NACK (0x5) to I3C_ERROR_M2.
>> So a generic core retry on I3C_ERROR_M2 is only as good as each driver's
>> error reporting. Our DW series maps specific hardware status bits to
>> M0/M2, but we agree the wider driver behaviour should be reviewed before
>> relying on M2 retry across all masters.
>>
>> We can either narrow this series to DW-only behaviour for now, or follow
>> up with a separate audit/fix of M2 reporting in other drivers. Happy to
>> take your preference.
> 
> I am wondering if we should ignore the nature of the error and just
> retry after any error.
> 
> Also it seems like the number of retries should be passed by the caller,
> so that different commands could be treated differently.  There is now
> i3c_master_i3c_dev_present() which does its own retries.
> 
On retry: agreed — we will drop M0/M2-based retry and retry on any error 
where a caller decides it is appropriate.
For retry count, we would like ask your preference between two approaches:
(i) Sysfs ccc_retry_count on the I3C master (similar to the existing 
dev_nack_retry_count): default 1, user-tunable via sysfs. Callers that 
need retries would read this from the master controller (or a shared 
helper would).
(ii) Caller-local constants (like your i3c_master_i3c_dev_present() 
pattern : each helper that needs retries defines its own MAX_CNT / 
backoff and loops around i3c_master_send_ccc_cmd_locked() — no new sysfs 
knob.
Which do you prefer?
>>>> arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
>>>
>>> What has section 5.1.2.2.3 got to do with I3C_ERROR_M2?
>>>
>> Section 5.1.2.2.3 describes the recovery behaviour for a transient
>> address-header NACK (e.g. IBI/CRR arbitration), not the definition of
>> I3C_ERROR_M2 itself.
>>
>> M2 is the address-header NACK error code; §5.1.2.2.3 is one case where
>> the spec says software should re-issue the transfer. We will reword the
>> commit message to make that distinction clear and drop the implication
>> that M2 is defined in that section.
> 
> The spec. has a different definition of M2:
> 
> "If the Master does not receive an ACK of a transmitted Broadcast Address (7’h7E),"
> 
> Whereas section 5.1.2.2.3 is about Frames starting with the target address.
> That could be covered by DEV_NACK_RETRY_CNT (which we should change to
> default to 1 not 0 by the way)
> 
Agreed. M2 in spec is the broadcast-address (7'h7E) NACK case, not 
address-header NACK error code. I will reword the commit message in v4.

For the DEV_NACK_RETRY_CNT default value, we will update it in separate 
patch.>>>> 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;
>>>
>>> It would be better to contain individual CCC information in
>>> the caller of i3c_master_send_ccc_cmd_locked().  Perhaps
>>> add optional_bytes to struct i3c_ccc_cmd_payload:
>>>           For I3C_CCC_GETMRL, optional_bytes = 1
>>>           For I3C_CCC_GETMXDS, optional_bytes = 3
>>>
>> Agreed. The GETMRL/GETMXDS length rules are caller-specific and fit
>> better at the call site than as hardcoded CCC IDs in
>> i3c_master_send_ccc_cmd_locked().
>>
>> Setting optional_bytes in the caller (e.g. getmrl_locked() → 1,
>> getmxds_locked() → 3). We can respin v4 with that approach.>> +
>>>> +     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);
>>>
>>> Simpler to add actual_len to struct i3c_ccc_cmd_payload and
>>> amend controller drivers to use that.
>>>
>> Agreed. We will add actual_len to struct i3c_ccc_cmd_payload in v4.>> +
>>                      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 *
>>>
>>
> 

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

[Adrian Hunter]

On 24/06/2026 11:28, NG, TZE YEE wrote:
> On 23/6/2026 8:29 pm, Adrian Hunter wrote:
>> On 19/06/2026 11:09, NG, TZE YEE wrote:
>>> On 16/6/2026 3:41 pm, Adrian Hunter wrote:
>>>> [You don't often get email from adrian.hunter@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>
>>>> On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
>>>>> 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
>>>> Some controller drivers do not set I3C_ERROR_M2 correctly:
>>>>     svc-i3c-master seems to set I3C_ERROR_M2 on all errors
>>>>     mipi-i3c-hci sets it also on error status 0x5: NACK: Address was NACK’ed
>>>>
>>>> Others need to be checked
>>>>
>>> I checked the same paths:
>>> - svc-i3c-master sets I3C_ERROR_M2 on any CCC failure.
>>> - mipi-i3c-hci maps RESP_ERR_NACK (0x5) to I3C_ERROR_M2.
>>> So a generic core retry on I3C_ERROR_M2 is only as good as each driver's
>>> error reporting. Our DW series maps specific hardware status bits to
>>> M0/M2, but we agree the wider driver behaviour should be reviewed before
>>> relying on M2 retry across all masters.
>>>
>>> We can either narrow this series to DW-only behaviour for now, or follow
>>> up with a separate audit/fix of M2 reporting in other drivers. Happy to
>>> take your preference.
>>
>> I am wondering if we should ignore the nature of the error and just
>> retry after any error.
>>
>> Also it seems like the number of retries should be passed by the caller,
>> so that different commands could be treated differently.  There is now
>> i3c_master_i3c_dev_present() which does its own retries.
>>
> On retry: agreed — we will drop M0/M2-based retry and retry on any error 
> where a caller decides it is appropriate.
> For retry count, we would like ask your preference between two approaches:
> (i) Sysfs ccc_retry_count on the I3C master (similar to the existing 
> dev_nack_retry_count): default 1, user-tunable via sysfs. Callers that 
> need retries would read this from the master controller (or a shared 
> helper would).

Let's not add sysfs knobs to start with.  If a platform or device needs
special treatment, it is better to configure that via device properties
or DMI quirks or some such.

> (ii) Caller-local constants (like your i3c_master_i3c_dev_present() 
> pattern : each helper that needs retries defines its own MAX_CNT / 
> backoff and loops around i3c_master_send_ccc_cmd_locked() — no new sysfs 
> knob.
> Which do you prefer?

Could define a default:

#define I3C_CCC_RETRIES		1

Perhaps add i3c_ccc_cmd_init_retries() like i3c_ccc_cmd_init() but with
a retries parameter, and then i3c_ccc_cmd_init() just calls
i3c_ccc_cmd_init_retries(..., I3C_CCC_RETRIES)

Call sites that need a different number of retries can call
i3c_ccc_cmd_init_retries() instead of i3c_ccc_cmd_init()

While we are only doing 1 retry anyway, maybe let's not bother with a
backoff.

>>>>> arbitration per I3C spec section 5.1.2.2.3). SET CCCs are not retried
>>>>
>>>> What has section 5.1.2.2.3 got to do with I3C_ERROR_M2?
>>>>
>>> Section 5.1.2.2.3 describes the recovery behaviour for a transient
>>> address-header NACK (e.g. IBI/CRR arbitration), not the definition of
>>> I3C_ERROR_M2 itself.
>>>
>>> M2 is the address-header NACK error code; §5.1.2.2.3 is one case where
>>> the spec says software should re-issue the transfer. We will reword the
>>> commit message to make that distinction clear and drop the implication
>>> that M2 is defined in that section.
>>
>> The spec. has a different definition of M2:
>>
>> "If the Master does not receive an ACK of a transmitted Broadcast Address (7’h7E),"
>>
>> Whereas section 5.1.2.2.3 is about Frames starting with the target address.
>> That could be covered by DEV_NACK_RETRY_CNT (which we should change to
>> default to 1 not 0 by the way)
>>
> Agreed. M2 in spec is the broadcast-address (7'h7E) NACK case, not 
> address-header NACK error code. I will reword the commit message in v4.
> 
> For the DEV_NACK_RETRY_CNT default value, we will update it in separate 
> patch.>>>> 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;
>>>>
>>>> It would be better to contain individual CCC information in
>>>> the caller of i3c_master_send_ccc_cmd_locked().  Perhaps
>>>> add optional_bytes to struct i3c_ccc_cmd_payload:
>>>>           For I3C_CCC_GETMRL, optional_bytes = 1
>>>>           For I3C_CCC_GETMXDS, optional_bytes = 3
>>>>
>>> Agreed. The GETMRL/GETMXDS length rules are caller-specific and fit
>>> better at the call site than as hardcoded CCC IDs in
>>> i3c_master_send_ccc_cmd_locked().
>>>
>>> Setting optional_bytes in the caller (e.g. getmrl_locked() → 1,
>>> getmxds_locked() → 3). We can respin v4 with that approach.>> +
>>>>> +     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);
>>>>
>>>> Simpler to add actual_len to struct i3c_ccc_cmd_payload and
>>>> amend controller drivers to use that.
>>>>
>>> Agreed. We will add actual_len to struct i3c_ccc_cmd_payload in v4.>> +
>>>                      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 *
>>>>
>>>
>>
> 

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

[NG, TZE YEE]

On 20/6/2026 3:28 am, Frank Li wrote:
> On Wed, Jun 10, 2026 at 06:54:08PM -0700, tze.yee.ng@altera.com wrote:
>> 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);
> 
> why need retry here?
> 
> Frank
> 
Hi Frank,

Retry is required for Direct GET CCC per section 5.1.9.2.3 (section 
5.2.1.2.3 in HDR): if a target cannot respond to a Direct GET CCC on the 
first attempt, the master shall follow the SDR Direct GET retry model 
(typically one retry); the target should generally respond on the second 
attempt. Section 5.2.1.2.3 states the same for HDR Direct GET CCC. For 
Direct SET CCC, decline may mean the CCC/defining byte is unsupported, 
so the retry model does not necessarily apply — we will not auto-retry 
SET.>> +		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
>>