Re: [PATCH RFCv1 1/2] dmaengine: add support for scatterlist to scatterlist transfers

["Ira W. Snyder" <iws@ovro.caltech.edu>]

On Fri, Sep 24, 2010 at 02:53:14PM -0700, Dan Williams wrote:
> On Fri, 2010-09-24 at 14:24 -0700, Ira W. Snyder wrote:
> > On Fri, Sep 24, 2010 at 01:40:56PM -0700, Dan Williams wrote:
> > > I don't think any dma channels gracefully handle descriptors that were
> > > prepped but not submitted.  You would probably need to submit the
> > > backlog, poll for completion, and then return the error.
> > > Alternatively, the expectation is that descriptor allocations are
> > > transient, i.e. once previously submitted transactions are completed
> > > the descriptors will return to the available pool.  So you could do
> > > what async_tx routines do and just poll for a descriptor.
> > >
> > 
> > Can you give me an example? Even some pseudocode would help.
> 
> Here is one from do_async_gen_syndrome() in crypto/async_tx/async_pq.c:
> 
>         /* Since we have clobbered the src_list we are committed
>          * to doing this asynchronously.  Drivers force forward
>          * progress in case they can not provide a descriptor
>          */
>         for (;;) {
>                 tx = dma->device_prep_dma_pq(chan, dma_dest,
>                                              &dma_src[src_off],
>                                              pq_src_cnt,
>                                              &coefs[src_off], len,
>                                              dma_flags);
>                 if (likely(tx))
>                         break;  
>                 async_tx_quiesce(&submit->depend_tx);
>                 dma_async_issue_pending(chan);
>         }       
> 
> > The other DMAEngine functions (dma_async_memcpy_*()) don't do anything
> > with the descriptor if submit fails. Take for example
> > dma_async_memcpy_buf_to_buf(). If tx->tx_submit(tx); fails, any code
> > using it has no way to return the descriptor to the free pool.
> > 
> > Does tx_submit() implicitly return descriptors to the free pool if it
> > fails?
> 
> No, submit() failures are a hold over from when the ioatdma driver used
> to perform additional descriptor allocation at ->submit() time.  After
> prep() the expectation is that the engine is just waiting to be told
> "go" and can't fail.  The only reason ->submit() retains a return code
> is to support the "cookie" based method for polling for operation
> completion.  A dma driver should handle all descriptor submission
> failure scenarios at prep time.
> 

Ok, that's more like what I expected. So we still need the try forever
code similar to the above. I can add that for the next version.

> > Ok, I thought the list was clearer, but this is equally easy. How about
> > the following change that does away with the list completely. Then
> > things should work on ioatdma as well.
> > 
> > From d59569ff48a89ef5411af3cf2995af7b742c5cd3 Mon Sep 17 00:00:00 2001
> > From: Ira W. Snyder <iws@ovro.caltech.edu>
> > Date: Fri, 24 Sep 2010 14:18:09 -0700
> > Subject: [PATCH] dma: improve scatterlist to scatterlist transfer
> > 
> > This is an improved algorithm to improve support on the Intel I/OAT
> > driver.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> >  drivers/dma/dmaengine.c   |   52 +++++++++++++++++++++-----------------------
> >  include/linux/dmaengine.h |    3 --
> >  2 files changed, 25 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index 57ec1e5..cde775c 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -983,10 +983,13 @@ dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
> >         struct dma_async_tx_descriptor *tx;
> >         dma_cookie_t cookie = -ENOMEM;
> >         size_t dst_avail, src_avail;
> > -       struct list_head tx_list;
> > +       struct scatterlist *sg;
> >         size_t transferred = 0;
> > +       size_t dst_total = 0;
> > +       size_t src_total = 0;
> >         dma_addr_t dst, src;
> >         size_t len;
> > +       int i;
> > 
> >         if (dst_nents == 0 || src_nents == 0)
> >                 return -EINVAL;
> > @@ -994,12 +997,17 @@ dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
> >         if (dst_sg == NULL || src_sg == NULL)
> >                 return -EINVAL;
> > 
> > +       /* get the total count of bytes in each scatterlist */
> > +       for_each_sg(dst_sg, sg, dst_nents, i)
> > +               dst_total += sg_dma_len(sg);
> > +
> > +       for_each_sg(src_sg, sg, src_nents, i)
> > +               src_total += sg_dma_len(sg);
> > +
> 
> What about overrun or underrun do we not care if src_total != dst_total?
> 
> Otherwise looks ok.
> 

I don't know if we should care about that. The algorithm handles that
case just fine. It copies the maximum amount it can, which is exactly
min(src_total, dst_total). Whichever scatterlist runs out of entries
first is the shortest.

As a real world example, my driver verifies that both scatterlists have
exactly the right number of bytes available before trying to program the
hardware.

Ira