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

[Sinan Kaya]

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.