[PATCH v2 3/4] fsl-dma: change the release process of dma descriptor

[Liu Qiang-B32616]

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+qiang.liu=freescale.com at lists.ozlabs.org] On Behalf Of Liu Qiang-
> B32616
> Sent: Thursday, July 12, 2012 3:12 PM
> To: Ira W. Snyder
> Cc: herbert at gondor.apana.org.au; Vinod Koul; linux-crypto at vger.kernel.org;
> Dan Williams; linuxppc-dev at lists.ozlabs.org; davem at davemloft.net
> Subject: RE: [PATCH v2 3/4] fsl-dma: change the release process of dma
> descriptor
> 
> > -----Original Message-----
> > From: Ira W. Snyder [mailto:iws at ovro.caltech.edu]
> > Sent: Thursday, July 12, 2012 12:31 AM
> > To: Liu Qiang-B32616
> > Cc: linux-crypto at vger.kernel.org; linuxppc-dev at lists.ozlabs.org; Vinod
> > Koul; herbert at gondor.hengli.com.au; Dan Williams; davem at davemloft.net
> > Subject: Re: [PATCH v2 3/4] fsl-dma: change the release process of dma
> > descriptor
> >
> > 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.
> Yes, it's fine for fsl-dma itself, but it cannot work under complex
> condition.
> For example, we enable NET_DMA and SEC xor offload, if NAPI task is waken
> up to synchronize pending requests when raid5 dma copy was submitted, the
> process order of raid5 tx descriptor is not as our expected.
> Unfortunately, sometime we have to check this dependent tx descriptor
> which has was already released.
> 
> >
> > When it was written, it was expected that dma_do_tasklet() would run
> > only when the controller was idle.
> >
> > > Cc: Dan Williams <dan.j.williams at intel.com>
> > > Cc: Vinod Koul <vinod.koul at intel.com>
> > > Cc: Li Yang <leoli at freescale.com>
> > > Signed-off-by: Qiang Liu <qiang.liu at freescale.com>
> > > ---
> > >  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.
> Hi ira, this issue should be not related to dma status. The last
> descriptor is left as usb null descriptor, actually, this descriptor is
> used as usb null descriptor, at any case, I believe it has been already
> completed, but I will released it in next chain, it doesn't affect the
> upper api to use the data, and make sure async_tx api won't raise an
> exception (BUG_ON(async_tx_test_ack(depend_tx)), this depend_tx is the
> desc->async_tx).
I consider your concerns carefully, I think my implement is not a good/right
way.
First, there is possibility that happen to the callback is ignored.
Second, I missed some better ways which can resolve this issue, actually
async_tx api provide our methods to avoid this.

So, I agree with your concern. Thanks.
I will resolve your concerns in next version. Thanks again.

> 
> >
> > If you test without your spin_lock_bh() and spin_unlock_bh()
> > conversion patch, do you still hit the error?
> The error still happened. spin_lock_bh() and spin_unlock_bh() are
> modified after this patch.
> 
> >
> > 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.
> It won't be happened if use fsl-dma, because the right order is
> xor-copy-xor->callback, The callback which you concerned is implemented
> in talitos driver, callback should be null in fsl-dma.
> 
> >
> > >  	/* 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 at lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev