Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <01d0009f-743d-4d55-7a2f-dd92bcb0b4e5@intel.com>
Date:   Wed, 19 Oct 2022 10:08:52 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH v5 3/7] dmaengine: Add dmaengine_async_is_tx_complete
Content-Language: en-US
To:     Vinod Koul <vkoul@kernel.org>
CC:     <dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20220622193753.3044206-1-benjamin.walker@intel.com>
 <20220829203537.30676-1-benjamin.walker@intel.com>
 <20220829203537.30676-4-benjamin.walker@intel.com> <Y1AmTaT4oFDAWLLm@matsya>
From:   "Walker, Benjamin" <benjamin.walker@intel.com>
In-Reply-To: <Y1AmTaT4oFDAWLLm@matsya>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dEhrOWlrZFpTbkFxR25DYkhVUFh0YjF4WWR3ZnhBYTBrOEUwNWN4SFZNb2xy?=
 =?utf-8?B?MzVQK2l1Y0N5RHNPcnFLODJUcFhqWEdhSG5ldXZLZCtJWUQvVUhlZDQyVytq?=
 =?utf-8?B?bmNKWCtObm5icU5uRTlLMDE2eXJwcFJvY09DS0JCQnFnbCtTN2lPczRPMGlk?=
 =?utf-8?B?UkJNbDgzQ2k3emU3dzFPOFNjemUweS9hSnZoY21OUHp2ZjA0bUtFUjhNVVBT?=
 =?utf-8?B?V3J3Tk85U2NVd1Z4elZoTUxlM29hZGMreTJJcXR4ZG9vZlFXN1Voa0pmdzJ1?=
 =?utf-8?B?b1l2MEpseUVSM2NLYUhOWHc1Smo4R2E3NFlvU1lOK3BuWjZPNFFFL2NxZTM3?=
 =?utf-8?B?RE1BUDhlUHRIZ29JMU5PRUxaZCtaZ3NUbDYydElqK0w0U1BoVldsWE9YbDlq?=
 =?utf-8?B?dk0wVGFDVWFoVTJicGJUeEZwb2FDTTQrWktFN0wxYmtpSU1VVXdCL2pveVpJ?=
 =?utf-8?B?cm9FbTNxOHZQZWkzOXB5c01pdUthMFVpeTdwREhMTWNzUXB1OXIwV1RhcU5R?=
 =?utf-8?B?N1RlZVpwa0EwbE1uMmxhY2EvaENLS2U4dWdCdTRBcTNWSDN5WnF6SnpoUnhY?=
 =?utf-8?B?ZUNYUHJRL3pLeGp2azh0ZUlIa0pZQVpmK0srMWxGT2xhZURQd1NJcms4N1JQ?=
 =?utf-8?B?NFhxZzlKbk1LREFmTVpOSDVDYlpTcDNEeFN5Mi9nNHp2WWlsV2w0Z0dMWnJl?=
 =?utf-8?B?OE9PbGUvRmppQTRoVm0xTE1OaWt5dTRXL2JXank1bjhNRVZFTUJjcVpzd2VY?=
 =?utf-8?B?RTRIKzNTd01mSmMzTGFpalhqMzBGZks3R0VIRWJIZlg0bm10NWJGRmszZXZV?=
 =?utf-8?B?dE5IdHdOZ2c3aGxhQ1VUeUNoT2Z5c3pqeDZEYlpFc2dKWTVONURqTjgrQUpp?=
 =?utf-8?B?dFNIdTkvNVB5cWcxNWVzRUZ3SktVbXF2eStDRkN1NFduNWRIT3BZU3JleTZl?=
 =?utf-8?B?UnZ0MnlpeEN1TVplZ1VUaDBMQjh1VWtZcy9tUTJSKzB5d2FKQmNGQi9hTStv?=
 =?utf-8?B?ZGpzUnJvR2todHBPbUh2MUVlLzFJRldQMDFIeVBTeU8wZ3RFMWhrWGQ3OUNW?=
 =?utf-8?B?YW9FemFROE50UWdzYk9XWTc1NFVNalYvR0gwTzJ6TXpnanZzS1grbjhDcFpE?=
 =?utf-8?B?TXVqYzJwblp6S2E4RnpjYm9WTk53SjI3eWVnb2lHY01Ob21MeFVSZUhHZ3BM?=
 =?utf-8?B?YkRKS0JKdEJ5ZHVYUVY2RjJqNnZUcEZKakphTmtGc201NVI5bGI1YnhRZWp5?=
 =?utf-8?B?aDNSN3JydnlEbmgwV2xVQnVnaXJmQXVoOVpUUkdHaE82S21QeXNuV0FFb0o3?=
 =?utf-8?B?bkhzbEcrRFB4NjBSK3V4dnVhVGtmYjQ1YThZMmtlQ0Rwbi9ka3Q3UXp3ODlP?=
 =?utf-8?B?aDJ1dThIdXVzWVVLaVJra3ZaN2lIU1ltYlhQc1ZMZm1hOE0vTGlPd0VCMHFq?=
 =?utf-8?B?RzZnbGRLL3Q4ektyOW8vOFJrKzNQajBrbm1vVWU0SzNFaU5pUTNnTEpla2I4?=
 =?utf-8?B?YXpGZmV6YkFnaUxSVWR2Qnp5SVVLMjV4MEEyQ25lT0cwYWFHRk05RlNRN1NO?=
 =?utf-8?B?TkVLSy91SGliTFRuOXVNK3VqdHBlWStiMGZmVWdkM3dWN2NMMzcwQVNOLzR1?=
 =?utf-8?B?S0RoRE1IcWMzS2ZzZjlWMjFNZ0VVRUxWR0YxUUw2MEFCSVBSakZSQ1FnRVo4?=
 =?utf-8?B?R00zbUFsY25kWDh2Tm4ydFFsSCtaaE1rV0FvY0Q3U1FLeWtBeFRUUXhJTTEx?=
 =?utf-8?B?aS9tNEdOVmw4WERHdSswalB6S21iRHlQWDZ6eGs4VkIxQkc5dXhXSVgxeWda?=
 =?utf-8?B?QjRocWQyWmY2ZjVSL1BhdzV3eXJKaUFKbTZhckVpcjN1czM5S2c1ZFh0Syts?=
 =?utf-8?B?cDQ5am9UZlZYRERlREt5QVE2Z2Q1ajBlWVZ0dWgyakdSWFF1Q3FoNER6NkdG?=
 =?utf-8?B?bnNLY3dLSVBzdENqSWdsWGZxOWxMMUFESk90dTl2djdIbzJGajN4S0pKbUxM?=
 =?utf-8?B?Zm9HaHpmaXB3ckJER1l5UFp2QU9YaDcyTHczSGtITHFqYnNzVlZqTHNsWVRQ?=
 =?utf-8?B?MlJya3lub1hMOGxkVWNDay9GeTJUbU9ZakdtK1AvMGNkSzJKb1dmemsrNWJ2?=
 =?utf-8?B?SWYzZTkwalJDRG1PbDgva1JvS2FOMW5NTytOeXRVWlZkOTlBSEpMQjVOS0tJ?=
 =?utf-8?B?MlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a624312-b665-44cc-01ae-08dab1f49a74
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB2824.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Oct 2022 17:08:54.9183
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 23KxMIjSwR1Jr6D9MWNGZie+FT9KPjfhbGKotkOiaSX+Elgf/co+NU9ckrF7dyJhyhCD5bfg0eDOqEWJ/NXWkF2XKv09eEGp6+MRm76kg+8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6285
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/19/2022 9:31 AM, Vinod Koul wrote:
> On 29-08-22, 13:35, Ben Walker wrote:
>> This is the replacement for dma_async_is_tx_complete with two changes:
>> 1) The name prefix is 'dmaengine' as per convention
>> 2) It no longer reports the 'last' or 'used' cookie
> 
> Thanks for this cleanup. This is good :)
> 
> But, why should we retain async is API here. I think lets cleanup
> properly and rename it dmaengine_is_tx_complete()
> 
> we _really_ need to drop async and have everything dmaengine_*

