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

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Jul 08, 2026 at 12:17:40AM -0700, tze.yee.ng@altera.com wrote:
> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
>
> Add retries to struct i3c_ccc_cmd. Validate GET payload length in
> i3c_master_send_ccc_cmd_locked() after a successful transfer.
>
> Retry failed Direct GET CCCs up to cmd->retries times when the driver
> reports failure or an I3C error; validation failures are not retried.
> SET CCCs are not retried by default.
>
> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v5:
> - Split from v4 patch 3/3: retry and strict validation only; optional_bytes
>   moved to patch 5/5.
> - Validate GET payload length after a successful transfer.
> - Retry failed Direct GET CCCs when the driver reports failure or an I3C
>   error; validation failures are not retried.
>
> Changes in v4:
> - Add optional_bytes to struct i3c_ccc_cmd_payload and retries to
>   struct i3c_ccc_cmd (default I3C_CCC_RETRIES for GET, 0 for SET).
> - Replace CCC-ID-specific payload checks with generic validation using
>   actual_len, len, and optional_bytes.
> - GETMRL and GETMXDS set optional_bytes at the call site instead of
>   hardcoding lengths in the core.
> - Retry failed Direct GET CCCs on any error up to cmd->retries times,
>   per I3C spec Direct GET single-retry model (§5.1.9.2.3); drop M0/M2-
>   gated retry logic.
> - Drop req_lens save/restore and the stack/kmalloc bookkeeping for
>   payload.len.
> - Update I3C master drivers (SVC, Cadence, ADI, Renesas, MIPI HCI) to
>   populate actual_len on successful GET transfers.
> - Use actual_len in getmrl_locked(), getmwl_locked(), getmxds_locked(),
>   and gethdrcap_locked().
> ---
>  drivers/i3c/master.c    | 59 ++++++++++++++++++++++++++++++++++++++---
>  include/linux/i3c/ccc.h |  5 ++++
>  2 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 037b3b980717..3b6d3ecd9d12 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -915,17 +915,46 @@ static void i3c_ccc_cmd_dest_cleanup(struct i3c_ccc_cmd_dest *dest)
>  	kfree(dest->payload.data);
>  }
>
> -static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
> -			     struct i3c_ccc_cmd_dest *dests,
> -			     unsigned int ndests)
> +static void i3c_ccc_cmd_init_retries(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
> +				     struct i3c_ccc_cmd_dest *dests,
> +				     unsigned int ndests, unsigned int retries)
>  {
>  	cmd->rnw = rnw ? 1 : 0;
>  	cmd->id = id;
>  	cmd->dests = dests;
>  	cmd->ndests = ndests;
> +	cmd->retries = retries;
>  	cmd->err = I3C_ERROR_UNKNOWN;
>  }
>
> +static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
> +			     struct i3c_ccc_cmd_dest *dests,
> +			     unsigned int ndests)
> +{
> +	i3c_ccc_cmd_init_retries(cmd, rnw, id, dests, ndests,
> +				 rnw ? I3C_CCC_RETRIES : 0);
> +}
> +
> +static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd)
> +{
> +	unsigned int i;
> +
> +	if (!cmd->rnw)
> +		return 0;
> +
> +	for (i = 0; i < cmd->ndests; i++) {
> +		struct i3c_ccc_cmd_payload *p = &cmd->dests[i].payload;
> +
> +		if (p->actual_len > p->len)
> +			return -EIO;
> +
> +		if (p->len && p->actual_len != p->len)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * i3c_master_send_ccc_cmd_locked() - send a CCC (Common Command Codes)
>   * @master: master used to send frames on the bus
> @@ -937,6 +966,9 @@ static void i3c_ccc_cmd_init(struct i3c_ccc_cmd *cmd, bool rnw, u8 id,
>  static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
>  					  struct i3c_ccc_cmd *cmd)
>  {
> +	unsigned int attempt, max_attempts;
> +	int ret;
> +
>  	if (!cmd || !master)
>  		return -EINVAL;
>
> @@ -954,7 +986,25 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
>  	    !master->ops->supports_ccc_cmd(master, cmd))
>  		return -EOPNOTSUPP;
>
> -	return master->ops->send_ccc_cmd(master, cmd);
> +	max_attempts = cmd->retries + 1;
> +	ret = -EIO;
> +	for (attempt = 0; attempt < max_attempts; attempt++) {
> +		unsigned int i;
> +
> +		if (cmd->rnw)
> +			for (i = 0; i < cmd->ndests; i++)
> +				cmd->dests[i].payload.actual_len = 0;
> +
> +		cmd->err = I3C_ERROR_UNKNOWN;
> +		ret = master->ops->send_ccc_cmd(master, cmd);
> +		if (!ret && cmd->err == I3C_ERROR_UNKNOWN)
> +			break;
> +	}
> +
> +	if (!ret)
> +		ret = i3c_ccc_validate_payload_len(cmd);
> +
> +	return ret;
>
>  static struct i2c_dev_desc *
> @@ -1372,6 +1422,7 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>  		 * while expecting shorter length from this CCC command.
>  		 */
>  		dest.payload.len -= 3;
> +		i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
>  		ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>  		if (ret)
>  			goto out;
> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
> index d8052949e57e..2506d83b8255 100644
> --- a/include/linux/i3c/ccc.h
> +++ b/include/linux/i3c/ccc.h
> @@ -12,6 +12,8 @@
>  #include <linux/i3c/device.h>
>
>  /* I3C CCC (Common Command Codes) related definitions */
> +#define I3C_CCC_RETRIES	1
> +
>  #define I3C_CCC_DIRECT			BIT(7)
>
>  #define I3C_CCC_ID(id, broadcast)	\
> @@ -374,12 +376,15 @@ struct i3c_ccc_cmd_dest {
>   * @ndests: number of destinations. Should always be one for broadcast commands
>   * @dests: array of destinations and associated payload for this CCC. Most of
>   *	   the time, only one destination is provided
> + * @retries: number of times to retry a failed Direct GET CCC (see
> + *	     &I3C_CCC_RETRIES)
>   * @err: I3C error code
>   */
>  struct i3c_ccc_cmd {
>  	u8 rnw;
>  	u8 id;
>  	unsigned int ndests;
> +	unsigned int retries;
>  	struct i3c_ccc_cmd_dest *dests;
>  	enum i3c_error_code err;
>  };
> --
> 2.43.7
>
>