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

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

Hi Geert,

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 :-)

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.

-- 
Regards,

Laurent Pinchart