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

[Adrian Hunter]

On 19/06/2026 11:09, NG, TZE YEE wrote:
> On 16/6/2026 3:41 pm, Adrian Hunter wrote:
>> [You don't often get email from adrian.hunter@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On 11/06/2026 04:54, tze.yee.ng@xxxxxxxxxx wrote:
>>> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@xxxxxxxxxx>
>>>
>>> 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
>>
> I checked the same paths:
> - svc-i3c-master sets I3C_ERROR_M2 on any CCC failure.
> - mipi-i3c-hci maps RESP_ERR_NACK (0x5) to I3C_ERROR_M2.
> So a generic core retry on I3C_ERROR_M2 is only as good as each driver's 
> error reporting. Our DW series maps specific hardware status bits to 
> M0/M2, but we agree the wider driver behaviour should be reviewed before 
> relying on M2 retry across all masters.
> 
> We can either narrow this series to DW-only behaviour for now, or follow 
> up with a separate audit/fix of M2 reporting in other drivers. Happy to 
> take your preference.

I am wondering if we should ignore the nature of the error and just
retry after any error.

Also it seems like the number of retries should be passed by the caller,
so that different commands could be treated differently.  There is now
i3c_master_i3c_dev_present() which does its own retries.

>>> 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?
>>
> Section 5.1.2.2.3 describes the recovery behaviour for a transient 
> address-header NACK (e.g. IBI/CRR arbitration), not the definition of 
> I3C_ERROR_M2 itself.
> 
> M2 is the address-header NACK error code; §5.1.2.2.3 is one case where 
> the spec says software should re-issue the transfer. We will reword the 
> commit message to make that distinction clear and drop the implication 
> that M2 is defined in that section.

The spec. has a different definition of M2:

"If the Master does not receive an ACK of a transmitted Broadcast Address (7’h7E),"

Whereas section 5.1.2.2.3 is about Frames starting with the target address.
That could be covered by DEV_NACK_RETRY_CNT (which we should change to
default to 1 not 0 by the way)

>>> 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@xxxxxxxxxx>
>>> Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
>>> ---
>>> 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
>>
> Agreed. The GETMRL/GETMXDS length rules are caller-specific and fit 
> better at the call site than as hardcoded CCC IDs in 
> i3c_master_send_ccc_cmd_locked().
> 
> Setting optional_bytes in the caller (e.g. getmrl_locked() → 1, 
> getmxds_locked() → 3). We can respin v4 with that approach.>> +
>>> +     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.
>>
> Agreed. We will add actual_len to struct i3c_ccc_cmd_payload in v4.>> + 
>                     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 *
>>
>