[PATCH] ioremap_cached()

[PATCH] ioremap_cached()

[Matthew Wilcox]

We currently have three ways for getting access to device memory --
ioremap(), ioremap_nocache() and pci_iomap().  99% of the callers of
ioremap() are doing it to access device registers, and really, really
want to use ioremap_nocache() instead.  I presume nobody notices on PCs
because they have write-through caches, but it ought to trip up people
trying to flush writes.

pci_iomap() gets this right -- it calls ioremap() if the resource is
tagged with IORESOURCE_CACHEABLE and ioremap_nocache() if it isn't.
Unfortunately, the only resources tagged with IORESOURCE_CACHEABLE
are PCI ROM resources, so even frame buffers and similar that could be
cached aren't.

It seems clear to me that ioremap() and ioremap_nocache() are the
wrong way around.  The default needs to be uncached -- and the obvious
(rare) alternative becomes ioremap_cached().

So here's a patch for i386 to add ioremap_cached() and make ioremap()
uncached.  ioremap_nocache() remains as an alias for ioremap() so we
don't needlessly break old drivers.  Architecture maintainers will need
to fix up their ports if this patch is accepted.


Index: include/asm-i386/io.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-i386/io.h,v
retrieving revision 1.10
diff -u -p -r1.10 io.h
--- a/include/asm-i386/io.h	17 Jan 2006 14:52:36 -0000	1.10
+++ b/include/asm-i386/io.h	30 Mar 2006 16:39:22 -0000
@@ -115,12 +115,13 @@ extern void __iomem * __ioremap(unsigned
  * address. 
  */
 
-static inline void __iomem * ioremap(unsigned long offset, unsigned long size)
+static inline void __iomem * ioremap_cached(unsigned long offset, unsigned long size)
 {
 	return __ioremap(offset, size, 0);
 }
 
-extern void __iomem * ioremap_nocache(unsigned long offset, unsigned long size);
+#define ioremap_nocache(a, b) ioremap(a, b)
+extern void __iomem * ioremap(unsigned long offset, unsigned long size);
 extern void iounmap(volatile void __iomem *addr);
 
 /*
Index: arch/i386/mm/ioremap.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/i386/mm/ioremap.c,v
retrieving revision 1.13
diff -u -p -r1.13 ioremap.c
--- a/arch/i386/mm/ioremap.c	19 Dec 2005 12:42:12 -0000	1.13
+++ b/arch/i386/mm/ioremap.c	30 Mar 2006 16:39:22 -0000
@@ -167,11 +167,11 @@ void __iomem * __ioremap(unsigned long p
 EXPORT_SYMBOL(__ioremap);
 
 /**
- * ioremap_nocache     -   map bus memory into CPU space
+ * ioremap     -   map bus memory into CPU space
  * @offset:    bus address of the memory
  * @size:      size of the resource to map
  *
- * ioremap_nocache performs a platform specific sequence of operations to
+ * ioremap performs a platform specific sequence of operations to
  * make bus memory CPU accessible via the readb/readw/readl/writeb/
  * writew/writel functions and the other mmio helpers. The returned
  * address is not guaranteed to be usable directly as a virtual
@@ -188,7 +188,7 @@ EXPORT_SYMBOL(__ioremap);
  * Must be freed with iounmap.
  */
 
-void __iomem *ioremap_nocache (unsigned long phys_addr, unsigned long size)
+void __iomem *ioremap(unsigned long phys_addr, unsigned long size)
 {
 	unsigned long last_addr;
 	void __iomem *p = __ioremap(phys_addr, size, _PAGE_PCD);
@@ -221,7 +221,7 @@ void __iomem *ioremap_nocache (unsigned 
 
 	return p;					
 }
-EXPORT_SYMBOL(ioremap_nocache);
+EXPORT_SYMBOL(ioremap);
 
 /**
  * iounmap - Free a IO remapping

Re: [PATCH] ioremap_cached()

[Andi Kleen]

Matthew Wilcox <matthew@wil.cx> writes:

> We currently have three ways for getting access to device memory --
> ioremap(), ioremap_nocache() and pci_iomap().  99% of the callers of
> ioremap() are doing it to access device registers, and really, really
> want to use ioremap_nocache() instead.  I presume nobody notices on PCs
> because they have write-through caches, but it ought to trip up people
> trying to flush writes.

Actually MTRRs take care of that on x86.
So essentially on x86 ioremap() for devices is already ioremap_uncached()
And ioremap on memory is cached.

That's nice and simple semantics that other platforms can emulate too.
Doing things differently will just cause pain for the other platforms
when they have to fix up drivers all the time.

It all works fine until someone wants WC too. I would rather add a 
ioremap_wc(), that would be more useful.

-Andi

Re: [PATCH] ioremap_cached()

[Matthew Wilcox]

On Thu, Mar 30, 2006 at 08:27:53PM +0200, Andi Kleen wrote:
> Matthew Wilcox <matthew@wil.cx> writes:
> 
> > We currently have three ways for getting access to device memory --
> > ioremap(), ioremap_nocache() and pci_iomap().  99% of the callers of
> > ioremap() are doing it to access device registers, and really, really
> > want to use ioremap_nocache() instead.  I presume nobody notices on PCs
> > because they have write-through caches, but it ought to trip up people
> > trying to flush writes.
> 
> Actually MTRRs take care of that on x86.
> So essentially on x86 ioremap() for devices is already ioremap_uncached()
> And ioremap on memory is cached.
> 
> That's nice and simple semantics that other platforms can emulate too.
> Doing things differently will just cause pain for the other platforms
> when they have to fix up drivers all the time.

That doesn't make any sense.  What's the point of ioremap_nocache() if
ioremap() does magic things that make things uncached?  And who says
you're allowed to ioremap() memory anyway?

> It all works fine until someone wants WC too. I would rather add a 
> ioremap_wc(), that would be more useful.

ioremap_wc() sounds like a good idea.

Re: [PATCH] ioremap_cached()

[Andi Kleen]

On Thursday 30 March 2006 21:34, Matthew Wilcox wrote:
> On Thu, Mar 30, 2006 at 08:27:53PM +0200, Andi Kleen wrote:
> > Matthew Wilcox <matthew@wil.cx> writes:
> > 
> > > We currently have three ways for getting access to device memory --
> > > ioremap(), ioremap_nocache() and pci_iomap().  99% of the callers of
> > > ioremap() are doing it to access device registers, and really, really
> > > want to use ioremap_nocache() instead.  I presume nobody notices on PCs
> > > because they have write-through caches, but it ought to trip up people
> > > trying to flush writes.
> > 
> > Actually MTRRs take care of that on x86.
> > So essentially on x86 ioremap() for devices is already ioremap_uncached()
> > And ioremap on memory is cached.
> > 
> > That's nice and simple semantics that other platforms can emulate too.
> > Doing things differently will just cause pain for the other platforms
> > when they have to fix up drivers all the time.
> 
> That doesn't make any sense.  What's the point of ioremap_nocache() if
> ioremap() does magic things that make things uncached? 

In theory ioremap_nocache would force uncached even if there is no MTRR
and is better/clearer.

But on x86 there normally is, so lots of code gets it wrong.

My point is just that forcing a semantics that's not enforced
on x86 would be a uphill battle for everybody else. Probably not
a good idea. Better fake x86.

> And who says 
> you're allowed to ioremap() memory anyway?

Why not? There are cases when it's needed.

> > It all works fine until someone wants WC too. I would rather add a 
> > ioremap_wc(), that would be more useful.
> 
> ioremap_wc() sounds like a good idea.

It's unfortunately tricky to get it fully right on x86. The issue
is to have a good way avoid illegal cache aliases. There were some
attempts, but so far they never were polished up from the initial
prototypes.

-Andi

Re: [PATCH] ioremap_cached()

[Matthew Wilcox]

On Thu, Mar 30, 2006 at 09:50:05PM +0200, Andi Kleen wrote:
> > That doesn't make any sense.  What's the point of ioremap_nocache() if
> > ioremap() does magic things that make things uncached? 
> 
> In theory ioremap_nocache would force uncached even if there is no MTRR
> and is better/clearer.
> 
> But on x86 there normally is, so lots of code gets it wrong.
> 
> My point is just that forcing a semantics that's not enforced
> on x86 would be a uphill battle for everybody else. Probably not
> a good idea. Better fake x86.

I think you misunderstood.  The right interface to call, that should
work everywhere, should be the simple, obvious one.  ioremap().  That
effectively is what everyone gets anyway (since they test on x86).
So change the *definition* of ioremap() to be uncached.  Then we can add
ioremap_wc() and ioremap_cached() for these special purpose mappings.

> It's unfortunately tricky to get it fully right on x86. The issue
> is to have a good way avoid illegal cache aliases. There were some
> attempts, but so far they never were polished up from the initial
> prototypes.

I know there's similar issues with Itanium.  IIRC, the EFI memory map
helps here by saying how various different types of memory should be
mapped.

Re: [PATCH] ioremap_cached()

[Andi Kleen]

On Thursday 30 March 2006 22:14, Matthew Wilcox wrote:

> I think you misunderstood.  The right interface to call, that should
> work everywhere, should be the simple, obvious one.  ioremap().  That
> effectively is what everyone gets anyway (since they test on x86).
> So change the *definition* of ioremap() to be uncached.  Then we can add
> ioremap_wc() and ioremap_cached() for these special purpose mappings.

That would break all the current users who do ioremap on memory
and want it cached.

-Andi

Re: [PATCH] ioremap_cached()

[Kumar Gala]

On Mar 30, 2006, at 2:17 PM, Andi Kleen wrote:

> On Thursday 30 March 2006 22:14, Matthew Wilcox wrote:
>
>> I think you misunderstood.  The right interface to call, that should
>> work everywhere, should be the simple, obvious one.  ioremap().  That
>> effectively is what everyone gets anyway (since they test on x86).
>> So change the *definition* of ioremap() to be uncached.  Then we  
>> can add
>> ioremap_wc() and ioremap_cached() for these special purpose mappings.
>
> That would break all the current users who do ioremap on memory
> and want it cached.

What's an example of this?  I ask since on powerpc ioremap() is  
always _PAGE_NO_CACHE.

- kumar

Re: [PATCH] ioremap_cached()

[Andi Kleen]

On Thursday 30 March 2006 22:21, Kumar Gala wrote:
> 
> On Mar 30, 2006, at 2:17 PM, Andi Kleen wrote:
> 
> > On Thursday 30 March 2006 22:14, Matthew Wilcox wrote:
> >
> >> I think you misunderstood.  The right interface to call, that should
> >> work everywhere, should be the simple, obvious one.  ioremap().  That
> >> effectively is what everyone gets anyway (since they test on x86).
> >> So change the *definition* of ioremap() to be uncached.  Then we  
> >> can add
> >> ioremap_wc() and ioremap_cached() for these special purpose mappings.
> >
> > That would break all the current users who do ioremap on memory
> > and want it cached.
> 
> What's an example of this?  I ask since on powerpc ioremap() is  
> always _PAGE_NO_CACHE.

ACPI, IPMI, DMI, ... I bet there are more. It's needed every time
a driver needs to look for some firmware table because it might
not be mapped.

-Andi

Re: [PATCH] ioremap_cached()

[Arjan van de Ven]

> 
> It seems clear to me that ioremap() and ioremap_nocache() are the
> wrong way around.  The default needs to be uncached -- and the obvious
> (rare) alternative becomes ioremap_cached().
> 
> So here's a patch for i386 to add ioremap_cached() and make ioremap()
> uncached.  ioremap_nocache() remains as an alias for ioremap() so we
> don't needlessly break old drivers.  Architecture maintainers will need
> to fix up their ports if this patch is accepted.

I'd actually suggest deprecating ioremap() and moving away from it to
make sure all users think of which variant they want. Explicit naming is
always better than "some" unknown behavior (especially for old drivers
that work on multiple kernels... while you allow them to keep compiling,
you change the world under them so I think eventually break them
compiling is actually better than having them limp along broken)