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

[Vinod Koul <vkoul@kernel.org>]

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

It also sounds like 2 patches to me...

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

-- 
~Vinod