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:22:14 Geert Uytterhoeven wrote:
> On Fri, Jul 11, 2014 at 1:47 AM, Laurent Pinchart wrote:
> > On Thursday 10 July 2014 13:55:43 Geert Uytterhoeven wrote:
> >> On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart wrote:
> >> >> --- a/drivers/spi/spi-rspi.c
> >> >> +++ b/drivers/spi/spi-rspi.c
> >> >> @@ -477,7 +477,7 @@ static int rspi_dma_transfer(struct rspi_data
> >> >> *rspi,
> >> >> struct sg_table *tx, tx->sgl, tx->nents, DMA_TO_DEVICE,
> >> >>                                       DMA_PREP_INTERRUPT |
> >> >>                                       DMA_CTRL_ACK);
> >> >>               if (!desc_tx)
> >> >> -                     return -EIO;
> >> >> +                     goto no_dma;
> >> >> 
> >> >>               irq_mask |= SPCR_SPTIE;
> >> >>       }
> >> >> @@ -486,7 +486,7 @@ static int rspi_dma_transfer(struct rspi_data
> >> >> *rspi,
> >> >> struct sg_table *tx, rx->sgl, rx->nents, DMA_FROM_DEVICE,
> >> >>                                       DMA_PREP_INTERRUPT |
> >> >>                                       DMA_CTRL_ACK);
> >> >>               if (!desc_rx)
> >> >> -                     return -EIO;
> >> >> +                     goto no_dma;
> >> > 
> >> > This is not a new issue introduced by this patch, but aren't you
> >> > leaking desc_tx here ?
> >> 
> >> 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 ?

> > The best way to move forward would be to decide on a behaviour and
> > document it. If nobody objects, drivers that don't implement the correct
> > behaviour could be considered as broken, and should be fixed. If someone
> > objects, then a discussion should spring up, and hopefully an agreement
> > will be achieved on what the correct behaviour is.
> 
> Right...
> 
> The document already says "the descriptor must then be submitted",
> which matches with the above.

-- 
Regards,

Laurent Pinchart