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

[NG, TZE YEE]

On 1/7/2026 2:48 am, Frank Li wrote:
> 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.
> 

Hi Frank,

Sure, I will split them in v5.

>>   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?
> 
>> +}
>> +
> ...

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, that's why we don't apply 
auto-retry SET.

>>
>> @@ -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.
> 

Agree. I will drop this cmd->rnw check in v5.

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

The I3C spec retry rules in §5.1.9.2.3 (Direct GET) and §5.1.10.2.x 
(M0/M2) apply to bus-level transfer failures detected during the CCC — 
frame errors, broadcast NACK, command not completing — not to software 
payload-length validation after a successful transfer.

Therefore, i3c_ccc_validate_payload_len() failures should not trigger 
retry; retry should be limited to send_ccc_cmd() driver/HW failures.

v4 will be updated so the retry loop only re-issues send_ccc_cmd() on 
driver/HW failure. Validation runs after a successful transfer and is 
not retried.

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

Will fix in v5.

Thanks,
Tze Yee

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