| Message ID | 4F7EC6AF.6000603@freescale.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 06, 2012 at 06:34:23PM +0800, Huang Shijie wrote: ... > It's maybe too late to assign the DMA cookie after > mxs_dma_enable_chan() in mxs_dma_tx_submit(). > The interrupt may arise before the dma_cookie_assign() finishes. > > Why mmc/audio do not have this bug? their interrupt arise too slow. > > I tested the following code : Great. Send a formal patch to Vinod? Regards, Shawn > ============================================================== > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > index 5978113..0f5b09a 100644 > --- a/drivers/dma/mxs-dma.c > +++ b/drivers/dma/mxs-dma.c > @@ -202,10 +202,12 @@ static struct mxs_dma_chan > *to_mxs_dma_chan(struct dma_cha > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > + dma_cookie_t c; > > + c = dma_cookie_assign(tx); > mxs_dma_enable_chan(mxs_chan); > > - return dma_cookie_assign(tx); > + return c; > } > > static void mxs_dma_tasklet(unsigned long data) >
> > > It's maybe too late to assign the DMA cookie after mxs_dma_enable_chan() in > mxs_dma_tx_submit(). > The interrupt may arise before the dma_cookie_assign() finishes. > > Why mmc/audio do not have this bug? their interrupt arise too slow. > > I tested the following code : > ============================================================== > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > index 5978113..0f5b09a 100644 > --- a/drivers/dma/mxs-dma.c > +++ b/drivers/dma/mxs-dma.c > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct > dma_cha > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > + dma_cookie_t c; > > + c = dma_cookie_assign(tx); > mxs_dma_enable_chan(mxs_chan); > > - return dma_cookie_assign(tx); > + return c; > } > If this is the issue, then why not have mxs_dma_enable_chan itself call dma_cookie_assign() and return that cookie. That way mxs_dma_enable_chan() is self contained function that assign the cookie, enables channel and returns assigned cookie. And nobody will make mistake of calling just mxs_dma_enable_chan without first calling dma_cookie_assign? -Sam
On Fri, Apr 06, 2012 at 06:49:05AM -0700, Sam Gandhi wrote: > > > > > > It's maybe too late to assign the DMA cookie after mxs_dma_enable_chan() in > > mxs_dma_tx_submit(). > > The interrupt may arise before the dma_cookie_assign() finishes. > > > > Why mmc/audio do not have this bug? their interrupt arise too slow. > > > > I tested the following code : > > ============================================================== > > > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > > index 5978113..0f5b09a 100644 > > --- a/drivers/dma/mxs-dma.c > > +++ b/drivers/dma/mxs-dma.c > > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct > > dma_cha > > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > > { > > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > > + dma_cookie_t c; > > > > + c = dma_cookie_assign(tx); > > mxs_dma_enable_chan(mxs_chan); > > > > - return dma_cookie_assign(tx); > > + return c; > > } > > > If this is the issue, then why not have mxs_dma_enable_chan itself > call dma_cookie_assign() and return that cookie. > As the function name tells, mxs_dma_enable_chan is to only operate dma hardware to enable the channel but nothing else. > That way mxs_dma_enable_chan() is self contained function that assign > the cookie, enables channel and returns assigned cookie. And nobody > will make mistake of calling just mxs_dma_enable_chan without first > calling dma_cookie_assign? > mxs_dma_enable_chan() is a mxs-dma private call. Nobody but only mxs-dma driver itself can call it.
On Fri, Apr 6, 2012 at 6:59 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Fri, Apr 06, 2012 at 06:49:05AM -0700, Sam Gandhi wrote: >> > >> > >> > It's maybe too late to assign the DMA cookie after mxs_dma_enable_chan() in >> > mxs_dma_tx_submit(). >> > The interrupt may arise before the dma_cookie_assign() finishes. >> > >> > Why mmc/audio do not have this bug? their interrupt arise too slow. >> > >> > I tested the following code : >> > ============================================================== >> > >> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c >> > index 5978113..0f5b09a 100644 >> > --- a/drivers/dma/mxs-dma.c >> > +++ b/drivers/dma/mxs-dma.c >> > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct >> > dma_cha >> > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) >> > { >> > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); >> > + dma_cookie_t c; >> > >> > + c = dma_cookie_assign(tx); >> > mxs_dma_enable_chan(mxs_chan); >> > >> > - return dma_cookie_assign(tx); >> > + return c; >> > } >> > >> If this is the issue, then why not have mxs_dma_enable_chan itself >> call dma_cookie_assign() and return that cookie. >> > As the function name tells, mxs_dma_enable_chan is to only operate dma > hardware to enable the channel but nothing else. > Well then change the name of the function mxs_assign_cookie_enable_chan(), my point still stands. As the case illustrates cookie needs to assigned before enabling the channel then why not do it in that function itself. At worst please put a comment in mxs_assign_cookie_enable() that says cookie must be assigned prior to calling enable channel. -Sam
On Fri, Apr 06, 2012 at 07:06:41AM -0700, Sam Gandhi wrote: ... > Well then change the name of the function > mxs_assign_cookie_enable_chan(), my point still stands. > I just do not see the need of such churn. > As the case illustrates cookie needs to assigned before enabling the > channel then why not do it in that function itself. It's simply because cookie is cookie and channel is channel. > At worst please > put a comment in mxs_assign_cookie_enable() that says cookie must be > assigned prior to calling enable channel. > It's the best rather than worst to me.
> I tested the following code : > ============================================================== > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > index 5978113..0f5b09a 100644 > --- a/drivers/dma/mxs-dma.c > +++ b/drivers/dma/mxs-dma.c > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct > dma_cha > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > + dma_cookie_t c; > > + c = dma_cookie_assign(tx); > mxs_dma_enable_chan(mxs_chan); > > - return dma_cookie_assign(tx); > + return c; > } > > static void mxs_dma_tasklet(unsigned long data) > > Ack on testing. I reverted the change that Vinod had suggested earlier, and applied Huang's suggested change and it fixed the issue. Successfully erased NAND, made UBI partitions and ran integck tests for several hours. -Sam
On Fri, Apr 06, 2012 at 06:34:23PM +0800, Huang Shijie wrote: > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > index 5978113..0f5b09a 100644 > --- a/drivers/dma/mxs-dma.c > +++ b/drivers/dma/mxs-dma.c > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct > dma_cha > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > { > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > + dma_cookie_t c; > > + c = dma_cookie_assign(tx); > mxs_dma_enable_chan(mxs_chan); > > - return dma_cookie_assign(tx); > + return c; Hang on, why are you enabling a channel (and presumably allowing this transaction to be issued) before dma_async_issue_pending() has been called for the channel?
On Tue, Apr 10, 2012 at 10:42:23AM +0100, Russell King - ARM Linux wrote: > On Fri, Apr 06, 2012 at 06:34:23PM +0800, Huang Shijie wrote: > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c > > index 5978113..0f5b09a 100644 > > --- a/drivers/dma/mxs-dma.c > > +++ b/drivers/dma/mxs-dma.c > > @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct > > dma_cha > > static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) > > { > > struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); > > + dma_cookie_t c; > > > > + c = dma_cookie_assign(tx); > > mxs_dma_enable_chan(mxs_chan); > > > > - return dma_cookie_assign(tx); > > + return c; > > Hang on, why are you enabling a channel (and presumably allowing this > transaction to be issued) before dma_async_issue_pending() has been > called for the channel? When mxs-dma driver was created, we chose to only support a single descriptor at the beginning, and mxs_dma_issue_pending() is an empty function right now . This is something needs to be improved, but it should be orthogonal to the bug fix posted here.
On Tue, Apr 10, 2012 at 08:39:58PM +0800, Shawn Guo wrote: > When mxs-dma driver was created, we chose to only support a single > descriptor at the beginning, and mxs_dma_issue_pending() is an empty > function right now . This is something needs to be improved, but it > should be orthogonal to the bug fix posted here. You could view it as being the reason that the bug occurred, because had the issue_pending() function been used to start the transfer, things would naturally happen in the right order. So, one solution to the bug would be to do exactly that - move the channel enable to the issue pending function, which will have the same effect as reversing the order of cookie assignment and channel enable. However, the risk is that because you have a no-op issue_pending(), some of your drivers may have decided its not worth calling and omitted it.
============================================================== diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c index 5978113..0f5b09a 100644 --- a/drivers/dma/mxs-dma.c +++ b/drivers/dma/mxs-dma.c @@ -202,10 +202,12 @@ static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_cha static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) { struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan); + dma_cookie_t c; + c = dma_cookie_assign(tx); mxs_dma_enable_chan(mxs_chan); - return dma_cookie_assign(tx); + return c; }