Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 19 Jan 2022 09:53:52 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Sanjay R Mehta <sanmehta@amd.com>
Cc:     Sanjay R Mehta <Sanju.Mehta@amd.com>, 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: <YeeSWEZi+BOWb3CK@matsya>
References: <1639735118-9798-1-git-send-email-Sanju.Mehta@amd.com>
 <YdLfLc8lfOektWmi@matsya>
 <38ae8876-610a-3c32-5025-1419466167e4@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <38ae8876-610a-3c32-5025-1419466167e4@amd.com>
Precedence: bulk

On 10-01-22, 13:27, Sanjay R Mehta wrote:
> 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.

ok


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

2 patches please

-- 
~Vinod