Re: [PATCH v2 2/4] i3c: mipi-i3c-hci: Use core helpers for DMA mapping and bounce buffering

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

On 8/18/25 7:17 PM, Frank Li wrote:
> On Fri, Aug 15, 2025 at 05:12:40PM +0300, Jarkko Nikula wrote:
>> So far only I3C private and I2C transfers have required a bounce buffer
>> for DMA transfers when buffer is not DMA'able.
>>
>> It was observed that when the device DMA is IOMMU mapped and the receive
>> length is not a multiple of DWORDs (32-bit), 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 DWORD aligned.
>>
>> To prepare for the device DMA being IOMMU mapped and to address the
>> above issue, use helpers from I3C core for DMA mapping and bounce
>> buffering for all DMA transfers.
>>
>> For now, require bounce buffer 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  | 32 +++++++++---------------
>>   drivers/i3c/master/mipi-i3c-hci/hci.h  |  3 +--
>>   3 files changed, 13 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>> index 60f1175f1f37..b2977b6ac9f7 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 int i3c_hci_alloc_safe_xfer_buf(struct i3c_hci *hci,
>> -				       struct hci_xfer *xfer)
>> -{
>> -	if (hci->io != &mipi_i3c_hci_dma ||
>> -	    xfer->data == NULL || !is_vmalloc_addr(xfer->data))
>> -		return 0;
>> -
>> -	if (xfer->rnw)
>> -		xfer->bounce_buf = kzalloc(xfer->data_len, GFP_KERNEL);
>> -	else
>> -		xfer->bounce_buf = kmemdup(xfer->data,
>> -					   xfer->data_len, GFP_KERNEL);
>> -
>> -	return xfer->bounce_buf == NULL ? -ENOMEM : 0;
>> -}
>> -
>> -static void i3c_hci_free_safe_xfer_buf(struct i3c_hci *hci,
>> -				       struct hci_xfer *xfer)
>> -{
>> -	if (hci->io != &mipi_i3c_hci_dma || xfer->bounce_buf == NULL)
>> -		return;
>> -
>> -	if (xfer->rnw)
>> -		memcpy(xfer->data, xfer->bounce_buf, xfer->data_len);
>> -
>> -	kfree(xfer->bounce_buf);
>> -}
>> -
>>   static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>>   			      struct i3c_priv_xfer *i3c_xfers,
>>   			      int nxfers)
>> @@ -333,9 +305,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>>   		}
>>   		hci->cmd->prep_i3c_xfer(hci, dev, &xfer[i]);
>>   		xfer[i].cmd_desc[0] |= CMD_0_ROC;
>> -		ret = i3c_hci_alloc_safe_xfer_buf(hci, &xfer[i]);
>> -		if (ret)
>> -			goto out;
>>   	}
>>   	last = i - 1;
>>   	xfer[last].cmd_desc[0] |= CMD_0_TOC;
>> @@ -359,9 +328,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>>   	}
>>
>>   out:
>> -	for (i = 0; i < nxfers; i++)
>> -		i3c_hci_free_safe_xfer_buf(hci, &xfer[i]);
>> -
>>   	hci_free_xfer(xfer, nxfers);
>>   	return ret;
>>   }
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> index 491dfe70b660..295671daae09 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -342,16 +342,11 @@ static int hci_dma_init(struct i3c_hci *hci)
>>   static void hci_dma_unmap_xfer(struct i3c_hci *hci,
>>   			       struct hci_xfer *xfer_list, unsigned int n)
>>   {
>> -	struct hci_xfer *xfer;
>>   	unsigned int i;
>>
>>   	for (i = 0; i < n; i++) {
>> -		xfer = xfer_list + i;
>> -		if (!xfer->data)
>> -			continue;
>> -		dma_unmap_single(&hci->master.dev,
>> -				 xfer->data_dma, xfer->data_len,
>> -				 xfer->rnw ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
>> +		struct hci_xfer *xfer = xfer_list + i;
>> +		struct i3c_dma *dma_xfer __free(i3c_master_dma_unmap_single) = xfer->dma;
> 
> same here, directlly call i3c_master_dma_unmap_single();
> 
Does that then make needless to define DEFINE_FREE() for the 
i3c_master_dma_unmap_single() in the patch 1/4 since then there won't be 
user for it?

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