Re: [PATCH v6 5/5] i3c: master: Add optional_bytes for variable-length GET CCC validation

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

On Thu, Jul 09, 2026 at 02:56:37AM +0000, NG, TZE YEE wrote:
> On 8/7/2026 11:01 pm, Frank Li wrote:
> > On Wed, Jul 08, 2026 at 12:17:41AM -0700, tze.yee.ng@altera.com wrote:
> >> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
> >>
> >> 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. Extend i3c_ccc_validate_payload_len() to honour it.
> >>
> >> 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 v5:
> >> - Split from v4 patch 3/3: optional_bytes and variable-length GET rules only.
> >> - Simplify validation: drop redundant exact-length check when
> >>    optional_bytes == 0 (per review).
> >> - Clarify GETMXDS fallback: with optional_bytes = 3, a 2-byte success on
> >>    the first attempt no longer needs the fallback; it remains for when the
> >>    5-byte request fails rather than short-reading.
> >> ---
> >>   drivers/i3c/master.c    | 29 +++++++++++++++++++++++------
> >>   include/linux/i3c/ccc.h |  2 ++
> >>   2 files changed, 25 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> >> index 3b6d3ecd9d12..12ca7c929ee5 100644
> >> --- a/drivers/i3c/master.c
> >> +++ b/drivers/i3c/master.c
> >> @@ -902,6 +902,7 @@ 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
> >> @@ -944,11 +945,19 @@ static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd)
> >>
> >>   	for (i = 0; i < cmd->ndests; i++) {
> >>   		struct i3c_ccc_cmd_payload *p = &cmd->dests[i].payload;
> >> +		u16 min_len;
> >> +
> >> +		if (p->optional_bytes > p->len)
> >> +			return -EINVAL;
> >>
> >>   		if (p->actual_len > p->len)
> >>   			return -EIO;
> >>
> >> -		if (p->len && p->actual_len != p->len)
> >> +		if (!p->len)
> >> +			continue;
> >> +
> >> +		min_len = p->len - p->optional_bytes;
> >> +		if (p->actual_len < min_len)
> >>   			return -EIO;
> >>   	}
> >>
> >> @@ -1342,10 +1351,14 @@ static int i3c_master_getmrl_locked(struct i3c_master_controller *master,
> >>   		return -ENOMEM;
> >>
> >>   	/*
> >> -	 * When the device does not have IBI payload GETMRL only returns 2
> >> -	 * bytes of data.
> >> +	 * GETMRL returns 2 bytes (max read length) when the device does not
> >> +	 * advertise IBI payload, or 2 or 3 bytes when it does (the optional
> >> +	 * third byte is max IBI length). Use optional_bytes to allow either
> >> +	 * length when IBI payload is supported.
> >>   	 */
> >> -	if (!(info->bcr & I3C_BCR_IBI_PAYLOAD))
> >> +	if (info->bcr & I3C_BCR_IBI_PAYLOAD)
> >> +		dest.payload.optional_bytes = 1;
> >> +	else
> >>   		dest.payload.len -= 1;
> >>
> >>   	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMRL, &dest, 1);
> >> @@ -1414,14 +1427,18 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
> >>   	if (!getmaxds)
> >>   		return -ENOMEM;
> >>
> >> +	dest.payload.optional_bytes = 3;
> >> +
> >>   	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
> >>   	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> >>   	if (ret) {
> >>   		/*
> >> -		 * Retry when the device does not support max read turnaround
> >> -		 * while expecting shorter length from this CCC command.
> >> +		 * optional_bytes = 3 accepts a 2-byte response on the first
> >> +		 * attempt, so this fallback runs only when the 5-byte request
> >> +		 * fails rather than returning a short read.
> >>   		 */
> >>   		dest.payload.len -= 3;
> >> +		dest.payload.optional_bytes = 0;
> >>   		i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
> >
> > where use retry != 1 case ? if all is 1, patch 4 is not necessary.
> >
> > Frank
> >
>
> Hi Frank,
>
> The retry count is not always 1. retries is 1 for GET and 0 for SET (rnw
> ? I3C_CCC_RETRIES : 0). max_attempts = retries + 1 gives 2 attempts for
> Direct GET (spec §5.1.9.2.3) and 1 for side-effecting SET CCCs. This
> follows Adrian Hunter’s v4 suggestion in
> https://lore.kernel.org/all/382a7c2d-2e9e-4a83-94d1-b917921d91f1@intel.com/;
> patch 4 is still required for validation and the central retry path, not
> only for the field itself.
>
> Patch 4 is still needed for the central retry path, GET payload
> validation after a successful transfer, and resetting actual_len between
> attempts — not only for exposing retries in struct i3c_ccc_cmd.

Okay

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

>
> Thanks,
> Tze Yee
>
> >>   		ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> >>   		if (ret)
> >> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
> >> index 2506d83b8255..7ad677baf761 100644
> >> --- a/include/linux/i3c/ccc.h
> >> +++ b/include/linux/i3c/ccc.h
> >> @@ -347,11 +347,13 @@ struct i3c_ccc_getxtime {
> >>    *
> >>    * @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
> >>    * @data: payload data. This buffer must be DMA-able
> >>    */
> >>   struct i3c_ccc_cmd_payload {
> >>   	u16 len;
> >>   	u16 actual_len;
> >> +	u16 optional_bytes;
> >>   	void *data;
> >>   };
> >>
> >> --
> >> 2.43.7
> >>
> >>
>