Re: [RESEND 03/10] dmaengine: Remove the last, used parameters in dma_async_is_tx_complete

["Walker, Benjamin" <benjamin.walker@xxxxxxxxx>]

On 5/3/2022 7:13 AM, Vinod Koul wrote:
> On 01-02-22, 13:38, Ben Walker wrote:
> > These are only used by a single driver (pxa_camera) which knows exactly
> > which DMA engine it is dealing with and therefore knows the behavior
> > of the cookie values. It can grab the value it needs directly from
> > the dma_chan instead. No other DMA client assumes anything about
> > the behavior of the cookie values, so the cookie becomes an opaque
> > handle after this patch.
> >
> > Signed-off-by: Ben Walker <benjamin.walker@xxxxxxxxx>
> > ---
> >   Documentation/driver-api/dmaengine/client.rst  | 17 +++--------------
> >   .../driver-api/dmaengine/provider.rst          |  6 +++---
> >   drivers/crypto/stm32/stm32-hash.c              |  3 +--
> >   drivers/dma/dmaengine.c                        |  2 +-
> >   drivers/dma/dmatest.c                          |  3 +--
> >   drivers/media/platform/omap/omap_vout_vrfb.c   |  2 +-
> >   drivers/media/platform/pxa_camera.c            | 13 +++++++++++--
> >   drivers/rapidio/devices/rio_mport_cdev.c       |  3 +--
> >   include/linux/dmaengine.h                      | 18 +++---------------
>
> This needs to be split per subsysytem and cc respective maintainers so
> that they can review and ack it...

Ack. Will split.

> >   9 files changed, 25 insertions(+), 42 deletions(-)
> >
> > diff --git a/Documentation/driver-api/dmaengine/client.rst b/Documentation/driver-api/dmaengine/client.rst
> > index 85ecec2c40005..6e43c9c428e68 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 dma_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.
> > @@ -340,22 +340,11 @@ 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.
> > +		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 0072c9c7efd34..e9e9de18d6b02 100644
> > --- a/Documentation/driver-api/dmaengine/provider.rst
> > +++ b/Documentation/driver-api/dmaengine/provider.rst
> > @@ -543,10 +543,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.
>
> why drop this?

The exact behavior of the cookie (incrementing over time) is no longer a 
part of the API after this is removed. It becomes an opaque handle.

> >   
> > -- 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/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
> > index d33006d43f761..9c3b6526e39f8 100644
> > --- a/drivers/crypto/stm32/stm32-hash.c
> > +++ b/drivers/crypto/stm32/stm32-hash.c
> > @@ -453,8 +453,7 @@ static int stm32_hash_xmit_dma(struct stm32_hash_dev *hdev,
> >   					 msecs_to_jiffies(100)))
> >   		err = -ETIMEDOUT;
> >   
> > -	if (dma_async_is_tx_complete(hdev->dma_lch, cookie,
> > -				     NULL, NULL) != DMA_COMPLETE)
> > +	if (dma_async_is_tx_complete(hdev->dma_lch, cookie) != DMA_COMPLETE)
> >   		err = -ETIMEDOUT;
> >   
> >   	if (err) {
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index 2cfa8458b51be..590f238a0671a 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 = dma_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 f696246f57fdb..0574090a80c8c 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -832,8 +832,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 = dma_async_is_tx_complete(chan, cookie);
> >   		}
> >   
> >   		if (!done->done) {
> > diff --git a/drivers/media/platform/omap/omap_vout_vrfb.c b/drivers/media/platform/omap/omap_vout_vrfb.c
> > index 0cfa0169875f0..d762939ffa5de 100644
> > --- a/drivers/media/platform/omap/omap_vout_vrfb.c
> > +++ b/drivers/media/platform/omap/omap_vout_vrfb.c
> > @@ -289,7 +289,7 @@ int omap_vout_prepare_vrfb(struct omap_vout_device *vout,
> >   					 vout->vrfb_dma_tx.tx_status == 1,
> >   					 VRFB_TX_TIMEOUT);
> >   
> > -	status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> > +	status = dma_async_is_tx_complete(chan, cookie);
> >   
> >   	if (vout->vrfb_dma_tx.tx_status == 0) {
> >   		pr_err("%s: Timeout while waiting for DMA\n", __func__);
> > diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> > index 3ba00b0f93200..6d5c36ea6622a 100644
> > --- a/drivers/media/platform/pxa_camera.c
> > +++ b/drivers/media/platform/pxa_camera.c
> > @@ -1049,8 +1049,17 @@ static void pxa_camera_dma_irq(struct pxa_camera_dev *pcdev,
> >   	last_buf = list_entry(pcdev->capture.prev,
> >   			      struct pxa_buffer, queue);
> >   	last_status = dma_async_is_tx_complete(pcdev->dma_chans[chan],
> > -					       last_buf->cookie[chan],
> > -					       NULL, &last_issued);
> > +					       last_buf->cookie[chan]);
> > +	/*
> > +	 * Peek into the channel and read the last cookie that was issued.
> > +	 * This is a layering violation - the dmaengine API does not officially
> > +	 * provide this information. Since this camera driver is tightly coupled
> > +	 * with a specific DMA device we know exactly how this cookie value will
> > +	 * behave. Otherwise, this wouldn't be safe.
> > +	 */
> > +	last_issued = pcdev->dma_chans[chan]->cookie;
> > +	barrier();
> > +
> >   	if (camera_status & overrun &&
> >   	    last_status != DMA_COMPLETE) {
> >   		dev_dbg(pcdev_to_dev(pcdev), "FIFO overrun! CISR: %x\n",
> > diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
> > index 7df466e222820..790e0c7a3306c 100644
> > --- a/drivers/rapidio/devices/rio_mport_cdev.c
> > +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> > @@ -597,8 +597,7 @@ static void dma_xfer_callback(void *param)
> >   	struct mport_dma_req *req = (struct mport_dma_req *)param;
> >   	struct mport_cdev_priv *priv = req->priv;
> >   
> > -	req->status = dma_async_is_tx_complete(priv->dmach, req->cookie,
> > -					       NULL, NULL);
> > +	req->status = dma_async_is_tx_complete(priv->dmach, req->cookie);
> >   	complete(&req->req_comp);
> >   	kref_put(&req->refcount, dma_req_free);
> >   }
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 8c4934bc038ec..5f884fffe74cc 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -1436,25 +1436,13 @@ static inline void dma_async_issue_pending(struct dma_chan *chan)
> >    * dma_async_is_tx_complete - poll for transaction completion
> >    * @chan: DMA channel
> >    * @cookie: transaction identifier to check status of
> > - * @last: returns last completed cookie, can be NULL
> > - * @used: returns last issued cookie, can be NULL
> > - *
> > - * If @last and @used are passed in, upon return they reflect the most
> > - * recently submitted (used) cookie and the most recently completed
> > - * cookie.
> >    */
> >   static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
> > -	dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
> > +	dma_cookie_t cookie)
>
> With this I think we should take this opprtunity to rename the api to
> dmaengine_async_is_tx_complete()

Ok, I can add a patch in my v2 series to do the rename.