Message-ID: <20120802155539.GB14412@ovro.caltech.edu>
Date:	Thu, 2 Aug 2012 08:55:39 -0700
From:	"Ira W. Snyder" <iws@...o.caltech.edu>
To:	Liu Qiang-B32616 <B32616@...escale.com>
Cc:	"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dan.j.williams@...il.com" <dan.j.williams@...il.com>,
	Vinod Koul <vinod.koul@...el.com>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	Dan Williams <dan.j.williams@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v5 3/6] fsl-dma: change release process of dma
 descriptor for supporting async_tx

On Thu, Aug 02, 2012 at 07:21:51AM +0000, Liu Qiang-B32616 wrote:
> > -----Original Message-----
> > From: Ira W. Snyder [mailto:iws@...o.caltech.edu]
> > Sent: Thursday, August 02, 2012 1:25 AM
> > To: Liu Qiang-B32616
> > Cc: linux-crypto@...r.kernel.org; linuxppc-dev@...ts.ozlabs.org; linux-
> > kernel@...r.kernel.org; dan.j.williams@...il.com; Vinod Koul;
> > herbert@...dor.hengli.com.au; Dan Williams; davem@...emloft.net
> > Subject: Re: [PATCH v5 3/6] fsl-dma: change release process of dma
> > descriptor for supporting async_tx
> > 
> > On Wed, Aug 01, 2012 at 04:49:17PM +0800, qiang.liu@...escale.com wrote:
> > > From: Qiang Liu <qiang.liu@...escale.com>
> > >
> > > Fix the potential risk when enable config NET_DMA and ASYNC_TX.
> > > Async_tx is lack of support in current release process of dma
> > > descriptor, all descriptors will be released whatever is acked or
> > > no-acked by async_tx, so there is a potential race condition when dma
> > > engine is uesd by others clients (e.g. when enable NET_DMA to offload
> > TCP).
> > >
> > > In our case, a race condition which is raised when use both of talitos
> > > and dmaengine to offload xor is because napi scheduler will sync all
> > > pending requests in dma channels, it affects the process of raid
> > > operations due to ack_tx is not checked in fsl dma. The no-acked
> > > descriptor is freed which is submitted just now, as a dependent tx,
> > > this freed descriptor trigger
> > > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> > >
> > > TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
> > > GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4
> > > 00000000 00000001
> > > GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4
> > > ed576d98 00000000
> > > GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000
> > > ed3015e8 c15a7aa0
> > > GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0
> > > ef640c30 ecf41ca0 NIP [c02b048c] async_tx_submit+0x6c/0x2b4 LR
> > > [c02b068c] async_tx_submit+0x26c/0x2b4 Call Trace:
> > > [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> > > [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c [ecf41d20] [c0421064]
> > > async_copy_data+0xa0/0x17c [ecf41d70] [c0421cf4]
> > > __raid_run_ops+0x874/0xe10 [ecf41df0] [c0426ee4]
> > > handle_stripe+0x820/0x25e8 [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> > > [ecf41f40] [c04329b8] md_thread+0x138/0x16c [ecf41f90] [c008277c]
> > > kthread+0x8c/0x90 [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
> > >
> > > Another major modification in this patch is the change to completed
> > > descriptors, there is a potential risk which caused by exception
> > > interrupt, all descriptors in ld_running list are seemed completed
> > > when an interrupt raised, it works fine under normal condition, but if
> > > there is an exception occured, it cannot work as our excepted.
> > > Hardware should not depend on s/w list, the right way is to read
> > > current descriptor address register to find the last completed
> > > descriptor. If an interrupt is raised by an error, all descriptors in
> > > ld_running should not be seemed finished, or these unfinished
> > descriptors in ld_running will be released wrongly.
> > >
> > > A simple way to reproduce,
> > > Enable dmatest first, then insert some bad descriptors which can
> > > trigger Programming Error interrupts before the good descriptors.
> > > Last, the good descriptors will be freed before they are processsed
> > > because of the exception intrerrupt.
> > >
> > > Note: the bad descriptors are only for simulating an exception
> > interrupt.
> > > This case can illustrate the potential risk in current fsl-dma very
> > well.
> > >
> > 
> > I've never managed to trigger a PE (programming error) interrupt on the
> > 83xx hardware. Any time I intentionally caused an error, the hardware
> > wedged itself. The CB (channel busy) bit is stuck high, and cannot be
> > cleared without a hard reset of the board.
> Sorry, the exception indeed will be occurred, actually, the capability DMA_INTERRUPT
> will reproduce the issue under conditions. It will trigger a exception because of
> bad descriptor (length is zero, src and dst are zero, a->b->c->bada->badb->d, we cannot find out which one is really finished in an interrupt tasklet).
> So, we'd better consider this case.
> 
> BTW, I have already found out your patch which is used to resolve the issue of dma lock,
> http://lkml.indiana.edu/hypermail/linux/kernel/1103.0/01519.html
> 

Ok. I haven't tested bad descriptors since several years ago. I agree
that it can happen, so we should fix it.

> > 
> > I agree the "snoop on the hardware" technique works. As far as I can tell,
> > you have implemented the code correctly.
> > 
> > The MPC8349EARM.pdf from Freescale indicates that the hardware will halt
> > in response to a programming error, and generate a PE interrupt. See
> > section 12.5.3.3 (pg 568).
> > 
> > The driver, as it is written, will never recover from such a condition.
> > Since you are complaining about this situation, do you intend to fix it?
> Frankly, I don't think your patch really can resolve the issue. Now, I understand what problem happen to you, I will follow it.
> 

You are correct. My patch does not resolve the issue.

> Yes, you are right, the driver will never recover except reset the board.
> I see your description and I can reproduce it with dmatest on p1022ds with latest kernel, Dmatest with 6 threads, 200,000 iterations per thread several is passed with or without my patch, but dma is locked when up to 300,000 itrerations even though drop my patch.
> Another test on p4080, 8 threads with 1,000,000 iterations per thread is passed with/without my patch.
> I will follow this issue and try to find the root cause, but it should be another topic:)
> 

I agree, it can be another topic. We can focus on getting the MD RAID
problems fixed first, and then fix the lockup.

Do you have CONFIG_NET_DMA enabled? I have always set CONFIG_NET_DMA=n,
I do not use it. I wonder if this is a factor.

> Here, I agree with yours most comments, I will merge some functions from your patch, I will send v6 if you agree with my comments. Thanks.
> Please see my comments inline.
> 

All of the comments below look good. I look forward to reviewing v6.

Thanks for responding,
Ira

> > 
> > > Cc: Dan Williams <dan.j.williams@...el.com>
> > > Cc: Dan Williams <dan.j.williams@...il.com>
> > > Cc: Vinod Koul <vinod.koul@...el.com>
> > > Cc: Li Yang <leoli@...escale.com>
> > > Cc: Ira W. Snyder <iws@...o.caltech.edu>
> > > Signed-off-by: Qiang Liu <qiang.liu@...escale.com>
> > > ---
> > >  drivers/dma/fsldma.c |  242 +++++++++++++++++++++++++++++++++++-------
> > --------
> > >  drivers/dma/fsldma.h |    1 +
> > >  2 files changed, 172 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index
> > > 4f2f212..87f52c0 100644
> > > --- a/drivers/dma/fsldma.c
> > > +++ b/drivers/dma/fsldma.c
> > > @@ -400,6 +400,125 @@ out_splice:
> > >  	list_splice_tail_init(&desc->tx_list, &chan->ld_pending);  }
> > >
> > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan);
> > > +static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan);
> > > +
> > 
> > As noted in my reply to patch 4/6, please swap the order of this patch
> > and the following patch.
> > 
> > These lines should not be added or removed in either patch.
> Ok.
> 
> > 
> > > +/**
> > > + * fsldma_clean_completed_descriptor - free all descriptors which
> > > + * has been completed and acked
> > > + * @chan: Freescale DMA channel
> > > + *
> > > + * This function is used on all completed and acked descriptors.
> > > + * All descriptors should only be freed in this function.
> > > + */
> > > +static int
> > > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
> > 
> > This should be 'static void'. It does not return an error code.
> > 
> Ok.
> 
> > > +{
> > > +	struct fsl_desc_sw *desc, *_desc;
> > > +
> > > +	/* Run the callback for each descriptor, in order */
> > > +	list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node) {
> > > +
> > > +		if (async_tx_test_ack(&desc->async_tx)) {
> > > +			/* Remove from the list of transactions */
> > > +			list_del(&desc->node);
> > > +#ifdef FSL_DMA_LD_DEBUG
> > > +			chan_dbg(chan, "LD %p free\n", desc); #endif
> > > +			dma_pool_free(chan->desc_pool, desc,
> > > +					desc->async_tx.phys);
> > 
> > This code appears in multiple places in the driver. Please consider
> > adding my patch 3/7 titled "[PATCH 3/7] fsl-dma: add
> > fsl_dma_free_descriptor() to reduce code duplication" to your patch
> > series.
> Accept.
> 
> > 
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * fsldma_run_tx_complete_actions - cleanup and free a single link
> > > +descriptor
> > 
> > This documentation is incorrect. This code NEVER frees a descriptor.
> I will correct it.
> 
> > 
> > > + * @chan: Freescale DMA channel
> > > + * @desc: descriptor to cleanup and free
> > > + * @cookie: Freescale DMA transaction identifier
> > > + *
> > > + * This function is used on a descriptor which has been executed by
> > > +the DMA
> > > + * controller. It will run any callbacks, submit any dependencies.
> > > + */
> > > +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsl_desc_sw
> > *desc,
> > > +		struct fsldma_chan *chan, dma_cookie_t cookie)
> > 
> > Please change the parameter order to:
> > 
> > static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan
> > *chan,
> > 		struct fsl_desc_sw *desc, dma_cookie_t cookie)
> > 
> > Every other function in the driver uses this parameter order. Channel
> > comes first, then descriptor.
> > 
> My fault, I will correct it.
> 
> > > +{
> > > +	struct dma_async_tx_descriptor *txd = &desc->async_tx;
> > > +	struct device *dev = chan->common.device->dev;
> > > +	dma_addr_t src = get_desc_src(chan, desc);
> > > +	dma_addr_t dst = get_desc_dst(chan, desc);
> > > +	u32 len = get_desc_cnt(chan, desc);
> > > +
> > > +	BUG_ON(txd->cookie < 0);
> > > +
> > > +	if (txd->cookie > 0) {
> > 
> > It will significantly reduce your patch size if you move this if
> > statement to the function which calls this one. I've provided an example
> > down below, in the one place where this code is used.
> My comments as below.
> 
> > 
> > > +		cookie = txd->cookie;
> > > +
> > > +		/* Run the link descriptor callback function */
> > > +		if (txd->callback) {
> > > +#ifdef FSL_DMA_LD_DEBUG
> > > +			chan_dbg(chan, "LD %p callback\n", desc); #endif
> > > +			txd->callback(txd->callback_param);
> > > +		}
> > > +
> > > +		/* Unmap the dst buffer, if requested */
> > > +		if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > > +			if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > > +				dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > > +			else
> > > +				dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > > +		}
> > > +
> > > +		/* Unmap the src buffer, if requested */
> > > +		if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > > +			if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > > +				dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > > +			else
> > > +				dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > > +		}
> > > +	}
> > > +
> > > +	/* Run any dependencies */
> > > +	dma_run_dependencies(txd);
> > > +
> > > +	return cookie;
> > > +}
> > > +
> > > +/**
> > > + * fsldma_clean_running_descriptor - move the completed descriptor
> > > +from
> > > + * ld_running to ld_completed
> > > + * @chan: Freescale DMA channel
> > > + * @desc: the descriptor which is completed
> > > + *
> > > + * Free the descriptor directly if acked by async_tx api, or move it
> > > +to
> > > + * queue ld_completed.
> > > + */
> > > +static int
> > 
> > This code never returns an error code. It should be 'static void'.
> I will correct it.
> 
> > 
> > > +fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> > > +		struct fsl_desc_sw *desc)
> > > +{
> > > +	/* Remove from the list of transactions */
> > > +	list_del(&desc->node);
> > > +	/*
> > > +	 * the client is allowed to attach dependent operations
> > > +	 * until 'ack' is set
> > > +	 */
> > > +	if (!async_tx_test_ack(&desc->async_tx)) {
> > > +		/*
> > > +		 * Move this descriptor to the list of descriptors which is
> > > +		 * completed, but still awaiting the 'ack' bit to be set.
> > > +		 */
> > > +		list_add_tail(&desc->node, &chan->ld_completed);
> > > +		return 0;
> > > +	}
> > > +
> > > +	dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> > > +	return 0;
> > > +}
> > > +
> > >  static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor
> > > *tx)  {
> > >  	struct fsldma_chan *chan = to_fsl_chan(tx->chan); @@ -534,8 +653,10
> > > @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
> > >
> > >  	chan_dbg(chan, "free all channel resources\n");
> > >  	spin_lock_irqsave(&chan->desc_lock, flags);
> > > +	fsldma_cleanup_descriptor(chan);
> > >  	fsldma_free_desc_list(chan, &chan->ld_pending);
> > >  	fsldma_free_desc_list(chan, &chan->ld_running);
> > > +	fsldma_free_desc_list(chan, &chan->ld_completed);
> > >  	spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > >  	dma_pool_destroy(chan->desc_pool);
> > > @@ -819,46 +940,53 @@ static int fsl_dma_device_control(struct dma_chan
> > *dchan,
> > >   * controller. It will run any callbacks, submit any dependencies, and
> > then
> > >   * free the descriptor.
> > >   */
> > 
> > This documentation is now wrong. This function no longer operates on a
> > single descriptor. It operates on all descriptors in ld_running and
> > ld_completed.
> > 
> > Please fix the documentation, and add locking notes.
> No, it only frees one descriptor.
> 
> > 
> > > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> > > -				      struct fsl_desc_sw *desc)
> > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
> > 
> > I think the name should change to fsldma_cleanup_descriptors(). It cleans
> > up one or more descriptors now.
> It only frees one descriptor as its designed.
> 
> > 
> > >  {
> > > -	struct dma_async_tx_descriptor *txd = &desc->async_tx;
> > > -	struct device *dev = chan->common.device->dev;
> > > -	dma_addr_t src = get_desc_src(chan, desc);
> > > -	dma_addr_t dst = get_desc_dst(chan, desc);
> > > -	u32 len = get_desc_cnt(chan, desc);
> > > +	struct fsl_desc_sw *desc, *_desc;
> > > +	dma_cookie_t cookie = 0;
> > > +	dma_addr_t curr_phys = get_cdar(chan);
> > > +	int idle = dma_is_idle(chan);
> > > +	int seen_current = 0;
> > >
> > 
> > The hardware can advance quite a bit between here, where you save the
> > current descriptor address and idle status.
> > 
> > > -	/* Run the link descriptor callback function */
> > > -	if (txd->callback) {
> > > -#ifdef FSL_DMA_LD_DEBUG
> > > -		chan_dbg(chan, "LD %p callback\n", desc);
> > > -#endif
> > > -		txd->callback(txd->callback_param);
> > > -	}
> > > +	fsldma_clean_completed_descriptor(chan);
> > >
> > > -	/* Run any dependencies */
> > > -	dma_run_dependencies(txd);
> > > +	/* Run the callback for each descriptor, in order */
> > > +	list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > > +		/*
> > > +		 * do not advance past the current descriptor loaded into the
> > > +		 * hardware channel, subsequent descriptors are either in
> > > +		 * process or have not been submitted
> > > +		 */
> > > +		if (seen_current)
> > > +			break;
> > >
> > > -	/* Unmap the dst buffer, if requested */
> > > -	if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > > -		if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > > -			dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > > -		else
> > > -			dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > > -	}
> > > +		/*
> > > +		 * stop the search if we reach the current descriptor and the
> > > +		 * channel is busy
> > > +		 */
> > > +		if (desc->async_tx.phys == curr_phys) {
> > > +			seen_current = 1;
> > > +			if (!idle)
> > > +				break;
> > > +		}
> > 
> > And here, where you check the current descriptor address and idle status.
> > 
> > Should this change to:
> > 
> > if (desc->async_tx.phys == get_cdar(chan)) {
> > 	seen_current = 1;
> > 	if (!dma_is_idle(chan))
> > 		break;
> > }
> Ok, I will use your code here.
> 
> > 
> > > +
> > > +		cookie = fsldma_run_tx_complete_actions(desc, chan, cookie);
> > > +
> > 
> > I would prefer if the code just kept track of the cookie here, rather
> > than passing it through this function call. This code also illustrates
> > how you can remove the "if (txd->cookie > 0)" check from
> > fsldma_run_tx_complete_actions() to reduce the patch size.
> > 
> I cannot agree with you, patch size is important, but program readable is also important.
> My reason as below,
> According to my knowledge, cookie is used to judge whether this descriptor is finished, if it is zero, it means we didn't assign a value for it. We should keep it original meaning for clear?
> Second, I think we should not set a complex process to free descriptor, especially according to different state of the descriptor, the interface should be seemed more reusable and common.
> Last, I don't want to see the interface is coupled too many functions. It's easier extended for future.
> How's your thinking? Of course, your implement is also ok.
> 
> > /*
> >  * Only descriptors with non-zero cookies need their completion
> >  * actions run.
> >  */
> > if (desc->async_tx.cookie > 0) {
> > 	cookie = desc->async_tx.cookie;
> > 	fsldma_run_tx_complete_actions(chan, desc);
> > 	desc->async_tx.cookie = 0;
> > }
> > 
> > /* This descriptor has been ACKed, free it */ if
> > (async_tx_test_ack(&desc->async_tx)) {
> > 	fsl_dma_free_descriptor(chan, desc);
> > 	continue;
> > }
> > 
> > /*
> >  * This descriptor was not ACKed, add it to the ld_completed
> >  * list, to be freed after the ACK bit is set.
> >  */
> > list_del(&desc->node);
> > list_add_tail(&desc->node, &chan->ld_completed);
> > 
> > 
> > > +		if (fsldma_clean_running_descriptor(chan, desc))
> > > +			break;
> > >
> > 
> > This if statement will never trigger. fsldma_clean_running_descriptor()
> > only returns 0. It is useless.
> > 
> > > -	/* Unmap the src buffer, if requested */
> > > -	if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > > -		if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > > -			dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > > -		else
> > > -			dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > >  	}
> > >
> > > -#ifdef FSL_DMA_LD_DEBUG
> > > -	chan_dbg(chan, "LD %p free\n", desc);
> > > -#endif
> > > -	dma_pool_free(chan->desc_pool, desc, txd->phys);
> > > +	/*
> > > +	 * Start any pending transactions automatically
> > > +	 *
> > > +	 * In the ideal case, we keep the DMA controller busy while we go
> > > +	 * ahead and free the descriptors below.
> > > +	 */
> > > +	fsl_chan_xfer_ld_queue(chan);
> > > +
> > > +	if (cookie > 0)
> > > +		chan->common.completed_cookie = cookie;
> > >  }
> > >
> > >  /**
> > > @@ -954,11 +1082,15 @@ static enum dma_status fsl_tx_status(struct
> > dma_chan *dchan,
> > >  	enum dma_status ret;
> > >  	unsigned long flags;
> > >
> > > -	spin_lock_irqsave(&chan->desc_lock, flags);
> > >  	ret = dma_cookie_status(dchan, cookie, txstate);
> > > +	if (ret == DMA_SUCCESS)
> > > +		return ret;
> > > +
> > > +	spin_lock_irqsave(&chan->desc_lock, flags);
> > > +	fsldma_cleanup_descriptor(chan);
> > >  	spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > > -	return ret;
> > > +	return dma_cookie_status(dchan, cookie, txstate);
> > >  }
> > >
> > >
> > > /*--------------------------------------------------------------------
> > > --------*/ @@ -1035,52 +1167,19 @@ 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;
> > > -	LIST_HEAD(ld_cleanup);
> > >  	unsigned long flags;
> > >
> > >  	chan_dbg(chan, "tasklet entry\n");
> > >
> > >  	spin_lock_irqsave(&chan->desc_lock, flags);
> > >
> > > -	/* update the cookie if we have some descriptors to cleanup */
> > > -	if (!list_empty(&chan->ld_running)) {
> > > -		dma_cookie_t cookie;
> > > -
> > > -		desc = to_fsl_desc(chan->ld_running.prev);
> > > -		cookie = desc->async_tx.cookie;
> > > -		dma_cookie_complete(&desc->async_tx);
> > > -
> > > -		chan_dbg(chan, "completed_cookie=%d\n", cookie);
> > > -	}
> > > -
> > > -	/*
> > > -	 * 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);
> > > -
> > >  	/* the hardware is now idle and ready for more */
> > >  	chan->idle = true;
> > >
> > > -	/*
> > > -	 * Start any pending transactions automatically
> > > -	 *
> > > -	 * In the ideal case, we keep the DMA controller busy while we go
> > > -	 * ahead and free the descriptors below.
> > > -	 */
> > > -	fsl_chan_xfer_ld_queue(chan);
> > > -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> > > -
> > > -	/* Run the callback for each descriptor, in order */
> > > -	list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
> > > +	/* Run all cleanup for this descriptor */
> > 
> > Nitpick. This should be:
> > 
> > /* Run cleanup for all descriptors */
> No, this "all" means all operations for one descriptor but not "all descriptors".
> 
> > 
> > > +	fsldma_cleanup_descriptor(chan);
> > >
> > > -		/* Remove from the list of transactions */
> > > -		list_del(&desc->node);
> > > -
> > > -		/* Run all cleanup for this descriptor */
> > > -		fsldma_cleanup_descriptor(chan, desc);
> > > -	}
> > > +	spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > >  	chan_dbg(chan, "tasklet exit\n");
> > >  }
> > > @@ -1262,6 +1361,7 @@ static int __devinit fsl_dma_chan_probe(struct
> > fsldma_device *fdev,
> > >  	spin_lock_init(&chan->desc_lock);
> > >  	INIT_LIST_HEAD(&chan->ld_pending);
> > >  	INIT_LIST_HEAD(&chan->ld_running);
> > > +	INIT_LIST_HEAD(&chan->ld_completed);
> > >  	chan->idle = true;
> > >
> > >  	chan->common.device = &fdev->common; diff --git
> > > a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h index f5c3879..7ede908
> > > 100644
> > > --- a/drivers/dma/fsldma.h
> > > +++ b/drivers/dma/fsldma.h
> > > @@ -140,6 +140,7 @@ struct fsldma_chan {
> > >  	spinlock_t desc_lock;		/* Descriptor operation lock */
> > >  	struct list_head ld_pending;	/* Link descriptors queue */
> > >  	struct list_head ld_running;	/* Link descriptors queue */
> > > +	struct list_head ld_completed;	/* Link descriptors queue */
> > 
> > It may help to add some documentation here. It would have helped me to
> > review this patch. Something like this:
> > 
> > /*
> >  * Descriptors which are queued to run, but have not yet been handed
> >  * to the hardware for execution
> >  */
> > struct list_head ld_pending;
> > 
> > /*
> >  * Descriptors which are currently being executed by the hardware  */
> > struct list_head ld_running;
> > 
> > /*
> >  * Descriptors which have finished execution by the hardware. These
> >  * descriptors have already had their cleanup actions run. They are
> >  * waiting for the ACK bit to be set by the async_tx API.
> >  */
> > struct list_head ld_completed;
> Ok, I will add these comments. Thanks.
> 
> > 
> > >  	struct dma_chan common;		/* DMA common channel */
> > >  	struct dma_pool *desc_pool;	/* Descriptors pool */
> > >  	struct device *dev;		/* Channel device */
> > > --
> > > 1.7.5.1
> > >
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@...ts.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists