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

[Adrian Hunter <adrian.hunter@intel.com>]

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 *


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