From: Liu Qiang-B32616 Subject: RE: [PATCH v3 3/4] fsl-dma: change release process of dma descriptor for supporting async_tx Date: Wed, 18 Jul 2012 03:31:49 +0000 Message-ID: References: <1342411709-29895-1-git-send-email-qiang.liu@freescale.com> <20120716200124.GD1227@ovro.caltech.edu> <20120717161653.GA20669@ovro.caltech.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2246862812048194811==" Cc: Li Yang-R58472 , Vinod Koul , Phillips Kim-R1AAHA , "herbert@gondor.hengli.com.au" , "linux-crypto@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "davem@davemloft.net" To: "Ira W. Snyder" , Dan Williams Return-path: In-Reply-To: <20120717161653.GA20669@ovro.caltech.edu> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: linux-crypto.vger.kernel.org --===============2246862812048194811== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_BCB48C05FCE8BC4D9E61E841ECBE6DB70C2CF8039SN2MPN1011039d_" --_000_BCB48C05FCE8BC4D9E61E841ECBE6DB70C2CF8039SN2MPN1011039d_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ira, My comments inline. Hi Ira and Dan, Here is a introduction about my scenario of this case. My case is use async_tx API to offload raid5, Use fsl-talitos to compute parity calculation, fsl-dma to process memcpy. What bug occurred to me? Exception is thrown by BUG_ON(async_tx_ack_test(depend_tx)) in async_tx_sub= mit(). TASK =3D 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 What is the root cause of this bug? The async_tx descriptor(depend_tx) is free'ed before async_tx_submit ack it= . That means async_tx descriptor should be acked before it's free'ed. In othe= r word, only ack'ed async_tx descriptor should be free'ed by dma engine. How to fix it? Add a queue ld_completed, move all completed descriptors in this queue, fre= e these descriptors at a proper time (these descriptors also should be acked)= . Dan, do you think my understand about "ack" is right? Thanks. - Qiang > -----Original Message----- > From: Ira W. Snyder [mailto:iws@ovro.caltech.edu] > Sent: Wednesday, July 18, 2012 12:17 AM > To: Liu Qiang-B32616 > Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Phillips > Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemloft.net; Dan > Williams; Vinod Koul; Li Yang-R58472 > Subject: Re: [PATCH v3 3/4] fsl-dma: change release process of dma > descriptor for supporting async_tx > > On Tue, Jul 17, 2012 at 07:06:33AM +0000, Liu Qiang-B32616 wrote: > > > -----Original Message----- > > > From: Ira W. Snyder [mailto:iws@ovro.caltech.edu] > > > Sent: Tuesday, July 17, 2012 4:01 AM > > > To: Liu Qiang-B32616 > > > Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > Phillips > > > Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemloft.net; Dan > > > Williams; Vinod Koul; Li Yang-R58472 > > > Subject: Re: [PATCH v3 3/4] fsl-dma: change release process of dma > > > descriptor for supporting async_tx > > > > > > On Mon, Jul 16, 2012 at 12:08:29PM +0800, Qiang Liu wrote: > > > > 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(). > > > > > > > > Cc: Dan Williams > > > > Cc: Vinod Koul > > > > Cc: Li Yang > > > > Cc: Ira W. Snyder > > > > Signed-off-by: Qiang Liu > > > > --- > > > > drivers/dma/fsldma.c | 378 +++++++++++++++++++++++++++++--------- > ---- > > > ------- > > > > drivers/dma/fsldma.h | 1 + > > > > 2 files changed, 225 insertions(+), 154 deletions(-) > > > > > > > > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index > > > > 4f2f212..4ee1b8f 100644 > > > > --- a/drivers/dma/fsldma.c > > > > +++ b/drivers/dma/fsldma.c > > > > @@ -400,6 +400,217 @@ out_splice: > > > > list_splice_tail_init(&desc->tx_list, &chan->ld_pending); = } > > > > > > > > +/** > > > > + * fsl_chan_xfer_ld_queue - transfer any pending transactions > > > > + * @chan : Freescale DMA channel > > > > + * > > > > + * HARDWARE STATE: idle > > > > + * LOCKING: must hold chan->desc_lock */ static void > > > > +fsl_chan_xfer_ld_queue(struct fsldma_chan *chan) { > > > > + struct fsl_desc_sw *desc; > > > > + > > > > + /* > > > > + * If the list of pending descriptors is empty, then we > > > > + * don't need to do any work at all > > > > + */ > > > > + if (list_empty(&chan->ld_pending)) { > > > > + chan_dbg(chan, "no pending LDs\n"); > > > > + return; > > > > + } > > > > + > > > > + /* > > > > + * The DMA controller is not idle, which means that the > interrupt > > > > + * handler will start any queued transactions when it runs > after > > > > + * this transaction finishes > > > > + */ > > > > + if (!chan->idle) { > > > > + chan_dbg(chan, "DMA controller still busy\n"); > > > > + return; > > > > + } > > > > + > > > > + /* > > > > + * If there are some link descriptors which have not been > > > > + * transferred, we need to start the controller > > > > + */ > > > > + > > > > + /* > > > > + * Move all elements from the queue of pending transactions > > > > + * onto the list of running transactions > > > > + */ > > > > + chan_dbg(chan, "idle, starting controller\n"); > > > > + desc =3D list_first_entry(&chan->ld_pending, struct fsl_des= c_sw, > > > node); > > > > + list_splice_tail_init(&chan->ld_pending, &chan->ld_running)= ; > > > > + > > > > + /* > > > > + * The 85xx DMA controller doesn't clear the channel start > bit > > > > + * automatically at the end of a transfer. Therefore we mus= t > clear > > > > + * it in software before starting the transfer. > > > > + */ > > > > + if ((chan->feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85X= X) { > > > > + u32 mode; > > > > + > > > > + mode =3D DMA_IN(chan, &chan->regs->mr, 32); > > > > + mode &=3D ~FSL_DMA_MR_CS; > > > > + DMA_OUT(chan, &chan->regs->mr, mode, 32); > > > > + } > > > > + > > > > + /* > > > > + * Program the descriptor's address into the DMA controller= , > > > > + * then start the DMA transaction > > > > + */ > > > > + set_cdar(chan, desc->async_tx.phys); > > > > + get_cdar(chan); > > > > + > > > > + dma_start(chan); > > > > + chan->idle =3D false; > > > > +} > > > > + > > > > > > Please add a note about the locking requirements here, and for the > other > > > new functions you've added. > > > > > > You call this function from two places: > > > > > > 1) fsldma_cleanup_descriptor() - called with mod->desc_lock held > > > 2) fsl_tx_status() - WITHOUT mod->desc_lock held > > > > > > One of them is definitely wrong, and I'd bet that it is #2. You're > > > modifying ld_completed without a lock. > > Yes, My bad, I will correct it. > > > > > > > > > +static int > > > > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan) { > > > > + 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); #endi= f > > > > + dma_pool_free(chan->desc_pool, desc, > > > > + desc->async_tx.phys); > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/** > > > > + * fsldma_run_tx_complete_actions - cleanup and free a single link > > > > +descriptor > > > > + * @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, > > > > +and then > > > > + * free the descriptor. > > > > + */ > > > > +static dma_cookie_t fsldma_run_tx_complete_actions(struct > fsl_desc_sw > > > *desc, > > > > + struct fsldma_chan *chan, dma_cookie_t cookie) { > > > > + struct dma_async_tx_descriptor *txd =3D &desc->async_tx; > > > > + struct device *dev =3D chan->common.device->dev; > > > > + dma_addr_t src =3D get_desc_src(chan, desc); > > > > + dma_addr_t dst =3D get_desc_dst(chan, desc); > > > > + u32 len =3D get_desc_cnt(chan, desc); > > > > + > > > > + BUG_ON(txd->cookie < 0); > > > > + > > > > + if (txd->cookie > 0) { > > > > + cookie =3D 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_SINGL= E) > > > > + 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; > > > > +} > > > > + > > > > +static int > > > > +fsldma_clean_running_descriptor(struct fsl_desc_sw *desc, > > > > + struct fsldma_chan *chan) > > > > +{ > > > > + /* Remove from the list of transactions */ > > > > + list_del(&desc->node); > > > > + /* the client is allowed to attach dependent operations > > > > + * until 'ack' is set > > > > + */ > > > > > > This comment is does not follow the coding style. It should be: > > > > > > /* > > > * the client is allowed to attech dependent operations > > > * until 'ack' is set > > > */ > > Fine, I will correct it in v4. Include another 2 places related to > coding style. > > Thanks. > > > > > > > > > + if (!async_tx_test_ack(&desc->async_tx)) { > > > > + /* move this slot to the completed */ > > > > > > Perhaps a better comment would be: > > > > > > Move this descriptor to the list of descriptors which is complete, > but > > > still awaiting the 'ack' bit to be set. > > Accept. > > > > > > > > > + list_add_tail(&desc->node, &chan->ld_completed); > > > > + return 0; > > > > + } > > > > + > > > > + dma_pool_free(chan->desc_pool, desc, > > > > + desc->async_tx.phys); > > > > > > This should all be on one line. It is less than 80 characters wide. > > > > > Accept. > > > > > > + return 0; > > > > +} > > > > + > > > > > > Locking notes please. > > I will add it in v4, please check. Thanks. > > > > > > > > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan) { > > > > + struct fsl_desc_sw *desc, *_desc; > > > > + dma_cookie_t cookie =3D 0; > > > > + dma_addr_t curr_phys =3D get_cdar(chan); > > > > + int idle =3D dma_is_idle(chan); > > > > + int seen_current =3D 0; > > > > + > > > > + fsldma_clean_completed_descriptor(chan); > > > > + > > > > + /* Run the callback for each descriptor, in order */ > > > > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, no= de) > { > > > > + /* do not advance past the current descriptor loade= d > into the > > > > + * hardware channel, subsequent descriptors are eit= her > in > > > > + * process or have not been submitted > > > > + */ > > > > > > Coding style. > > > > > > > + if (seen_current) > > > > + break; > > > > + > > > > + /* stop the search if we reach the current descript= or > and the > > > > + * channel is busy > > > > + */ > > > > > > Coding style. > > > > > > > + if (desc->async_tx.phys =3D=3D curr_phys) { > > > > + seen_current =3D 1; > > > > + if (!idle) > > > > + break; > > > > + } > > > > + > > > > > > This trick where you try to look at the hardware state to determine > how > > > much to clean up has been a source of headaches in the past versions > of > > > this driver. > > I know a little about the history of this driver. Could you tell me > what kind > > headaches in the past versions, I can have a test and fix it in my > patch. And > > I also think use current phys addr should be more explicit. > > Thanks. > > > > It has been a very long time since I last worked on this code, but I > will try to remember. > > I remember one problem where the currently running descriptor was free'd > while the hardware was still executing it. > > There was another related problem where the DMA hardware would lock up, > and it is impossible to recover without a hard reset. The CB (channel > busy) bit gets stuck on, and the running transfer never completes. This > may have been due to a programming issue in a user of the DMAEngine API, > and not this driver itself. I don't remember for sure anymore. > > Our current system here at work has 120 boards with this DMA controller. > Each one runs 100+ DMA operations per second, 24/7/365. They've been > online for more than two years. I know for sure that the current code > doesn't lock up the DMA controller. :) > > I think your bug is real, and needs to be fixed. I'm just trying to make > sure I understand the change, so it won't break other users. I'm only > trying to help. Thanks for your information. I will do more explanation of my idea in the following answer. Could you provide some sample code for reproduce bug, I don't know dmatest.= c whether is enough. I will add dma self test in fsldma.c. > > > > > > > It is much easier to reason about the state of the hardware and avoid > > > race conditions if you complete entire blocks of descriptors after > the > > > hardware interrupts you to tell you it is finished. > > In current driver, it's ok, but there is a potential issue when enable > > async_tx api. How can you determine these descriptors in ld_running > whether > > have been completed in fsl_tx_status()? (I mean in my patch.) > > > > I think your idea using an ld_completed list for descriptors which have > been run by the hardware, but not yet 'ack'ed by software is fine. > > Would something similar to the following work? > > ld_pending: build a list of descriptors until issue_pending() is called. > This is exactly what it does now, no changes needed. Yes, no changes. > > ld_running: this list of descriptors is currently executing. Mostly > unchanged from how it works now. Yes, no changes. I will add descriptions about all new interfaces. > > When you get an interrupt, you know the descriptors in ld_running are > finished. Update the cookie to the highest number from ld_running. Free > the descriptors which have been 'ack'ed immediately. Move the ones which > are not 'ack'ed onto ld_completed. As part of this process, you should > run dma_run_dependencies() and callbacks as needed. Right, below is new implement of dma_do_tasklet(); First, dma_do_tasklet() -> fsldma_cleanup_descriptor() -> fsldma_clean_completed_descriptor(), free the descriptor= s which has been ack'ed (I mean in queue ld_completed, not include other qu= eues); Second, transverse the queue ld_running -> 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(); -> fsldma_clean_running_descriptor(), move this descriptor from ld_= running to ld_completed. Last, -> fsl_chan_xfer_ld_queue(), submit much more descriptors from ld_p= ending to ld_running. I hope my description is clear enough. > > I think that this solves the problem you are seeing, which is that > descriptors which have not been 'ack'ed are free'd. > > If this is unclear, I can write some code to demonstrate. If you could > write a small test driver, similar to drivers/dma/dmatest.c, which tests > the async_tx API without any extra hardware needed, I can run it. I have > never used the async_tx API before. Your description is clear, I agree with you, I will add fsldma self test in the file. > > > > > > > This is the philosophy employed by the driver before your > modifications: > > > ld_pending: a block of descriptors which is ready to run as soon as > the > > > hardware becomes idle. > > > ld_running: a block of descriptors which is being executed by the > > > hardware. > > These 2 lists are not enough, async_tx descriptors must be kept order > as > > expected of its submitter, we only can process the descriptor which has > been > > acked by async_tx api, but not free all descriptors in ld_running. > > For example, > > Step 1, in async_tx memcpy api, > > tx2 =3D device->device_prep_dma_memcpy(...), > > tx2->depend_tx =3D tx1; > > step 2, in async_tx submit api, > > async_tx_submit() will check tx2->depend_tx whether has been acked, > > unfortunately, tx2->depend_tx is freed before step 2 (dma channels is > flushed > > by other drivers), the contents of tx2->depend_tx is set to > POOL_POISON_FREED > > if DMAPOOL_DEBUG is enabled (illegal pointer will be used if disable > this config > > option); > > step 3, BUG_ON(async_tx_test_ack(depend_tx)) is triggered. > > > > For resolve this potential risk, first, ack flag must be checked in dma > driver, > > second, ld_completed is added to restore all descriptors which have > been acked > > and completed. > > In my following answer, most of all are around this idea. > > > > > > > > > + cookie =3D fsldma_run_tx_complete_actions(desc, cha= n, > cookie); > > > > + > > > > + if (fsldma_clean_running_descriptor(desc, chan)) > > > > + break; > > > > + > > > > + } > > > > + > > > > + /* > > > > + * 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 =3D cookie; } > > > > + > > > > static dma_cookie_t fsl_dma_tx_submit(struct > dma_async_tx_descriptor > > > > *tx) { > > > > struct fsldma_chan *chan =3D to_fsl_chan(tx->chan); @@ -534= ,8 > +745,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); > > > > @@ -811,124 +1024,6 @@ static int fsl_dma_device_control(struct > > > > dma_chan *dchan, } > > > > > > > > /** > > > > - * fsldma_cleanup_descriptor - cleanup and free a single link > > > > descriptor > > > > - * @chan: Freescale DMA channel > > > > - * @desc: descriptor to cleanup and free > > > > - * > > > > - * This function is used on a descriptor which has been executed > by > > > > the DMA > > > > - * controller. It will run any callbacks, submit any dependencies, > > > > and then > > > > - * free the descriptor. > > > > - */ > > > > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan, > > > > - struct fsl_desc_sw *desc) > > > > -{ > > > > - struct dma_async_tx_descriptor *txd =3D &desc->async_tx; > > > > - struct device *dev =3D chan->common.device->dev; > > > > - dma_addr_t src =3D get_desc_src(chan, desc); > > > > - dma_addr_t dst =3D get_desc_dst(chan, desc); > > > > - u32 len =3D get_desc_cnt(chan, desc); > > > > - > > > > - /* 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); > > > > - } > > > > - > > > > - /* Run any dependencies */ > > > > - dma_run_dependencies(txd); > > > > - > > > > - /* 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_DE= VICE); > > > > - else > > > > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVI= CE); > > > > - } > > > > - > > > > - /* 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_DEVI= CE); > > > > - 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); > > > > -} > > > > - > > > > -/** > > > > - * fsl_chan_xfer_ld_queue - transfer any pending transactions > > > > - * @chan : Freescale DMA channel > > > > - * > > > > - * HARDWARE STATE: idle > > > > - * LOCKING: must hold chan->desc_lock > > > > - */ > > > > -static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan) -{ > > > > - struct fsl_desc_sw *desc; > > > > - > > > > - /* > > > > - * If the list of pending descriptors is empty, then we > > > > - * don't need to do any work at all > > > > - */ > > > > - if (list_empty(&chan->ld_pending)) { > > > > - chan_dbg(chan, "no pending LDs\n"); > > > > - return; > > > > - } > > > > - > > > > - /* > > > > - * The DMA controller is not idle, which means that the > interrupt > > > > - * handler will start any queued transactions when it runs > after > > > > - * this transaction finishes > > > > - */ > > > > - if (!chan->idle) { > > > > - chan_dbg(chan, "DMA controller still busy\n"); > > > > - return; > > > > - } > > > > - > > > > - /* > > > > - * If there are some link descriptors which have not been > > > > - * transferred, we need to start the controller > > > > - */ > > > > - > > > > - /* > > > > - * Move all elements from the queue of pending transactions > > > > - * onto the list of running transactions > > > > - */ > > > > - chan_dbg(chan, "idle, starting controller\n"); > > > > - desc =3D list_first_entry(&chan->ld_pending, struct fsl_des= c_sw, > > > node); > > > > - list_splice_tail_init(&chan->ld_pending, &chan->ld_running)= ; > > > > - > > > > - /* > > > > - * The 85xx DMA controller doesn't clear the channel start > bit > > > > - * automatically at the end of a transfer. Therefore we mus= t > clear > > > > - * it in software before starting the transfer. > > > > - */ > > > > - if ((chan->feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85X= X) { > > > > - u32 mode; > > > > - > > > > - mode =3D DMA_IN(chan, &chan->regs->mr, 32); > > > > - mode &=3D ~FSL_DMA_MR_CS; > > > > - DMA_OUT(chan, &chan->regs->mr, mode, 32); > > > > - } > > > > - > > > > - /* > > > > - * Program the descriptor's address into the DMA controller= , > > > > - * then start the DMA transaction > > > > - */ > > > > - set_cdar(chan, desc->async_tx.phys); > > > > - get_cdar(chan); > > > > - > > > > - dma_start(chan); > > > > - chan->idle =3D false; > > > > -} > > > > - > > > > -/** > > > > * fsl_dma_memcpy_issue_pending - Issue the DMA start command > > > > * @chan : Freescale DMA channel > > > > */ > > > > @@ -954,11 +1049,17 @@ 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 =3D dma_cookie_status(dchan, cookie, txstate); > > > > + if (ret =3D=3D DMA_SUCCESS) { > > > > + fsldma_clean_completed_descriptor(chan); > > > > + return ret; > > > > + } > > > > + > > > > + spin_lock_irqsave(&chan->desc_lock, flags); > > > > + fsldma_cleanup_descriptor(chan); > > > > > > You've made status from a very quick operation (compare two cookies) > into > > > a potentially long running operation. It may now run callbacks, unmap > > > pages, etc. > > Yes, that's right, callbacks and unmap pages will be invoked if DMA > status > > is not DMA_SUCCESS. > > > > > > > > I note that Documentation/crypto/async-tx-api.txt section 3.6 > > > "Constraints" does say that async_*() functions cannot be called from > IRQ > > > context. However, the DMAEngine API itself does not have anything to > say > > > about IRQ context. > > > > > > I expect fsl_tx_status() to be quick, and not potentially call other > > > arbitrary code. > > fsl_tx_status() is not in IRQ context. It's reasonable to unmap pages > and > > run dependency of descriptor as fsldma_run_tx_complete_actions() does. > > > > I looked at several other DMAEngine API drivers. Some of them do run > dependencies and callbacks from tx_status(). You are correct, this is > fine. > > > > > > > Is it possible to leave this function unchanged? > > According to my knowledge, it is unlikely to be left unchanged, or it > will violate > > the design of this interface. If DMA status is not DMA_SUCCESS, that > means there > > are new descriptors completed, we should move them to ld_completed, and > do actions > > to its async_tx descriptors, then dma descriptor will be freed at > another time. > > Most of my idea is from iop-adma.c, the original interface is not align > with it. > > Async_tx flags (expecially ack_test) is not considered when dma > descriptor is freed, > > so some random errors happened, I think you must familiar with this > history. > > As an important "synchronization flag", async_tx api must control the > order of tx > > descriptor. Dma driver also should make sure keeping order when free > the descriptor. > > That's the reason why I add ld_completed list. > > > > Is it possible to put the descriptors into three different states? > > 1) ld_pending: ready to run, but not yet executed on the hardware > > Descriptors move to #2 through either dma_async_issue_pending() or by > the interrupt which happens when the hardware completes executing a set > of DMA descriptors. > > 2) ld_running: currently executing on the hardware > > Descriptors move to #3 through the interrupt which happens when the > hardware completes executing a set of DMA descriptors. > > Dependencies and callbacks are executed as the descriptors are moved > onto ld_completed. > > 3) ld_completed: finished executing on the hardware, but not yet 'ack'ed > > When they have been 'ack'ed, the descriptors are free'd. > > > As noted above, I think I could code this up fairly quickly if this > explanation is unclear. Totally right. I think you understand my idea. > > Also note that I may not understand your bug completely, and my idea may > be completely wrong! I hope you've already understand my issue:) Thanks. > > Ira > > > > > > > Also note that if you leave this function unchanged, the locking > error > > > pointed out above (fsldma_clean_completed_descriptor() is not called > with > > > the lock held) goes away. > > I know. Thanks. > > > > > > > > > spin_unlock_irqrestore(&chan->desc_lock, flags); > > > > > > > > - return ret; > > > > + return dma_cookie_status(dchan, cookie, txstate); > > > > } > > > > > > > > > > > > /*----------------------------------------------------------------- > --- > > > > --------*/ @@ -1035,53 +1136,21 @@ static irqreturn_t > > > > fsldma_chan_irq(int irq, void *data) static void > > > > dma_do_tasklet(unsigned long data) { > > > > struct fsldma_chan *chan =3D (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 =3D to_fsl_desc(chan->ld_running.prev); > > > > - cookie =3D desc->async_tx.cookie; > > > > - dma_cookie_complete(&desc->async_tx); > > > > - > > > > - chan_dbg(chan, "completed_cookie=3D%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); > > > > + /* Run all cleanup for this descriptor */ > > > > + fsldma_cleanup_descriptor(chan); > > > > > > > > /* the hardware is now idle and ready for more */ > > > > chan->idle =3D 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) { > > > > - > > > > - /* Remove from the list of transactions */ > > > > - list_del(&desc->node); > > > > - > > > > - /* Run all cleanup for this descriptor */ > > > > - fsldma_cleanup_descriptor(chan, desc); > > > > - } > > > > - > > > > chan_dbg(chan, "tasklet exit\n"); > > > > } > > > > > > > > @@ -1262,6 +1331,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 =3D true; > > > > > > > > chan->common.device =3D &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 loc= k */ > > > > struct list_head ld_pending; /* Link descriptors queue *= / > > > > struct list_head ld_running; /* Link descriptors queue *= / > > > > + struct list_head ld_completed; /* Link descriptors queue > */ > > > > struct dma_chan common; /* DMA common channel */ > > > > struct dma_pool *desc_pool; /* Descriptors pool */ > > > > struct device *dev; /* Channel device */ > > > > -- > > > > 1.7.5.1 > > > > > > > > > > > > --_000_BCB48C05FCE8BC4D9E61E841ECBE6DB70C2CF8039SN2MPN1011039d_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Hi Ira,
 
