Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443034 > unrolled thread

[PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

Back to article view | Back to linux.kernel

Contents

  [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Sinan Kaya <okaya@codeaurora.org> - 2016-07-14 05:00 +0200
    Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-07-16 03:10 +0200
      Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-07-24 08:20 +0200
        Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-07-25 16:30 +0200
          Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-04 14:50 +0200
            Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-04 16:20 +0200
              Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-04 16:50 +0200
                Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-04 17:40 +0200
                  Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-04 18:10 +0200
                    Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-04 18:20 +0200
                      Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Robert Jarzmik <robert.jarzmik@free.fr> - 2016-08-05 08:40 +0200
                        Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-05 10:40 +0200
                          Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-05 17:20 +0200
                  Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-04 18:10 +0200
                    Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-08 11:10 +0200
                      Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Lars-Peter Clausen <lars@metafoo.de> - 2016-08-08 14:30 +0200
                        Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-10 20:50 +0200
                Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-04 17:40 +0200
                  Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-08 11:00 +0200
                    Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-08 16:50 +0200
                      Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-10 21:00 +0200
                        Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Sinan Kaya <okaya@codeaurora.org> - 2016-08-10 22:50 +0200
              Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback Vinod Koul <vinod.koul@intel.com> - 2016-08-08 10:50 +0200
                Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the  callback okaya@codeaurora.org - 2016-08-08 14:20 +0200

Page 1 of 2  [1] 2  Next page →

#1443034 — [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

There is a race condition between data transfer callback and descriptor
free code. The callback routine may decide to clear the resources even
though the descriptor has not yet been freed.

Instead of calling the callback first and then releasing the memory,
this code is changing the order to return the descriptor back to the
free pool and then call the user provided callback.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/dma/qcom/hidma.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 41b5c6d..c41696e 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -111,6 +111,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 	struct dma_async_tx_descriptor *desc;
 	dma_cookie_t last_cookie;
 	struct hidma_desc *mdesc;
+	struct hidma_desc *next;
 	unsigned long irqflags;
 	struct list_head list;
 
@@ -122,28 +123,31 @@ static void hidma_process_completed(struct hidma_chan *mchan)
 	spin_unlock_irqrestore(&mchan->lock, irqflags);
 
 	/* Execute callbacks and run dependencies */
-	list_for_each_entry(mdesc, &list, node) {
-		enum dma_status llstat;
+	list_for_each_entry_safe(mdesc, next, &list, node) {
+		dma_async_tx_callback callback;
+		void *param;
 
 		desc = &mdesc->desc;
 
 		spin_lock_irqsave(&mchan->lock, irqflags);
-		dma_cookie_complete(desc);
+		if (hidma_ll_status(mdma->lldev, mdesc->tre_ch)
+			== DMA_COMPLETE)
+			dma_cookie_complete(desc);
 		spin_unlock_irqrestore(&mchan->lock, irqflags);
 
-		llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
-		if (desc->callback && (llstat == DMA_COMPLETE))
-			desc->callback(desc->callback_param);
+		callback = desc->callback;
+		param = desc->callback_param;
 
 		last_cookie = desc->cookie;
 		dma_run_dependencies(desc);
-	}
 
-	/* Free descriptors */
-	spin_lock_irqsave(&mchan->lock, irqflags);
-	list_splice_tail_init(&list, &mchan->free);
-	spin_unlock_irqrestore(&mchan->lock, irqflags);
+		spin_lock_irqsave(&mchan->lock, irqflags);
+		list_move(&mdesc->node, &mchan->free);
+		spin_unlock_irqrestore(&mchan->lock, irqflags);
 
+		if (callback)
+			callback(param);
+	}
 }
 
 /*
-- 
1.8.2.1

[toc] | [next] | [standalone]

#1444712 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

Hi Vinod,

On 7/13/2016 10:57 PM, Sinan Kaya wrote:
> There is a race condition between data transfer callback and descriptor
> free code. The callback routine may decide to clear the resources even
> though the descriptor has not yet been freed.
> 
> Instead of calling the callback first and then releasing the memory,
> this code is changing the order to return the descriptor back to the
> free pool and then call the user provided callback.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/dma/qcom/hidma.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
> index 41b5c6d..c41696e 100644
> --- a/drivers/dma/qcom/hidma.c
> +++ b/drivers/dma/qcom/hidma.c
> @@ -111,6 +111,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
>  	struct dma_async_tx_descriptor *desc;
>  	dma_cookie_t last_cookie;
>  	struct hidma_desc *mdesc;
> +	struct hidma_desc *next;
>  	unsigned long irqflags;
>  	struct list_head list;
>  
> @@ -122,28 +123,31 @@ static void hidma_process_completed(struct hidma_chan *mchan)
>  	spin_unlock_irqrestore(&mchan->lock, irqflags);
>  
>  	/* Execute callbacks and run dependencies */
> -	list_for_each_entry(mdesc, &list, node) {
> -		enum dma_status llstat;
> +	list_for_each_entry_safe(mdesc, next, &list, node) {
> +		dma_async_tx_callback callback;
> +		void *param;
>  
>  		desc = &mdesc->desc;
>  
>  		spin_lock_irqsave(&mchan->lock, irqflags);
> -		dma_cookie_complete(desc);
> +		if (hidma_ll_status(mdma->lldev, mdesc->tre_ch)
> +			== DMA_COMPLETE)
> +			dma_cookie_complete(desc);

It looks like I introduced a behavioral change while refactoring the code.
The previous one would call the callback only if the transfer was successful
but it would always call dma_cookie_complete.

The new behavior is to call dma_cookie_complete only if the transfer is successful
and it calls the callback even in the case of error cases. Then, the client has
to query if transfer was successful.

Which one is the correct behavior?


>  		spin_unlock_irqrestore(&mchan->lock, irqflags);
>  
> -		llstat = hidma_ll_status(mdma->lldev, mdesc->tre_ch);
> -		if (desc->callback && (llstat == DMA_COMPLETE))
> -			desc->callback(desc->callback_param);
> +		callback = desc->callback;
> +		param = desc->callback_param;
>  
>  		last_cookie = desc->cookie;
>  		dma_run_dependencies(desc);
> -	}
>  
> -	/* Free descriptors */
> -	spin_lock_irqsave(&mchan->lock, irqflags);
> -	list_splice_tail_init(&list, &mchan->free);
> -	spin_unlock_irqrestore(&mchan->lock, irqflags);
> +		spin_lock_irqsave(&mchan->lock, irqflags);
> +		list_move(&mdesc->node, &mchan->free);
> +		spin_unlock_irqrestore(&mchan->lock, irqflags);
>  
> +		if (callback)
> +			callback(param);
> +	}
>  }
>  
>  /*
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1449072 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Fri, Jul 15, 2016 at 09:00:52PM -0400, Sinan Kaya wrote:
> Hi Vinod,
> 
> On 7/13/2016 10:57 PM, Sinan Kaya wrote:
> > There is a race condition between data transfer callback and descriptor
> > free code. The callback routine may decide to clear the resources even
> > though the descriptor has not yet been freed.
> > 
> > Instead of calling the callback first and then releasing the memory,
> > this code is changing the order to return the descriptor back to the
> > free pool and then call the user provided callback.
> > 
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > ---
> >  drivers/dma/qcom/hidma.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
> > index 41b5c6d..c41696e 100644
> > --- a/drivers/dma/qcom/hidma.c
> > +++ b/drivers/dma/qcom/hidma.c
> > @@ -111,6 +111,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
> >  	struct dma_async_tx_descriptor *desc;
> >  	dma_cookie_t last_cookie;
> >  	struct hidma_desc *mdesc;
> > +	struct hidma_desc *next;
> >  	unsigned long irqflags;
> >  	struct list_head list;
> >  
> > @@ -122,28 +123,31 @@ static void hidma_process_completed(struct hidma_chan *mchan)
> >  	spin_unlock_irqrestore(&mchan->lock, irqflags);
> >  
> >  	/* Execute callbacks and run dependencies */
> > -	list_for_each_entry(mdesc, &list, node) {
> > -		enum dma_status llstat;
> > +	list_for_each_entry_safe(mdesc, next, &list, node) {
> > +		dma_async_tx_callback callback;
> > +		void *param;
> >  
> >  		desc = &mdesc->desc;
> >  
> >  		spin_lock_irqsave(&mchan->lock, irqflags);
> > -		dma_cookie_complete(desc);
> > +		if (hidma_ll_status(mdma->lldev, mdesc->tre_ch)
> > +			== DMA_COMPLETE)
> > +			dma_cookie_complete(desc);
> 
> It looks like I introduced a behavioral change while refactoring the code.
> The previous one would call the callback only if the transfer was successful
> but it would always call dma_cookie_complete.
> 
> The new behavior is to call dma_cookie_complete only if the transfer is successful
> and it calls the callback even in the case of error cases. Then, the client has
> to query if transfer was successful.
> 
> Which one is the correct behavior?

Hi Sinan,

Cookie is always completed. That simply means trasactions was completed and
has no indication if the transaction was successfull or not.

Uptill now we had no way of reporting error, Dave's series adds that up, so
you can use it.

Callback is optional for users. Again we didnt convey success of
transaction, but a callback for reporting that trasaction was completed. So
invoking callback makes sense everytime.

HTH

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1449544 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

Hi,

On 7/24/2016 2:24 AM, Vinod Koul wrote:
> On Fri, Jul 15, 2016 at 09:00:52PM -0400, Sinan Kaya wrote:
>> Hi Vinod,
>>
>> On 7/13/2016 10:57 PM, Sinan Kaya wrote:
>>> There is a race condition between data transfer callback and descriptor
>>> free code. The callback routine may decide to clear the resources even
>>> though the descriptor has not yet been freed.
>>>
>>> Instead of calling the callback first and then releasing the memory,
>>> this code is changing the order to return the descriptor back to the
>>> free pool and then call the user provided callback.
>>>
>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>> ---
>>>  drivers/dma/qcom/hidma.c | 26 +++++++++++++++-----------
>>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
>>> index 41b5c6d..c41696e 100644
>>> --- a/drivers/dma/qcom/hidma.c
>>> +++ b/drivers/dma/qcom/hidma.c
>>> @@ -111,6 +111,7 @@ static void hidma_process_completed(struct hidma_chan *mchan)
>>>  	struct dma_async_tx_descriptor *desc;
>>>  	dma_cookie_t last_cookie;
>>>  	struct hidma_desc *mdesc;
>>> +	struct hidma_desc *next;
>>>  	unsigned long irqflags;
>>>  	struct list_head list;
>>>  
>>> @@ -122,28 +123,31 @@ static void hidma_process_completed(struct hidma_chan *mchan)
>>>  	spin_unlock_irqrestore(&mchan->lock, irqflags);
>>>  
>>>  	/* Execute callbacks and run dependencies */
>>> -	list_for_each_entry(mdesc, &list, node) {
>>> -		enum dma_status llstat;
>>> +	list_for_each_entry_safe(mdesc, next, &list, node) {
>>> +		dma_async_tx_callback callback;
>>> +		void *param;
>>>  
>>>  		desc = &mdesc->desc;
>>>  
>>>  		spin_lock_irqsave(&mchan->lock, irqflags);
>>> -		dma_cookie_complete(desc);
>>> +		if (hidma_ll_status(mdma->lldev, mdesc->tre_ch)
>>> +			== DMA_COMPLETE)
>>> +			dma_cookie_complete(desc);
>>
>> It looks like I introduced a behavioral change while refactoring the code.
>> The previous one would call the callback only if the transfer was successful
>> but it would always call dma_cookie_complete.
>>
>> The new behavior is to call dma_cookie_complete only if the transfer is successful
>> and it calls the callback even in the case of error cases. Then, the client has
>> to query if transfer was successful.
>>
>> Which one is the correct behavior?
> 
> Hi Sinan,
> 
> Cookie is always completed. That simply means trasactions was completed and
> has no indication if the transaction was successfull or not.
> 
> Uptill now we had no way of reporting error, Dave's series adds that up, so
> you can use it.
> 
> Callback is optional for users. Again we didnt convey success of
> transaction, but a callback for reporting that trasaction was completed. So
> invoking callback makes sense everytime.
> 
> HTH
> 

Let's put Dave's series aside for the moment and assume an error case where
something bad happened during the transfer. I can add the error code once Dave's
series get merged.

Here is the callback from dmatest.

static void dmatest_callback(void *arg)
{ 
	done->done = true;
}

Here is how the request is made.

dma_async_issue_pending(chan);

wait_event_freezable_timeout(done_wait, done.done,
			     msecs_to_jiffies(params->timeout));

status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
if (!done.done) {
	timeout
} else if (status != DMA_COMPLETE) { 
	error
}

success.

Based on what I see here, receiving callback all the time is OK. The client
checks if the callback is received or not first. 

Next, the client checks the status of the tx_status. 

In the error case mentioned, the callback will be executed. done.done will be true.

If I set dma_cookie_complete(desc) in error case, it would be wrong to tell the client
that the transfer is successful. 

In my opinion, the new behavior is correct. Calling dma_cookie_complete(desc) all the time
is not. Do you agree?

If yes, I can divide this patch into two. One to correct the ordering. Another one
for behavioral change.


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1456386 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Mon, Jul 25, 2016 at 10:19:44AM -0400, Sinan Kaya wrote:
> >>
> >> It looks like I introduced a behavioral change while refactoring the code.
> >> The previous one would call the callback only if the transfer was successful
> >> but it would always call dma_cookie_complete.
> >>
> >> The new behavior is to call dma_cookie_complete only if the transfer is successful
> >> and it calls the callback even in the case of error cases. Then, the client has
> >> to query if transfer was successful.
> >>
> >> Which one is the correct behavior?
> > 
> > Hi Sinan,
> > 
> > Cookie is always completed. That simply means trasactions was completed and
> > has no indication if the transaction was successfull or not.
> > 
> > Uptill now we had no way of reporting error, Dave's series adds that up, so
> > you can use it.
> > 
> > Callback is optional for users. Again we didnt convey success of
> > transaction, but a callback for reporting that trasaction was completed. So
> > invoking callback makes sense everytime.
> > 
> 
> Let's put Dave's series aside for the moment and assume an error case where
> something bad happened during the transfer. I can add the error code once Dave's
> series get merged.

Fair enough..

> Here is the callback from dmatest.
> 
> static void dmatest_callback(void *arg)
> { 
> 	done->done = true;
> }
> 
> Here is how the request is made.
> 
> dma_async_issue_pending(chan);
> 
> wait_event_freezable_timeout(done_wait, done.done,
> 			     msecs_to_jiffies(params->timeout));
> 
> status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
> if (!done.done) {
> 	timeout
> } else if (status != DMA_COMPLETE) { 
> 	error
> }
> 
> success.
> 
> Based on what I see here, receiving callback all the time is OK. The client
> checks if the callback is received or not first. 

Callback is optional from API PoV. Yes ppl do implement it :)

> Next, the client checks the status of the tx_status. 
> 
> In the error case mentioned, the callback will be executed. done.done will be true.
> 
> If I set dma_cookie_complete(desc) in error case, it would be wrong to tell the client
> that the transfer is successful. 

And here is the thing that you missed :)

Dmaengine tells transaction is complete. It does not say if the txn is
success or failure. It can transfer data and not say if data was
correct. A successful transaction implies data integrity as well, which
dmaengine can't provide.

> In my opinion, the new behavior is correct. Calling dma_cookie_complete(desc) all the time
> is not. Do you agree?
> 
> If yes, I can divide this patch into two. One to correct the ordering. Another one
> for behavioral change.

See above..

A callback or tx_status will only tell you the txn is completed. That is
why we have DMA_COMPLETE and not DMA_SUCCESS.

So current order seems fine to me!

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1456422 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 8/4/2016 8:55 AM, Vinod Koul wrote:
> Dmaengine tells transaction is complete. It does not say if the txn is
> success or failure. It can transfer data and not say if data was
> correct. A successful transaction implies data integrity as well, which
> dmaengine can't provide.

Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
I now understand that tx_success API just returns information that the request
was executed whether the result is error or not. This makes sense now.

However, if the txn is failure; then we should never call the client callback
since DMA engine cannot provide such feedback to the client without Dave's patch.
You are saying that the calling the callback is optional.

Then, the callback cannot be optional in the error case for old behavior.

How does the client know if memcpy executed or not? The client got its callback
and tx_status is also DMA_COMPLETE.

Is the client supposed to do a memcmp ? (BTW, it doesn't make sense).


>> In my opinion, the new behavior is correct. Calling dma_cookie_complete(desc) all the time
>> > is not. Do you agree?
>> > 
>> > If yes, I can divide this patch into two. One to correct the ordering. Another one
>> > for behavioral change.
> See above..
> 
> A callback or tx_status will only tell you the txn is completed. That is
> why we have DMA_COMPLETE and not DMA_SUCCESS.
> 

Still calling the callback and returning DMA_COMPLETE isn't right. There is no indication
of an actual DMA error. The transaction is complete but data integrity failed.

> So current order seems fine to me!

I posted v2 of this patch without introducing the behavior change leaving the behavior discussion
out for another patch. The current code will not call the callback if error was observed.

This patch is needed to fix a race condition as the commit message describes.
The callback is called before returning the descriptor back to free pool. 

If the client calls free resources, the descriptor that was not returned to free pool gets lost due
to race condition.

I'll refactor the code after Dave's change for passing the error code while calling the
callback. That will be a different patch anyhow.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1456444 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote:
> On 8/4/2016 8:55 AM, Vinod Koul wrote:
> > Dmaengine tells transaction is complete. It does not say if the txn is
> > success or failure. It can transfer data and not say if data was
> > correct. A successful transaction implies data integrity as well, which
> > dmaengine can't provide.
> 
> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
> I now understand that tx_success API just returns information that the request
> was executed whether the result is error or not. This makes sense now.
> 
> However, if the txn is failure; then we should never call the client callback
> since DMA engine cannot provide such feedback to the client without Dave's patch.
> You are saying that the calling the callback is optional.
> 
> Then, the callback cannot be optional in the error case for old behavior.
> 
> How does the client know if memcpy executed or not? The client got its callback
> and tx_status is also DMA_COMPLETE.

If an error occurred, then dma_async_is_tx_complete() is supposed to
return DMA_ERROR.  It's up to the DMA engine driver to ensure that
this happens if it has error detection abilities.

Most of the helpers in drivers/dma/dmaengine.h are there to _assist_
the driver writer - they can't do magic.  dma_cookie_status() will
return from the point of view of the generic DMA code what the status
of a particular cookie is, and the cookie state.  It doesn't take
care of whether a particular transaction associated with a cookie
failed or not - that's up to the driver.

So, if dma_cookie_status() says that a cookie has DMA_COMPLETED
status, and the DMA engine is able to detect errors on individual
transfers, then the driver needs to do further status lookup to
determine whether the particular transaction referred to by the
cookie did fail, and modify the returned status appropriately.

If dma_cookie_status() says that the cookie is DMA_IN_PROGRESS,
then the driver is expected to calculate and report the residue
(the remaining number of bytes) of the referred to transaction.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1456490 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote:
> On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote:
> > On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote:
> >> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
> >> I now understand that tx_success API just returns information that the request
> >> was executed whether the result is error or not. This makes sense now.
> >>
> >> However, if the txn is failure; then we should never call the client callback
> >> since DMA engine cannot provide such feedback to the client without Dave's patch.
> >> You are saying that the calling the callback is optional.
> >>
> >> Then, the callback cannot be optional in the error case for old behavior.
> >>
> >> How does the client know if memcpy executed or not? The client got its callback
> >> and tx_status is also DMA_COMPLETE.
> > 
> > If an error occurred, then dma_async_is_tx_complete() is supposed to
> > return DMA_ERROR.  It's up to the DMA engine driver to ensure that
> > this happens if it has error detection abilities.
> > 
> 
> Well, that's not what I'm getting from Vinod and also from the current usage
> in most of the drivers that I looked.
> 
> The current drivers implement tx_status as follows.
> 
> static enum dma_status xyz_tx_status(struct dma_chan *chan,
> 	dma_cookie_t cookie, struct dma_tx_state *state) 
> {
> ...
> 	ret = dma_cookie_status(&c->vc.chan, cookie, state);
> 	if (ret == DMA_COMPLETE)
> 		return ret; 
> ...
> }

And... how many of those drivers detect an error occuring in the DMA?
If they don't detect an error during DMA, I'm curious what would you
expect the above to look like.

> I also made the argument that the driver should not call dma_cookie_complete
> for failure cases and somehow return an error for transactions that failed. 

You can't omit individual dma_cookie_complete() calls like that (I think
you have little understanding how the cookie system works.)

The cookies consist of continuous pool of numbers.  Each transaction
gets allocated a cookie which is incremented from the previous
transaction.  Any transaction can be identified as complete or pending
depending on whether the cookie value that it has is earlier or later
than the current completion cookie value.

What this means is if you try to do this:

- transaction 1 completes successfully, call dma_cookie_complete()
- transaction 2 completes with an error, omit dma_cookie_complete()
- transaction 3 completes successfully, call dma_cookie_complete()

The effect at the end of that sequence will be as if transaction 2 had
called dma_cookie_complete() - because the completed cookie value will
now be ahead of transaction 2's cookie.

So, playing bakes with dma_cookie_complete() really won't work.  Please
forget this idea.  dma_cookie_complete() merely indicates whether the
transaction completed or is still in progress.  It's got nothing to do
with error status.

What you instead need to do is to find some way to record in your
driver that transaction 2 failed, and when dma_cookie_status() says
that a transaction has DMA_COMPLETE status, you need to look up to
see whether it failed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1456518 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 8/4/2016 11:38 AM, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote:
>> On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote:
>>> On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote:
>>>> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
>>>> I now understand that tx_success API just returns information that the request
>>>> was executed whether the result is error or not. This makes sense now.
>>>>
>>>> However, if the txn is failure; then we should never call the client callback
>>>> since DMA engine cannot provide such feedback to the client without Dave's patch.
>>>> You are saying that the calling the callback is optional.
>>>>
>>>> Then, the callback cannot be optional in the error case for old behavior.
>>>>
>>>> How does the client know if memcpy executed or not? The client got its callback
>>>> and tx_status is also DMA_COMPLETE.
>>>
>>> If an error occurred, then dma_async_is_tx_complete() is supposed to
>>> return DMA_ERROR.  It's up to the DMA engine driver to ensure that
>>> this happens if it has error detection abilities.
>>>
>>
>> Well, that's not what I'm getting from Vinod and also from the current usage
>> in most of the drivers that I looked.
>>
>> The current drivers implement tx_status as follows.
>>
>> static enum dma_status xyz_tx_status(struct dma_chan *chan,
>> 	dma_cookie_t cookie, struct dma_tx_state *state) 
>> {
>> ...
>> 	ret = dma_cookie_status(&c->vc.chan, cookie, state);
>> 	if (ret == DMA_COMPLETE)
>> 		return ret; 
>> ...
>> }
> 

I have patiently looked at all dma_cookie_status calls from LXR.
I have not seen a single one implementing what you are suggesting.
I can't believe that there is no driver that does error checking.

Feel free to correct me if you have a good example I could use as
a reference. 

> And... how many of those drivers detect an error occuring in the DMA?
> If they don't detect an error during DMA, I'm curious what would you
> expect the above to look like.

I have this function that returns the real status of the transaction.

enum dma_status hidma_ll_status(struct hidma_lldev *llhndl, u32 tre_ch); 

I can do another look up here. 

> 
>> I also made the argument that the driver should not call dma_cookie_complete
>> for failure cases and somehow return an error for transactions that failed. 
> 
> You can't omit individual dma_cookie_complete() calls like that (I think
> you have little understanding how the cookie system works.)
> 
> The cookies consist of continuous pool of numbers.  Each transaction
> gets allocated a cookie which is incremented from the previous
> transaction.  Any transaction can be identified as complete or pending
> depending on whether the cookie value that it has is earlier or later
> than the current completion cookie value.
> 
> What this means is if you try to do this:
> 
> - transaction 1 completes successfully, call dma_cookie_complete()
> - transaction 2 completes with an error, omit dma_cookie_complete()
> - transaction 3 completes successfully, call dma_cookie_complete()
> 
> The effect at the end of that sequence will be as if transaction 2 had
> called dma_cookie_complete() - because the completed cookie value will
> now be ahead of transaction 2's cookie.
> 
> So, playing bakes with dma_cookie_complete() really won't work.  Please
> forget this idea.  dma_cookie_complete() merely indicates whether the
> transaction completed or is still in progress.  It's got nothing to do
> with error status.

Fair enough. All the cookie business seemed very convoluted to me when
I tried to understand how it works. I relied on what other drivers are
doing instead.

> 
> What you instead need to do is to find some way to record in your
> driver that transaction 2 failed, and when dma_cookie_status() says
> that a transaction has DMA_COMPLETE status, you need to look up to
> see whether it failed.
> 

I already have this information available. I just need confirmation from
Vinod that this is the right way to do it in terms of DMA engine API.

The other way is I can feed this information to what Dave just introduced
as part of the callback mechanism and not touch this.

The main discussion here is that 

"tx_status does not indicate whether the final transaction is successful or
not" whether the driver has the capability to determine error or not.

It is an API question not implementation question.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1456526 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 08/04/2016 06:08 PM, Sinan Kaya wrote:
[...]
> The other way is I can feed this information to what Dave just introduced
> as part of the callback mechanism and not touch this.

Use the callback mechanism. It is a lot easier to implement correctly than
the tx_status() mechanism.

> The main discussion here is that 
> 
> "tx_status does not indicate whether the final transaction is successful or
> not" whether the driver has the capability to determine error or not.

tx_status() is supposed to be able to indicate whether a transfer failed or
not. But in my opinion this feature is broken by design and implementing it
correctly is very difficult without creating memory leaks. Which is probably
why so few drivers actually implement it.

[toc] | [prev] | [next] | [standalone]

#1456892

Lars-Peter Clausen <lars@metafoo.de> writes:

> On 08/04/2016 06:08 PM, Sinan Kaya wrote:
> [...]
>> The other way is I can feed this information to what Dave just introduced
>> as part of the callback mechanism and not touch this.
>
> Use the callback mechanism. It is a lot easier to implement correctly than
> the tx_status() mechanism.
>
>> The main discussion here is that 
>> 
>> "tx_status does not indicate whether the final transaction is successful or
>> not" whether the driver has the capability to determine error or not.
>
> tx_status() is supposed to be able to indicate whether a transfer failed or
> not. But in my opinion this feature is broken by design and implementing it
> correctly is very difficult without creating memory leaks. Which is probably
> why so few drivers actually implement it.

I think you can implement the error reporting by remembering the "last" cookie
where an error occurred such as in :
 - e093bf60ca49 ("dmaengine: pxa: handle bus errors")
   => see the part of the commit beginning with "As dma_cookie_status() ..."

The point about error reporting was already discussed  with Vinod in here:
   https://lkml.org/lkml/2016/4/13/471
   => Vinod and I were seeing the reporting can be improved

Yet the description made by Russell of the DMA API semantics can be implemented
and used to find whether a specific tx failed or not, it's rather the "in
progress" part I find misleading.

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1456948 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 08/05/2016 08:32 AM, Robert Jarzmik wrote:
> Lars-Peter Clausen <lars@metafoo.de> writes:
> 
>> On 08/04/2016 06:08 PM, Sinan Kaya wrote:
>> [...]
>>> The other way is I can feed this information to what Dave just introduced
>>> as part of the callback mechanism and not touch this.
>>
>> Use the callback mechanism. It is a lot easier to implement correctly than
>> the tx_status() mechanism.
>>
>>> The main discussion here is that 
>>>
>>> "tx_status does not indicate whether the final transaction is successful or
>>> not" whether the driver has the capability to determine error or not.
>>
>> tx_status() is supposed to be able to indicate whether a transfer failed or
>> not. But in my opinion this feature is broken by design and implementing it
>> correctly is very difficult without creating memory leaks. Which is probably
>> why so few drivers actually implement it.
> 
> I think you can implement the error reporting by remembering the "last" cookie
> where an error occurred such as in :
>  - e093bf60ca49 ("dmaengine: pxa: handle bus errors")
>    => see the part of the commit beginning with "As dma_cookie_status() ..."
> 
> The point about error reporting was already discussed  with Vinod in here:
>    https://lkml.org/lkml/2016/4/13/471
>    => Vinod and I were seeing the reporting can be improved

But that allows you only to report the error for the last descriptor that
failed. If two or more have failed any but the last will return DMA_COMPLETE
instead of DMA_ERROR. In your case that sort of works because you completely
stop execution after a descriptor failed. But as you noted in the commit
message this will report misleading status values for the descriptors after
the descriptor that failed.

I believe we need to invest some effort to come up with clear semantics on
what is the expected behavior when transferring a descriptor fails.
Potentially allowing clients to choose the desired behavior, e.g. either
abort all descriptors on error or continue with the next one.

[toc] | [prev] | [next] | [standalone]

#1457158 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 8/5/2016 4:34 AM, Lars-Peter Clausen wrote:
> I believe we need to invest some effort to come up with clear semantics on
> what is the expected behavior when transferring a descriptor fails.
> Potentially allowing clients to choose the desired behavior, e.g. either
> abort all descriptors on error or continue with the next one.

I agree. I was leaning towards not calling the callback when an error happens
to keep the implementation simple and backwards compatible.

After Dave's change, I need to call the callback with the actual error in question.
Now, I have broken tx_status. 

If I implement DMA_ERROR into tx_status like Russell indicated, then I have the 
address space explosion problem like you indicated. 

If I report the error for the last failing cookie, is it good enough?

Or another approach is tx_status is just an indication of HW accepting the request.
All existing clients need to be changed to use Dave's error reporting for deciding
on actual success or failure for a request that was accepted by HW. tx_status
can no longer be used to check for transaction errors. It can still be used to see
if HW accepts the request (like parameter checking etc. but not for the final
result)

I like this one better.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1456521 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 08/04/2016 05:38 PM, Russell King - ARM Linux wrote:
[...]
> What you instead need to do is to find some way to record in your
> driver that transaction 2 failed, and when dma_cookie_status() says
> that a transaction has DMA_COMPLETE status, you need to look up to
> see whether it failed.

In my opinion this is where the current API is broken by design. For each
transfer that fails you need to store the cookie associated with that
transfer in some kind of lookup table. Since there is no lifetime associated
with a cookie entries in this table would need to be retained forever and it
will grow unbound.

Ideally we'd mark error reporting through this interface as deprecated and
discourage new users of the interface. As far as I can see most of the few
drivers that do return DMA_ERROR get it wrong anyway, e.g. return it
unconditionally for all cookies when an error occurred for any of them.

- Lars

[toc] | [prev] | [next] | [standalone]

#1457628 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Thu, Aug 04, 2016 at 05:59:30PM +0200, Lars-Peter Clausen wrote:
> On 08/04/2016 05:38 PM, Russell King - ARM Linux wrote:
> [...]
> > What you instead need to do is to find some way to record in your
> > driver that transaction 2 failed, and when dma_cookie_status() says
> > that a transaction has DMA_COMPLETE status, you need to look up to
> > see whether it failed.
> 
> In my opinion this is where the current API is broken by design. For each
> transfer that fails you need to store the cookie associated with that
> transfer in some kind of lookup table. Since there is no lifetime associated
> with a cookie entries in this table would need to be retained forever and it
> will grow unbound.

And how many drivers can report errors? And how many drivers can guarantee
DMA_COMPLETE implies transaction was succesful.

> Ideally we'd mark error reporting through this interface as deprecated and
> discourage new users of the interface. As far as I can see most of the few
> drivers that do return DMA_ERROR get it wrong anyway, e.g. return it
> unconditionally for all cookies when an error occurred for any of them.

Error reporting is quite tricky as detection is a problem. So yes if you
can do so, it is highly encouraged to report using new interface which is
better than client checking after callback.

Btw what is the behaviour after error? I would think that client will see an
error and report to upper layer while initiaite closure of transaction. So
does driver need to keep the state for a longer time :-)

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1457758 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 08/08/2016 11:08 AM, Vinod Koul wrote:
> On Thu, Aug 04, 2016 at 05:59:30PM +0200, Lars-Peter Clausen wrote:
>> On 08/04/2016 05:38 PM, Russell King - ARM Linux wrote:
>> [...]
>>> What you instead need to do is to find some way to record in your
>>> driver that transaction 2 failed, and when dma_cookie_status() says
>>> that a transaction has DMA_COMPLETE status, you need to look up to
>>> see whether it failed.
>>
>> In my opinion this is where the current API is broken by design. For each
>> transfer that fails you need to store the cookie associated with that
>> transfer in some kind of lookup table. Since there is no lifetime associated
>> with a cookie entries in this table would need to be retained forever and it
>> will grow unbound.
> 
> And how many drivers can report errors? And how many drivers can guarantee
> DMA_COMPLETE implies transaction was succesful.

