[MTD/NAND] Blackfin NFC driver DMA bug ?

[MTD/NAND] Blackfin NFC driver DMA bug ?

[Ivan Djelic]

Hello All,

While working on a NAND flash MTD driver, I came across the following piece of
code in the Blackfin bf5xx NAND flash controller driver:

>From linux-2.6.24.2, file bf5xx_nand.c:418, function bf5xx_nand_dma_rw():

	/*
	 * Before starting a dma transfer, be sure to invalidate/flush
	 * the cache over the address range of your DMA buffer to
	 * prevent cache coherency problems. Otherwise very subtle bugs
	 * can be introduced to your driver.
	 */
	if (is_read)
		invalidate_dcache_range((unsigned int)buf,
				(unsigned int)(buf + page_size));
	else
		flush_dcache_range((unsigned int)buf,
				(unsigned int)(buf + page_size));

Since 'buf' is allocated outside MTD, are we allowed to assume it is
cache-aligned ? Because if it's not, invalidating dcache on read is not enough
to prevent cache coherency problems. For instance, a cache line partially
spanning across the buffer address range could be flushed just after DMA has
completed, corrupting DMA data in the process...
Or am I missing something, since I am not familiar with the Blackfin arch ?

Thanks,

Ivan

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[Bryan Wu]

On Fri, Feb 22, 2008 at 1:58 AM, Ivan Djelic <ivan.djelic@parrot.com> wrote:
> Hello All,
>
>  While working on a NAND flash MTD driver, I came across the following piece of
>  code in the Blackfin bf5xx NAND flash controller driver:
>

Thanks, I am always here to answer questions, -;)

>  >From linux-2.6.24.2, file bf5xx_nand.c:418, function bf5xx_nand_dma_rw():
>
>         /*
>          * Before starting a dma transfer, be sure to invalidate/flush
>          * the cache over the address range of your DMA buffer to
>          * prevent cache coherency problems. Otherwise very subtle bugs
>          * can be introduced to your driver.
>          */
>         if (is_read)
>                 invalidate_dcache_range((unsigned int)buf,
>                                 (unsigned int)(buf + page_size));
>         else
>                 flush_dcache_range((unsigned int)buf,
>                                 (unsigned int)(buf + page_size));
>
>  Since 'buf' is allocated outside MTD, are we allowed to assume it is
>  cache-aligned ? Because if it's not, invalidating dcache on read is not enough
>  to prevent cache coherency problems. For instance, a cache line partially
>  spanning across the buffer address range could be flushed just after DMA has
>  completed, corrupting DMA data in the process...

Oh, I am not fully understand your concern.  The code is invalidating
or flushing buf before DMA operation.
And invalidate and flush operation is OK for buf which is not
cache-aligned on Blackfin arch. it also should be
OK for other arch.

-Bryan

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[David Woodhouse]

On Fri, 2008-02-22 at 10:52 +0800, Bryan Wu wrote:
> Oh, I am not fully understand your concern.  The code is invalidating
> or flushing buf before DMA operation.
> And invalidate and flush operation is OK for buf which is not
> cache-aligned on Blackfin arch. it also should be
> OK for other arch.

The problem occurs when your buffer is not aligned at the beginning of a
cache line. If there is other data on the _same_ cache line as your
buffer, and you invalidate the cache, then you may cause data loss
outside the buffer.

Also, in some systems you must make sure that allocations to be used for
DMA are from a certain memory pool.

As for the original question... I'm not sure. At the moment I don't
believe it's true that all such buffers are suitable for DMA. Perhaps it
would be sensible for us to redefine the MTD API so that it is required
(and fix the users).

For a long time, flash I/O was always done by the CPU instead of DMA, so
it wasn't an interesting question. I did start wondering when I
implemented support for the CAFÉ controller on OLPC, but then that
turned out to need bounce buffers anyway so it escaped my attention
again.

-- 
dwmw2

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[Bryan Wu]

On Fri, Feb 22, 2008 at 3:05 PM, David Woodhouse <dwmw2@infradead.org> wrote:
>
>  On Fri, 2008-02-22 at 10:52 +0800, Bryan Wu wrote:
>  > Oh, I am not fully understand your concern.  The code is invalidating
>  > or flushing buf before DMA operation.
>  > And invalidate and flush operation is OK for buf which is not
>  > cache-aligned on Blackfin arch. it also should be
>  > OK for other arch.
>
>  The problem occurs when your buffer is not aligned at the beginning of a
>  cache line. If there is other data on the _same_ cache line as your
>  buffer, and you invalidate the cache, then you may cause data loss
>  outside the buffer.
>

Yes, I understand that. I prefer to make sure this buffer issue in the
MTD core API.

>  Also, in some systems you must make sure that allocations to be used for
>  DMA are from a certain memory pool.
>
>  As for the original question... I'm not sure. At the moment I don't
>  believe it's true that all such buffers are suitable for DMA. Perhaps it
>  would be sensible for us to redefine the MTD API so that it is required
>  (and fix the users).
>

