Re: [PATCH v4 3/3] i3c: master: Validate GET CCC payload length and retry Direct GET once

[Frank Li]

On Tue, Jun 30, 2026 at 06:20:27AM -0700, tze.yee.ng@xxxxxxxxxx wrote:
> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@xxxxxxxxxx>
>
> Add optional_bytes to struct i3c_ccc_cmd_payload so callers describe
> variable-length GET CCC responses. GETMRL and GETMXDS set optional_bytes
> at the call site.
>
> Validate GET payload length in i3c_master_send_ccc_cmd_locked() using
> actual_len and optional_bytes. Retry failed Direct GET CCCs up to
> cmd->retries times (default I3C_CCC_RETRIES) on any error; SET CCCs are
> not retried by default.
>
> Add i3c_ccc_cmd_init_retries() and set actual_len in I3C master drivers
> on successful GET transfers.
>
> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@xxxxxxxxxx>
> Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> ---
>  drivers/i3c/master.c                   | 92 ++++++++++++++++++++++----
>  drivers/i3c/master/adi-i3c-master.c    |  2 +
>  drivers/i3c/master/i3c-master-cdns.c   |  2 +
>  drivers/i3c/master/mipi-i3c-hci/core.c |  5 +-
>  drivers/i3c/master/renesas-i3c.c       |  2 +
>  drivers/i3c/master/svc-i3c-master.c    |  4 +-
>  include/linux/i3c/ccc.h                |  7 ++

Can you spit ccc.h and master.c to one patch, other driver change to
anthoer patche.

>  7 files changed, 98 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5cd4e5da2233..29dc0793a5a4 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -901,6 +901,8 @@ static void *i3c_ccc_cmd_dest_init(struct i3c_ccc_cmd_dest *dest, u8 addr,
>  {
>  	dest->addr = addr;
>  	dest->payload.len = payloadlen;
> +	dest->payload.actual_len = 0;
> +	dest->payload.optional_bytes = 0;
>  	if (payloadlen)
>  		dest->payload.data = kzalloc(payloadlen, GFP_KERNEL);
>  	else
> @@ -914,17 +916,55 @@ 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)
> +{
> +	i3c_ccc_cmd_init_retries(cmd, rnw, id, dests, ndests,
> +				 rnw ? I3C_CCC_RETRIES : 0);

why only read need retry?

> +}
> +
...
>
> @@ -953,7 +996,24 @@ 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->rnw)

i3c_ccc_validate_payload_len() already checked cmd->rnw, needn't check here
again.

> +			ret = i3c_ccc_validate_payload_len(cmd);
> +		if (!ret && cmd->err == I3C_ERROR_UNKNOWN)
> +			break;

if i3c_ccc_validate_payload_len() return failure, why need try here.
suppose only need retry when target NACK request.

> +	}
> +
> +	return ret;
>  }
>
>  static struct i2c_dev_desc *
...
> @@ -1363,6 +1427,8 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>  	if (!getmaxds)
>  		return -ENOMEM;
>
> +	dest.payload.optional_bytes = 3;
> +

move these set optional_bytes to new patch

>  	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
>  	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>  	if (ret) {
...
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca..fec614700843 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -805,6 +805,8 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
>  	ret = xfer->ret;
>  	if (ret)
>  		ccc->err = I3C_ERROR_M2;
> +	else if (ccc->rnw)
> +		ccc->dests[0].payload.actual_len = cmd->rx_count;

update actual_len's patch should just flow add field actual_len's patch

>
>  	return ret;
>  }
...
> +
>  /**
>   * struct i3c_ccc_cmd_payload - CCC payload
>   *
>   * @len: requested payload length
>   * @actual_len: number of bytes received on a GET CCC (filled by the driver)
> + * @optional_bytes: GET CCCs may return up to this many fewer bytes than @len

					   up to @len - @optional_bytes

Frank
>   * @data: payload data. This buffer must be DMA-able
>   */
>  struct i3c_ccc_cmd_payload {
>  	u16 len;
>  	u16 actual_len;
> +	u16 optional_bytes;
>  	void *data;
>  };
>
> @@ -374,12 +378,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
>