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;
Subject: Re: [PATCH] dmaengine: ptdma: fix concurrency issue with multiple dma
 transfer
From:   Sanjay R Mehta <sanmehta@amd.com>
To:     Vinod Koul <vkoul@kernel.org>, 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
References: <1639735118-9798-1-git-send-email-Sanju.Mehta@amd.com>
 <YdLfLc8lfOektWmi@matsya> <38ae8876-610a-3c32-5025-1419466167e4@amd.com>
Message-ID: <0ab6a63f-95cd-447f-df10-f73031e71fed@amd.com>
Date:   Tue, 18 Jan 2022 17:35:16 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <38ae8876-610a-3c32-5025-1419466167e4@amd.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Ri9aUVhlOG5YQzZnZ0xqMDJNK1dzY3RMQVJvWTVyZTdFSkdWZU5mS1JCdXdj?=
 =?utf-8?B?azRQblJ2cDkwM2dObi9rNjJkOWRqZ0NpR0NYcXd6VUgwS1k4UUI3ZnNPK3JZ?=
 =?utf-8?B?bnZIWUFPUEs3bi9ORFpBbk44SzdLakNaa0VyQUt5M21HWGxqbTIwNEtxa2ZT?=
 =?utf-8?B?SXNTeXFydVMwSnJXNVd1dzg1Sjd4RnRrNU5jSi9BaFUwUkdQWWJ1cHByOUZt?=
 =?utf-8?B?V1ZGanhXcThtRFR0S1djM2MwL21hTlRmcDZ1OU1LUVBIamRZK1lVUHlpWUZv?=
 =?utf-8?B?VHFGSW95bUxkOEd0TUc2SWFvTTl2UUJNWDJUa29tVUFCbTZWMjlaQVl1NWk0?=
 =?utf-8?B?WU5CWG9iYXgrTGpkVHRSZ0x5S3dJVE1mN2ZzVnhxMWZ4WFJxelg2VDJ0Ym9S?=
 =?utf-8?B?VlphOXJpNEcyZ1NCdEhKbWg5TE9HNVZjMEphSi8vMGttck5nLytWOEwzUEow?=
 =?utf-8?B?TXNjN2FUOUtHLzg2SmZZZTZvOUFsOE9ZRVozbFluNkU4UGgyTDRkc3NGSHRW?=
 =?utf-8?B?bE13dStNOFZhdzg0V2ZWaGdENGRxcHp3N1JweE1nazI3R0RzTE9MM2ozTlJZ?=
 =?utf-8?B?TU1nWUFzTmhQWG1qSVdyTEppOU92NEhOMXFBTG0reFlLTXZmdkMxZ3pTR2VH?=
 =?utf-8?B?M0VsUVh3VWdCTFk3emR5NTE1eHFRc2huVm5NZGI4ODZVMk5Qa29wUVltSlFT?=
 =?utf-8?B?OGYxZ2FWbGQveGgwbEJSYXF1elZVYlI2NHdyeGUxZ3VNeWw2MkdvNDJZWlU5?=
 =?utf-8?B?WUlhenJjdG1rUUZqWWc3OG5zdWF3L01DakZNVDE3ZVg4M29JTlVLTGJXelpk?=
 =?utf-8?B?akZPTHZQMzArVEV6cFZwT2p2T0cxcjRWWDFVY3JVV2FkdThZdmYzWjVLSm5V?=
 =?utf-8?B?ZXF2bXBUS2tkYmpaTm1qcElxNkdKNzZOako3QWJIcUJzUGlBTW1GeHVWMTZz?=
 =?utf-8?B?ZjRMbjZQdzZ6Z3dzV1JRYU1OWlhSUS9wSjkxeEZFYWwyUjRlNFR3UnZySGNT?=
 =?utf-8?B?bExEb0JqdWhROGhHOHNqRWRjR0owM0p3VUR6cVdCR1dDMWhXaXRGcEJJSEw3?=
 =?utf-8?B?ZnFKcGZpcEM4aktCZytRdzRSdUtkSld2NEZraGtXOGlLNEx6clZIcXovcWoz?=
 =?utf-8?B?WEc4RG5KdzMwYkZLSlliRkowcUxzMEFoNXJteE96WXpYN1lOMkRlNTROTHA1?=
 =?utf-8?B?ZHV0L0RMT1RFWDIwV1huQmJBT0UxbnVOaUlUb0gzekdZWHlIc2FoVGhxdkVF?=
 =?utf-8?B?STY2U1BNVTV1VkF2Nm5QOGd3b3hjZFFGMmY4dGYwRjlvTlNlc01Ba3lsYVFt?=
 =?utf-8?B?bHZ2QkhNTSsvRTQ3ZmJrd3FuSFdDQ2crcGxHeVNaeElyVGdtUS9YeEVWdW5L?=
 =?utf-8?B?ekNYb3pYOEFmSlBwNERYVkRGV0hXaUZhRVZTbUw1YnR3Qm5PRURVKzdlcm1l?=
 =?utf-8?B?NjA4NHJscjdsS3prMUl3cHRpeG9uZWFpOERmSnVHdzNwelV2NDNpdmJvdDBL?=
 =?utf-8?B?a1FJcFNLaks4V0ZlTGs2ekdyY2ExSnYwZmgyWXJrYmFGcTdTcXlWcEpTaFJM?=
 =?utf-8?B?L0F2QW92U3hYSm53SWVDMDM3bktFSXdHRTZwVk9iRklmWW1VZ3Z4dHVqeXE4?=
 =?utf-8?B?eDg3cE9qQW1qM3FqdVlBZ1AzSU4xaW55dndncWNBc0h4N1hvQ2ZqeGRLSTcv?=
 =?utf-8?B?ZGEzUXJqY0V5WVQxaWVITk43OE9SUGFyanBSUW4rUzk3YTRaalpsYWVsUjBj?=
 =?utf-8?B?RnRqaHh3dW9XQjZjVGVhLzhzWDZnNzY2VVVIUkZGN2YwazdaZmhIOC9BZXNv?=
 =?utf-8?B?Y3hZL05BU0ltN001S2poMUIvUHQyMXZPMlg3MTZSYUIxUXZvOWRzdXhLcVAw?=
 =?utf-8?B?R3RHK2pYWU9EblJRbUEyUTJ3ZHgrRUc2VzIzRHNFQzVFckY4UkNBWjVsNVBp?=
 =?utf-8?B?Y1J0NmRuQlZ0c2RpdFlvS2c4Y3c3Zm1EYmRCeUlZdWdNMmxaNStBTEltVVJY?=
 =?utf-8?B?QTRrQXhyUFk1VXVLMHFiSDgvOUoyS3RMeVpaUVBuTWoxQTgyZnUzbjg0dlpn?=
 =?utf-8?B?VGhicUR2Z2ZIRkRrUC9pMGR2dTlyYUdZaG1SK1VFTkJhOEtvUlNnTDNDU051?=
 =?utf-8?B?RUw1Q3RCNkY2eDZSVmw1ZUJvQ0UvYTJQU2htL1RLaG9JYzJTdTgxaHhHUXcw?=
 =?utf-8?Q?jSK5ehFrlg+fn6u9w57Ykao=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 29f79d81-722f-4fb3-fd8f-08d9da7ad483
X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1242.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2022 12:05:33.8733
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EBQPgB+AdqqGLdzIgP9EiCbD1EIB7hLMqJ+9H61Jc0uz/cr2G7HU9I2ztN3xGrchjbbcBfsirAh84Hjo2ldGcg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4046
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/10/2022 1:27 PM, 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.
> 
>>
>> 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.
> 

Hi Vinod,

Any further comments for this patch? Need your help to get this upstreamed.


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