Exactly, maybe xxx_nand_dma_read() and xxx_nand_dma_write().
The buffer for this DMA functions should be cache-aligned.

>  For a long time, flash I/O was always done by the CPU instead of DMA, so
>  it wasn't an interesting question. I did start wondering when I
>  implemented support for the CAFÉ controller on OLPC, but then that
>  turned out to need bounce buffers anyway so it escaped my attention
>  again.
>

We might need to add DMA support in the MTD NAND interface, because
more and more SoC support on-chip NAND controller with DMA support.

Thanks a lot
-Bryan Wu

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[Ivan Djelic]

On Fri, Feb 22, 2008 at 10:52:49AM +0800, Bryan Wu wrote:
> >         /*
> >          * Before starting a dma transfer, be sure to invalidate/flush
> >          * the cache over the address range of your DMA buffer to
> >          * prevent cache coherency problems. Otherwise very subtle bugs
> >          * can be introduced to your driver.
> >          */
> >         if (is_read)
> >                 invalidate_dcache_range((unsigned int)buf,
> >                                 (unsigned int)(buf + page_size));
> >         else
> >                 flush_dcache_range((unsigned int)buf,
> >                                 (unsigned int)(buf + page_size));
> >
> >  Since 'buf' is allocated outside MTD, are we allowed to assume it is
> >  cache-aligned ? Because if it's not, invalidating dcache on read is not enough
> >  to prevent cache coherency problems. For instance, a cache line partially
> >  spanning across the buffer address range could be flushed just after DMA has
> >  completed, corrupting DMA data in the process...
> 
> Oh, I am not fully understand your concern.  The code is invalidating
> or flushing buf before DMA operation.
> And invalidate and flush operation is OK for buf which is not
> cache-aligned on Blackfin arch. it also should be
> OK for other arch.

Well, consider the following scenario, in which 'buf' is not cache-aligned:

1) function bf5xx_nand_dma_rw() is called for reading data.

2) dcache is partially invalidated on a range containing 'buf': this range is
cache-aligned, of course.

3) at this point, imagine some variable residing nearby 'buf' (in the same
cache line interval) is read and modified, resulting in a cache line being
fetched and modified (hence becoming dirty).

4) DMA transfer happens, modifying data in memory.

5) now, the dirty cache line from 3) can be flushed to memory anytime...
corrupting DMA data, because its contents were read *before* the DMA transfer !

Or consider this other scenario:

1) and 2) same as above.

3) some variable residing nearby 'buf' (in the same cache line interval) is
read (not modified), resulting in a cache line being fetched. Cache line is not
dirty.

4) DMA transfer happens.

5) now, if you read data from 'buf', you will get wrong (stale) data because the
cache line contents were read *before* the DMA transfer.


To avoid those kind of problems you would need a reliable way to prevent the
cache from messing with your buffer during the whole DMA transfer.

Ivan

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[Ivan Djelic]

On Fri, Feb 22, 2008 at 09:45:33AM +0100, Ivan Djelic wrote:
> > Oh, I am not fully understand your concern.  The code is invalidating
> > or flushing buf before DMA operation.
> > And invalidate and flush operation is OK for buf which is not
> > cache-aligned on Blackfin arch. it also should be
> > OK for other arch.
> 
> Well, consider the following scenario, in which 'buf' is not cache-aligned:
> 
> 1) function bf5xx_nand_dma_rw() is called for reading data.
> 
> 2) dcache is partially invalidated on a range containing 'buf': this range is
> cache-aligned, of course.
> 
> 3) some variable residing nearby 'buf' (in the same cache line interval) is
> read (not modified), resulting in a cache line being fetched. Cache line is not
> dirty.

Also, the variable contents in 3) may also be corrupted because of the
invalidation in 2).

Re: [MTD/NAND] Blackfin NFC driver DMA bug ?

[Bryan Wu]

On Fri, Feb 22, 2008 at 4:54 PM, Ivan Djelic <ivan.djelic@parrot.com> wrote:
> On Fri, Feb 22, 2008 at 09:45:33AM +0100, Ivan Djelic wrote:
>  > > Oh, I am not fully understand your concern.  The code is invalidating
>  > > or flushing buf before DMA operation.
>  > > And invalidate and flush operation is OK for buf which is not
>  > > cache-aligned on Blackfin arch. it also should be
>  > > OK for other arch.
>  >
>  > Well, consider the following scenario, in which 'buf' is not cache-aligned:
>  >
>  > 1) function bf5xx_nand_dma_rw() is called for reading data.
>  >
>  > 2) dcache is partially invalidated on a range containing 'buf': this range is
>  > cache-aligned, of course.
>  >
>
> > 3) some variable residing nearby 'buf' (in the same cache line interval) is
>  > read (not modified), resulting in a cache line being fetched. Cache line is not
>  > dirty.
>
>  Also, the variable contents in 3) may also be corrupted because of the
>  invalidation in 2).
>

Thanks a lot for the clarification.

If can be fixed in drivers or in MTD API, maybe MTD API is good place
to fix it completely as David mentioned.
I will found some way to fix it in my driver firstly.

Regards,
-Bryan Wu