Ack. Misunderstood earlier feedback to change dma -> dmaengine. I agree 
the async needs to go. Fixed in next version.

> 
>>
>> Drivers should convert to using dmaengine_async_is_tx_complete.
>>
>> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
>> ---
>>   Documentation/driver-api/dmaengine/client.rst | 19 ++++---------------
>>   .../driver-api/dmaengine/provider.rst         |  6 +++---
>>   drivers/dma/dmaengine.c                       |  2 +-
>>   drivers/dma/dmatest.c                         |  3 +--
>>   include/linux/dmaengine.h                     | 16 ++++++++++++++++
>>   5 files changed, 25 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/driver-api/dmaengine/client.rst b/Documentation/driver-api/dmaengine/client.rst
>> index 85ecec2c40005..9ae489a4ca97f 100644
>> --- a/Documentation/driver-api/dmaengine/client.rst
>> +++ b/Documentation/driver-api/dmaengine/client.rst
>> @@ -259,8 +259,8 @@ The details of these operations are:
>>   
>>         dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc)
>>   
>> -   This returns a cookie can be used to check the progress of DMA engine
>> -   activity via other DMA engine calls not covered in this document.
>> +   This returns a cookie that can be used to check the progress of a transaction
>> +   via dmaengine_async_is_tx_complete().
>>   
>>      dmaengine_submit() will not start the DMA operation, it merely adds
>>      it to the pending queue. For this, see step 5, dma_async_issue_pending.
>> @@ -339,23 +339,12 @@ Further APIs
>>   
>>      .. code-block:: c
>>   
>> -      enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
>> -		dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
>> -
>> -   This can be used to check the status of the channel. Please see
>> -   the documentation in include/linux/dmaengine.h for a more complete
>> -   description of this API.
>> +      enum dma_status dmaengine_async_is_tx_complete(struct dma_chan *chan,
>> +		dma_cookie_t cookie)
>>   
>>      This can be used with the cookie returned from dmaengine_submit()
>>      to check for completion of a specific DMA transaction.
>>   
>> -   .. note::
>> -
>> -      Not all DMA engine drivers can return reliable information for
>> -      a running DMA channel. It is recommended that DMA engine users
>> -      pause or stop (via dmaengine_terminate_all()) the channel before
>> -      using this API.
>> -
>>   5. Synchronize termination API
>>   
>>      .. code-block:: c
>> diff --git a/Documentation/driver-api/dmaengine/provider.rst b/Documentation/driver-api/dmaengine/provider.rst
>> index ceac2a300e328..1d0da2777921d 100644
>> --- a/Documentation/driver-api/dmaengine/provider.rst
>> +++ b/Documentation/driver-api/dmaengine/provider.rst
>> @@ -539,10 +539,10 @@ where to put them)
>>   
>>   dma_cookie_t
>>   
>> -- it's a DMA transaction ID that will increment over time.
>> +- it's a DMA transaction ID.
>>   
>> -- Not really relevant any more since the introduction of ``virt-dma``
>> -  that abstracts it away.
>> +- The value can be chosen by the provider, or use the helper APIs
>> +  such as dma_cookie_assign() and dma_cookie_complete().
>>   
>>   DMA_CTRL_ACK
>>   
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>> index c741b6431958c..2816b8f492dab 100644
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -523,7 +523,7 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie)
>>   
>>   	dma_async_issue_pending(chan);
>>   	do {
>> -		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
>> +		status = dmaengine_async_is_tx_complete(chan, cookie);
>>   		if (time_after_eq(jiffies, dma_sync_wait_timeout)) {
>>   			dev_err(chan->device->dev, "%s: timeout!\n", __func__);
>>   			return DMA_ERROR;
>> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
>> index 9fe2ae7943169..dde7b9b626336 100644
>> --- a/drivers/dma/dmatest.c
>> +++ b/drivers/dma/dmatest.c
>> @@ -831,8 +831,7 @@ static int dmatest_func(void *data)
>>   					done->done,
>>   					msecs_to_jiffies(params->timeout));
>>   
>> -			status = dma_async_is_tx_complete(chan, cookie, NULL,
>> -							  NULL);
>> +			status = dmaengine_async_is_tx_complete(chan, cookie);
>>   		}
>>   
>>   		if (!done->done) {
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 5ae881729b620..0ee21887b3924 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -1426,6 +1426,8 @@ static inline void dma_async_issue_pending(struct dma_chan *chan)
>>    * @last: returns last completed cookie, can be NULL
>>    * @used: returns last issued cookie, can be NULL
>>    *
>> + * Note: This is deprecated. Use dmaengine_async_is_tx_complete instead.
>> + *
>>    * If @last and @used are passed in, upon return they reflect the most
>>    * recently submitted (used) cookie and the most recently completed
>>    * cookie.
>> @@ -1444,6 +1446,20 @@ static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
>>   	return status;
>>   }
>>   
>> +/**
>> + * dmaengine_async_is_tx_complete - poll for transaction completion
>> + * @chan: DMA channel
>> + * @cookie: transaction identifier to check status of
>> + *
>> + */
>> +static inline enum dma_status dmaengine_async_is_tx_complete(struct dma_chan *chan,
>> +	dma_cookie_t cookie)
>> +{
>> +	struct dma_tx_state state;
>> +
>> +	return chan->device->device_tx_status(chan, cookie, &state);
>> +}
>> +
>>   #ifdef CONFIG_DMA_ENGINE
>>   struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type);
>>   enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie);
>> -- 
>> 2.37.1
>