Patchwork Using GPMI-NAND driver on iMX28 using 3.4-rc1?

login
register
mail settings
Submitter Huang Shijie
Date April 6, 2012, 10:34 a.m.
Message ID <4F7EC6AF.6000603@freescale.com>
Download mbox | patch
Permalink /patch/151163/
State New
Headers show

Comments

Huang Shijie - April 6, 2012, 10:34 a.m.
于 2012年04月06日 10:23, Shawn Guo 写道:
> On Thu, Apr 05, 2012 at 05:57:41PM +0530, Vinod Koul wrote:
>> On Thu, 2012-04-05 at 05:03 -0700, Sam Gandhi wrote:
>>> 2012/4/4 Huang Shijie<b32955@freescale.com>:
> ...
>>>>>> flash_erase /dev/mtd1 0 0
>>>>>> Erasing 1------------[ cut here ]------------
>>>>>> kernel BUG at /home/sam/linux/drivers/dma/dmaengine.h:53!
>>>> the mxs-dma has added some patches about the cookie.
>>>> The bug is in the dmaengine.h.
>>>>
>>>> So let more people know this bug.
>>>>
> I'm still trying to understand why I did run into the BUG_ON when I
> test it with mxs mmc and sound drivers.
>
> ...
>
>> This means you are trying to mark a cookie complete when it is already
>> marked so!, hence dmaengine screams.
>> The bug is is mxs-dma.
>> Let me know if below fixes it (assuming you are not using cyclic API,
>> that would need fixup as well)
>>
>> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
>> index c81ef7e..5ddd84e 100644
>> --- a/drivers/dma/mxs-dma.c
>> +++ b/drivers/dma/mxs-dma.c
>> @@ -399,6 +399,10 @@ static struct dma_async_tx_descriptor
>> *mxs_dma_prep_slave_sg(
>>   		ccw->bits&= ~CCW_DEC_SEM;
>>   	} else {
>>   		idx = 0;
>> +		/* assign cookie here,
>> +		 * hopefully for above case we dont need it
>> +		 */
>> +		dma_cookie_assign(&mxs_chan->desc);
> Isn't it done in mxs_dma_tx_submit() already? The gpmi driver somehow

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 :
static void mxs_dma_tasklet(unsigned long data)


> misses the call to dmaengine_submit() for some case?
>
> Regards,
> Shawn
>
>>   	}
>>
>>   	if (direction == DMA_TRANS_NONE) {
>>
>> --
Shawn Guo - April 6, 2012, 12:49 p.m.
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)
>
Sam Gandhi - April 6, 2012, 1:49 p.m.
>
>
> 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
Shawn Guo - April 6, 2012, 1:59 p.m.
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.
Sam Gandhi - April 6, 2012, 2:06 p.m.
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
Shawn Guo - April 6, 2012, 2:21 p.m.
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.
Sam Gandhi - April 6, 2012, 2:53 p.m.
> 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
Russell King - ARM Linux - April 10, 2012, 9:42 a.m.
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?
Shawn Guo - April 10, 2012, 12:39 p.m.
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.
Russell King - ARM Linux - April 10, 2012, 12:47 p.m.
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.

Patch

==============================================================

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;
}