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 23/6/2026 8:29 pm, Adrian Hunter wrote:
> 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@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.
> 
> 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.
> 
On retry: agreed — we will drop M0/M2-based retry and retry on any error 
where a caller decides it is appropriate.
For retry count, we would like ask your preference between two approaches:
(i) Sysfs ccc_retry_count on the I3C master (similar to the existing 
dev_nack_retry_count): default 1, user-tunable via sysfs. Callers that 
need retries would read this from the master controller (or a shared 
helper would).
(ii) Caller-local constants (like your i3c_master_i3c_dev_present() 
pattern : each helper that needs retries defines its own MAX_CNT / 
backoff and loops around i3c_master_send_ccc_cmd_locked() — no new sysfs 
knob.
Which do you prefer?
>>>> 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)
> 
Agreed. M2 in spec is the broadcast-address (7'h7E) NACK case, not 
address-header NACK error code. I will reword the commit message in v4.

For the DEV_NACK_RETRY_CNT default value, we will update it in separate 
patch.>>>> 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 *
>>>
>>
>