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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
