From: Liu Qiang-B32616 <B32616@freescale.com>
To: "Ira W. Snyder" <iws@ovro.caltech.edu>
Cc: "arnd@arndb.de" <arnd@arndb.de>,
"vinod.koul@intel.com" <vinod.koul@intel.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"dan.j.williams@gmail.com" <dan.j.williams@gmail.com>,
"herbert@gondor.hengli.com.au" <herbert@gondor.hengli.com.au>,
"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
"davem@davemloft.net" <davem@davemloft.net>
Subject: RE: [PATCH v6 5/8] fsl-dma: change release process of dma descriptor for supporting async_tx
Date: Tue, 7 Aug 2012 09:01:38 +0000 [thread overview]
Message-ID: <BCB48C05FCE8BC4D9E61E841ECBE6DB70E11A9@039-SN2MPN1-013.039d.mgd.msft.net> (raw)
In-Reply-To: <20120806175115.GA23815@ovro.caltech.edu>
Hi Ira,
I remember you said you always disable CONFIG_NET_DMA to make sure dma engi=
ne won't be locked, actually if this options may affect some behavior of fs=
l-dma in current kernel, it is possible to issue pending descriptors if the=
re is any network actions.
Set CONFIG_NET_DMA=3Dy, dma_issue_pending_all will be invoked in net/core/d=
ev.c (e.g. triggered by NAPI), it will activate all dma channels which are =
registered to kernel. I don't know whether it will make some troubles in yo=
ur system.
If any question please let me know. Thanks.
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Tuesday, August 07, 2012 1:51 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> dan.j.williams@intel.com; linux-kernel@vger.kernel.org;
> dan.j.williams@gmail.com; vinod.koul@intel.com; arnd@arndb.de;
> gregkh@linuxfoundation.org; herbert@gondor.hengli.com.au;
> davem@davemloft.net
> Subject: Re: [PATCH v6 5/8] fsl-dma: change release process of dma
> descriptor for supporting async_tx
>=20
> On Mon, Aug 06, 2012 at 06:14:33PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Fix the potential risk when enable config NET_DMA and ASYNC_TX.
> > Async_tx is lack of support in current release process of dma
> descriptor,
> > all descriptors will be released whatever is acked or no-acked by
> async_tx,
> > so there is a potential race condition when dma engine is uesd by
> others
> > clients (e.g. when enable NET_DMA to offload TCP).
> >
> > In our case, a race condition which is raised when use both of talitos
> > and dmaengine to offload xor is because napi scheduler will sync all
> > pending requests in dma channels, it affects the process of raid
> operations
> > due to ack_tx is not checked in fsl dma. The no-acked descriptor is
> freed
> > which is submitted just now, as a dependent tx, this freed descriptor
> trigger
> > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> >
> > TASK =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
> >
> > Another modification in this patch is the change of completed
> descriptors,
> > there is a potential risk which caused by exception interrupt, all
> descriptors
> > in ld_running list are seemed completed when an interrupt raised, it
> works fine
> > under normal condition, but if there is an exception occured, it cannot
> work
> > as our excepted. Hardware should not be depend on s/w list, the right
> way is
> > to read current descriptor address register to find the last completed
> > descriptor. If an interrupt is raised by an error, all descriptors in
> ld_running
> > should not be seemed finished, or these unfinished descriptors in
> ld_running
> > will be released wrongly.
> >
> > A simple way to reproduce,
> > Enable dmatest first, then insert some bad descriptors which can
> trigger
> > Programming Error interrupts before the good descriptors. Last, the
> good
> > descriptors will be freed before they are processsed because of the
> exception
> > intrerrupt.
> >
> > Note: the bad descriptors are only for simulating an exception
> interrupt.
> > This case can illustrate the potential risk in current fsl-dma very
> well.
> >
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Dan Williams <dan.j.williams@gmail.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>=20
> There are two minor nitpicks below. Other than that, the patch looks
> excellent to me.
>=20
> Ira
>=20
> > ---
> > drivers/dma/fsldma.c | 232 ++++++++++++++++++++++++++++++++++--------
> --------
> > drivers/dma/fsldma.h | 17 +++-
> > 2 files changed, 174 insertions(+), 75 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> > index 36490a3..938d8c1 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -472,6 +472,110 @@ static struct fsl_desc_sw
> *fsl_dma_alloc_descriptor(struct fsldma_chan *chan)
> > }
> >
> > /**
> > + * fsldma_clean_completed_descriptor - free all descriptors which
> > + * has been completed and acked
> > + * @chan: Freescale DMA channel
> > + *
> > + * This function is used on all completed and acked descriptors.
> > + * All descriptors should only be freed in this function.
> > + */
> > +static void
> > +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))
> > + fsl_dma_free_descriptor(chan, desc);
> > +}
> > +
> > +/**
> > + * fsldma_run_tx_complete_actions - cleanup 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.
> > + */
> > +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan
> *chan,
> > + struct fsl_desc_sw *desc, 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_SINGLE)
> > + dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > + else
> > + dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > + }
> > +
> > + /* Unmap the src buffer, if requested */
> > + if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > + if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > + dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > + else
> > + dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > + }
> > + }
> > +
> > + /* Run any dependencies */
> > + dma_run_dependencies(txd);
> > +
> > + return cookie;
> > +}
> > +
> > +/**
> > + * fsldma_clean_running_descriptor - move the completed descriptor
> from
> > + * ld_running to ld_completed
> > + * @chan: Freescale DMA channel
> > + * @desc: the descriptor which is completed
> > + *
> > + * Free the descriptor directly if acked by async_tx api, or move it
> to
> > + * queue ld_completed.
> > + */
> > +static void
> > +fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> > + struct fsl_desc_sw *desc)
> > +{
> > + /* Remove from the list of transactions */
> > + list_del(&desc->node);
>=20
> Minor nitpick. Add a blank line here.
>=20
> > + /*
> > + * the client is allowed to attach dependent operations
> > + * until 'ack' is set
> > + */
> > + if (!async_tx_test_ack(&desc->async_tx)) {
> > + /*
> > + * Move this descriptor to the list of descriptors which is
> > + * completed, but still awaiting the 'ack' bit to be set.
> > + */
> > + list_add_tail(&desc->node, &chan->ld_completed);
> > + return;
> > + }
> > +
> > + dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> > +}
> > +
> > +/**
> > * fsl_chan_xfer_ld_queue - transfer any pending transactions
> > * @chan : Freescale DMA channel
> > *
> > @@ -539,51 +643,58 @@ static void fsl_chan_xfer_ld_queue(struct
> fsldma_chan *chan)
> > }
> >
> > /**
> > - * fsldma_cleanup_descriptor - cleanup and free a single link
> descriptor
> > + * fsldma_cleanup_descriptors - cleanup link descriptors which are
> completed
> > + * and move them to ld_completed to free until flag 'ack' is set
> > * @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.
> > + * This function is used on descriptors which have been executed by
> the DMA
> > + * controller. It will run any callbacks, submit any dependencies,
> then
> > + * free these descriptors if flag 'ack' is set.
> > */
> > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> > - struct fsl_desc_sw *desc)
> > +static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
> > {
> > - 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);
> > + struct fsl_desc_sw *desc, *_desc;
> > + dma_cookie_t cookie =3D 0;
> > + dma_addr_t curr_phys =3D get_cdar(chan);
> > + int seen_current =3D 0;
> >
> > - /* Run the link descriptor callback function */
> > - if (txd->callback) {
> > -#ifdef FSL_DMA_LD_DEBUG
> > - chan_dbg(chan, "LD %p callback\n", desc);
> > -#endif
> > - txd->callback(txd->callback_param);
> > - }
> > + fsldma_clean_completed_descriptor(chan);
> >
> > - /* Run any dependencies */
> > - dma_run_dependencies(txd);
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > + /*
> > + * do not advance past the current descriptor loaded into the
> > + * hardware channel, subsequent descriptors are either in
> > + * process or have not been submitted
> > + */
> > + if (seen_current)
> > + break;
> >
> > - /* Unmap the dst buffer, if requested */
> > - if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > - if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > - dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > - else
> > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > - }
> > + /*
> > + * stop the search if we reach the current descriptor and the
> > + * channel is busy
> > + */
> > + if (desc->async_tx.phys =3D=3D curr_phys) {
> > + seen_current =3D 1;
> > + if (!dma_is_idle(chan))
> > + break;
> > + }
>=20
> I wonder if this is better:
>=20
> if (desc->async_tx.phys =3D=3D get_cdar(chan)) {
> seen_current =3D 1;
> if (!dma_is_idle(chan))
> break;
> }
>=20
> Your version works fine, it just might stop earlier than necessary. This
> is just a nitpick.
>=20
> >
> > - /* 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);
> > + cookie =3D fsldma_run_tx_complete_actions(chan, desc, cookie);
> > +
> > + fsldma_clean_running_descriptor(chan, desc);
> > }
> >
> > - fsl_dma_free_descriptor(chan, desc);
> > + /*
> > + * 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;
> > }
> >
> > /**
> > @@ -654,8 +765,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_descriptors(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);
> > @@ -893,6 +1006,7 @@ static int fsl_dma_device_control(struct dma_chan
> *dchan,
> > /* Remove and free all of the descriptors in the LD queue */
> > fsldma_free_desc_list(chan, &chan->ld_pending);
> > fsldma_free_desc_list(chan, &chan->ld_running);
> > + fsldma_free_desc_list(chan, &chan->ld_completed);
> > chan->idle =3D true;
> >
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> > @@ -956,11 +1070,15 @@ static enum dma_status fsl_tx_status(struct
> dma_chan *dchan,
> > enum dma_status ret;
> > unsigned long flags;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > ret =3D dma_cookie_status(dchan, cookie, txstate);
> > + if (ret =3D=3D DMA_SUCCESS)
> > + return ret;
> > +
> > + spin_lock_irqsave(&chan->desc_lock, flags);
> > + fsldma_cleanup_descriptors(chan);
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > - return ret;
> > + return dma_cookie_status(dchan, cookie, txstate);
> > }
> >
> > /*--------------------------------------------------------------------
> --------*/
> > @@ -1037,52 +1155,19 @@ 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);
> > -
> > /* 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) {
> > + /* Run all cleanup for descriptors which have been completed */
> > + fsldma_cleanup_descriptors(chan);
> >
> > - /* Remove from the list of transactions */
> > - list_del(&desc->node);
> > -
> > - /* Run all cleanup for this descriptor */
> > - fsldma_cleanup_descriptor(chan, desc);
> > - }
> > + spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > chan_dbg(chan, "tasklet exit\n");
> > }
> > @@ -1264,6 +1349,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..a58275a 100644
> > --- a/drivers/dma/fsldma.h
> > +++ b/drivers/dma/fsldma.h
> > @@ -138,8 +138,21 @@ struct fsldma_chan {
> > char name[8]; /* Channel name */
> > struct fsldma_chan_regs __iomem *regs;
> > spinlock_t desc_lock; /* Descriptor operation lock */
> > - struct list_head ld_pending; /* Link descriptors queue */
> > - struct list_head ld_running; /* Link descriptors queue */
> > + /*
> > + * Descriptors which are queued to run, but have not yet been
> > + * submitted to the hardware for execution
> > + */
> > + struct list_head ld_pending;
> > + /*
> > + * Descriptors which are currently being executed by the hardware
> > + */
> > + struct list_head ld_running;
> > + /*
> > + * Descriptors which have finished execution by the hardware. These
> > + * descriptors have already had their cleanup actions run. They are
> > + * waiting for the ACK bit to be set by the async_tx API.
> > + */
> > + struct list_head ld_completed; /* 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
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
prev parent reply other threads:[~2012-08-07 9:01 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-06 10:14 [PATCH v6 5/8] fsl-dma: change release process of dma descriptor for supporting async_tx qiang.liu
2012-08-06 17:51 ` Ira W. Snyder
2012-08-07 3:22 ` Liu Qiang-B32616
2012-08-07 9:01 ` Liu Qiang-B32616 [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BCB48C05FCE8BC4D9E61E841ECBE6DB70E11A9@039-SN2MPN1-013.039d.mgd.msft.net \
--to=b32616@freescale.com \
--cc=arnd@arndb.de \
--cc=dan.j.williams@gmail.com \
--cc=dan.j.williams@intel.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=herbert@gondor.hengli.com.au \
--cc=iws@ovro.caltech.edu \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=vinod.koul@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).