Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma transfer

[Sanjay R Mehta <sanmehta@amd.com>]

On 1/3/2022 5:04 PM, Vinod Koul wrote:
> On 17-12-21, 03:58, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>
>> The command should be submitted only if the engine is idle,
>> for this, the next available descriptor is checked and set the flag
>> to false in case the descriptor is non-empty.
>>
>> Also need to segregate the cases when DMA is complete or not.
>> In case if DMA is already complete there is no need to handle it
>> again and gracefully exit from the function.
>>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> ---
>>  drivers/dma/ptdma/ptdma-dmaengine.c | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
>> index c9e52f6..91b93e8 100644
>> --- a/drivers/dma/ptdma/ptdma-dmaengine.c
>> +++ b/drivers/dma/ptdma/ptdma-dmaengine.c
>> @@ -100,12 +100,17 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>  		spin_lock_irqsave(&chan->vc.lock, flags);
>>  
>>  		if (desc) {
>> -			if (desc->status != DMA_ERROR)
>> -				desc->status = DMA_COMPLETE;
>> -
>> -			dma_cookie_complete(tx_desc);
>> -			dma_descriptor_unmap(tx_desc);
>> -			list_del(&desc->vd.node);
>> +			if (desc->status != DMA_COMPLETE) {
>> +				if (desc->status != DMA_ERROR)
>> +					desc->status = DMA_COMPLETE;
>> +
>> +				dma_cookie_complete(tx_desc);
>> +				dma_descriptor_unmap(tx_desc);
>> +				list_del(&desc->vd.node);
>> +			} else {
>> +				/* Don't handle it twice */
>> +				tx_desc = NULL;
>> +			}
>>  		}
>>  
>>  		desc = pt_next_dma_desc(chan);
>> @@ -233,9 +238,14 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>  	struct pt_dma_desc *desc;
>>  	unsigned long flags;
>> +	bool engine_is_idle = true;
>>  
>>  	spin_lock_irqsave(&chan->vc.lock, flags);
>>  
>> +	desc = pt_next_dma_desc(chan);
>> +	if (desc)
>> +		engine_is_idle = false;
>> +
>>  	vchan_issue_pending(&chan->vc);
>>  
>>  	desc = pt_next_dma_desc(chan);
>> @@ -243,7 +253,7 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>>  
>>  	/* If there was nothing active, start processing */
>> -	if (desc)
>> +	if (engine_is_idle)
> 
> Can you explain why do you need this flag and why desc is not
> sufficient..

Here it is required to know if the engine was idle or not before
submitting new desc to the active list (i.e, before calling
"vchan_issue_pending()" API). So that if there was nothing active then
start processing this desc otherwise later.

Here desc is submitted to the engine after vchan_issue_pending() API
called which will actually put the desc into the active list and then if
I get the next desc, the condition will always be true. Therefore used
this flag here to solve this issue.

> 
> It also sounds like 2 patches to me...

Once the desc is submitted to the engine that will be handled by
pt_handle_active_desc() function. This issue was resolved by making
these changes together. Hence kept into the single patch.

Please suggest to me, if this still needs to be split. I'll make the
changes accordingly.

- Sanjay

> 
>>  		pt_cmd_callback(desc, 0);
>>  }
>>  
>> -- 
>> 2.7.4
>