[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/19/2014 05:12 PM, Vinod Koul wrote:
> >>
> >>     desc = dmaengine_prep_slave_single(rxchan, …);
> >>     rx_cookie = dmaengine_submit(desc);
> >>     dma_async_issue_pending(rxchan);
> >>
> >>     ssleep(2);
> >>     /* Now assume that the transfer did not start */
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> >>     /* st is now DMA_IN_PROGRESS as expected */
> >>
> >>     dmaengine_terminate_all(rxchan);
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> > and here is the culprit. You have terminated the channel. This means that
> > dmaengine driver is free to clean up all the allocated descriptors on the
> > channels and do whatever it decides to do with them.
> 
> descriptors, yes.
and by that logic when you query the driver would have freed up!

> > You have already terminated the channel so what is the point in querying the
> > status of the cookie, which you shouldn't use anyway after invoking
> > terminate_all() as its result is not correct.
> 
> The point is to check (later, after terminate_all()) if there is an
> outstanding DMA transfer or not _and_ how much data was really
> transfered. Looking at edma it does not really support the latter if
> the transfer is already completed. On the plus side the HW does not
> support partly transfers :)
well that can be achieved properly and differently!
Why don't we pause the channel, get the residue, status and then
terminate.

> But where is it written that the life time of the cookie is limited?
> Looking at the "cooking check" code there is no such thing. It is
> simply compare of completed vs passed number but okay, this is an
> implementation detail.
> From [0] it says under "4. Submit the transaction":
> 
> | 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.
> 
> no life time limit mentioned here. Which brings to the question: Why is
> it okay to use the cookie after the transaction "terminated" normally
> but not if it has been canceled?
Due to the special nature of terminate. The point here is that you don't
terminate a transaction but channel operation

> And from [0] the API explanation  "4. … dma_async_is_tx_complete()":


> 
> |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.
> 
> So the documentation says to use the cookie with
> dma_async_is_tx_complete() and before doing so it is recommended that
> the transfer should be paused or stopped. _Exactly_ what is done here.
> 
> >>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
> >>      * it has been terminated / canceled
> >>      */
> >>
> >> Both dma driver clean up all / terminate all descriptors as required but
> >> _none_ of them completes the cookie. As a result dma_cookie_status()
> >> still thinks that the transfer is in progress.
> > 
> > Btw how does it matter in the driver here if the transaction completed or
> > not after terminate_all() was invoked. What is the purpose of querying
> > status.
> 
> In the RX interrupt (of the 8250 unit), the code checks if the transfer
> has been already started or not via querying the status. So if it
> returns DMA_COMPLETE then a new transfer will be started. If it returns
> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
> engine should start moving data anytime now so the RX interrupt goes
> away.
> 
> That means: If the transfer is canceled then it won't be started again.
> 
> Btw: the current (probably only) dma driver that is used by 8250-dma is
> dw/core.c. That one does cookie complete on terminate:
>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>  dma_cookie_complete().
Yes but would above flow work for you :)

-- 
~Vinod

> 
> That is why it works for them…
> 
> [0] Documentation/dmaengine.txt
> 
> > 
> > Thanks
> > 
> 
> Sebastian

--