| Message ID | 1404901583-31366-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 85912a88c1ebcad04a5cfec971771195ce8d6691 |
| Headers | show |
Hi Geert, Thank you for the patch. On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote: > As typically a shmobile SoC has less DMA channels than devices that can use > DMA, we may want to prioritize access to the DMA channels in the future. > This means that dmaengine_prep_slave_sg() may start failing arbitrarily. > > Handle dmaengine_prep_slave_sg() failures gracefully by falling back to > PIO. Just a random thought, do you think there would be performance-sensitive cases where failing the transfer completely would be better than using PIO ? > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> -EAGAIN might not be the best error code to return in this case, as it would imply that the caller should just call rspi_dma_transfer() again. On the other hand I don't see another error code that would be a perfect match, so Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c > index 38fd938d6360..c850dfdfa9e3 100644 > --- 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; > > irq_mask |= SPCR_SPRIE; > } > @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi, > struct sg_table *tx, enable_irq(rspi->rx_irq); > > return ret; > + > +no_dma: > + pr_warn_once("%s %s: DMA not available, falling back to PIO\n", > + dev_driver_string(&rspi->master->dev), > + dev_name(&rspi->master->dev)); > + return -EAGAIN; > } > > static void rspi_receive_init(const struct rspi_data *rspi) > @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi, > > if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > /* rx_buf can be NULL on RSPI on SH in TX-only Mode */ > - return rspi_dma_transfer(rspi, &xfer->tx_sg, > - xfer->rx_buf ? &xfer->rx_sg : NULL); > + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, > + xfer->rx_buf ? &xfer->rx_sg : NULL); > + if (ret != -EAGAIN) > + return ret; > } > > ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len); > @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, > struct spi_transfer *xfer) { > int ret; > > - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) > - return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); > + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); > + if (ret != -EAGAIN) > + return ret; > + } > > ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len); > if (ret < 0) > @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, > struct spi_transfer *xfer) > > static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer > *xfer) { > - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) > - return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); > + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > + int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); > + if (ret != -EAGAIN) > + return ret; > + } > > return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len); > }
Hi Geert, On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote: > As typically a shmobile SoC has less DMA channels than devices that can use > DMA, we may want to prioritize access to the DMA channels in the future. > This means that dmaengine_prep_slave_sg() may start failing arbitrarily. > > Handle dmaengine_prep_slave_sg() failures gracefully by falling back to > PIO. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c > index 38fd938d6360..c850dfdfa9e3 100644 > --- 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 ? > irq_mask |= SPCR_SPRIE; > } > @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi, > struct sg_table *tx, enable_irq(rspi->rx_irq); > > return ret; > + > +no_dma: > + pr_warn_once("%s %s: DMA not available, falling back to PIO\n", > + dev_driver_string(&rspi->master->dev), > + dev_name(&rspi->master->dev)); > + return -EAGAIN; > } > > static void rspi_receive_init(const struct rspi_data *rspi) > @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi, > > if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > /* rx_buf can be NULL on RSPI on SH in TX-only Mode */ > - return rspi_dma_transfer(rspi, &xfer->tx_sg, > - xfer->rx_buf ? &xfer->rx_sg : NULL); > + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, > + xfer->rx_buf ? &xfer->rx_sg : NULL); > + if (ret != -EAGAIN) > + return ret; > } > > ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len); > @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, > struct spi_transfer *xfer) { > int ret; > > - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) > - return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); > + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); > + if (ret != -EAGAIN) > + return ret; > + } > > ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len); > if (ret < 0) > @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, > struct spi_transfer *xfer) > > static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer > *xfer) { > - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) > - return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); > + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { > + int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); > + if (ret != -EAGAIN) > + return ret; > + } > > return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len); > }
Hi Laurent, On Thu, Jul 10, 2014 at 1:05 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wednesday 09 July 2014 12:26:22 Geert Uytterhoeven wrote: >> As typically a shmobile SoC has less DMA channels than devices that can use >> DMA, we may want to prioritize access to the DMA channels in the future. >> This means that dmaengine_prep_slave_sg() may start failing arbitrarily. >> >> Handle dmaengine_prep_slave_sg() failures gracefully by falling back to >> PIO. > > Just a random thought, do you think there would be performance-sensitive cases > where failing the transfer completely would be better than using PIO ? QSPI Flash reads gain most from DMA. However, failing transfers (and thus the whole SPI message, which typically consists of multiple transfers) means we'll have to retry the message later. This could be done by the SPI driver, the SPI subsystem, or the upper layer. E.g. m25p80 and mtd do not retry SPI FLASH reads and writes. >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > -EAGAIN might not be the best error code to return in this case, as it would > imply that the caller should just call rspi_dma_transfer() again. On the other > hand I don't see another error code that would be a perfect match, so I followed the exact same reasoning to arrive at -EAGAIN... BTW, calling rspi_dma_transfer() again is an option ;-) > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, (cc dmaengine) On Thu, Jul 10, 2014 at 1:08 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Hi Laurent, On Fri, Jul 11, 2014 at 1:47 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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. > 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. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Hi Laurent, On Fri, Jul 11, 2014 at 3:27 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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. Note that only ioat, iop-adma, mv_xor, and ppc4xx do not implement .device_control() and/or DMA_TERMINATE_ALL. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Wed, Jul 09, 2014 at 12:26:22PM +0200, Geert Uytterhoeven wrote: > As typically a shmobile SoC has less DMA channels than devices that can use > DMA, we may want to prioritize access to the DMA channels in the future. > This means that dmaengine_prep_slave_sg() may start failing arbitrarily. Applied both, thanks.
diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c index 38fd938d6360..c850dfdfa9e3 100644 --- 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; irq_mask |= SPCR_SPRIE; } @@ -540,6 +540,12 @@ static int rspi_dma_transfer(struct rspi_data *rspi, struct sg_table *tx, enable_irq(rspi->rx_irq); return ret; + +no_dma: + pr_warn_once("%s %s: DMA not available, falling back to PIO\n", + dev_driver_string(&rspi->master->dev), + dev_name(&rspi->master->dev)); + return -EAGAIN; } static void rspi_receive_init(const struct rspi_data *rspi) @@ -593,8 +599,10 @@ static int rspi_common_transfer(struct rspi_data *rspi, if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { /* rx_buf can be NULL on RSPI on SH in TX-only Mode */ - return rspi_dma_transfer(rspi, &xfer->tx_sg, - xfer->rx_buf ? &xfer->rx_sg : NULL); + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, + xfer->rx_buf ? &xfer->rx_sg : NULL); + if (ret != -EAGAIN) + return ret; } ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len); @@ -648,8 +656,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer) { int ret; - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) - return rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { + ret = rspi_dma_transfer(rspi, &xfer->tx_sg, NULL); + if (ret != -EAGAIN) + return ret; + } ret = rspi_pio_transfer(rspi, xfer->tx_buf, NULL, xfer->len); if (ret < 0) @@ -663,8 +674,11 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct spi_transfer *xfer) static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer) { - if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) - return rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); + if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) { + int ret = rspi_dma_transfer(rspi, NULL, &xfer->rx_sg); + if (ret != -EAGAIN) + return ret; + } return rspi_pio_transfer(rspi, NULL, xfer->rx_buf, xfer->len); }
As typically a shmobile SoC has less DMA channels than devices that can use DMA, we may want to prioritize access to the DMA channels in the future. This means that dmaengine_prep_slave_sg() may start failing arbitrarily. Handle dmaengine_prep_slave_sg() failures gracefully by falling back to PIO. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/spi/spi-rspi.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)