| Message ID | 20231208134929.49523-5-jankul@alatek.krakow.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Miscellaneous xdma driver enhancements | expand |
On 08-12-23, 14:49, Jan Kuliga wrote: > Simplify xdma_xfer_stop(). Stop the dma engine and clear its status > register unconditionally - just do what its name states. This change > also allows to call it without grabbing a lock, which minimizes > the total time spent with a spinlock held. > > Delete the currently processed vd.node from the vc.desc_issued list > prior to passing it to vchan_terminate_vdesc(). In case there's more > than one descriptor pending on vc.desc_issued list, calling > vchan_terminate_desc() results in losing the link between > vc.desc_issued list head and the second descriptor on the list. Doing so > results in resources leakege, as vchan_dma_desc_free_list() won't be > able to properly free memory resources attached to descriptors, > resulting in dma_pool_destroy() failure. > > Don't call vchan_dma_desc_free_list() from within xdma_terminate_all(). > Move all terminated descriptors to the vc.desc_terminated list instead. > This allows to postpone freeing memory resources associated with > descriptors until the call to vchan_synchronize(), which is called from > xdma_synchronize() callback. This is the right way to do it - > xdma_terminate_all() should return as soon as possible, while freeing > resources (that may be time consuming in case of large number of > descriptors) can be done safely later. > > Fixes: 290bb5d2d1e2 > ("dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks") > > Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl> > --- > drivers/dma/xilinx/xdma.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c > index 1bce48e5d86c..521ba2a653b6 100644 > --- a/drivers/dma/xilinx/xdma.c > +++ b/drivers/dma/xilinx/xdma.c > @@ -379,20 +379,20 @@ static int xdma_xfer_start(struct xdma_chan *xchan) > */ > static int xdma_xfer_stop(struct xdma_chan *xchan) > { > - struct virt_dma_desc *vd = vchan_next_desc(&xchan->vchan); > - struct xdma_device *xdev = xchan->xdev_hdl; > int ret; > - > - if (!vd || !xchan->busy) > - return -EINVAL; > + u32 val; > + struct xdma_device *xdev = xchan->xdev_hdl; > > /* clear run stop bit to prevent any further auto-triggering */ > ret = regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_CONTROL_W1C, > - CHAN_CTRL_RUN_STOP); > + CHAN_CTRL_RUN_STOP); Why this change, checkpatch would tell you this is not expected alignment (run with strict) > if (ret) > return ret; > > - xchan->busy = false; > + /* Clear the channel status register */ > + ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val); > + if (ret) > + return ret; > > return 0; > } > @@ -505,25 +505,25 @@ static void xdma_issue_pending(struct dma_chan *chan) > static int xdma_terminate_all(struct dma_chan *chan) > { > struct xdma_chan *xdma_chan = to_xdma_chan(chan); > - struct xdma_desc *desc = NULL; > struct virt_dma_desc *vd; > unsigned long flags; > LIST_HEAD(head); > > - spin_lock_irqsave(&xdma_chan->vchan.lock, flags); > xdma_xfer_stop(xdma_chan); > > + spin_lock_irqsave(&xdma_chan->vchan.lock, flags); > + > + xdma_chan->busy = false; > vd = vchan_next_desc(&xdma_chan->vchan); > - if (vd) > - desc = to_xdma_desc(vd); > - if (desc) { > - dma_cookie_complete(&desc->vdesc.tx); > - vchan_terminate_vdesc(&desc->vdesc); > + if (vd) { > + list_del(&vd->node); > + dma_cookie_complete(&vd->tx); > + vchan_terminate_vdesc(vd); > } > - > vchan_get_all_descriptors(&xdma_chan->vchan, &head); > + list_splice_tail(&head, &xdma_chan->vchan.desc_terminated); > + > spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags); > - vchan_dma_desc_free_list(&xdma_chan->vchan, &head); > > return 0; > } > -- > 2.34.1
Hi Vinod, Thanks for reviewing my patchset. On 11.12.2023 11:54, Vinod Koul wrote: > On 08-12-23, 14:49, Jan Kuliga wrote: >> Simplify xdma_xfer_stop(). Stop the dma engine and clear its status >> register unconditionally - just do what its name states. This change >> also allows to call it without grabbing a lock, which minimizes >> the total time spent with a spinlock held. >> >> Delete the currently processed vd.node from the vc.desc_issued list >> prior to passing it to vchan_terminate_vdesc(). In case there's more >> than one descriptor pending on vc.desc_issued list, calling >> vchan_terminate_desc() results in losing the link between >> vc.desc_issued list head and the second descriptor on the list. Doing so >> results in resources leakege, as vchan_dma_desc_free_list() won't be >> able to properly free memory resources attached to descriptors, >> resulting in dma_pool_destroy() failure. >> >> Don't call vchan_dma_desc_free_list() from within xdma_terminate_all(). >> Move all terminated descriptors to the vc.desc_terminated list instead. >> This allows to postpone freeing memory resources associated with >> descriptors until the call to vchan_synchronize(), which is called from >> xdma_synchronize() callback. This is the right way to do it - >> xdma_terminate_all() should return as soon as possible, while freeing >> resources (that may be time consuming in case of large number of >> descriptors) can be done safely later. >> >> Fixes: 290bb5d2d1e2 >> ("dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks") >> >> Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl> >> --- >> drivers/dma/xilinx/xdma.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c >> index 1bce48e5d86c..521ba2a653b6 100644 >> --- a/drivers/dma/xilinx/xdma.c >> +++ b/drivers/dma/xilinx/xdma.c >> @@ -379,20 +379,20 @@ static int xdma_xfer_start(struct xdma_chan *xchan) >> */ >> static int xdma_xfer_stop(struct xdma_chan *xchan) >> { >> - struct virt_dma_desc *vd = vchan_next_desc(&xchan->vchan); >> - struct xdma_device *xdev = xchan->xdev_hdl; >> int ret; >> - >> - if (!vd || !xchan->busy) >> - return -EINVAL; >> + u32 val; >> + struct xdma_device *xdev = xchan->xdev_hdl; >> >> /* clear run stop bit to prevent any further auto-triggering */ >> ret = regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_CONTROL_W1C, >> - CHAN_CTRL_RUN_STOP); >> + CHAN_CTRL_RUN_STOP); > > Why this change, checkpatch would tell you this is not expected > alignment (run with strict) Actually, it does not. I've run it like this: $LINUX_DIR/scripts/checkpatch.pl --strict -g <commit-id> and it produced no output related to this line. Anyway, I've already prepared v5 patchset, that conforms to your hint: Message-Id: 20231218113904.9071-1-jankul@alatek.krakow.pl > >> i.f (ret) >> return ret; >> >> - xchan->busy = false; >> + /* Clear the channel status register */ >> + ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val); >> + if (ret) >> + return ret; >> >> return 0; >> } >> @@ -505,25 +505,25 @@ static void xdma_issue_pending(struct dma_chan *chan) >> static int xdma_terminate_all(struct dma_chan *chan) >> { >> struct xdma_chan *xdma_chan = to_xdma_chan(chan); >> - struct xdma_desc *desc = NULL; >> struct virt_dma_desc *vd; >> unsigned long flags; >> LIST_HEAD(head); >> >> - spin_lock_irqsave(&xdma_chan->vchan.lock, flags); >> xdma_xfer_stop(xdma_chan); >> >> + spin_lock_irqsave(&xdma_chan->vchan.lock, flags); >> + >> + xdma_chan->busy = false; >> vd = vchan_next_desc(&xdma_chan->vchan); >> - if (vd) >> - desc = to_xdma_desc(vd); >> - if (desc) { >> - dma_cookie_complete(&desc->vdesc.tx); >> - vchan_terminate_vdesc(&desc->vdesc); >> + if (vd) { >> + list_del(&vd->node); >> + dma_cookie_complete(&vd->tx); >> + vchan_terminate_vdesc(vd); >> } >> - >> vchan_get_all_descriptors(&xdma_chan->vchan, &head); >> + list_splice_tail(&head, &xdma_chan->vchan.desc_terminated); >> + >> spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags); >> - vchan_dma_desc_free_list(&xdma_chan->vchan, &head); >> >> return 0; >> } >> -- >> 2.34.1 > Thanks, Jan
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c index 1bce48e5d86c..521ba2a653b6 100644 --- a/drivers/dma/xilinx/xdma.c +++ b/drivers/dma/xilinx/xdma.c @@ -379,20 +379,20 @@ static int xdma_xfer_start(struct xdma_chan *xchan) */ static int xdma_xfer_stop(struct xdma_chan *xchan) { - struct virt_dma_desc *vd = vchan_next_desc(&xchan->vchan); - struct xdma_device *xdev = xchan->xdev_hdl; int ret; - - if (!vd || !xchan->busy) - return -EINVAL; + u32 val; + struct xdma_device *xdev = xchan->xdev_hdl; /* clear run stop bit to prevent any further auto-triggering */ ret = regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_CONTROL_W1C, - CHAN_CTRL_RUN_STOP); + CHAN_CTRL_RUN_STOP); if (ret) return ret; - xchan->busy = false; + /* Clear the channel status register */ + ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val); + if (ret) + return ret; return 0; } @@ -505,25 +505,25 @@ static void xdma_issue_pending(struct dma_chan *chan) static int xdma_terminate_all(struct dma_chan *chan) { struct xdma_chan *xdma_chan = to_xdma_chan(chan); - struct xdma_desc *desc = NULL; struct virt_dma_desc *vd; unsigned long flags; LIST_HEAD(head); - spin_lock_irqsave(&xdma_chan->vchan.lock, flags); xdma_xfer_stop(xdma_chan); + spin_lock_irqsave(&xdma_chan->vchan.lock, flags); + + xdma_chan->busy = false; vd = vchan_next_desc(&xdma_chan->vchan); - if (vd) - desc = to_xdma_desc(vd); - if (desc) { - dma_cookie_complete(&desc->vdesc.tx); - vchan_terminate_vdesc(&desc->vdesc); + if (vd) { + list_del(&vd->node); + dma_cookie_complete(&vd->tx); + vchan_terminate_vdesc(vd); } - vchan_get_all_descriptors(&xdma_chan->vchan, &head); + list_splice_tail(&head, &xdma_chan->vchan.desc_terminated); + spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags); - vchan_dma_desc_free_list(&xdma_chan->vchan, &head); return 0; }
Simplify xdma_xfer_stop(). Stop the dma engine and clear its status register unconditionally - just do what its name states. This change also allows to call it without grabbing a lock, which minimizes the total time spent with a spinlock held. Delete the currently processed vd.node from the vc.desc_issued list prior to passing it to vchan_terminate_vdesc(). In case there's more than one descriptor pending on vc.desc_issued list, calling vchan_terminate_desc() results in losing the link between vc.desc_issued list head and the second descriptor on the list. Doing so results in resources leakege, as vchan_dma_desc_free_list() won't be able to properly free memory resources attached to descriptors, resulting in dma_pool_destroy() failure. Don't call vchan_dma_desc_free_list() from within xdma_terminate_all(). Move all terminated descriptors to the vc.desc_terminated list instead. This allows to postpone freeing memory resources associated with descriptors until the call to vchan_synchronize(), which is called from xdma_synchronize() callback. This is the right way to do it - xdma_terminate_all() should return as soon as possible, while freeing resources (that may be time consuming in case of large number of descriptors) can be done safely later. Fixes: 290bb5d2d1e2 ("dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks") Signed-off-by: Jan Kuliga <jankul@alatek.krakow.pl> --- drivers/dma/xilinx/xdma.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) -- 2.34.1