arm926_dma_flush_range undefined!

arm926_dma_flush_range undefined!

[Nicolas Ferre]

Hi all,

I am trying to compile a recent kernel 
(v2.6.34-rc6-201-g722154e) and I am 
having this kind of error:

ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!

As the arm926_dma_flush_range assembler 
function seems to exits in the kernel, I 
have difficulties to figure out what the 
issue comes from...
As I am maintainer of this driver, I would 
like to fix this very fast ;-)

This error shows up while I compile this 
driver as a module.

Do you have any hint?

Thanks, best regards,
-- 
Nicolas Ferre

arm926_dma_flush_range undefined!

[Catalin Marinas]

On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
> I am trying to compile a recent kernel 
> (v2.6.34-rc6-201-g722154e) and I am 
> having this kind of error:
> 
> ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!

The driver seems to use dmac_flush_range() directly. That's not part of
the DMA API. Could you not use one of the supported DMA API functions?

-- 
Catalin

arm926_dma_flush_range undefined!

[Russell King - ARM Linux]

On Thu, May 06, 2010 at 03:07:11PM +0100, Catalin Marinas wrote:
> On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
> > I am trying to compile a recent kernel 
> > (v2.6.34-rc6-201-g722154e) and I am 
> > having this kind of error:
> > 
> > ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!
> 
> The driver seems to use dmac_flush_range() directly. That's not part of
> the DMA API. Could you not use one of the supported DMA API functions?

Indeed; I've always said that I don't care about drivers directly using
the internals of the DMA API, and drivers doing this will be constantly
subjected to breakage.

I really do not regard the above to be a regression; it's a latent
programming error.  AT91 folk need to fix their driver(s) to use the
proper interfaces rather than using internal functionality.

arm926_dma_flush_range undefined!

[Nicolas Ferre]

Le 06/05/2010 19:59, Russell King - ARM Linux :
> On Thu, May 06, 2010 at 03:07:11PM +0100, Catalin Marinas wrote:
>> On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
>>> I am trying to compile a recent kernel 
>>> (v2.6.34-rc6-201-g722154e) and I am 
>>> having this kind of error:
>>>
>>> ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!
>>
>> The driver seems to use dmac_flush_range() directly. That's not part of
>> the DMA API. Could you not use one of the supported DMA API functions?

The difficult part for me is to choose the proper one ;-)

And also, I am wondering if this call is needed as we do a
kunmap_atomic() on the same scatterlist pointer just before. Does not
kunmap_atomic() embed a cache flushing directive already?


> Indeed; I've always said that I don't care about drivers directly using
> the internals of the DMA API, and drivers doing this will be constantly
> subjected to breakage.
> 
> I really do not regard the above to be a regression; it's a latent
> programming error.  AT91 folk need to fix their driver(s) to use the
> proper interfaces rather than using internal functionality.

For sure, I will !

Thanks, best regards,
-- 
Nicolas Ferre

arm926_dma_flush_range undefined!

[Russell King - ARM Linux]

On Tue, May 11, 2010 at 11:43:17AM +0200, Nicolas Ferre wrote:
> Le 06/05/2010 19:59, Russell King - ARM Linux :
> > On Thu, May 06, 2010 at 03:07:11PM +0100, Catalin Marinas wrote:
> >> On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
> >>> I am trying to compile a recent kernel 
> >>> (v2.6.34-rc6-201-g722154e) and I am 
> >>> having this kind of error:
> >>>
> >>> ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!
> >>
> >> The driver seems to use dmac_flush_range() directly. That's not part of
> >> the DMA API. Could you not use one of the supported DMA API functions?
> 
> The difficult part for me is to choose the proper one ;-)
> 
> And also, I am wondering if this call is needed as we do a
> kunmap_atomic() on the same scatterlist pointer just before. Does not
> kunmap_atomic() embed a cache flushing directive already?

No it doesn't.  What are you trying to do here?

arm926_dma_flush_range undefined!

[Nicolas Ferre]

Le 11/05/2010 11:44, Russell King - ARM Linux :
> On Tue, May 11, 2010 at 11:43:17AM +0200, Nicolas Ferre wrote:
>> Le 06/05/2010 19:59, Russell King - ARM Linux :
>>> On Thu, May 06, 2010 at 03:07:11PM +0100, Catalin Marinas wrote:
>>>> On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
>>>>> I am trying to compile a recent kernel 
>>>>> (v2.6.34-rc6-201-g722154e) and I am 
>>>>> having this kind of error:
>>>>>
>>>>> ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko] undefined!
>>>>
>>>> The driver seems to use dmac_flush_range() directly. That's not part of
>>>> the DMA API. Could you not use one of the supported DMA API functions?
>>
>> The difficult part for me is to choose the proper one ;-)
>>
>> And also, I am wondering if this call is needed as we do a
>> kunmap_atomic() on the same scatterlist pointer just before. Does not
>> kunmap_atomic() embed a cache flushing directive already?
> 
> No it doesn't.  What are you trying to do here?

After a read with DMA from sd/mmc interface, a DMA buffer allocated by
dma_alloc_coherent() is filled.
This buffer is then copied to the scatterlist provided by the mmc
request (from drivers/mmc/card/block.c it seems that sg buffers are
alocacted via kmalloc()).

For each buffer of the scatterlist, I do this dmac_flush_rage()...

As copy is done between a coherent and a kernel memory buffers, I guess
that the flushing routine is not needed?

Best regards,
-- 
Nicolas Ferre

arm926_dma_flush_range undefined!

[Russell King - ARM Linux]

On Tue, May 11, 2010 at 03:32:04PM +0200, Nicolas Ferre wrote:
> After a read with DMA from sd/mmc interface, a DMA buffer allocated by
> dma_alloc_coherent() is filled.
> This buffer is then copied to the scatterlist provided by the mmc
> request (from drivers/mmc/card/block.c it seems that sg buffers are
> alocacted via kmalloc()).

I assume that this means you can't handle scatterlists with your DMA
engine?

> For each buffer of the scatterlist, I do this dmac_flush_rage()...

I think you need to look at how mmci.c does its flushing, and do something
along those lines.  You could use flush_kernel_dcache_page() instead of
flush_dcache_page(), which would be slightly cheaper.

> As copy is done between a coherent and a kernel memory buffers, I guess
> that the flushing routine is not needed?

The issue of coherency with "PIO" writes to block IO pages is going
around the same old loops of discussion yet again at the moment; it's
been a problem for ARM for about the last 10 years or so, and I'm not
expecting there to suddenly be a major change of heart or political
will to fix the issue.

So I think we're stuck with having to have the drivers we really care
about and have our own control over do the flushing themselves.

[PATCH] MMC: at91_mci: modify cache flush routines

[Nicolas Ferre]

As we were using an internal dma flushing routine, this patch changes to the
DMA API flush_kernel_dcache_page(). Driver is able to compile now.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/mmc/host/at91_mci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/at91_mci.c b/drivers/mmc/host/at91_mci.c
index a6dd7da..813d208 100644
--- a/drivers/mmc/host/at91_mci.c
+++ b/drivers/mmc/host/at91_mci.c
@@ -315,7 +315,7 @@ static void at91_mci_post_dma_read(struct at91mci_host *host)
 		}
 
 		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
-		dmac_flush_range((void *)sgbuffer, ((void *)sgbuffer) + amount);
+		flush_kernel_dcache_page(sg_page(sg));
 		data->bytes_xfered += amount;
 		if (size == 0)
 			break;
-- 
1.5.6.5

[PATCH] MMC: at91_mci: modify cache flush routines

[Andrew Morton]

On Tue, 11 May 2010 19:09:53 +0200
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> As we were using an internal dma flushing routine, this patch changes to the
> DMA API flush_kernel_dcache_page(). Driver is able to compile now.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/mmc/host/at91_mci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/at91_mci.c b/drivers/mmc/host/at91_mci.c
> index a6dd7da..813d208 100644
> --- a/drivers/mmc/host/at91_mci.c
> +++ b/drivers/mmc/host/at91_mci.c
> @@ -315,7 +315,7 @@ static void at91_mci_post_dma_read(struct at91mci_host *host)
>  		}
>  
>  		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
> -		dmac_flush_range((void *)sgbuffer, ((void *)sgbuffer) + amount);
> +		flush_kernel_dcache_page(sg_page(sg));
>  		data->bytes_xfered += amount;
>  		if (size == 0)
>  			break;

The flush_kernel_dcache_page() documentation specifically says that
thou shalt run flush_kernel_dcache_page() _prior_ to kunmapping the
page.

I don't know if that makes a difference in the real world, but heck why
not:

--- a/drivers/mmc/host/at91_mci.c~mmc-at91_mci-modify-cache-flush-routines-fix
+++ a/drivers/mmc/host/at91_mci.c
@@ -314,8 +314,8 @@ static void at91_mci_post_dma_read(struc
 			dmabuf = (unsigned *)tmpv;
 		}
 
-		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
 		flush_kernel_dcache_page(sg_page(sg));
+		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
 		data->bytes_xfered += amount;
 		if (size == 0)
 			break;
_

However, I'm wondering why you chose flush_kernel_dcache_page() instead
of plain old flush_dcache_page().  Is this a pagecache or possibly
direct-io page we're dealing with here?

[PATCH] MMC: at91_mci: modify cache flush routines

[Russell King - ARM Linux]

On Wed, May 12, 2010 at 02:19:29PM -0700, Andrew Morton wrote:
> The flush_kernel_dcache_page() documentation specifically says that
> thou shalt run flush_kernel_dcache_page() _prior_ to kunmapping the
> page.

Hmm, interesting - I can't see why that would be, but it doesn't make
any difference for ARM (and this is an ARM only driver).

> I don't know if that makes a difference in the real world, but heck why
> not:

In the interests of stopping cut'n'paste bugs into other drivers, I'd
say this is a good idea even if it makes no difference to ARM.

> However, I'm wondering why you chose flush_kernel_dcache_page() instead
> of plain old flush_dcache_page().  Is this a pagecache or possibly
> direct-io page we're dealing with here?

It's whatever the block layers hand us - which would be page cache pages,
and I'd assume also DIO pages (I'm not up on DIO stuff though.)

It's also my understanding that the preferred interface for drivers which
write to page cache pages is flush_kernel_dcache_page() rather than
flush_dcache_page().

[PATCH] MMC: at91_mci: modify cache flush routines

[Nicolas Ferre]

Le 12/05/2010 23:19, Andrew Morton :
> On Tue, 11 May 2010 19:09:53 +0200
> Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
>> As we were using an internal dma flushing routine, this patch changes to the
>> DMA API flush_kernel_dcache_page(). Driver is able to compile now.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>  drivers/mmc/host/at91_mci.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/at91_mci.c b/drivers/mmc/host/at91_mci.c
>> index a6dd7da..813d208 100644
>> --- a/drivers/mmc/host/at91_mci.c
>> +++ b/drivers/mmc/host/at91_mci.c
>> @@ -315,7 +315,7 @@ static void at91_mci_post_dma_read(struct at91mci_host *host)
>>  		}
>>  
>>  		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
>> -		dmac_flush_range((void *)sgbuffer, ((void *)sgbuffer) + amount);
>> +		flush_kernel_dcache_page(sg_page(sg));
>>  		data->bytes_xfered += amount;
>>  		if (size == 0)
>>  			break;
> 
> The flush_kernel_dcache_page() documentation specifically says that
> thou shalt run flush_kernel_dcache_page() _prior_ to kunmapping the
> page.
> 
> I don't know if that makes a difference in the real world, but heck why
> not:
> 
> --- a/drivers/mmc/host/at91_mci.c~mmc-at91_mci-modify-cache-flush-routines-fix
> +++ a/drivers/mmc/host/at91_mci.c
> @@ -314,8 +314,8 @@ static void at91_mci_post_dma_read(struc
>  			dmabuf = (unsigned *)tmpv;
>  		}
>  
> -		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
>  		flush_kernel_dcache_page(sg_page(sg));
> +		kunmap_atomic(sgbuffer, KM_BIO_SRC_IRQ);
>  		data->bytes_xfered += amount;
>  		if (size == 0)
>  			break;
> _

Andrew, thanks a lot for folding this in my patch.
I had no access to my emails last days and it was good to learn that
this patch went to 2.6.34-final.

Best regards,
-- 
Nicolas Ferre

arm926_dma_flush_range undefined!

[Catalin Marinas]

On Tue, 2010-05-11 at 14:44 +0100, Russell King - ARM Linux wrote:
> On Tue, May 11, 2010 at 03:32:04PM +0200, Nicolas Ferre wrote:
> > As copy is done between a coherent and a kernel memory buffers, I guess
> > that the flushing routine is not needed?
> 
> The issue of coherency with "PIO" writes to block IO pages is going
> around the same old loops of discussion yet again at the moment; it's
> been a problem for ARM for about the last 10 years or so, and I'm not
> expecting there to suddenly be a major change of heart or political
> will to fix the issue.
> 
> So I think we're stuck with having to have the drivers we really care
> about and have our own control over do the flushing themselves.

For non-aliasing VIPT hardware we still have the option of doing what
the IA-64 and PowerPC guys have done (flushing in set_pte_at with
PG_arch_1 meaning "clean"). It may even work for VIVT caches if we take
care to also flush the kernel alias in set_pte_at.

If you are ok with this approach, I'll prepare some patches (it seems
that trying to get an agreement with other architectures isn't very
productive).

-- 
Catalin

arm926_dma_flush_range undefined!

[Russell King - ARM Linux]

On Wed, May 12, 2010 at 12:18:40PM +0100, Catalin Marinas wrote:
> For non-aliasing VIPT hardware we still have the option of doing what
> the IA-64 and PowerPC guys have done (flushing in set_pte_at with
> PG_arch_1 meaning "clean"). It may even work for VIVT caches if we take
> care to also flush the kernel alias in set_pte_at.

It should work for VIVT as well, so lets just go ahead and negate the
PG_arch_1 meaning.  I was rather expecting this to be done a month or
so ago while we still had plenty of time before the next merge window -
and given where we are now (maybe 2 weeks away) I think it's too risky
to push into the upcoming window.

I'm already planning on scheduling the ioremap and LMB changes for the
following window, and I think this should be scheduled likewise.

> If you are ok with this approach, I'll prepare some patches (it seems
> that trying to get an agreement with other architectures isn't very
> productive).

That's what I've always found with this issue; whenever I've talked
with the mainline community about PIO cache coherency, the result has
always been non-productive.  Hence why it's always remained a problem
for us.

The issue is that too many people have their own strong ideas about
how "we" should fix the problem - and each idea is different, and they
don't accept each other's ideas.  They won't even talk to each other
and discuss their differences to resolve it either, presumably because
they see that it's not their problem to resolve.

I'm rather heartened by the outcome of your attempts to discuss this;
it has illustrated that there's no possibility of an agreement being
reached, which although sad, merely confirms my conclusion formed over
the past years.

However, thanks for making the effort; it's a shame it wasn't more
productive in the end.

arm926_dma_flush_range undefined!

[Catalin Marinas]

On Wed, 2010-05-12 at 19:42 +0100, Russell King - ARM Linux wrote:
> On Wed, May 12, 2010 at 12:18:40PM +0100, Catalin Marinas wrote:
> > For non-aliasing VIPT hardware we still have the option of doing what
> > the IA-64 and PowerPC guys have done (flushing in set_pte_at with
> > PG_arch_1 meaning "clean"). It may even work for VIVT caches if we take
> > care to also flush the kernel alias in set_pte_at.
> 
> It should work for VIVT as well, so lets just go ahead and negate the
> PG_arch_1 meaning.  I was rather expecting this to be done a month or
> so ago while we still had plenty of time before the next merge window -
> and given where we are now (maybe 2 weeks away) I think it's too risky
> to push into the upcoming window.

