| Message ID | 1862788.NYkJDoIfhG@avalon (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 08/15/2015 01:42 AM, Laurent Pinchart wrote: > Hi Geert, > > On Thursday 16 July 2015 20:36:49 Geert Uytterhoeven wrote: >> Hi Laurent, >> >> While working on DMA for R-Car Gen2 using the sh-sci serial driver with >> rcar-dmac, I ran into two issues: >> >> 1. Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA >> engine driver does not support resubmitting a DMA descriptor. >> I first tried the patch below, until I ran into the race condition, >> after which I changed sh-sci to not reuse DMA descriptors. > > Is reusing descriptors something that the DMA engine API explicitly allows ? No. It explicitly forbids it. dmaengine_submit() must always be called in a pair with dmaenine_prep_*(). But there is some work in-progress to add support for re-usable descriptors, see http://www.spinics.net/lists/dmaengine/msg05554.html -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 15, 2015 at 11:40 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 08/15/2015 01:42 AM, Laurent Pinchart wrote: >> On Thursday 16 July 2015 20:36:49 Geert Uytterhoeven wrote: >>> While working on DMA for R-Car Gen2 using the sh-sci serial driver with >>> rcar-dmac, I ran into two issues: >>> >>> 1. Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA >>> engine driver does not support resubmitting a DMA descriptor. >>> I first tried the patch below, until I ran into the race condition, >>> after which I changed sh-sci to not reuse DMA descriptors. >> >> Is reusing descriptors something that the DMA engine API explicitly allows ? > > No. It explicitly forbids it. dmaengine_submit() must always be called in a > pair with dmaenine_prep_*(). But there is some work in-progress to add Thanks for the confirmation! So the sh-sci driver violates the DMA engine API. > support for re-usable descriptors, see > http://www.spinics.net/lists/dmaengine/msg05554.html Thanks, in the same thread: * DMA_CTRL_ACK + - If set, does not mean descriptor can be reused http://www.spinics.net/lists/dmaengine/msg05552.html 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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On Sat, Aug 15, 2015 at 1:42 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > From 131968befd5de3400631b879b1574beea27b8239 Mon Sep 17 00:00:00 2001 > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Date: Sat, 15 Aug 2015 01:28:28 +0300 > Subject: [PATCH] dmaengine: rcar-dmac: Fix residue reporting for pending > descriptors > > Cookies corresponding to pending transfers have a residue value equal to > the full size of the corresponding descriptor. The driver miscomputes > that and uses the size of the active descriptor instead. Fix it. Thanks! > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 7820d07e7bee..a98596a1f12f 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -1143,19 +1143,43 @@ static unsigned int rcar_dmac_chan_get_residue(struct > rcar_dmac_chan *chan, > struct rcar_dmac_desc *desc = chan->desc.running; > struct rcar_dmac_xfer_chunk *running = NULL; > struct rcar_dmac_xfer_chunk *chunk; > + enum dma_status status; > unsigned int residue = 0; > unsigned int dptr = 0; > > if (!desc) > return 0; > > + > + /* > + * If the cookie corresponds to a descriptor that has been completed > + * there is no residue. The same check has already been performed by the > + * caller but without holding the channel lock, so the descriptor could > + * now be complete. > + */ > + status = dma_cookie_status(&chan->chan, cookie, NULL); > + if (status == DMA_COMPLETE) > + return 0; > + > /* > * If the cookie doesn't correspond to the currently running transfer > * then the descriptor hasn't been processed yet, and the residue is > * equal to the full descriptor size. > */ > - if (cookie != desc->async_tx.cookie) > - return desc->size; > + if (cookie != desc->async_tx.cookie) { > + list_for_each_entry(desc, &chan->desc.pending, node) { > + if (cookie == desc->async_tx.cookie) > + return desc->size; > + } The descriptor may be either on the pending or active list, so I had to add + list_for_each_entry(desc, &chan->desc.active, node) { + if (cookie == desc->async_tx.cookie) + return desc->size; + } After that it seems to work fine. Will do more testing... > + > + /* > + * No descriptor found for the cookie, there's thus no residue. > + * This shouldn't happen if the calling driver passes a correct > + * cookie value. > + */ > + WARN(1, "No descriptor for cookie!"); > + return 0; > + } 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-sh" 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 Monday 17 August 2015 16:03:52 Geert Uytterhoeven wrote: > On Sat, Aug 15, 2015 at 1:42 AM, Laurent Pinchart wrote: > > From 131968befd5de3400631b879b1574beea27b8239 Mon Sep 17 00:00:00 2001 > > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Date: Sat, 15 Aug 2015 01:28:28 +0300 > > Subject: [PATCH] dmaengine: rcar-dmac: Fix residue reporting for pending > > > > descriptors > > > > Cookies corresponding to pending transfers have a residue value equal to > > the full size of the corresponding descriptor. The driver miscomputes > > that and uses the size of the active descriptor instead. Fix it. > > Thanks! > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > > index 7820d07e7bee..a98596a1f12f 100644 > > --- a/drivers/dma/sh/rcar-dmac.c > > +++ b/drivers/dma/sh/rcar-dmac.c > > @@ -1143,19 +1143,43 @@ static unsigned int > > rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, > > struct rcar_dmac_desc *desc = chan->desc.running; > > struct rcar_dmac_xfer_chunk *running = NULL; > > struct rcar_dmac_xfer_chunk *chunk; > > + enum dma_status status; > > unsigned int residue = 0; > > unsigned int dptr = 0; > > > > if (!desc) > > return 0; > > + > > + /* > > + * If the cookie corresponds to a descriptor that has been > > completed > > + * there is no residue. The same check has already been performed > > by the > > + * caller but without holding the channel lock, so the descriptor > > could > > + * now be complete. > > + */ > > + status = dma_cookie_status(&chan->chan, cookie, NULL); > > + if (status == DMA_COMPLETE) > > + return 0; > > + > > /* > > * If the cookie doesn't correspond to the currently running > > transfer > > * then the descriptor hasn't been processed yet, and the residue > > is > > * equal to the full descriptor size. > > */ > > - if (cookie != desc->async_tx.cookie) > > - return desc->size; > > + if (cookie != desc->async_tx.cookie) { > > + list_for_each_entry(desc, &chan->desc.pending, node) { > > + if (cookie == desc->async_tx.cookie) > > + return desc->size; > > + } > > The descriptor may be either on the pending or active list, so I had to add > > + list_for_each_entry(desc, &chan->desc.active, node) { > + if (cookie == desc->async_tx.cookie) > + return desc->size; > + } > > After that it seems to work fine. Will do more testing... My bad. I'll fix that and submit the patch. Thank you for testing it. > > + > > + /* > > + * No descriptor found for the cookie, there's thus no > > residue. > > + * This shouldn't happen if the calling driver passes a > > correct > > + * cookie value. > > + */ > > + WARN(1, "No descriptor for cookie!"); > > + return 0; > > + }
Hi Laurent, While using this patch from Geert's scif-dma-v3 branch, I see that it makes the rcar_dmac_chan_get_residue method very noisy, because of the WARN_ON in the method. I am using sh-sci driver, which inquires the status of a completed cookie on time-out. The current transaction status is updated in the IRQ thread, which not necessarily schedules right after the interrupt. Time-out occurs frequently, hence, printing out this warning on the console. It was so noisy that I had to disable the WARN_ON in your patch. If you want a warning message here, I'd recommend a dev_dbg instead Best, Hamza On Sat, Aug 15, 2015 at 1:42 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Geert, > > On Thursday 16 July 2015 20:36:49 Geert Uytterhoeven wrote: >> Hi Laurent, >> >> While working on DMA for R-Car Gen2 using the sh-sci serial driver with >> rcar-dmac, I ran into two issues: >> >> 1. Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA >> engine driver does not support resubmitting a DMA descriptor. >> I first tried the patch below, until I ran into the race condition, >> after which I changed sh-sci to not reuse DMA descriptors. > > Is reusing descriptors something that the DMA engine API explicitly allows ? > >> 2. rcar_dmac_chan_get_residue() has: >> >> static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, >> dma_cookie_t cookie) >> { >> struct rcar_dmac_desc *desc = chan->desc.running; >> ... >> >> /* >> * If the cookie doesn't correspond to the currently running >> transfer * then the descriptor hasn't been processed yet, and the residue >> is * equal to the full descriptor size. >> */ >> if (cookie != desc->async_tx.cookie) >> return desc->size; >> >> If the cookie doesn't correspond to the currently running transfer, >> it returns the full descriptor size of the _currently running >> transfer_, not the transfer the cookie corresponds to. > > How about the following (untested) patch ? > > From 131968befd5de3400631b879b1574beea27b8239 Mon Sep 17 00:00:00 2001 > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Date: Sat, 15 Aug 2015 01:28:28 +0300 > Subject: [PATCH] dmaengine: rcar-dmac: Fix residue reporting for pending > descriptors > > Cookies corresponding to pending transfers have a residue value equal to > the full size of the corresponding descriptor. The driver miscomputes > that and uses the size of the active descriptor instead. Fix it. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/dma/sh/rcar-dmac.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 7820d07e7bee..a98596a1f12f 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -1143,19 +1143,43 @@ static unsigned int rcar_dmac_chan_get_residue(struct > rcar_dmac_chan *chan, > struct rcar_dmac_desc *desc = chan->desc.running; > struct rcar_dmac_xfer_chunk *running = NULL; > struct rcar_dmac_xfer_chunk *chunk; > + enum dma_status status; > unsigned int residue = 0; > unsigned int dptr = 0; > > if (!desc) > return 0; > > + > + /* > + * If the cookie corresponds to a descriptor that has been completed > + * there is no residue. The same check has already been performed by the > + * caller but without holding the channel lock, so the descriptor could > + * now be complete. > + */ > + status = dma_cookie_status(&chan->chan, cookie, NULL); > + if (status == DMA_COMPLETE) > + return 0; > + > /* > * If the cookie doesn't correspond to the currently running transfer > * then the descriptor hasn't been processed yet, and the residue is > * equal to the full descriptor size. > */ > - if (cookie != desc->async_tx.cookie) > - return desc->size; > + if (cookie != desc->async_tx.cookie) { > + list_for_each_entry(desc, &chan->desc.pending, node) { > + if (cookie == desc->async_tx.cookie) > + return desc->size; > + } > + > + /* > + * No descriptor found for the cookie, there's thus no residue. > + * This shouldn't happen if the calling driver passes a correct > + * cookie value. > + */ > + WARN(1, "No descriptor for cookie!"); > + return 0; > + } > > /* > * In descriptor mode the descriptor running pointer is not maintained > >> I believe this the reason why the sh-sci driver once thought DMA >> transfered 4294967265 (= -31) bytes (for SCIF, descriptor lengths >> are either 1 or 32 bytes, and (length) 1 - (residue) 32 = >> (transfered) -31). >> >> Thanks for your comments! >> >> From 589dbd908a59dba6efc2a78fca24645962235ec2 Mon Sep 17 00:00:00 2001 >> From: Geert Uytterhoeven <geert+renesas@glider.be> >> Date: Tue, 14 Jul 2015 11:27:14 +0200 >> Subject: [PATCH] [RFC] dmaengine: rcar-dmac: Allow resubmission of DMA >> descriptors >> >> Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA engine >> driver does not support resubmitting a DMA descriptor. If a DMA slave >> resubmits a descriptor, the descriptor will be added to the "pending >> list", while it wasn't removed from the "wait" list. This will cause >> list corruption, leading to an infinite loop in >> rcar_dmac_chan_reinit(). >> >> Find out if the descriptor is reused by looking at the current cookie >> valie, and remove it from the other list if needed: >> - cookie is initialized to -EBUSY (by rcar-dma) for fresh and properly >> recycled descriptors, >> - cookie is set to a strict-positive value by dma_cookie_assign() (in >> core dmaengine code) on submission, >> - cookie is reset to zero by dma_cookie_complete() (in core dmaengine >> code) on completion, >> >> Fix this by removing it from a list if the cookie is not -EBUSY. >> >> FIXME Unfortunately this is racy: the recycled descriptors are not part >> of a list while the DMA descriptor callback is running. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/dma/sh/rcar-dmac.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c >> index 11e5003a6cc27b40..92a8fddb025e6729 100644 >> --- a/drivers/dma/sh/rcar-dmac.c >> +++ b/drivers/dma/sh/rcar-dmac.c >> @@ -437,8 +437,20 @@ static dma_cookie_t rcar_dmac_tx_submit(struct >> dma_async_tx_descriptor *tx) unsigned long flags; >> dma_cookie_t cookie; >> >> + >> spin_lock_irqsave(&chan->lock, flags); >> >> + if (desc->async_tx.cookie != -EBUSY) { >> + /* >> + * If the descriptor is reused, it's currently part of a list. >> + * Hence it must be removed from that list first, before it can >> + * be added to the list of pending requests. >> + */ >> + dev_dbg(chan->chan.device->dev, "chan%u: resubmit active desc %p\n", >> + chan->index, desc); >> + list_del(&desc->node); >> + } >> + >> cookie = dma_cookie_assign(tx); >> >> dev_dbg(chan->chan.device->dev, "chan%u: submit #%d@%p\n", > > -- > Regards, > > Laurent Pinchart > -- > To unsubscribe from this list: send the line "unsubscribe linux-sh" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 7820d07e7bee..a98596a1f12f 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1143,19 +1143,43 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan, struct rcar_dmac_desc *desc = chan->desc.running; struct rcar_dmac_xfer_chunk *running = NULL; struct rcar_dmac_xfer_chunk *chunk; + enum dma_status status; unsigned int residue = 0; unsigned int dptr = 0; if (!desc) return 0; + + /* + * If the cookie corresponds to a descriptor that has been completed + * there is no residue. The same check has already been performed by the + * caller but without holding the channel lock, so the descriptor could + * now be complete. + */ + status = dma_cookie_status(&chan->chan, cookie, NULL); + if (status == DMA_COMPLETE) + return 0; + /* * If the cookie doesn't correspond to the currently running transfer * then the descriptor hasn't been processed yet, and the residue is * equal to the full descriptor size. */ - if (cookie != desc->async_tx.cookie) - return desc->size; + if (cookie != desc->async_tx.cookie) { + list_for_each_entry(desc, &chan->desc.pending, node) { + if (cookie == desc->async_tx.cookie) + return desc->size; + } + + /* + * No descriptor found for the cookie, there's thus no residue. + * This shouldn't happen if the calling driver passes a correct + * cookie value. + */ + WARN(1, "No descriptor for cookie!"); + return 0; + } /*