Re: [PATCH 1/2] [RFC] spi: rspi: Handle dmaengine_prep_slave_sg() failures gracefully

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

On Friday 11 July 2014 15:58:07 Geert Uytterhoeven wrote:
> On Fri, Jul 11, 2014 at 3:27 PM, Laurent Pinchart wrote:
> >> >> AFAIK, descriptors are cleaned up automatically after use, and the
> >> >> only function that takes a dma_async_tx_descriptor is
> >> >> dmaengine_submit(). But indeed, if you don't use them, that doesn't
> >> >> happen. So calling dmaengine_terminate_all() seems appropriate to fix
> >> >> this.
> >> >> 
> >> >> But, Documentation/dmaengine.txt says:
> >> >>         desc = dmaengine_prep_slave_sg(chan, sgl, nr_sg, direction,
> >> >>         flags);
> >> >>    
> >> >>    Once a descriptor has been obtained, the callback information can
> >> >>    be added and the descriptor must then be submitted.  Some DMA
> >> >>    engine drivers may hold a spinlock between a successful preparation
> >> >>    and submission so it is important that these two operations are
> >> >>    closely paired.
> >> >> 
> >> >> W.r.t. the spinlock, is it safe to call dmaengine_terminate_all() for
> >> >> a prepared but not submitted transfer?
> >> >> Is there another/better way?
> >> > 
> >> > Basically, I have no idea. I'm pretty sure some drivers will support
> >> > it, others won't. Reading the code won't help much, as there's no
> >> > available information regarding what the expected behaviour is. Welcome
> >> > to the wonderful world of the undocumented DMA engine API :-)
> >> 
> >> I did dive a bit into the code...
> >> 
> >> 1.  The spinlock comment seems to apply to INTEL_IOATDMA only.
> >> 
> >>     This driver doesn't implement dma_device.device_control(), so
> >>     dmaengine_terminate_all() is a no-op there, and there doesn't seem to
> >>     be a way to release a descriptor without submitting it first.
> >> 
> >> 2.  While I thought dmaengine_terminate_all() would release all
> >>     descriptors on shdma, as it calls shdma_chan_ld_cleanup(), it only
> >>     releases the descriptors that are at least in submitted state.
> >>     Hence after a while, you get
> >>     
> >>         sh-dma-engine e6700020.dma-controller: No free link descriptor
> >>         available
> >>     
> >>     Interestingly, this contradicts with the comment in
> >>     shdma_free_chan_resources():
> >>     
> >>         /* Prepared and not submitted descriptors can still be on the
> >>         queue */
> >>         if (!list_empty(&schan->ld_queue))
> >>                 shdma_chan_ld_cleanup(schan, true);
> >> 
> >> As dmaengine_submit() will not start the DMA operation, but merely add it
> >> to the pending queue (starting needs a call to
> >> dma_async_issue_pending()),it seems the right solution is to continue
> >> submitting the request for which preparation succeeded, and then aborting
> >> everything using dmaengine_terminate_all().
> >> 
> >> Note that this also means that if submitting the RX request failed, you
> >> should still submit the TX request, as it has been prepared.
> >> 
> >> Alternatively, you can interleave prep and submit for both channels,
> >> which
> >> makes the error recovery code less convoluted.
> > 
> > How about fixing the DMA API to allow cleaning up a prepared request
> > without submitting it ?
> 
> That's another option. But that would require updating all drivers.

Isn't it worth it if the API can be made simpler and more robust ? Even though 
the number of drivers to change isn't small, the update could be rolled out 
gradually.

I wonder what the rationale for the DMA engine cookie system was, and if it 
still applies today. Wouldn't it be easier if slave drivers stored pointers to 
the async_tx descriptors instead of storing cookies, and used them in place of 
cookies through the whole API ? Slaves would then need to release the 
descriptors explicitly, which could occur at any point of time if they were 
reference counted.

> Note that only ioat, iop-adma, mv_xor, and ppc4xx do not implement
> .device_control() and/or DMA_TERMINATE_ALL.

-- 
Regards,

Laurent Pinchart