[PATCH 1/3] i3c: mipi-i3c-hci: Make bounce buffer code generic to all DMA transfers

[Jarkko Nikula]

On 6/6/25 6:02 PM, Frank Li wrote:
> On Fri, Jun 06, 2025 at 10:16:44AM +0300, Jarkko Nikula wrote:
>> Hi
>>
>> On 6/5/25 6:13 PM, Frank Li wrote:
>>> On Thu, Jun 05, 2025 at 05:07:19PM +0300, Jarkko Nikula wrote:
>>>> Hi
>>>>
>>>> On 6/4/25 6:00 PM, Frank Li wrote:
>>>>> On Wed, Jun 04, 2025 at 03:55:11PM +0300, Jarkko Nikula wrote:
>>>>>> Move DMA bounce buffer code for I3C private transfers to be generic for
>>>>>> all DMA transfers, and round up the receive bounce buffer size to a
>>>>>> multiple of DWORDs.
>>>>>>
>>>>>> It was observed that when the device DMA is IOMMU mapped and the receive
>>>>>> length is not a multiple of DWORDs, the last DWORD is padded with stale
>>>>>> data from the RX FIFO, corrupting 1-3 bytes beyond the expected data.
>>>>>>
>>>>>> A similar issue, though less severe, occurs when an I3C target returns
>>>>>> less data than requested. In this case, the padding does not exceed the
>>>>>> requested number of bytes, assuming the device DMA is not IOMMU mapped.
>>>>>>
>>>>>> Therefore, all I3C private transfer, CCC command payload and I2C
>>>>>> transfer receive buffers must be properly sized for the DMA being IOMMU
>>>>>> mapped. Even if those buffers are already DMA safe, their size may not
>>>>>> be, and I don't have a clear idea how to guarantee this other than
>>>>>> using a local bounce buffer.
>>>>>>
>>>>>> To prepare for the device DMA being IOMMU mapped and to address the
>>>>>> above issue, implement a local, properly sized bounce buffer for all
>>>>>> DMA transfers. For now, allocate it only when the buffer is in the
>>>>>> vmalloc() area to avoid unnecessary copying with CCC commands and
>>>>>> DMA-safe I2C transfers.
>>>>>>
>>>>>> Signed-off-by: Jarkko Nikula <jarkko.nikula at linux.intel.com>
>>>>>> ---
>>>>>>     drivers/i3c/master/mipi-i3c-hci/core.c | 34 -------------------
>>>>>>     drivers/i3c/master/mipi-i3c-hci/dma.c  | 47 +++++++++++++++++++++++++-
>>>>>>     2 files changed, 46 insertions(+), 35 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>>>>>> index bc4538694540..24c5e7d5b439 100644
>>>>>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>>>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>>>>>> @@ -272,34 +272,6 @@ static int i3c_hci_daa(struct i3c_master_controller *m)
>>>>>>     	return hci->cmd->perform_daa(hci);
>>>>>>     }
>>>>>>
>>>>> ...
>>>>>>     }
>>>>>>
>>>>>> +static void *hci_dma_alloc_safe_xfer_buf(struct i3c_hci *hci,
>>>>>> +					 struct hci_xfer *xfer)
>>>>>> +{
>>>>>> +	if (!is_vmalloc_addr(xfer->data))
>>>>>> +		return xfer->data;
>>>>>> +
>>>>>> +	if (xfer->rnw)
>>>>>> +		/*
>>>>>> +		 * Round up the receive bounce buffer length to a multiple of
>>>>>> +		 * DWORDs. Independently of buffer alignment, DMA_FROM_DEVICE
>>>>>> +		 * transfers may corrupt the last DWORD when transfer length is
>>>>>> +		 * not a multiple of DWORDs. This was observed when the device
>>>>>> +		 * DMA is IOMMU mapped or when an I3C target device returns
>>>>>> +		 * less data than requested. Latter case is less severe and
>>>>>> +		 * does not exceed the requested number of bytes, assuming the
>>>>>> +		 * device DMA is not IOMMU mapped.
>>>>>> +		 */
>>>>>> +		xfer->bounce_buf = kzalloc(ALIGN(xfer->data_len, 4),
>>>>>> +					   GFP_KERNEL);
>>>>>> +	else
>>>>>> +		xfer->bounce_buf = kmemdup(xfer->data, xfer->data_len,
>>>>>> +					   GFP_KERNEL);
>>>>>> +
>>>>>> +	return xfer->bounce_buf;
>>>>>
>>>>> Why need use bounce_buf? why not use dma_map_single()?, which will check
>>>>> alignment and size to decide if use swiotlb as bounce buffer.
>>>>>
>>>> We do pass the buffer to the dma_map_single(). I've understood swiotlb is
>>>> transparently used when the DMA cannot directly access the memory but that's
>>>> not the case here.
>>>
>>> why not? even though you have to use internal buf, why not use
>>> dma_alloc_coherent().
>>>
>> I don't think it's suitable for "smallish" per transfer allocations/mappings
>> and buffer is not accessed concurrently by the CPU and DMA during the
>> transfer.
> 
> I still can't understand why need this special handle for i3c. Basic it use
> dma transfer to transfer data. Can you simple descript hci's data flow?
> 
Unfortunately my knowledge doesn't go deeply enough in HW but I could 
guess it's somewhere in MIPI I3C HCI DMA engine implementation and/or 
HCI IP to memory bridge/bus integration.

If it would be a cache line issue then I would think we would see 
corruption independently is the DMA IOMMU mapped or not and in larger 
number of bytes than 1-3 last bytes.

Also another finding that if I3C target returns less data than expected 
then there is also padding in the last DWORD with the real data. But not 
more than requested, i.e. 2 bytes expected, target returns 1, only the 
2nd byte contains stale data and bytes 3-4 untouched. Or 8 bytes 
expected, target returns 1, bytes 2-4 contain stale data and bytes 5-8 
untouched.

So something around trailing bytes handling in the HW when the transfer 
length is not multiple of DWORDs as far as I could guess.