Well, I started and then I had to go on holiday (and drive back ~1500
miles because of the volcanic ash).

> I'm already planning on scheduling the ioremap and LMB changes for the
> following window, and I think this should be scheduled likewise.

The cache flushing change would need some wider testing, so maybe 6-8
weeks should be enough. I'll try to get some rfc patches by the end of
this week.

> > If you are ok with this approach, I'll prepare some patches (it seems
> > that trying to get an agreement with other architectures isn't very
> > productive).
> 
> That's what I've always found with this issue; whenever I've talked
> with the mainline community about PIO cache coherency, the result has
> always been non-productive.  Hence why it's always remained a problem
> for us.

The good thing is that I found out how PowerPC deals with it :)

-- 
Catalin

arm926_dma_flush_range undefined!

[Wolfgang Mües]

Hi,

Am Dienstag, 11. Mai 2010 schrieb Nicolas Ferre:
> Le 11/05/2010 11:44, Russell King - ARM Linux :
> > On Tue, May 11, 2010 at 11:43:17AM +0200, Nicolas Ferre wrote:
> >> Le 06/05/2010 19:59, Russell King - ARM Linux :
> >>> On Thu, May 06, 2010 at 03:07:11PM +0100, Catalin Marinas wrote:
> >>>> On Thu, 2010-05-06 at 15:11 +0200, Nicolas Ferre wrote:
> >>>>> I am trying to compile a recent kernel
> >>>>> (v2.6.34-rc6-201-g722154e) and I am
> >>>>> having this kind of error:
> >>>>> 
> >>>>> ERROR: "arm926_dma_flush_range" [drivers/mmc/host/at91_mci.ko]
> >>>>> undefined!
> >>>> 
> >>>> The driver seems to use dmac_flush_range() directly. That's not part
> >>>> of the DMA API. Could you not use one of the supported DMA API
> >>>> functions?
> >> 
> >> The difficult part for me is to choose the proper one ;-)
> >> 
> >> And also, I am wondering if this call is needed as we do a
> >> kunmap_atomic() on the same scatterlist pointer just before. Does not
> >> kunmap_atomic() embed a cache flushing directive already?
> > 
> > No it doesn't.  What are you trying to do here?
> 
> After a read with DMA from sd/mmc interface, a DMA buffer allocated by
> dma_alloc_coherent() is filled.
> This buffer is then copied to the scatterlist provided by the mmc
> request (from drivers/mmc/card/block.c it seems that sg buffers are
> alocacted via kmalloc()).
> 
> For each buffer of the scatterlist, I do this dmac_flush_rage()...
Yepp!
 
> As copy is done between a coherent and a kernel memory buffers, I guess
> that the flushing routine is not needed?
Where do you get this wisdom?

The coherent buffer does not need a cache flush, never.
But the target buffer does need a flush(). If the loaded data from SD card is 
an executable or library, the target buffer may be mapped into the text 
segment of a process and be executed. And this will fail if parts of the
data are only inside the data cache (especially on ARM architectures where
data and instruction cache are separate).

The basic assumption is that the scatterlist buffer is filled directly with a 
DMA controller: data is in memory, not in the cache.
If you fill the scatterlist buffer with the help of the CPU, you need a data 
cache flush afterwards to fullfill this assumption.
It's a shame that the function to do so is not available on all platforms. 
IMHO it should.

best regards
 
i. A. Wolfgang M?es
-- 
Auerswald Gesellschaft f?r Datensysteme mbH
Hardware Development
Telefon: +49 (0)5306 9219 562
Telefax: +49 (0)5306 9219 94 
E-Mail: Wolfgang.Mues at Auerswald.de
Web: http://www.auerswald.de
 
--------------------------------------------------------------
Auerswald Gesellschaft f?r Datensysteme mbH
Vor den Grash?fen 1, 38162 Cremlingen
Registriert beim AG Braunschweig HRB 7499
Gesch?ftsf?hrer: Dipl-Ing. Gerhard Auerswald