My comments inline.
 
Hi Ira and Dan,
 
Here is a introduction about my scenario of this case.
 
My case is use async_tx API to offload raid5,
Use fsl-talitos to compute parity calculation, fsl-dma to process memc= py.
 
What bug occurred to me?
Exception is thrown by BUG_ON(async_tx_ack_test(depend_tx)) in async_t= x_submit().
 
TASK =3D ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4 0000= 0000 00000001
GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4 ed57= 6d98 00000000
GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000 ed30= 15e8 c15a7aa0
GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0 ef64= 0c30 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
 
What is the root cause of this bug?
The async_tx descriptor(depend_tx) is free'ed before async_tx_submit a= ck it.
That means async_tx descriptor should be acked before it's free'ed. In= other
word, only ack'ed async_tx descriptor should be free'ed by dma engine.=
 
How to fix it?
Add a queue ld_completed, move all completed descriptors in this queue= , free
these descriptors at a proper time (these descriptors also should be a= cked).
 
Dan, do you think my understand about "ack" is right? Thanks= .
 
- Qiang
 
> -----Original Message-----
> From: Ira W. Snyder [mail= to:iws@ovro.caltech.edu]
> Sent: Wednesday, July 18, 2012 12:17 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; = Phillips
> Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemloft.net; Da= n
> Williams; Vinod Koul; Li Yang-R58472
> Subject: Re: [PATCH v3 3/4] fsl-dma: change release process of dm= a
> descriptor for supporting async_tx
>
> On Tue, Jul 17, 2012 at 07:06:33AM +0000, Liu Qiang-B32616 wr= ote:
> > > -----Original Message-----
> > > From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> > > Sent: Tuesday, July 17, 2012 4:01 AM
> > > To: Liu Qiang-B32616
> > > Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.oz= labs.org;
> Phillips
> > > Kim-R1AAHA; herbert@gondor.hengli.com.au; davem@davemlo= ft.net; Dan
> > > Williams; Vinod Koul; Li Yang-R58472
> > > Subject: Re: [PATCH v3 3/4] fsl-dma: change release pro= cess of dma
> > > descriptor for supporting async_tx
> > >
> > > On Mon, Jul 16, 2012 at 12:08:29PM +0800, Qiang Liu= wrote:
> > > > Fix the potential risk when enable config NET_DMA = and ASYNC_TX.
> > > > Async_tx is lack of support in current release pro= cess of dma
> > > > descriptor, all descriptors will be released whate= ver 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 sched= uler will sync
> all
> > > > pending requests in dma channels, it affects the p= rocess of raid
> > > > operations due to ack_tx is not checked in fsl dma= . The no-acked
> > > > descriptor is freed which is submitted just now, a= s a dependent tx,
> > > > this freed descriptor trigger
> > > > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_s= ubmit().
> > > >
> > > > Cc: Dan Williams <dan.j.williams@intel.com><= /div>
> > > > Cc: Vinod Koul <vinod.koul@intel.com>
> > > > Cc: Li Yang <leoli@freescale.com>
> > > > Cc: Ira W. Snyder <iws@ovro.caltech.edu>
> > > > Signed-off-by: Qiang Liu <qiang.liu@freescale.c= om>
> > > > ---
> > > >  drivers/dma/fsldma.c |  378 ++&= #43;++++++++++++++&= #43;+++++++++++---------
> ----
> > > -------
> > > >  drivers/dma/fsldma.h |    1 &= #43;
> > > >  2 files changed, 225 insertions(+), 154 = deletions(-)
> > > >
> > > > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fs= ldma.c index
> > > > 4f2f212..4ee1b8f 100644
> > > > --- a/drivers/dma/fsldma.c
> > > > +++ b/drivers/dma/fsldma.c
> > > > @@ -400,6 +400,217 @@ out_splice:
> > > >        &nb= sp; list_splice_tail_init(&desc->tx_list, &chan->ld_pending);=   }
> > > >
> > > > +/**
> > > > + * fsl_chan_xfer_ld_queue - transfer any pend= ing transactions
> > > > + * @chan : Freescale DMA channel
> > > > + *
> > > > + * HARDWARE STATE: idle
> > > > + * LOCKING: must hold chan->desc_lock = ; */ static void
> > > > +fsl_chan_xfer_ld_queue(struct fsldma_chan *ch= an) {
> > > > +       struct f= sl_desc_sw *desc;
> > > > +
> > > > +       /*
> > > > +        * = If the list of pending descriptors is empty, then we
> > > > +        * = don't need to do any work at all
> > > > +        */=
> > > > +       if (list= _empty(&chan->ld_pending)) {
> > > > +       &nb= sp;       chan_dbg(chan, "no pending LDs= \n");
> > > > +       &nb= sp;       return;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * = The DMA controller is not idle, which means that the
> interrupt
> > > > +        * = handler will start any queued transactions when it runs
> after
> > > > +        * = this transaction finishes
> > > > +        */=
> > > > +       if (!cha= n->idle) {
> > > > +       &nb= sp;       chan_dbg(chan, "DMA controller= still busy\n");
> > > > +       &nb= sp;       return;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * = If there are some link descriptors which have not been
> > > > +        * = transferred, we need to start the controller
> > > > +        */=
> > > > +
> > > > +       /*
> > > > +        * = Move all elements from the queue of pending transactions
> > > > +        * = onto the list of running transactions
> > > > +        */=
> > > > +       chan_dbg= (chan, "idle, starting controller\n");
> > > > +       desc =3D= list_first_entry(&chan->ld_pending, struct fsl_desc_sw,
> > > node);
> > > > +       list_spl= ice_tail_init(&chan->ld_pending, &chan->ld_running);
> > > > +
> > > > +       /*
> > > > +        * = The 85xx DMA controller doesn't clear the channel start
> bit
> > > > +        * = automatically at the end of a transfer. Therefore we must
> clear
> > > > +        * = it in software before starting the transfer.
> > > > +        */=
> > > > +       if ((cha= n->feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85XX) {
> > > > +       &nb= sp;       u32 mode;
> > > > +
> > > > +       &nb= sp;       mode =3D DMA_IN(chan, &chan->= ;regs->mr, 32);
> > > > +       &nb= sp;       mode &=3D ~FSL_DMA_MR_CS;
> > > > +       &nb= sp;       DMA_OUT(chan, &chan->regs-&g= t;mr, mode, 32);
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * = Program the descriptor's address into the DMA controller,
> > > > +        * = then start the DMA transaction
> > > > +        */=
> > > > +       set_cdar= (chan, desc->async_tx.phys);
> > > > +       get_cdar= (chan);
> > > > +
> > > > +       dma_star= t(chan);
> > > > +       chan->= ;idle =3D false;
> > > > +}
> > > > +
> > >
> > > Please add a note about the locking requirements here, = and for the
> other
> > > new functions you've added.
> > >
> > > You call this function from two places:
> > >
> > > 1) fsldma_cleanup_descriptor() - called with mod->de= sc_lock held
> > > 2) fsl_tx_status() - WITHOUT mod->desc_lock held
> > >
> > > One of them is definitely wrong, and I'd bet that it is= #2. You're
> > > modifying ld_completed without a lock.
> > Yes, My bad, I will correct it.
> >
> > >
> > > > +static int
> > > > +fsldma_clean_completed_descriptor(struct fsld= ma_chan *chan) {
> > > > +       struct f= sl_desc_sw *desc, *_desc;
> > > > +
> > > > +       /* Run t= he callback for each descriptor, in order */
> > > > +       list_for= _each_entry_safe(desc, _desc, &chan->ld_completed,
> node) {
> > > > +
> > > > +       &nb= sp;       if (async_tx_test_ack(&desc->= ;async_tx)) {
> > > > +       &nb= sp;            =    /* Remove from the list of transactions */
> > > > +       &nb= sp;            =    list_del(&desc->node);
> > > > +#ifdef FSL_DMA_LD_DEBUG
> > > > +       &nb= sp;            =    chan_dbg(chan, "LD %p free\n", desc); #endif
> > > > +       &nb= sp;            =    dma_pool_free(chan->desc_pool, desc,
> > > > +       &nb= sp;            =             &nb= sp;      desc->async_tx.phys);
> > > > +       &nb= sp;       }
> > > > +       }
> > > > +
> > > > +       return 0= ;
> > > > +}
> > > > +
> > > > +/**
> > > > + * fsldma_run_tx_complete_actions - cleanup a= nd free a single link
> > > > +descriptor
> > > > + * @chan: Freescale DMA channel
> > > > + * @desc: descriptor to cleanup and free
> > > > + * @cookie: Freescale DMA transaction identif= ier
> > > > + *
> > > > + * This function is used on a descriptor whic= h has been executed
> by
> > > > +the DMA
> > > > + * controller. It will run any callbacks, sub= mit any dependencies,
> > > > +and then
> > > > + * free the descriptor.
> > > > + */
> > > > +static dma_cookie_t fsldma_run_tx_complete_ac= tions(struct
> fsl_desc_sw
> > > *desc,
> > > > +       &nb= sp;       struct fsldma_chan *chan, dma_cooki= e_t cookie) {
> > > > +       struct d= ma_async_tx_descriptor *txd =3D &desc->async_tx;
> > > > +       struct d= evice *dev =3D chan->common.device->dev;
> > > > +       dma_addr= _t src =3D get_desc_src(chan, desc);
> > > > +       dma_addr= _t dst =3D get_desc_dst(chan, desc);
> > > > +       u32 len = =3D get_desc_cnt(chan, desc);
> > > > +
> > > > +       BUG_ON(t= xd->cookie < 0);
> > > > +
> > > > +       if (txd-= >cookie > 0) {
> > > > +       &nb= sp;       cookie =3D txd->cookie;
> > > > +
> > > > +       &nb= sp;       /* Run the link descriptor callback= function */
> > > > +       &nb= sp;       if (txd->callback) {
> > > > +#ifdef FSL_DMA_LD_DEBUG
> > > > +       &nb= sp;            =    chan_dbg(chan, "LD %p callback\n", desc); #endif
> > > > +       &nb= sp;            =    txd->callback(txd->callback_param);
> > > > +       &nb= sp;       }
> > > > +
> > > > +       &nb= sp;       /* Unmap the dst buffer, if request= ed */
> > > > +       &nb= sp;       if (!(txd->flags & DMA_COMPL= _SKIP_DEST_UNMAP)) {
> > > > +       &nb= sp;            =    if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > > > +       &nb= sp;            =            dma_unmap_sing= le(dev, dst, len,
> DMA_FROM_DEVICE);
> > > > +       &nb= sp;            =    else
> > > > +       &nb= sp;            =            dma_unmap_page= (dev, dst, len,
> DMA_FROM_DEVICE);
> > > > +       &nb= sp;       }
> > > > +
> > > > +       &nb= sp;       /* Unmap the src buffer, if request= ed */
> > > > +       &nb= sp;       if (!(txd->flags & DMA_COMPL= _SKIP_SRC_UNMAP)) {
> > > > +       &nb= sp;            =    if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > > > +       &nb= sp;            =            dma_unmap_sing= le(dev, src, len,
> DMA_TO_DEVICE);
> > > > +       &nb= sp;            =    else
> > > > +       &nb= sp;            =            dma_unmap_page= (dev, src, len,
> DMA_TO_DEVICE);
> > > > +       &nb= sp;       }
> > > > +       }
> > > > +
> > > > +       /* Run a= ny dependencies */
> > > > +       dma_run_= dependencies(txd);
> > > > +
> > > > +       return c= ookie;
> > > > +}
> > > > +
> > > > +static int
> > > > +fsldma_clean_running_descriptor(struct fsl_de= sc_sw *desc,
> > > > +       &nb= sp;            =    struct fsldma_chan *chan)
> > > > +{
> > > > +       /* Remov= e from the list of transactions */
> > > > +       list_del= (&desc->node);
> > > > +       /* the c= lient is allowed to attach dependent operations
> > > > +        * = until 'ack' is set
> > > > +        */=
> > >
> > > This comment is does not follow the coding style. It sh= ould be:
> > >
> > > /*
> > >  * the client is allowed to attech dependent opera= tions
> > >  * until 'ack' is set
> > >  */
> > Fine, I will correct it in v4. Include another 2 places rela= ted to
> coding style.
> > Thanks.
> >
> > >
> > > > +       if (!asy= nc_tx_test_ack(&desc->async_tx)) {
> > > > +       &nb= sp;       /* move this slot to the completed = */
> > >
> > > Perhaps a better comment would be:
> > >
> > > Move this descriptor to the list of descriptors which i= s complete,
> but
> > > still awaiting the 'ack' bit to be set.
> > Accept.
> >
> > >
> > > > +       &nb= sp;       list_add_tail(&desc->node, &= amp;chan->ld_completed);
> > > > +       &nb= sp;       return 0;
> > > > +       }
> > > > +
> > > > +       dma_pool= _free(chan->desc_pool, desc,
> > > > +       &nb= sp;            =    desc->async_tx.phys);
> > >
> > > This should all be on one line. It is less than 80 char= acters wide.
> > >
> > Accept.
> >
> > > > +       return 0= ;
> > > > +}
> > > > +
> > >
> > > Locking notes please.
> > I will add it in v4, please check. Thanks.
> >
> > >
> > > > +static void fsldma_cleanup_descriptor(struct = fsldma_chan *chan) {
> > > > +       struct f= sl_desc_sw *desc, *_desc;
> > > > +       dma_cook= ie_t cookie =3D 0;
> > > > +       dma_addr= _t curr_phys =3D get_cdar(chan);
> > > > +       int idle= =3D dma_is_idle(chan);
> > > > +       int seen= _current =3D 0;
> > > > +
> > > > +       fsldma_c= lean_completed_descriptor(chan);
> > > > +
> > > > +       /* Run t= he callback for each descriptor, in order */
> > > > +       list_for= _each_entry_safe(desc, _desc, &chan->ld_running, node)
> {
> > > > +       &nb= sp;       /* do not advance past the current = descriptor loaded
> into the
> > > > +       &nb= sp;        * hardware channel, subsequen= t descriptors are either
> in
> > > > +       &nb= sp;        * process or have not been su= bmitted
> > > > +       &nb= sp;        */
> > >
> > > Coding style.
> > >
> > > > +       &nb= sp;       if (seen_current)
> > > > +       &nb= sp;            =    break;
> > > > +
> > > > +       &nb= sp;       /* stop the search if we reach the = current descriptor
> and the
> > > > +       &nb= sp;        * channel is busy
> > > > +       &nb= sp;        */
> > >
> > > Coding style.
> > >
> > > > +       &nb= sp;       if (desc->async_tx.phys =3D=3D c= urr_phys) {
> > > > +       &nb= sp;            =    seen_current =3D 1;
> > > > +       &nb= sp;            =    if (!idle)
> > > > +       &nb= sp;            =            break;
> > > > +       &nb= sp;       }
> > > > +
> > >
> > > This trick where you try to look at the hardware state = to determine
> how
> > > much to clean up has been a source of headaches in the = past versions
> of
> > > this driver.
> > I know a little about the history of this driver. Could you = tell me
> what kind
> > headaches in the past versions, I can have a test and fix it= in my
> patch. And
> > I also think use current phys addr should be more explicit.<= /div>
> > Thanks.
> >
>
> It has been a very long time since I last worked on this code, bu= t I
> will try to remember.
>
> I remember one problem where the currently running descriptor was= free'd
> while the hardware was still executing it.
>
> There was another related problem where the DMA hardware would lo= ck up,
> and it is impossible to recover without a hard reset. The CB (cha= nnel
> busy) bit gets stuck on, and the running transfer never completes= . This
> may have been due to a programming issue in a user of the DMAEngi= ne API,
> and not this driver itself. I don't remember for sure anymore.
>
> Our current system here at work has 120 boards with this DMA cont= roller.
> Each one runs 100+ DMA operations per second, 24/7/365. They'= ve been
> online for more than two years. I know for sure that the current = code
> doesn't lock up the DMA controller. :)
>
> I think your bug is real, and needs to be fixed. I'm just trying = to make
> sure I understand the change, so it won't break other users. I'm = only
> trying to help.
Thanks for your information. I will do more explanation of my idea in = the
following answer.
Could you provide some sample code for reproduce bug, I don't know dma= test.c
whether is enough. I will add dma self test in fsldma.c.
 
>
> > >
> > > It is much easier to reason about the state of the hard= ware and avoid
> > > race conditions if you complete entire blocks of descri= ptors after
> the
> > > hardware interrupts you to tell you it is finished.
> > In current driver, it's ok, but there is a potential issue w= hen enable
> > async_tx api. How can you determine these descriptors in ld_= running
> whether
> > have been completed in fsl_tx_status()? (I mean in my patch.= )
> >
>
> I think your idea using an ld_completed list for descriptors whic= h have
> been run by the hardware, but not yet 'ack'ed by software is fine= .
>
> Would something similar to the following work?
>
> ld_pending: build a list of descriptors until issue_pending() is = called.
> This is exactly what it does now, no changes needed.
Yes, no changes.
 
>
> ld_running: this list of descriptors is currently executing. Most= ly
> unchanged from how it works now.
Yes, no changes.
I will add descriptions about all new interfaces.
 
>
> When you get an interrupt, you know the descriptors in ld_running= are
> finished. Update the cookie to the highest number from ld_running= . Free
> the descriptors which have been 'ack'ed immediately. Move the one= s which
> are not 'ack'ed onto ld_completed. As part of this process, you s= hould
> run dma_run_dependencies() and callbacks as needed.
Right, below is new implement of dma_do_tasklet();
First, dma_do_tasklet()
      &nb= sp; -> fsldma_cle= anup_descriptor()
           &nbs= p;    -> fsldma_clean_completed_descriptor(), free the de= scriptors which has been ack'ed (I mean in queue ld_completed, not include = other queues);
Second, transverse the queue ld_running
        -> fsldma_run_tx_complete_act= ions(), find the last descriptor which is completed, update the cookie to t= he highest number, run callback, unmap dma pages, and run dma_run_dependenc= ies();
        -> fsldma_clean_running_= descriptor(), move this descriptor from ld_running to ld_completed.
Last,
        -> fsl_chan_xfer_ld_queu= e(), submit much more descriptors from ld_pending to ld_running.
I hope my description is clear enough.
 
>
> I think that this solves the problem you are seeing, which is tha= t
> descriptors which have not been 'ack'ed are free'd.
>
> If this is unclear, I can write some code to demonstrate. If you = could
> write a small test driver, similar to drivers/dma/dmatest.c, whic= h tests
> the async_tx API without any extra hardware needed, I can run it.= I have
> never used the async_tx API before.
Your description is clear, I agree with you, I will add fsldma self te= st in
the file.
 
>
> > >
> > > This is the philosophy employed by the driver before yo= ur
> modifications:
> > > ld_pending: a block of descriptors which is ready to ru= n as soon as
> the
> > > hardware becomes idle.
> > > ld_running: a block of descriptors which is being execu= ted by the
> > > hardware.
> > These 2 lists are not enough, async_tx descriptors must be k= ept order
> as
> > expected of its submitter, we only can process the descripto= r which has
> been
> > acked by async_tx api, but not free all descriptors in ld_ru= nning.
> > For example,
> > Step 1, in async_tx memcpy api,
> > tx2 =3D device->device_prep_dma_memcpy(...),
> > tx2->depend_tx =3D tx1;
> > step 2, in async_tx submit api,
> > async_tx_submit() will check tx2->depend_tx whether has b= een acked,
> > unfortunately, tx2->depend_tx is freed before step 2 (dma= channels is
> flushed
> > by other drivers), the contents of tx2->depend_tx is set = to
> POOL_POISON_FREED
> > if DMAPOOL_DEBUG is enabled (illegal pointer will be used if= disable
> this config
> > option);
> > step 3, BUG_ON(async_tx_test_ack(depend_tx)) is triggered.
> >
> > For resolve this potential risk, first, ack flag must be che= cked in dma
> driver,
> > second, ld_completed is added to restore all descriptors whi= ch have
> been acked
> > and completed.
> > In my following answer, most of all are around this idea.
> >
> > >
> > > > +       &nb= sp;       cookie =3D fsldma_run_tx_complete_a= ctions(desc, chan,
> cookie);
> > > > +
> > > > +       &nb= sp;       if (fsldma_clean_running_descriptor= (desc, chan))
> > > > +       &nb= sp;            =    break;
> > > > +
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * = Start any pending transactions automatically
> > > > +        *<= /div>
> > > > +        * = 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 (cook= ie > 0)
> > > > +       &nb= sp;       chan->common.completed_cookie = =3D cookie; }
> > > > +
> > > >  static dma_cookie_t fsl_dma_tx_submit(struct=
> dma_async_tx_descriptor
> > > > *tx)  {
> > > >        &nb= sp; struct fsldma_chan *chan =3D to_fsl_chan(tx->chan); @@ -534,8
> +745,10
> > > > @@ static void fsl_dma_free_chan_resources(struct = dma_chan *dchan)
> > > >
> > > >        &nb= sp; chan_dbg(chan, "free all channel resources\n");
> > > >        &nb= sp; spin_lock_irqsave(&chan->desc_lock, flags);
> > > > +       fsldma_c= leanup_descriptor(chan);
> > > >        &nb= sp; fsldma_free_desc_list(chan, &chan->ld_pending);
> > > >        &nb= sp; fsldma_free_desc_list(chan, &chan->ld_running);
> > > > +       fsldma_f= ree_desc_list(chan, &chan->ld_completed);
> > > >        &nb= sp; spin_unlock_irqrestore(&chan->desc_lock, flags);
> > > >
> > > >        &nb= sp; dma_pool_destroy(chan->desc_pool);
> > > > @@ -811,124 +1024,6 @@ static int fsl_dma_devi= ce_control(struct
> > > > dma_chan *dchan,  }
> > > >
> > > >  /**
> > > > - * fsldma_cleanup_descriptor - cleanup and free a= single link
> > > > descriptor
> > > > - * @chan: Freescale DMA channel
> > > > - * @desc: descriptor to cleanup and free
> > > > - *
> > > > - * This function is used on a descriptor which ha= s been executed
> by
> > > > the DMA
> > > > - * controller. It will run any callbacks, submit = any dependencies,
> > > > and then
> > > > - * free the descriptor.
> > > > - */
> > > > -static void fsldma_cleanup_descriptor(struct fsld= ma_chan *chan,
> > > > -        &= nbsp;           &nbs= p;            &= nbsp;   struct fsl_desc_sw *desc)
> > > > -{
> > > > -       struct dma_a= sync_tx_descriptor *txd =3D &desc->async_tx;
> > > > -       struct devic= e *dev =3D chan->common.device->dev;
> > > > -       dma_addr_t s= rc =3D get_desc_src(chan, desc);
> > > > -       dma_addr_t d= st =3D get_desc_dst(chan, desc);
> > > > -       u32 len =3D = get_desc_cnt(chan, desc);
> > > > -
> > > > -       /* Run the l= ink descriptor callback function */
> > > > -       if (txd->= callback) {
> > > > -#ifdef FSL_DMA_LD_DEBUG
> > > > -        &= nbsp;      chan_dbg(chan, "LD %p callback\n&q= uot;, desc);
> > > > -#endif
> > > > -        &= nbsp;      txd->callback(txd->callback_param= );
> > > > -       }
> > > > -
> > > > -       /* Run any d= ependencies */
> > > > -       dma_run_depe= ndencies(txd);
> > > > -
> > > > -       /* Unmap the= dst buffer, if requested */
> > > > -       if (!(txd-&g= t;flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > > > -        &= nbsp;      if (txd->flags & DMA_COMPL_DEST_= UNMAP_SINGLE)
> > > > -        &= nbsp;           &nbs= p;  dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > > > -        &= nbsp;      else
> > > > -        &= nbsp;           &nbs= p;  dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > > > -       }
> > > > -
> > > > -       /* Unmap the= src buffer, if requested */
> > > > -       if (!(txd-&g= t;flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > > > -        &= nbsp;      if (txd->flags & DMA_COMPL_SRC_U= NMAP_SINGLE)
> > > > -        &= nbsp;           &nbs= p;  dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > > > -        &= nbsp;      else
> > > > -        &= nbsp;           &nbs= p;  dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > > > -       }
> > > > -
> > > > -#ifdef FSL_DMA_LD_DEBUG
> > > > -       chan_dbg(cha= n, "LD %p free\n", desc);
> > > > -#endif
> > > > -       dma_pool_fre= e(chan->desc_pool, desc, txd->phys);
> > > > -}
> > > > -
> > > > -/**
> > > > - * fsl_chan_xfer_ld_queue - transfer any pending = transactions
> > > > - * @chan : Freescale DMA channel
> > > > - *
> > > > - * HARDWARE STATE: idle
> > > > - * LOCKING: must hold chan->desc_lock
> > > > - */
> > > > -static void fsl_chan_xfer_ld_queue(struct fsldma_= chan *chan) -{
> > > > -       struct fsl_d= esc_sw *desc;
> > > > -
> > > > -       /*
> > > > -        * If t= he list of pending descriptors is empty, then we
> > > > -        * don'= t need to do any work at all
> > > > -        */
> > > > -       if (list_emp= ty(&chan->ld_pending)) {
> > > > -        &= nbsp;      chan_dbg(chan, "no pending LDs\n&q= uot;);
> > > > -        &= nbsp;      return;
> > > > -       }
> > > > -
> > > > -       /*
> > > > -        * The = DMA controller is not idle, which means that the
> interrupt
> > > > -        * hand= ler will start any queued transactions when it runs
> after
> > > > -        * this= transaction finishes
> > > > -        */
> > > > -       if (!chan-&g= t;idle) {
> > > > -        &= nbsp;      chan_dbg(chan, "DMA controller sti= ll busy\n");
> > > > -        &= nbsp;      return;
> > > > -       }
> > > > -
> > > > -       /*
> > > > -        * If t= here are some link descriptors which have not been
> > > > -        * tran= sferred, we need to start the controller
> > > > -        */
> > > > -
> > > > -       /*
> > > > -        * Move= all elements from the queue of pending transactions
> > > > -        * onto= the list of running transactions
> > > > -        */
> > > > -       chan_dbg(cha= n, "idle, starting controller\n");
> > > > -       desc =3D lis= t_first_entry(&chan->ld_pending, struct fsl_desc_sw,
> > > node);
> > > > -       list_splice_= tail_init(&chan->ld_pending, &chan->ld_running);
> > > > -
> > > > -       /*
> > > > -        * The = 85xx DMA controller doesn't clear the channel start
> bit
> > > > -        * auto= matically at the end of a transfer. Therefore we must
> clear
> > > > -        * it i= n software before starting the transfer.
> > > > -        */
> > > > -       if ((chan-&g= t;feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85XX) {
> > > > -        &= nbsp;      u32 mode;
> > > > -
> > > > -        &= nbsp;      mode =3D DMA_IN(chan, &chan->reg= s->mr, 32);
> > > > -        &= nbsp;      mode &=3D ~FSL_DMA_MR_CS;
> > > > -        &= nbsp;      DMA_OUT(chan, &chan->regs->mr= , mode, 32);
> > > > -       }
> > > > -
> > > > -       /*
> > > > -        * Prog= ram the descriptor's address into the DMA controller,
> > > > -        * then= start the DMA transaction
> > > > -        */
> > > > -       set_cdar(cha= n, desc->async_tx.phys);
> > > > -       get_cdar(cha= n);
> > > > -
> > > > -       dma_start(ch= an);
> > > > -       chan->idl= e =3D false;
> > > > -}
> > > > -
> > > > -/**
> > > >   * fsl_dma_memcpy_issue_pending - Issue= the DMA start command
> > > >   * @chan : Freescale DMA channel
> > > >   */
> > > > @@ -954,11 +1049,17 @@ static enum dma_status = fsl_tx_status(struct
> > > dma_chan *dchan,
> > > >        &nb= sp; enum dma_status ret;
> > > >        &nb= sp; unsigned long flags;
> > > >
> > > > -       spin_lock_ir= qsave(&chan->desc_lock, flags);
> > > >        &nb= sp; ret =3D dma_cookie_status(dchan, cookie, txstate);
> > > > +       if (ret = =3D=3D DMA_SUCCESS) {
> > > > +       &nb= sp;       fsldma_clean_completed_descriptor(c= han);
> > > > +       &nb= sp;       return ret;
> > > > +       }
> > > > +
> > > > +       spin_loc= k_irqsave(&chan->desc_lock, flags);
> > > > +       fsldma_c= leanup_descriptor(chan);
> > >
> > > You've made status from a very quick operation (compare= two cookies)
> into
> > > a potentially long running operation. It may now run ca= llbacks, unmap
> > > pages, etc.
> > Yes, that's right, callbacks and unmap pages will be invoked= if DMA
> status
> > is not DMA_SUCCESS.
> >
> > >
> > > I note that Documentation/crypto/async-tx-api.txt secti= on 3.6
> > > "Constraints" does say that async_*() functio= ns cannot be called from
> IRQ
> > > context. However, the DMAEngine API itself does not hav= e anything to
> say
> > > about IRQ context.
> > >
> > > I expect fsl_tx_status() to be quick, and not potential= ly call other
> > > arbitrary code.
> > fsl_tx_status() is not in IRQ context. It's reasonable to un= map pages
> and
> > run dependency of descriptor as fsldma_run_tx_complete_actio= ns() does.
> >
>
> I looked at several other DMAEngine API drivers. Some of them do = run
> dependencies and callbacks from tx_status(). You are correct, thi= s is
> fine.
>
> > >
> > > Is it possible to leave this function unchanged?
> > According to my knowledge, it is unlikely to be left unchang= ed, or it
> will violate
> > the design of this interface. If DMA status is not DMA_SUCCE= SS, that
> means there
> > are new descriptors completed, we should move them to ld_com= pleted, and
> do actions
> > to its async_tx descriptors, then dma descriptor will be fre= ed at
> another time.
> > Most of my idea is from iop-adma.c, the original interface i= s not align
> with it.
> > Async_tx flags (expecially ack_test) is not considered when = dma
> descriptor is freed,
> > so some random errors happened, I think you must familiar wi= th this
> history.
> > As an important "synchronization flag", async_tx a= pi must control the
> order of tx
> > descriptor. Dma driver also should make sure keeping order w= hen free
> the descriptor.
> > That's the reason why I add ld_completed list.
> >
>
> Is it possible to put the descriptors into three different states= ?
>
> 1) ld_pending: ready to run, but not yet executed on the hardware=
>
> Descriptors move to #2 through either dma_async_issue_pending() o= r by
> the interrupt which happens when the hardware completes executing= a set
> of DMA descriptors.
>
> 2) ld_running: currently executing on the hardware
>
> Descriptors move to #3 through the interrupt which happens when t= he
> hardware completes executing a set of DMA descriptors.
>
> Dependencies and callbacks are executed as the descriptors are mo= ved
> onto ld_completed.
>
> 3) ld_completed: finished executing on the hardware, but not yet = 'ack'ed
>
> When they have been 'ack'ed, the descriptors are free'd.
>
>
> As noted above, I think I could code this up fairly quickly if th= is
> explanation is unclear.
Totally right. I think you understand my idea.
 
>
> Also note that I may not understand your bug completely, and my i= dea may
> be completely wrong!
I hope you've already understand my issue:)
Thanks.
 
>
> Ira
>
> > >
> > > Also note that if you leave this function unchanged, th= e locking
> error
> > > pointed out above (fsldma_clean_completed_descriptor() = is not called
> with
> > > the lock held) goes away.
> > I know. Thanks.
> >
> > >
> > > >        &nb= sp; spin_unlock_irqrestore(&chan->desc_lock, flags);
> > > >
> > > > -       return ret;<= /div>
> > > > +       return d= ma_cookie_status(dchan, cookie, txstate);
> > > >  }
> > > >
> > > >
> > > > /*------------------------------------------------= -----------------
> ---
> > > > --------*/ @@ -1035,53 +1136,21 @@ static irqr= eturn_t
> > > > fsldma_chan_irq(int irq, void *data)  static = void
> > > > dma_do_tasklet(unsigned long data)  {
> > > >        &nb= sp; struct fsldma_chan *chan =3D (struct fsldma_chan *)data;
> > > > -       struct fsl_d= esc_sw *desc, *_desc;
> > > > -       LIST_HEAD(ld= _cleanup);
> > > >        &nb= sp; unsigned long flags;
> > > >
> > > >        &nb= sp; chan_dbg(chan, "tasklet entry\n");
> > > >
> > > >        &nb= sp; spin_lock_irqsave(&chan->desc_lock, flags);
> > > >
> > > > -       /* update th= e cookie if we have some descriptors to cleanup
> */
> > > > -       if (!list_em= pty(&chan->ld_running)) {
> > > > -        &= nbsp;      dma_cookie_t cookie;
> > > > -
> > > > -        &= nbsp;      desc =3D to_fsl_desc(chan->ld_runnin= g.prev);
> > > > -        &= nbsp;      cookie =3D desc->async_tx.cookie;
> > > > -        &= nbsp;      dma_cookie_complete(&desc->async= _tx);
> > > > -
> > > > -        &= nbsp;      chan_dbg(chan, "completed_cookie= =3D%d\n", cookie);
> > > > -       }
> > > > -
> > > > -       /*
> > > > -        * move= the descriptors to a temporary list so we can drop
> the lock
> > > > -        * duri= ng the entire cleanup operation
> > > > -        */
> > > > -       list_splice_= tail_init(&chan->ld_running, &ld_cleanup);
> > > > +       /* Run a= ll cleanup for this descriptor */
> > > > +       fsldma_c= leanup_descriptor(chan);
> > > >
> > > >        &nb= sp; /* the hardware is now idle and ready for more */
> > > >        &nb= sp; chan->idle =3D true;
> > > >
> > > > -       /*
> > > > -        * Star= t any pending transactions automatically
> > > > -        *
> > > > -        * In t= he ideal case, we keep the DMA controller busy while
> we go
> > > > -        * ahea= d and free the descriptors below.
> > > > -        */
> > > >        &nb= sp; fsl_chan_xfer_ld_queue(chan);
> > > >        &nb= sp; spin_unlock_irqrestore(&chan->desc_lock, flags);
> > > >
> > > > -       /* Run the c= allback for each descriptor, in order */
> > > > -       list_for_eac= h_entry_safe(desc, _desc, &ld_cleanup, node) {
> > > > -
> > > > -        &= nbsp;      /* Remove from the list of transactions= */
> > > > -        &= nbsp;      list_del(&desc->node);
> > > > -
> > > > -        &= nbsp;      /* Run all cleanup for this descriptor = */
> > > > -        &= nbsp;      fsldma_cleanup_descriptor(chan, desc);<= /div>
> > > > -       }
> > > > -
> > > >        &nb= sp; chan_dbg(chan, "tasklet exit\n");
> > > >  }
> > > >
> > > > @@ -1262,6 +1331,7 @@ static int __devinit
> fsl_dma_chan_probe(struct
> > > fsldma_device *fdev,
> > > >        &nb= sp; spin_lock_init(&chan->desc_lock);
> > > >        &nb= sp; INIT_LIST_HEAD(&chan->ld_pending);
> > > >        &nb= sp; INIT_LIST_HEAD(&chan->ld_running);
> > > > +       INIT_LIS= T_HEAD(&chan->ld_completed);
> > > >        &nb= sp; chan->idle =3D true;
> > > >
> > > >        &nb= sp; chan->common.device =3D &fdev->common; diff --git
> > > > a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h inde= x
> f5c3879..7ede908
> > > > 100644
> > > > --- a/drivers/dma/fsldma.h
> > > > +++ b/drivers/dma/fsldma.h
> > > > @@ -140,6 +140,7 @@ struct fsldma_chan {
> > > >        &nb= sp; spinlock_t desc_lock;        &n= bsp;  /* Descriptor operation lock */
> > > >        &nb= sp; struct list_head ld_pending;    /* Link descriptors queu= e */
> > > >        &nb= sp; struct list_head ld_running;    /* Link descriptors queu= e */
> > > > +       struct l= ist_head ld_completed;  /* Link descriptors queue
> */
> > > >        &nb= sp; struct dma_chan common;        = /* DMA common channel */
> > > >        &nb= sp; struct dma_pool *desc_pool;     /* Descriptors pool= */
> > > >        &nb= sp; struct device *dev;        &nbs= p;    /* Channel device */
> > > > --
> > > > 1.7.5.1
> > > >
> > > >
> >
> >
 
 
--_000_BCB48C05FCE8BC4D9E61E841ECBE6DB70C2CF8039SN2MPN1011039d_-- --===============2246862812048194811== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev --===============2246862812048194811==--