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

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

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@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.

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c