The former just a handful, the later hopefully all.

> 
>> Ideally we'd mark error reporting through this interface as deprecated and
>> discourage new users of the interface. As far as I can see most of the few
>> drivers that do return DMA_ERROR get it wrong anyway, e.g. return it
>> unconditionally for all cookies when an error occurred for any of them.
> 
> Error reporting is quite tricky as detection is a problem. So yes if you
> can do so, it is highly encouraged to report using new interface which is
> better than client checking after callback.
> 
> Btw what is the behaviour after error? I would think that client will see an
> error and report to upper layer while initiaite closure of transaction. So
> does driver need to keep the state for a longer time :-)

The problem is that this is not really clearly defined.

1) What should be done when multiple descriptors are queued and an error is
encountered on one of them. Should the descriptors that are after the one in
the queue that caused the error be discarded or should they be executed as
normal?

2) How long does a error result need to be retained. Can it be discarded
when the terminate_all() is called, or can it be discarded when the next
issue_pending() is called or should it be retained forever?

Unless we can clearly define the semantics of error reporting it is very
difficult for drivers to use it. Which is probably one of the reasons why
there are only very few DMAengine consumers that do actual error checking.

[toc] | [prev] | [next] | [standalone]

#1459521 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Mon, Aug 08, 2016 at 02:25:28PM +0200, Lars-Peter Clausen wrote:
> On 08/08/2016 11:08 AM, Vinod Koul wrote:
> > On Thu, Aug 04, 2016 at 05:59:30PM +0200, Lars-Peter Clausen wrote:
> >> On 08/04/2016 05:38 PM, Russell King - ARM Linux wrote:
> >> [...]
> >>> What you instead need to do is to find some way to record in your
> >>> driver that transaction 2 failed, and when dma_cookie_status() says
> >>> that a transaction has DMA_COMPLETE status, you need to look up to
> >>> see whether it failed.
> >>
> >> In my opinion this is where the current API is broken by design. For each
> >> transfer that fails you need to store the cookie associated with that
> >> transfer in some kind of lookup table. Since there is no lifetime associated
> >> with a cookie entries in this table would need to be retained forever and it
> >> will grow unbound.
> > 
> > And how many drivers can report errors? And how many drivers can guarantee
> > DMA_COMPLETE implies transaction was succesful.
> 
> The former just a handful, the later hopefully all.
> 
> > 
> >> Ideally we'd mark error reporting through this interface as deprecated and
> >> discourage new users of the interface. As far as I can see most of the few
> >> drivers that do return DMA_ERROR get it wrong anyway, e.g. return it
> >> unconditionally for all cookies when an error occurred for any of them.
> > 
> > Error reporting is quite tricky as detection is a problem. So yes if you
> > can do so, it is highly encouraged to report using new interface which is
> > better than client checking after callback.
> > 
> > Btw what is the behaviour after error? I would think that client will see an
> > error and report to upper layer while initiaite closure of transaction. So
> > does driver need to keep the state for a longer time :-)
> 
> The problem is that this is not really clearly defined.
> 
> 1) What should be done when multiple descriptors are queued and an error is
> encountered on one of them. Should the descriptors that are after the one in
> the queue that caused the error be discarded or should they be executed as
> normal?

