Re: [PATCH v2 1/2] dmaengine: qcom: gpi: Add GPI immediate DMA support

[Jyothi Kumar Seerapu <quic_jseerapu@quicinc.com>]

On 11/30/2024 9:35 AM, Bjorn Andersson wrote:
> On Fri, Nov 29, 2024 at 05:02:22PM +0530, Jyothi Kumar Seerapu wrote:
>> On 11/28/2024 8:53 PM, Bjorn Andersson wrote:
>>> On Thu, Nov 28, 2024 at 07:03:50PM +0530, Jyothi Kumar Seerapu wrote:
>>>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> [..]
>>>
>>>>    	/* first create config tre if applicable */
>>>>    	if (direction == DMA_MEM_TO_DEV && spi->set_config) {
>>>> @@ -1763,14 +1767,32 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc,
>>>>    	tre_idx++;
>>>>    	address = sg_dma_address(sgl);
>>>> -	tre->dword[0] = lower_32_bits(address);
>>>> -	tre->dword[1] = upper_32_bits(address);
>>>> +	len = sg_dma_len(sgl);
>>>> -	tre->dword[2] = u32_encode_bits(sg_dma_len(sgl), TRE_DMA_LEN);
>>>> +	/* Support Immediate dma for write transfers for data length up to 8 bytes */
>>>
>>> And what happens if the developer writing the SPI driver forgets to read
>>> this comment and sets QCOM_GPI_IMMEDIATE_DMA for a 9 byte transfer?
>> In V2 patch, QCOM_GPI_IMMEDIATE_DMA is set based on
>> QCOM_GPI_IMMEDIATE_DMA_LEN only.
>>
> 
> I assume you mean "patch 2/2". So, what happens if someone refactors the
> SPI driver in the future, will they read this comment?
> 
>> As per Hardware programming guide, immediate dma support is for up to 8
>> bytes only.
>> Need to check what is the behavior if we want to handle 9 bytes using
>> immediate dma feature support.
>>
> 
> I'm saying that you have a comment here which says that the caller must
> not pass len > 8. Write that comment in code to avoid mistakes - either
> now or in the future.

Sure, i will update the comment in V3.
> 
>>>
>>>> +	if ((spi->flags & QCOM_GPI_IMMEDIATE_DMA) && direction == DMA_MEM_TO_DEV) {
>>>
>>> Why is this flag introduced?
>>>
>>> If I understand the next patch, all DMA_MEM_TO_DEV transfers of <=
>>> QCOM_GPI_IMMEDIATE_DMA_LEN can use the immediate mode, so why not move
>>> the condition here?
>>>
>>> Also ordering[1].
>>>
>>> 	if (direction == DMA_MEM_TO_DEV && len <= 2 * sizeof(tre->dword[0]))
>>>
>>>
>> Sure, thanks for the suggestion.
>> so, instead using "QCOM_GPI_IMMEDIATE_DMA_LEN" need to use " 2 *
>> sizeof(tre->dword[0])" for 8 bytes length check.
>>
> 
> Either one works, but I'm guessing that while 8 is the right number the
> reason for 8 is that the data is passed in 2 * dword.
Okay, i will use "2 * sizeof(tre->dword[0]" which gives 8 only.
> 
> 
> The important thing is that you're encoding the length check here, so
> that the client can't by mistake trigger immediate mode with > 8 bytes.
> As a side effect, you no longer need the QCOM_GPI_IMMEDIATE_DMA flag and
> should be able to drop patch 2.

Sure thanks, will update the changes in V3.
> 
>>> [1] Compare "all transfers of length 8 or less, which are mem to device"
>>> vs "all transfers which are mem to device, with a length of 8 or less".
>>> The bigger "selection criteria" is the direction, then that's fine tuned
>>> by the length query.
>>>
>>>> +		buf = sg_virt(sgl);
>>>
>>> It's a question of style, but I think you could just put the sg_virt()
>>> directly in the memcpy() call and avoid the extra variable.
>>
>> Okay, i will directly put sg_virt() in memcpy().
> 
> Try it out, pick the option that look the best.
Yes, will do it in V3.

> 
> Regards,
> Bjorn
>