<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from rtf -->
<style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 2px solid; } --></style>
</head>
<body>
<font face="Consolas" size="2"><span style="font-size:10.5pt;">
<div>Hi Ira,</div>
<div> </div>
<div>My comments inline.</div>
<div><font face="Times New Roman"> </font></div>
<div>Hi Ira and Dan,</div>
<div><font face="Times New Roman"> </font></div>
<div>Here is a introduction about my scenario of this case.</div>
<div><font face="Times New Roman"> </font></div>
<div>My case is use async_tx API to offload raid5,</div>
<div>Use fsl-talitos to compute parity calculation, fsl-dma to process memcpy.</div>
<div><font face="Times New Roman"> </font></div>
<div>What bug occurred to me?</div>
<div>Exception is thrown by BUG_ON(async_tx_ack_test(depend_tx)) in async_tx_submit().</div>
<div><font face="Times New Roman"> </font></div>
<div>TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0</div>
<div>GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4 00000000 00000001 </div>
<div>GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4 ed576d98 00000000 </div>
<div>GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000 ed3015e8 c15a7aa0 </div>
<div>GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0 ef640c30 ecf41ca0 </div>
<div>NIP [c02b048c] async_tx_submit+0x6c/0x2b4</div>
<div>LR [c02b068c] async_tx_submit+0x26c/0x2b4</div>
<div>Call Trace:</div>
<div>[ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)</div>
<div>[ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c</div>
<div>[ecf41d20] [c0421064] async_copy_data+0xa0/0x17c</div>
<div>[ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10</div>
<div>[ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8</div>
<div>[ecf41e90] [c0429080] raid5d+0x3d4/0x5b4</div>
<div>[ecf41f40] [c04329b8] md_thread+0x138/0x16c</div>
<div>[ecf41f90] [c008277c] kthread+0x8c/0x90</div>
<div>[ecf41ff0] [c0011630] kernel_thread+0x4c/0x68</div>
<div><font face="Times New Roman"> </font></div>
<div>What is the root cause of this bug?</div>
<div>The async_tx descriptor(depend_tx) is free'ed before async_tx_submit ack it.</div>
<div>That means async_tx descriptor should be acked before it's free'ed. In other</div>
<div>word, only ack'ed async_tx descriptor should be free'ed by dma engine.</div>
<div><font face="Times New Roman"> </font></div>
<div>How to fix it?</div>
<div>Add a queue ld_completed, move all completed descriptors in this queue, free</div>
<div>these descriptors at a proper time (these descriptors also should be acked).</div>
<div><font face="Times New Roman"> </font></div>
<div>Dan, do you think my understand about "ack" is right? Thanks. </div>
<div><font face="Times New Roman"> </font></div>
<div>- Qiang</div>
<div><font face="Times New Roman"> </font></div>
<div>> -----Original Message-----</div>
<div>> From: Ira W. Snyder [<a href="mailto:iws@ovro.caltech.edu">mailto:iws@ovro.caltech.edu</a>]</div>
<div>> Sent: Wednesday, July 18, 2012 12:17 AM</div>
<div>> To: Liu Qiang-B32616</div>
<div>> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Phillips</div>
<div>> Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemloft.net; Dan</div>
<div>> Williams; Vinod Koul; Li Yang-R58472</div>
<div>> Subject: Re: [PATCH v3 3/4] fsl-dma: change release process of dma</div>
<div>> descriptor for supporting async_tx</div>
<div>> </div>
<div>> On Tue, Jul 17, 2012 at 07:06:33AM +0000, Liu Qiang-B32616 wrote:</div>
<div>> > > -----Original Message-----</div>
<div>> > > From: Ira W. Snyder [<a href="mailto:iws@ovro.caltech.edu">mailto:iws@ovro.caltech.edu</a>]</div>
<div>> > > Sent: Tuesday, July 17, 2012 4:01 AM</div>
<div>> > > To: Liu Qiang-B32616</div>
<div>> > > Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;</div>
<div>> Phillips</div>
<div>> > > Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemloft.net; Dan</div>
<div>> > > Williams; Vinod Koul; Li Yang-R58472</div>
<div>> > > Subject: Re: [PATCH v3 3/4] fsl-dma: change release process of dma</div>
<div>> > > descriptor for supporting async_tx</div>
<div>> > ></div>
<div>> > > On Mon, Jul 16, 2012 at 12:08:29PM +0800, Qiang Liu wrote:</div>
<div>> > > > Fix the potential risk when enable config NET_DMA and ASYNC_TX.</div>
<div>> > > > Async_tx is lack of support in current release process of dma</div>
<div>> > > > descriptor, all descriptors will be released whatever is acked or</div>
<div>> > > > no-acked by async_tx, so there is a potential race condition when</div>
<div>> dma</div>
<div>> > > > engine is uesd by others clients (e.g. when enable NET_DMA to</div>
<div>> offload</div>
<div>> > > TCP).</div>
<div>> > > ></div>
<div>> > > > In our case, a race condition which is raised when use both of</div>
<div>> talitos</div>
<div>> > > > and dmaengine to offload xor is because napi scheduler will sync</div>
<div>> all</div>
<div>> > > > pending requests in dma channels, it affects the process of raid</div>
<div>> > > > operations due to ack_tx is not checked in fsl dma. The no-acked</div>
<div>> > > > descriptor is freed which is submitted just now, as a dependent tx,</div>
<div>> > > > this freed descriptor trigger</div>
<div>> > > > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().</div>
<div>> > > ></div>
<div>> > > > Cc: Dan Williams <dan.j.williams@intel.com></div>
<div>> > > > Cc: Vinod Koul <vinod.koul@intel.com></div>
<div>> > > > Cc: Li Yang <leoli@freescale.com></div>
<div>> > > > Cc: Ira W. Snyder <iws@ovro.caltech.edu></div>
<div>> > > > Signed-off-by: Qiang Liu <qiang.liu@freescale.com></div>
<div>> > > > ---</div>
<div>> > > > drivers/dma/fsldma.c | 378 +++++++++++++++++++++++++++++---------</div>
<div>> ----</div>
<div>> > > -------</div>
<div>> > > > drivers/dma/fsldma.h | 1 +</div>
<div>> > > > 2 files changed, 225 insertions(+), 154 deletions(-)</div>
<div>> > > ></div>
<div>> > > > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index</div>
<div>> > > > 4f2f212..4ee1b8f 100644</div>
<div>> > > > --- a/drivers/dma/fsldma.c</div>
<div>> > > > +++ b/drivers/dma/fsldma.c</div>
<div>> > > > @@ -400,6 +400,217 @@ out_splice:</div>
<div>> > > > list_splice_tail_init(&desc->tx_list, &chan->ld_pending); }</div>
<div>> > > ></div>
<div>> > > > +/**</div>
<div>> > > > + * fsl_chan_xfer_ld_queue - transfer any pending transactions</div>
<div>> > > > + * @chan : Freescale DMA channel</div>
<div>> > > > + *</div>
<div>> > > > + * HARDWARE STATE: idle</div>
<div>> > > > + * LOCKING: must hold chan->desc_lock */ static void</div>
<div>> > > > +fsl_chan_xfer_ld_queue(struct fsldma_chan *chan) {</div>
<div>> > > > + struct fsl_desc_sw *desc;</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * If the list of pending descriptors is empty, then we</div>
<div>> > > > + * don't need to do any work at all</div>
<div>> > > > + */</div>
<div>> > > > + if (list_empty(&chan->ld_pending)) {</div>
<div>> > > > + chan_dbg(chan, "no pending LDs\n");</div>
<div>> > > > + return;</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * The DMA controller is not idle, which means that the</div>
<div>> interrupt</div>
<div>> > > > + * handler will start any queued transactions when it runs</div>
<div>> after</div>
<div>> > > > + * this transaction finishes</div>
<div>> > > > + */</div>
<div>> > > > + if (!chan->idle) {</div>
<div>> > > > + chan_dbg(chan, "DMA controller still busy\n");</div>
<div>> > > > + return;</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * If there are some link descriptors which have not been</div>
<div>> > > > + * transferred, we need to start the controller</div>
<div>> > > > + */</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * Move all elements from the queue of pending transactions</div>
<div>> > > > + * onto the list of running transactions</div>
<div>> > > > + */</div>
<div>> > > > + chan_dbg(chan, "idle, starting controller\n");</div>
<div>> > > > + desc = list_first_entry(&chan->ld_pending, struct fsl_desc_sw,</div>
<div>> > > node);</div>
<div>> > > > + list_splice_tail_init(&chan->ld_pending, &chan->ld_running);</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * The 85xx DMA controller doesn't clear the channel start</div>
<div>> bit</div>
<div>> > > > + * automatically at the end of a transfer. Therefore we must</div>
<div>> clear</div>
<div>> > > > + * it in software before starting the transfer.</div>
<div>> > > > + */</div>
<div>> > > > + if ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX) {</div>
<div>> > > > + u32 mode;</div>
<div>> > > > +</div>
<div>> > > > + mode = DMA_IN(chan, &chan->regs->mr, 32);</div>
<div>> > > > + mode &= ~FSL_DMA_MR_CS;</div>
<div>> > > > + DMA_OUT(chan, &chan->regs->mr, mode, 32);</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * Program the descriptor's address into the DMA controller,</div>
<div>> > > > + * then start the DMA transaction</div>
<div>> > > > + */</div>
<div>> > > > + set_cdar(chan, desc->async_tx.phys);</div>
<div>> > > > + get_cdar(chan);</div>
<div>> > > > +</div>
<div>> > > > + dma_start(chan);</div>
<div>> > > > + chan->idle = false;</div>
<div>> > > > +}</div>
<div>> > > > +</div>
<div>> > ></div>
<div>> > > Please add a note about the locking requirements here, and for the</div>
<div>> other</div>
<div>> > > new functions you've added.</div>
<div>> > ></div>
<div>> > > You call this function from two places:</div>
<div>> > ></div>
<div>> > > 1) fsldma_cleanup_descriptor() - called with mod->desc_lock held</div>
<div>> > > 2) fsl_tx_status() - WITHOUT mod->desc_lock held</div>
<div>> > ></div>
<div>> > > One of them is definitely wrong, and I'd bet that it is #2. You're</div>
<div>> > > modifying ld_completed without a lock.</div>
<div>> > Yes, My bad, I will correct it.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > +static int</div>
<div>> > > > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan) {</div>
<div>> > > > + struct fsl_desc_sw *desc, *_desc;</div>
<div>> > > > +</div>
<div>> > > > + /* Run the callback for each descriptor, in order */</div>
<div>> > > > + list_for_each_entry_safe(desc, _desc, &chan->ld_completed,</div>
<div>> node) {</div>
<div>> > > > +</div>
<div>> > > > + if (async_tx_test_ack(&desc->async_tx)) {</div>
<div>> > > > + /* Remove from the list of transactions */</div>
<div>> > > > + list_del(&desc->node);</div>
<div>> > > > +#ifdef FSL_DMA_LD_DEBUG</div>
<div>> > > > + chan_dbg(chan, "LD %p free\n", desc); #endif</div>
<div>> > > > + dma_pool_free(chan->desc_pool, desc,</div>
<div>> > > > + desc->async_tx.phys);</div>
<div>> > > > + }</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + return 0;</div>
<div>> > > > +}</div>
<div>> > > > +</div>
<div>> > > > +/**</div>
<div>> > > > + * fsldma_run_tx_complete_actions - cleanup and free a single link</div>
<div>> > > > +descriptor</div>
<div>> > > > + * @chan: Freescale DMA channel</div>
<div>> > > > + * @desc: descriptor to cleanup and free</div>
<div>> > > > + * @cookie: Freescale DMA transaction identifier</div>
<div>> > > > + *</div>
<div>> > > > + * This function is used on a descriptor which has been executed</div>
<div>> by</div>
<div>> > > > +the DMA</div>
<div>> > > > + * controller. It will run any callbacks, submit any dependencies,</div>
<div>> > > > +and then</div>
<div>> > > > + * free the descriptor.</div>
<div>> > > > + */</div>
<div>> > > > +static dma_cookie_t fsldma_run_tx_complete_actions(struct</div>
<div>> fsl_desc_sw</div>
<div>> > > *desc,</div>
<div>> > > > + struct fsldma_chan *chan, dma_cookie_t cookie) {</div>
<div>> > > > + struct dma_async_tx_descriptor *txd = &desc->async_tx;</div>
<div>> > > > + struct device *dev = chan->common.device->dev;</div>
<div>> > > > + dma_addr_t src = get_desc_src(chan, desc);</div>
<div>> > > > + dma_addr_t dst = get_desc_dst(chan, desc);</div>
<div>> > > > + u32 len = get_desc_cnt(chan, desc);</div>
<div>> > > > +</div>
<div>> > > > + BUG_ON(txd->cookie < 0);</div>
<div>> > > > +</div>
<div>> > > > + if (txd->cookie > 0) {</div>
<div>> > > > + cookie = txd->cookie;</div>
<div>> > > > +</div>
<div>> > > > + /* Run the link descriptor callback function */</div>
<div>> > > > + if (txd->callback) {</div>
<div>> > > > +#ifdef FSL_DMA_LD_DEBUG</div>
<div>> > > > + chan_dbg(chan, "LD %p callback\n", desc); #endif</div>
<div>> > > > + txd->callback(txd->callback_param);</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /* Unmap the dst buffer, if requested */</div>
<div>> > > > + if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {</div>
<div>> > > > + if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)</div>
<div>> > > > + dma_unmap_single(dev, dst, len,</div>
<div>> DMA_FROM_DEVICE);</div>
<div>> > > > + else</div>
<div>> > > > + dma_unmap_page(dev, dst, len,</div>
<div>> DMA_FROM_DEVICE);</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /* Unmap the src buffer, if requested */</div>
<div>> > > > + if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {</div>
<div>> > > > + if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)</div>
<div>> > > > + dma_unmap_single(dev, src, len,</div>
<div>> DMA_TO_DEVICE);</div>
<div>> > > > + else</div>
<div>> > > > + dma_unmap_page(dev, src, len,</div>
<div>> DMA_TO_DEVICE);</div>
<div>> > > > + }</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /* Run any dependencies */</div>
<div>> > > > + dma_run_dependencies(txd);</div>
<div>> > > > +</div>
<div>> > > > + return cookie;</div>
<div>> > > > +}</div>
<div>> > > > +</div>
<div>> > > > +static int</div>
<div>> > > > +fsldma_clean_running_descriptor(struct fsl_desc_sw *desc,</div>
<div>> > > > + struct fsldma_chan *chan)</div>
<div>> > > > +{</div>
<div>> > > > + /* Remove from the list of transactions */</div>
<div>> > > > + list_del(&desc->node);</div>
<div>> > > > + /* the client is allowed to attach dependent operations</div>
<div>> > > > + * until 'ack' is set</div>
<div>> > > > + */</div>
<div>> > ></div>
<div>> > > This comment is does not follow the coding style. It should be:</div>
<div>> > ></div>
<div>> > > /*</div>
<div>> > > * the client is allowed to attech dependent operations</div>
<div>> > > * until 'ack' is set</div>
<div>> > > */</div>
<div>> > Fine, I will correct it in v4. Include another 2 places related to</div>
<div>> coding style.</div>
<div>> > Thanks.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > + if (!async_tx_test_ack(&desc->async_tx)) {</div>
<div>> > > > + /* move this slot to the completed */</div>
<div>> > ></div>
<div>> > > Perhaps a better comment would be:</div>
<div>> > ></div>
<div>> > > Move this descriptor to the list of descriptors which is complete,</div>
<div>> but</div>
<div>> > > still awaiting the 'ack' bit to be set.</div>
<div>> > Accept.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > + list_add_tail(&desc->node, &chan->ld_completed);</div>
<div>> > > > + return 0;</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + dma_pool_free(chan->desc_pool, desc,</div>
<div>> > > > + desc->async_tx.phys);</div>
<div>> > ></div>
<div>> > > This should all be on one line. It is less than 80 characters wide.</div>
<div>> > ></div>
<div>> > Accept.</div>
<div>> ></div>
<div>> > > > + return 0;</div>
<div>> > > > +}</div>
<div>> > > > +</div>
<div>> > ></div>
<div>> > > Locking notes please.</div>
<div>> > I will add it in v4, please check. Thanks.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan) {</div>
<div>> > > > + struct fsl_desc_sw *desc, *_desc;</div>
<div>> > > > + dma_cookie_t cookie = 0;</div>
<div>> > > > + dma_addr_t curr_phys = get_cdar(chan);</div>
<div>> > > > + int idle = dma_is_idle(chan);</div>
<div>> > > > + int seen_current = 0;</div>
<div>> > > > +</div>
<div>> > > > + fsldma_clean_completed_descriptor(chan);</div>
<div>> > > > +</div>
<div>> > > > + /* Run the callback for each descriptor, in order */</div>
<div>> > > > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node)</div>
<div>> {</div>
<div>> > > > + /* do not advance past the current descriptor loaded</div>
<div>> into the</div>
<div>> > > > + * hardware channel, subsequent descriptors are either</div>
<div>> in</div>
<div>> > > > + * process or have not been submitted</div>
<div>> > > > + */</div>
<div>> > ></div>
<div>> > > Coding style.</div>
<div>> > ></div>
<div>> > > > + if (seen_current)</div>
<div>> > > > + break;</div>
<div>> > > > +</div>
<div>> > > > + /* stop the search if we reach the current descriptor</div>
<div>> and the</div>
<div>> > > > + * channel is busy</div>
<div>> > > > + */</div>
<div>> > ></div>
<div>> > > Coding style.</div>
<div>> > ></div>
<div>> > > > + if (desc->async_tx.phys == curr_phys) {</div>
<div>> > > > + seen_current = 1;</div>
<div>> > > > + if (!idle)</div>
<div>> > > > + break;</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > ></div>
<div>> > > This trick where you try to look at the hardware state to determine</div>
<div>> how</div>
<div>> > > much to clean up has been a source of headaches in the past versions</div>
<div>> of</div>
<div>> > > this driver.</div>
<div>> > I know a little about the history of this driver. Could you tell me</div>
<div>> what kind</div>
<div>> > headaches in the past versions, I can have a test and fix it in my</div>
<div>> patch. And</div>
<div>> > I also think use current phys addr should be more explicit.</div>
<div>> > Thanks.</div>
<div>> ></div>
<div>> </div>
<div>> It has been a very long time since I last worked on this code, but I</div>
<div>> will try to remember.</div>
<div>> </div>
<div>> I remember one problem where the currently running descriptor was free'd</div>
<div>> while the hardware was still executing it.</div>
<div>> </div>
<div>> There was another related problem where the DMA hardware would lock up,</div>
<div>> and it is impossible to recover without a hard reset. The CB (channel</div>
<div>> busy) bit gets stuck on, and the running transfer never completes. This</div>
<div>> may have been due to a programming issue in a user of the DMAEngine API,</div>
<div>> and not this driver itself. I don't remember for sure anymore.</div>
<div>> </div>
<div>> Our current system here at work has 120 boards with this DMA controller.</div>
<div>> Each one runs 100+ DMA operations per second, 24/7/365. They've been</div>
<div>> online for more than two years. I know for sure that the current code</div>
<div>> doesn't lock up the DMA controller. :)</div>
<div>> </div>
<div>> I think your bug is real, and needs to be fixed. I'm just trying to make</div>
<div>> sure I understand the change, so it won't break other users. I'm only</div>
<div>> trying to help.</div>
<div>Thanks for your information. I will do more explanation of my idea in the</div>
<div>following answer.</div>
<div>Could you provide some sample code for reproduce bug, I don't know dmatest.c</div>
<div>whether is enough. I will add dma self test in fsldma.c.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> > ></div>
<div>> > > It is much easier to reason about the state of the hardware and avoid</div>
<div>> > > race conditions if you complete entire blocks of descriptors after</div>
<div>> the</div>
<div>> > > hardware interrupts you to tell you it is finished.</div>
<div>> > In current driver, it's ok, but there is a potential issue when enable</div>
<div>> > async_tx api. How can you determine these descriptors in ld_running</div>
<div>> whether</div>
<div>> > have been completed in fsl_tx_status()? (I mean in my patch.)</div>
<div>> ></div>
<div>> </div>
<div>> I think your idea using an ld_completed list for descriptors which have</div>
<div>> been run by the hardware, but not yet 'ack'ed by software is fine.</div>
<div>> </div>
<div>> Would something similar to the following work?</div>
<div>> </div>
<div>> ld_pending: build a list of descriptors until issue_pending() is called.</div>
<div>> This is exactly what it does now, no changes needed.</div>
<div>Yes, no changes.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> ld_running: this list of descriptors is currently executing. Mostly</div>
<div>> unchanged from how it works now.</div>
<div>Yes, no changes.</div>
<div>I will add descriptions about all new interfaces.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> When you get an interrupt, you know the descriptors in ld_running are</div>
<div>> finished. Update the cookie to the highest number from ld_running. Free</div>
<div>> the descriptors which have been 'ack'ed immediately. Move the ones which</div>
<div>> are not 'ack'ed onto ld_completed. As part of this process, you should</div>
<div>> run dma_run_dependencies() and callbacks as needed.</div>
<div>Right, below is new implement of dma_do_tasklet();</div>
<div>First, dma_do_tasklet()</div>
<div><font face="Times New Roman"> <font face="Consolas">-> </font><font face="Consolas">fsldma_cleanup_descriptor</font><font face="Consolas">()</font></font></div>
<div> -> fsldma_clean_completed_descriptor(), free the descriptors which has been ack'ed (I mean in queue ld_completed, not include other queues);</div>
<div>Second, transverse the queue ld_running<br>
-> fsldma_run_tx_complete_actions(), find the last descriptor which is completed, update the cookie to the highest number, run callback, unmap dma pages, and run dma_run_dependencies();</div>
<div> -> fsldma_clean_running_descriptor(), move this descriptor from ld_running to ld_completed.</div>
<div>Last,</div>
<div> -> fsl_chan_xfer_ld_queue(), submit much more descriptors from ld_pending to ld_running.</div>
<div>I hope my description is clear enough.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> I think that this solves the problem you are seeing, which is that</div>
<div>> descriptors which have not been 'ack'ed are free'd.</div>
<div>> </div>
<div>> If this is unclear, I can write some code to demonstrate. If you could</div>
<div>> write a small test driver, similar to drivers/dma/dmatest.c, which tests</div>
<div>> the async_tx API without any extra hardware needed, I can run it. I have</div>
<div>> never used the async_tx API before.</div>
<div>Your description is clear, I agree with you, I will add fsldma self test in</div>
<div>the file.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> > ></div>
<div>> > > This is the philosophy employed by the driver before your</div>
<div>> modifications:</div>
<div>> > > ld_pending: a block of descriptors which is ready to run as soon as</div>
<div>> the</div>
<div>> > > hardware becomes idle.</div>
<div>> > > ld_running: a block of descriptors which is being executed by the</div>
<div>> > > hardware.</div>
<div>> > These 2 lists are not enough, async_tx descriptors must be kept order</div>
<div>> as</div>
<div>> > expected of its submitter, we only can process the descriptor which has</div>
<div>> been</div>
<div>> > acked by async_tx api, but not free all descriptors in ld_running.</div>
<div>> > For example,</div>
<div>> > Step 1, in async_tx memcpy api,</div>
<div>> > tx2 = device->device_prep_dma_memcpy(...),</div>
<div>> > tx2->depend_tx = tx1;</div>
<div>> > step 2, in async_tx submit api,</div>
<div>> > async_tx_submit() will check tx2->depend_tx whether has been acked,</div>
<div>> > unfortunately, tx2->depend_tx is freed before step 2 (dma channels is</div>
<div>> flushed</div>
<div>> > by other drivers), the contents of tx2->depend_tx is set to</div>
<div>> POOL_POISON_FREED</div>
<div>> > if DMAPOOL_DEBUG is enabled (illegal pointer will be used if disable</div>
<div>> this config</div>
<div>> > option);</div>
<div>> > step 3, BUG_ON(async_tx_test_ack(depend_tx)) is triggered.</div>
<div>> ></div>
<div>> > For resolve this potential risk, first, ack flag must be checked in dma</div>
<div>> driver,</div>
<div>> > second, ld_completed is added to restore all descriptors which have</div>
<div>> been acked</div>
<div>> > and completed.</div>
<div>> > In my following answer, most of all are around this idea.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > + cookie = fsldma_run_tx_complete_actions(desc, chan,</div>
<div>> cookie);</div>
<div>> > > > +</div>
<div>> > > > + if (fsldma_clean_running_descriptor(desc, chan))</div>
<div>> > > > + break;</div>
<div>> > > > +</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + /*</div>
<div>> > > > + * Start any pending transactions automatically</div>
<div>> > > > + *</div>
<div>> > > > + * In the ideal case, we keep the DMA controller busy while</div>
<div>> we go</div>
<div>> > > > + * ahead and free the descriptors below.</div>
<div>> > > > + */</div>
<div>> > > > + fsl_chan_xfer_ld_queue(chan);</div>
<div>> > > > +</div>
<div>> > > > + if (cookie > 0)</div>
<div>> > > > + chan->common.completed_cookie = cookie; }</div>
<div>> > > > +</div>
<div>> > > > static dma_cookie_t fsl_dma_tx_submit(struct</div>
<div>> dma_async_tx_descriptor</div>
<div>> > > > *tx) {</div>
<div>> > > > struct fsldma_chan *chan = to_fsl_chan(tx->chan); @@ -534,8</div>
<div>> +745,10</div>
<div>> > > > @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)</div>
<div>> > > ></div>
<div>> > > > chan_dbg(chan, "free all channel resources\n");</div>
<div>> > > > spin_lock_irqsave(&chan->desc_lock, flags);</div>
<div>> > > > + fsldma_cleanup_descriptor(chan);</div>
<div>> > > > fsldma_free_desc_list(chan, &chan->ld_pending);</div>
<div>> > > > fsldma_free_desc_list(chan, &chan->ld_running);</div>
<div>> > > > + fsldma_free_desc_list(chan, &chan->ld_completed);</div>
<div>> > > > spin_unlock_irqrestore(&chan->desc_lock, flags);</div>
<div>> > > ></div>
<div>> > > > dma_pool_destroy(chan->desc_pool);</div>
<div>> > > > @@ -811,124 +1024,6 @@ static int fsl_dma_device_control(struct</div>
<div>> > > > dma_chan *dchan, }</div>
<div>> > > ></div>
<div>> > > > /**</div>
<div>> > > > - * fsldma_cleanup_descriptor - cleanup and free a single link</div>
<div>> > > > descriptor</div>
<div>> > > > - * @chan: Freescale DMA channel</div>
<div>> > > > - * @desc: descriptor to cleanup and free</div>
<div>> > > > - *</div>
<div>> > > > - * This function is used on a descriptor which has been executed</div>
<div>> by</div>
<div>> > > > the DMA</div>
<div>> > > > - * controller. It will run any callbacks, submit any dependencies,</div>
<div>> > > > and then</div>
<div>> > > > - * free the descriptor.</div>
<div>> > > > - */</div>
<div>> > > > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,</div>
<div>> > > > - struct fsl_desc_sw *desc)</div>
<div>> > > > -{</div>
<div>> > > > - struct dma_async_tx_descriptor *txd = &desc->async_tx;</div>
<div>> > > > - struct device *dev = chan->common.device->dev;</div>
<div>> > > > - dma_addr_t src = get_desc_src(chan, desc);</div>
<div>> > > > - dma_addr_t dst = get_desc_dst(chan, desc);</div>
<div>> > > > - u32 len = get_desc_cnt(chan, desc);</div>
<div>> > > > -</div>
<div>> > > > - /* Run the link descriptor callback function */</div>
<div>> > > > - if (txd->callback) {</div>
<div>> > > > -#ifdef FSL_DMA_LD_DEBUG</div>
<div>> > > > - chan_dbg(chan, "LD %p callback\n", desc);</div>
<div>> > > > -#endif</div>
<div>> > > > - txd->callback(txd->callback_param);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /* Run any dependencies */</div>
<div>> > > > - dma_run_dependencies(txd);</div>
<div>> > > > -</div>
<div>> > > > - /* Unmap the dst buffer, if requested */</div>
<div>> > > > - if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {</div>
<div>> > > > - if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)</div>
<div>> > > > - dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);</div>
<div>> > > > - else</div>
<div>> > > > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /* Unmap the src buffer, if requested */</div>
<div>> > > > - if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {</div>
<div>> > > > - if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)</div>
<div>> > > > - dma_unmap_single(dev, src, len, DMA_TO_DEVICE);</div>
<div>> > > > - else</div>
<div>> > > > - dma_unmap_page(dev, src, len, DMA_TO_DEVICE);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > -#ifdef FSL_DMA_LD_DEBUG</div>
<div>> > > > - chan_dbg(chan, "LD %p free\n", desc);</div>
<div>> > > > -#endif</div>
<div>> > > > - dma_pool_free(chan->desc_pool, desc, txd->phys);</div>
<div>> > > > -}</div>
<div>> > > > -</div>
<div>> > > > -/**</div>
<div>> > > > - * fsl_chan_xfer_ld_queue - transfer any pending transactions</div>
<div>> > > > - * @chan : Freescale DMA channel</div>
<div>> > > > - *</div>
<div>> > > > - * HARDWARE STATE: idle</div>
<div>> > > > - * LOCKING: must hold chan->desc_lock</div>
<div>> > > > - */</div>
<div>> > > > -static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan) -{</div>
<div>> > > > - struct fsl_desc_sw *desc;</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * If the list of pending descriptors is empty, then we</div>
<div>> > > > - * don't need to do any work at all</div>
<div>> > > > - */</div>
<div>> > > > - if (list_empty(&chan->ld_pending)) {</div>
<div>> > > > - chan_dbg(chan, "no pending LDs\n");</div>
<div>> > > > - return;</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * The DMA controller is not idle, which means that the</div>
<div>> interrupt</div>
<div>> > > > - * handler will start any queued transactions when it runs</div>
<div>> after</div>
<div>> > > > - * this transaction finishes</div>
<div>> > > > - */</div>
<div>> > > > - if (!chan->idle) {</div>
<div>> > > > - chan_dbg(chan, "DMA controller still busy\n");</div>
<div>> > > > - return;</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * If there are some link descriptors which have not been</div>
<div>> > > > - * transferred, we need to start the controller</div>
<div>> > > > - */</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * Move all elements from the queue of pending transactions</div>
<div>> > > > - * onto the list of running transactions</div>
<div>> > > > - */</div>
<div>> > > > - chan_dbg(chan, "idle, starting controller\n");</div>
<div>> > > > - desc = list_first_entry(&chan->ld_pending, struct fsl_desc_sw,</div>
<div>> > > node);</div>
<div>> > > > - list_splice_tail_init(&chan->ld_pending, &chan->ld_running);</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * The 85xx DMA controller doesn't clear the channel start</div>
<div>> bit</div>
<div>> > > > - * automatically at the end of a transfer. Therefore we must</div>
<div>> clear</div>
<div>> > > > - * it in software before starting the transfer.</div>
<div>> > > > - */</div>
<div>> > > > - if ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX) {</div>
<div>> > > > - u32 mode;</div>
<div>> > > > -</div>
<div>> > > > - mode = DMA_IN(chan, &chan->regs->mr, 32);</div>
<div>> > > > - mode &= ~FSL_DMA_MR_CS;</div>
<div>> > > > - DMA_OUT(chan, &chan->regs->mr, mode, 32);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * Program the descriptor's address into the DMA controller,</div>
<div>> > > > - * then start the DMA transaction</div>
<div>> > > > - */</div>
<div>> > > > - set_cdar(chan, desc->async_tx.phys);</div>
<div>> > > > - get_cdar(chan);</div>
<div>> > > > -</div>
<div>> > > > - dma_start(chan);</div>
<div>> > > > - chan->idle = false;</div>
<div>> > > > -}</div>
<div>> > > > -</div>
<div>> > > > -/**</div>
<div>> > > > * fsl_dma_memcpy_issue_pending - Issue the DMA start command</div>
<div>> > > > * @chan : Freescale DMA channel</div>
<div>> > > > */</div>
<div>> > > > @@ -954,11 +1049,17 @@ static enum dma_status fsl_tx_status(struct</div>
<div>> > > dma_chan *dchan,</div>
<div>> > > > enum dma_status ret;</div>
<div>> > > > unsigned long flags;</div>
<div>> > > ></div>
<div>> > > > - spin_lock_irqsave(&chan->desc_lock, flags);</div>
<div>> > > > ret = dma_cookie_status(dchan, cookie, txstate);</div>
<div>> > > > + if (ret == DMA_SUCCESS) {</div>
<div>> > > > + fsldma_clean_completed_descriptor(chan);</div>
<div>> > > > + return ret;</div>
<div>> > > > + }</div>
<div>> > > > +</div>
<div>> > > > + spin_lock_irqsave(&chan->desc_lock, flags);</div>
<div>> > > > + fsldma_cleanup_descriptor(chan);</div>
<div>> > ></div>
<div>> > > You've made status from a very quick operation (compare two cookies)</div>
<div>> into</div>
<div>> > > a potentially long running operation. It may now run callbacks, unmap</div>
<div>> > > pages, etc.</div>
<div>> > Yes, that's right, callbacks and unmap pages will be invoked if DMA</div>
<div>> status</div>
<div>> > is not DMA_SUCCESS.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > I note that Documentation/crypto/async-tx-api.txt section 3.6</div>
<div>> > > "Constraints" does say that async_*() functions cannot be called from</div>
<div>> IRQ</div>
<div>> > > context. However, the DMAEngine API itself does not have anything to</div>
<div>> say</div>
<div>> > > about IRQ context.</div>
<div>> > ></div>
<div>> > > I expect fsl_tx_status() to be quick, and not potentially call other</div>
<div>> > > arbitrary code.</div>
<div>> > fsl_tx_status() is not in IRQ context. It's reasonable to unmap pages</div>
<div>> and</div>
<div>> > run dependency of descriptor as fsldma_run_tx_complete_actions() does.</div>
<div>> ></div>
<div>> </div>
<div>> I looked at several other DMAEngine API drivers. Some of them do run</div>
<div>> dependencies and callbacks from tx_status(). You are correct, this is</div>
<div>> fine.</div>
<div>> </div>
<div>> > ></div>
<div>> > > Is it possible to leave this function unchanged?</div>
<div>> > According to my knowledge, it is unlikely to be left unchanged, or it</div>
<div>> will violate</div>
<div>> > the design of this interface. If DMA status is not DMA_SUCCESS, that</div>
<div>> means there</div>
<div>> > are new descriptors completed, we should move them to ld_completed, and</div>
<div>> do actions</div>
<div>> > to its async_tx descriptors, then dma descriptor will be freed at</div>
<div>> another time.</div>
<div>> > Most of my idea is from iop-adma.c, the original interface is not align</div>
<div>> with it.</div>
<div>> > Async_tx flags (expecially ack_test) is not considered when dma</div>
<div>> descriptor is freed,</div>
<div>> > so some random errors happened, I think you must familiar with this</div>
<div>> history.</div>
<div>> > As an important "synchronization flag", async_tx api must control the</div>
<div>> order of tx</div>
<div>> > descriptor. Dma driver also should make sure keeping order when free</div>
<div>> the descriptor.</div>
<div>> > That's the reason why I add ld_completed list.</div>
<div>> ></div>
<div>> </div>
<div>> Is it possible to put the descriptors into three different states?</div>
<div>> </div>
<div>> 1) ld_pending: ready to run, but not yet executed on the hardware</div>
<div>> </div>
<div>> Descriptors move to #2 through either dma_async_issue_pending() or by</div>
<div>> the interrupt which happens when the hardware completes executing a set</div>
<div>> of DMA descriptors.</div>
<div>> </div>
<div>> 2) ld_running: currently executing on the hardware</div>
<div>> </div>
<div>> Descriptors move to #3 through the interrupt which happens when the</div>
<div>> hardware completes executing a set of DMA descriptors.</div>
<div>> </div>
<div>> Dependencies and callbacks are executed as the descriptors are moved</div>
<div>> onto ld_completed.</div>
<div>> </div>
<div>> 3) ld_completed: finished executing on the hardware, but not yet 'ack'ed</div>
<div>> </div>
<div>> When they have been 'ack'ed, the descriptors are free'd.</div>
<div>> </div>
<div>> </div>
<div>> As noted above, I think I could code this up fairly quickly if this</div>
<div>> explanation is unclear.</div>
<div>Totally right. I think you understand my idea.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> Also note that I may not understand your bug completely, and my idea may</div>
<div>> be completely wrong!</div>
<div>I hope you've already understand my issue:)</div>
<div>Thanks.</div>
<div><font face="Times New Roman"> </font></div>
<div>> </div>
<div>> Ira</div>
<div>> </div>
<div>> > ></div>
<div>> > > Also note that if you leave this function unchanged, the locking</div>
<div>> error</div>
<div>> > > pointed out above (fsldma_clean_completed_descriptor() is not called</div>
<div>> with</div>
<div>> > > the lock held) goes away.</div>
<div>> > I know. Thanks.</div>
<div>> ></div>
<div>> > ></div>
<div>> > > > spin_unlock_irqrestore(&chan->desc_lock, flags);</div>
<div>> > > ></div>
<div>> > > > - return ret;</div>
<div>> > > > + return dma_cookie_status(dchan, cookie, txstate);</div>
<div>> > > > }</div>
<div>> > > ></div>
<div>> > > ></div>
<div>> > > > /*-----------------------------------------------------------------</div>
<div>> ---</div>
<div>> > > > --------*/ @@ -1035,53 +1136,21 @@ static irqreturn_t</div>
<div>> > > > fsldma_chan_irq(int irq, void *data) static void</div>
<div>> > > > dma_do_tasklet(unsigned long data) {</div>
<div>> > > > struct fsldma_chan *chan = (struct fsldma_chan *)data;</div>
<div>> > > > - struct fsl_desc_sw *desc, *_desc;</div>
<div>> > > > - LIST_HEAD(ld_cleanup);</div>
<div>> > > > unsigned long flags;</div>
<div>> > > ></div>
<div>> > > > chan_dbg(chan, "tasklet entry\n");</div>
<div>> > > ></div>
<div>> > > > spin_lock_irqsave(&chan->desc_lock, flags);</div>
<div>> > > ></div>
<div>> > > > - /* update the cookie if we have some descriptors to cleanup</div>
<div>> */</div>
<div>> > > > - if (!list_empty(&chan->ld_running)) {</div>
<div>> > > > - dma_cookie_t cookie;</div>
<div>> > > > -</div>
<div>> > > > - desc = to_fsl_desc(chan->ld_running.prev);</div>
<div>> > > > - cookie = desc->async_tx.cookie;</div>
<div>> > > > - dma_cookie_complete(&desc->async_tx);</div>
<div>> > > > -</div>
<div>> > > > - chan_dbg(chan, "completed_cookie=%d\n", cookie);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > - /*</div>
<div>> > > > - * move the descriptors to a temporary list so we can drop</div>
<div>> the lock</div>
<div>> > > > - * during the entire cleanup operation</div>
<div>> > > > - */</div>
<div>> > > > - list_splice_tail_init(&chan->ld_running, &ld_cleanup);</div>
<div>> > > > + /* Run all cleanup for this descriptor */</div>
<div>> > > > + fsldma_cleanup_descriptor(chan);</div>
<div>> > > ></div>
<div>> > > > /* the hardware is now idle and ready for more */</div>
<div>> > > > chan->idle = true;</div>
<div>> > > ></div>
<div>> > > > - /*</div>
<div>> > > > - * Start any pending transactions automatically</div>
<div>> > > > - *</div>
<div>> > > > - * In the ideal case, we keep the DMA controller busy while</div>
<div>> we go</div>
<div>> > > > - * ahead and free the descriptors below.</div>
<div>> > > > - */</div>
<div>> > > > fsl_chan_xfer_ld_queue(chan);</div>
<div>> > > > spin_unlock_irqrestore(&chan->desc_lock, flags);</div>
<div>> > > ></div>
<div>> > > > - /* Run the callback for each descriptor, in order */</div>
<div>> > > > - list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {</div>
<div>> > > > -</div>
<div>> > > > - /* Remove from the list of transactions */</div>
<div>> > > > - list_del(&desc->node);</div>
<div>> > > > -</div>
<div>> > > > - /* Run all cleanup for this descriptor */</div>
<div>> > > > - fsldma_cleanup_descriptor(chan, desc);</div>
<div>> > > > - }</div>
<div>> > > > -</div>
<div>> > > > chan_dbg(chan, "tasklet exit\n");</div>
<div>> > > > }</div>
<div>> > > ></div>
<div>> > > > @@ -1262,6 +1331,7 @@ static int __devinit</div>
<div>> fsl_dma_chan_probe(struct</div>
<div>> > > fsldma_device *fdev,</div>
<div>> > > > spin_lock_init(&chan->desc_lock);</div>
<div>> > > > INIT_LIST_HEAD(&chan->ld_pending);</div>
<div>> > > > INIT_LIST_HEAD(&chan->ld_running);</div>
<div>> > > > + INIT_LIST_HEAD(&chan->ld_completed);</div>
<div>> > > > chan->idle = true;</div>
<div>> > > ></div>
<div>> > > > chan->common.device = &fdev->common; diff --git</div>
<div>> > > > a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h index</div>
<div>> f5c3879..7ede908</div>
<div>> > > > 100644</div>
<div>> > > > --- a/drivers/dma/fsldma.h</div>
<div>> > > > +++ b/drivers/dma/fsldma.h</div>
<div>> > > > @@ -140,6 +140,7 @@ struct fsldma_chan {</div>
<div>> > > > spinlock_t desc_lock; /* Descriptor operation lock */</div>
<div>> > > > struct list_head ld_pending; /* Link descriptors queue */</div>
<div>> > > > struct list_head ld_running; /* Link descriptors queue */</div>
<div>> > > > + struct list_head ld_completed; /* Link descriptors queue</div>
<div>> */</div>
<div>> > > > struct dma_chan common; /* DMA common channel */</div>
<div>> > > > struct dma_pool *desc_pool; /* Descriptors pool */</div>
<div>> > > > struct device *dev; /* Channel device */</div>
<div>> > > > --</div>
<div>> > > > 1.7.5.1</div>
<div>> > > ></div>
<div>> > > ></div>
<div>> ></div>
<div>> ></div>
<div><font face="Times New Roman"> </font></div>
<div><font face="Times New Roman"> </font></div>
</span></font>
</body>
</html>