That is client's call.

But a reasonable way would be for client to propagate those errors up, so it
can terminate.

> 2) How long does a error result need to be retained. Can it be discarded
> when the terminate_all() is called, or can it be discarded when the next
> issue_pending() is called or should it be retained forever?

Uptill next terminate_all()

> Unless we can clearly define the semantics of error reporting it is very
> difficult for drivers to use it. Which is probably one of the reasons why
> there are only very few DMAengine consumers that do actual error checking.

Yes agreed, but most reasonable behaviour is to terminate. Also I would
expect the error reporting to be done thru new API and explcitly told that
we found error (if we can).

-  
~Vinod

[toc] | [prev] | [next] | [standalone]

#1456493 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote:
>> On 8/4/2016 8:55 AM, Vinod Koul wrote:
>>> Dmaengine tells transaction is complete. It does not say if the txn is
>>> success or failure. It can transfer data and not say if data was
>>> correct. A successful transaction implies data integrity as well, which
>>> dmaengine can't provide.
>>
>> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
>> I now understand that tx_success API just returns information that the request
>> was executed whether the result is error or not. This makes sense now.
>>
>> However, if the txn is failure; then we should never call the client callback
>> since DMA engine cannot provide such feedback to the client without Dave's patch.
>> You are saying that the calling the callback is optional.
>>
>> Then, the callback cannot be optional in the error case for old behavior.
>>
>> How does the client know if memcpy executed or not? The client got its callback
>> and tx_status is also DMA_COMPLETE.
> 
> If an error occurred, then dma_async_is_tx_complete() is supposed to
> return DMA_ERROR.  It's up to the DMA engine driver to ensure that
> this happens if it has error detection abilities.
> 

Well, that's not what I'm getting from Vinod and also from the current usage
in most of the drivers that I looked.

The current drivers implement tx_status as follows.

static enum dma_status xyz_tx_status(struct dma_chan *chan,
	dma_cookie_t cookie, struct dma_tx_state *state) 
{
...
	ret = dma_cookie_status(&c->vc.chan, cookie, state);
	if (ret == DMA_COMPLETE)
		return ret; 
...
}

What Vinod is telling me that I need to set the cookie to complete 
whether the transaction is successful or not if the request was accepted
by HW. xyz_tx_status is just an indication that the transaction was accepted
by HW. An error can happen as a result of transaction execution.

If I call dma_cookie_complete for all transactions regardless of transaction
success or not, then the xyz_tx_status returns DMA_COMPLETE. 

This also matches what Vinod is saying. The transaction is complete but
it may not be success. I'm saying that if we follow this scheme, then
we should not call the callback.

I also made the argument that the driver should not call dma_cookie_complete
for failure cases and somehow return an error for transactions that failed. 
This is your suggestion.

I don't think it matches Vinod's expectation.


> Most of the helpers in drivers/dma/dmaengine.h are there to _assist_
> the driver writer - they can't do magic.  dma_cookie_status() will
> return from the point of view of the generic DMA code what the status
> of a particular cookie is, and the cookie state.  It doesn't take
> care of whether a particular transaction associated with a cookie
> failed or not - that's up to the driver.
> 
> So, if dma_cookie_status() says that a cookie has DMA_COMPLETED
> status, and the DMA engine is able to detect errors on individual
> transfers, then the driver needs to do further status lookup to
> determine whether the particular transaction referred to by the
> cookie did fail, and modify the returned status appropriately.
> 
> If dma_cookie_status() says that the cookie is DMA_IN_PROGRESS,
> then the driver is expected to calculate and report the residue
> (the remaining number of bytes) of the referred to transaction.
> 

