Date:   Mon, 3 Jan 2022 17:04:05 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Sanjay R Mehta <Sanju.Mehta@amd.com>
Cc:     gregkh@linuxfoundation.org, dan.j.williams@intel.com,
        Thomas.Lendacky@amd.com, robh@kernel.org,
        mchehab+samsung@kernel.org, davem@davemloft.net,
        linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org
Subject: Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple
 dma transfer
Message-ID: <YdLfLc8lfOektWmi@matsya>
References: <1639735118-9798-1-git-send-email-Sanju.Mehta@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1639735118-9798-1-git-send-email-Sanju.Mehta@amd.com>
Precedence: bulk

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