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

["NG, TZE YEE" <tze.yee.ng@altera.com>]

On 16/6/2026 3:41 pm, Adrian Hunter wrote:
> [You don't often get email from adrian.hunter@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 11/06/2026 04:54, tze.yee.ng@altera.com wrote:
>> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
>>
>> 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.
>> 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.
>> 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@altera.com>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
>> ---
>> 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 *
>