This part is fine. I'm worried about transactions that are failing.



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1457622 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote:
> On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote:
> > On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote:
> >> On 8/4/2016 8:55 AM, Vinod Koul wrote:
> >>> Dmaengine tells transaction is complete. It does not say if the txn is
> >>> success or failure. It can transfer data and not say if data was
> >>> correct. A successful transaction implies data integrity as well, which
> >>> dmaengine can't provide.
> >>
> >> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE.
> >> I now understand that tx_success API just returns information that the request
> >> was executed whether the result is error or not. This makes sense now.
> >>
> >> However, if the txn is failure; then we should never call the client callback
> >> since DMA engine cannot provide such feedback to the client without Dave's patch.
> >> You are saying that the calling the callback is optional.
> >>
> >> Then, the callback cannot be optional in the error case for old behavior.
> >>
> >> How does the client know if memcpy executed or not? The client got its callback
> >> and tx_status is also DMA_COMPLETE.
> > 
> > If an error occurred, then dma_async_is_tx_complete() is supposed to
> > return DMA_ERROR.  It's up to the DMA engine driver to ensure that
> > this happens if it has error detection abilities.
> > 
> 
> Well, that's not what I'm getting from Vinod and also from the current usage
> in most of the drivers that I looked.

Sorry but, you are not interpreting it correctly. Me and Russell are saying the
same thing!

