Re: [PATCH v2 3/4] fsl-dma: change the release process of dma descriptor
flat view
From: Ira W. Snyder <hidden>
Date: 2012-07-11 16:31:03
Also in:
linux-crypto
On Wed, Jul 11, 2012 at 05:01:25PM +0800, Qiang Liu wrote:
Modify the release process of dma descriptor for avoiding exception when enable config NET_DMA, release dma descriptor from 1st to last second, the last descriptor which is reserved in current descriptor register may not be completed, race condition will be raised if free current descriptor. A race condition which is raised when use both of talitos and dmaengine to offload xor is because napi scheduler (NET_DMA is enabled) will sync all pending requests in dma channels, it affects the process of raid operations. The descriptor is freed which is submitted just now, but async_tx must check whether this depend tx descriptor is acked, there are poison contents in the invalid address, then BUG_ON() is thrown, so this descriptor will be freed in the next time.
This patch seems to be covering up a bug in the driver, rather than actually fixing it. When it was written, it was expected that dma_do_tasklet() would run only when the controller was idle.
quoted hunk ↗ jump to hunk
Cc: Dan Williams <redacted> Cc: Vinod Koul <redacted> Cc: Li Yang <redacted> Signed-off-by: Qiang Liu <redacted> --- drivers/dma/fsldma.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-)diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index 4f2f212..0ba3e40 100644 --- a/drivers/dma/fsldma.c +++ b/drivers/dma/fsldma.c@@ -1035,14 +1035,22 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data) static void dma_do_tasklet(unsigned long data) { struct fsldma_chan *chan = (struct fsldma_chan *)data; - struct fsl_desc_sw *desc, *_desc; + struct fsl_desc_sw *desc, *_desc, *prev = NULL; LIST_HEAD(ld_cleanup); unsigned long flags; + dma_addr_t curr_phys = get_cdar(chan); chan_dbg(chan, "tasklet entry\n"); spin_lock_irqsave(&chan->desc_lock, flags); + /* find the descriptor which is already completed */ + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) { + if (prev && desc->async_tx.phys == curr_phys) + break; + prev = desc; + } +
If the DMA controller was still busy processing transactions, you should have gotten the printout "irq: controller not idle!" from fsldma_chan_irq() just before it scheduled the dma_do_tasklet() to run. If you did not get this printout, how was dma_do_tasklet() entered with the controller still busy? I don't understand how it can happen. If you test without your spin_lock_bh() and spin_unlock_bh() conversion patch, do you still hit the error? What happens if a user submits exactly one DMA transaction, and then leaves the system idle? The callback for the last descriptor in the chain will never get run, right? That's a bug.
quoted hunk ↗ jump to hunk
/* update the cookie if we have some descriptors to cleanup */ if (!list_empty(&chan->ld_running)) { dma_cookie_t cookie;@@ -1058,13 +1066,14 @@ static void dma_do_tasklet(unsigned long data) * move the descriptors to a temporary list so we can drop the lock * during the entire cleanup operation */ - list_splice_tail_init(&chan->ld_running, &ld_cleanup); + list_cut_position(&ld_cleanup, &chan->ld_running, &prev->node); /* the hardware is now idle and ready for more */ chan->idle = true; /* - * Start any pending transactions automatically + * Start any pending transactions automatically if current descriptor + * list is completed * * In the ideal case, we keep the DMA controller busy while we go * ahead and free the descriptors below. --1.7.5.1 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev