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;
Date:   Wed, 19 Oct 2022 22:01:09 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Ben Walker <benjamin.walker@intel.com>
Cc:     dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/7] dmaengine: Add dmaengine_async_is_tx_complete
Message-ID: <Y1AmTaT4oFDAWLLm@matsya>
References: <20220622193753.3044206-1-benjamin.walker@intel.com>
 <20220829203537.30676-1-benjamin.walker@intel.com>
 <20220829203537.30676-4-benjamin.walker@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220829203537.30676-4-benjamin.walker@intel.com>
Precedence: bulk

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_*

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

-- 
~Vinod