> 
> The current drivers implement tx_status as follows.
> 
> static enum dma_status xyz_tx_status(struct dma_chan *chan,
> 	dma_cookie_t cookie, struct dma_tx_state *state) 
> {
> ...
> 	ret = dma_cookie_status(&c->vc.chan, cookie, state);
> 	if (ret == DMA_COMPLETE)
> 		return ret; 
> ...
> }
> 
> What Vinod is telling me that I need to set the cookie to complete 
> whether the transaction is successful or not if the request was accepted
> by HW. xyz_tx_status is just an indication that the transaction was accepted
> by HW. An error can happen as a result of transaction execution.

Nope, if the txn is completed you mark it complete. If you can detect error
(can you??) then you can report DMA_ERROR.

In that latter case do not use dma_async_is_complete() to check. You would
need to store and report that cookie 'x' failed which you report status in
.tx_statis()

> 
> If I call dma_cookie_complete for all transactions regardless of transaction
> success or not, then the xyz_tx_status returns DMA_COMPLETE. 

Again that is based on your implementation.

> This also matches what Vinod is saying. The transaction is complete but
> it may not be success. I'm saying that if we follow this scheme, then
> we should not call the callback.

That is not in driver's control. If the callback is set, you have to call
it. Client may choose to not set it.

> I also made the argument that the driver should not call dma_cookie_complete
> for failure cases and somehow return an error for transactions that failed. 
> This is your suggestion.
> 
> I don't think it matches Vinod's expectation.

