Re: [PATCH] i3c: mipi-i3c-hci: fix IBI payload length calculation for final status

[Billy Tsai]

>>>> In DMA mode, the IBI status descriptor encodes the payload using
>>>> CHUNKS (number of chunks) and DATA_LENGTH (valid bytes in the last
>>>> chunk). All preceding chunks are implicitly full-sized.
>>>>
>>>> The current code accumulates full chunk sizes for non-final status
>>>> descriptors, but for the final status descriptor it only adds
>>>> DATA_LENGTH. This ignores the contribution of the preceding full
>>>> chunks described by the same final status entry.
>>>>
>>>> As a result, the computed IBI payload length is truncated whenever
>>>> the final status spans multiple chunks. For example, with a chunk
>>>> size of 4 bytes, CHUNKS=2 and DATA_LENGTH=1 should result in a total
>>>> payload size of 5 bytes, but the current code reports only 1 byte.
>>>>
>>>> Fix the calculation by adding the size of (CHUNKS - 1) full chunks
>>>> plus DATA_LENGTH for the last chunk.
>>>>
>>>> Fixes: 9ad9a52cce28 ("i3c/master: introduce the mipi-i3c-hci driver")
>>>> Signed-off-by: Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/i3c/master/mipi-i3c-hci/dma.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> index b903a2da1fd1..f4c76f168276 100644
>>>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> @@ -721,6 +721,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>>>>               if (!(ibi_status & IBI_LAST_STATUS)) {
>>>>                       ibi_size += chunks * rh->ibi_chunk_sz;
>>>>               } else {
>>>> +                     ibi_size += (chunks - 1) * rh->ibi_chunk_sz;
>>
>>> That assumes chunks is not 0.  It would be better to
>>> defend against that possibility i.e.
>>>
>>>                         if (chunks) {
>>>                                 ibi_size += (chunks - 1) * rh->ibi_chunk_sz;
>>>                                 ibi_size += FIELD_GET(IBI_DATA_LENGTH, ibi_status);
>>>                         }
>>>
>>
>> As expected, the value should never be 0, as this is guaranteed by the hardware.

> Not sure the spec. actually says that anywhere.

Apologies—this assumption is too strong. I overlooked the IBI condition when no data
is present. It may be implicitly understood that the CHUNKS field should be set to 0.

> >
> > If we add a check for 0, it may be appropriate to include a WARN_ON message to
> > indicate unexpected hardware behavior.

> Please no.  If the target driver does not get the amount of data
> it is expecting, then it can complain.

Got it. I will update the code to handle the case of 0 chunks without adding a WARN_ON message.

Best regards,
Billy Tsai