It does!

> > Most of the helpers in drivers/dma/dmaengine.h are there to _assist_
> > the driver writer - they can't do magic.  dma_cookie_status() will
> > return from the point of view of the generic DMA code what the status
> > of a particular cookie is, and the cookie state.  It doesn't take
> > care of whether a particular transaction associated with a cookie
> > failed or not - that's up to the driver.
> > 
> > So, if dma_cookie_status() says that a cookie has DMA_COMPLETED
> > status, and the DMA engine is able to detect errors on individual
> > transfers, then the driver needs to do further status lookup to
> > determine whether the particular transaction referred to by the
> > cookie did fail, and modify the returned status appropriately.
> > 
> > If dma_cookie_status() says that the cookie is DMA_IN_PROGRESS,
> > then the driver is expected to calculate and report the residue
> > (the remaining number of bytes) of the referred to transaction.
> > 
> 
> This part is fine. I'm worried about transactions that are failing.

And you issue is complete orthogonal to this debate. I am not saying we
should not discuss this, but you fix would be entirely different here (going
by data you have provided till now)

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1457847 — Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

On 8/8/2016 5:02 AM, Vinod Koul wrote:
>> What Vinod is telling me that I need to set the cookie to complete 
>> > whether the transaction is successful or not if the request was accepted
>> > by HW. xyz_tx_status is just an indication that the transaction was accepted
>> > by HW. An error can happen as a result of transaction execution.
> Nope, if the txn is completed you mark it complete. If you can detect error
> (can you??) then you can report DMA_ERROR.
> 

Yes, the HW reports if a transaction failed or not. I have this information
available in hidma_ll_status function for a limited amount of time until the
descriptor gets reused.

> In that latter case do not use dma_async_is_complete() to check. You would
> need to store and report that cookie 'x' failed which you report status in
> .tx_statis()
> 

I really don't like the idea of telling 'hey client I finished your work and I
guarantee you it is complete. A month from now, by the way I actually didn't do
the work that day and I did not tell you'

That's why, I preferred not to call the callback when I observe an error which I
think it makes more sense. 

Where is the reliability in this? Some random bugs showing at random times. 
I'd rather not call the callback and be safe. Especially, if you are talking about
servers; this is plain unacceptable.

As Lars-Peter and I indicated in my last email, I think we need to kill this 
tx_status API and replace all the clients to use Dave's interface. It is practically
impossible to implement a reliable tx_status function.

Once this transition happens, I can implement Dave's interface not before.

Again, it will be a different patch than this one. I think v2 of this patch
needs to go in as it is.

https://lkml.org/lkml/2016/7/31/64

>> > 
>> > If I call dma_cookie_complete for all transactions regardless of transaction
>> > success or not, then the xyz_tx_status returns DMA_COMPLETE. 
> Again that is based on your implementation.
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web