diff mbox

[v2,02/22] asm-generic/io.h: add ioremap_nopost remap interface

Message ID 20170327094954.7162-3-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lorenzo Pieralisi March 27, 2017, 9:49 a.m. UTC
The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
Posting") mandate non-posted configuration transactions. As further
highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
Considerations for the Enhanced Configuration Access Mechanism"),
through ECAM and ECAM-derivative configuration mechanism, the memory
mapped transactions from the host CPU into Configuration Requests on the
PCI express fabric may create ordering problems for software because
writes to memory address are typically posted transactions (unless the
architecture can enforce through virtual address mapping non-posted
write transactions behaviour) but writes to Configuration Space are not
posted on the PCI express fabric.

Current DT and ACPI host bridge controllers map PCI configuration space
(ECAM and ECAM-derivative) into the virtual address space through
ioremap() calls, that are non-cacheable device accesses on most
architectures, but may provide "bufferable" or "posted" write semantics
in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
to be buffered in the bus connecting the host CPU to the PCI fabric;
this behaviour, as underlined in the PCIe specifications, may trigger
transactions ordering rules and must be prevented.

Introduce a new generic and explicit API to create a memory
mapping for ECAM and ECAM-derivative config space area that
defaults to ioremap_nocache() (which should provide a sane default
behaviour) but still allowing architectures on which ioremap_nocache()
results in posted write transactions to override the function
call with an arch specific implementation that complies with
the PCI specifications for configuration transactions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
---
 arch/alpha/include/asm/io.h | 1 +
 arch/avr32/include/asm/io.h | 1 +
 arch/frv/include/asm/io.h   | 1 +
 arch/ia64/include/asm/io.h  | 1 +
 arch/x86/include/asm/io.h   | 1 +
 include/asm-generic/io.h    | 4 ++++
 6 files changed, 9 insertions(+)

Comments

Bjorn Helgaas March 28, 2017, 1:41 a.m. UTC | #1
On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> Posting") mandate non-posted configuration transactions. As further
> highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> Considerations for the Enhanced Configuration Access Mechanism"),
> through ECAM and ECAM-derivative configuration mechanism, the memory
> mapped transactions from the host CPU into Configuration Requests on the
> PCI express fabric may create ordering problems for software because
> writes to memory address are typically posted transactions (unless the
> architecture can enforce through virtual address mapping non-posted
> write transactions behaviour) but writes to Configuration Space are not
> posted on the PCI express fabric.
> 
> Current DT and ACPI host bridge controllers map PCI configuration space
> (ECAM and ECAM-derivative) into the virtual address space through
> ioremap() calls, that are non-cacheable device accesses on most
> architectures, but may provide "bufferable" or "posted" write semantics
> in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> to be buffered in the bus connecting the host CPU to the PCI fabric;
> this behaviour, as underlined in the PCIe specifications, may trigger
> transactions ordering rules and must be prevented.
> 
> Introduce a new generic and explicit API to create a memory
> mapping for ECAM and ECAM-derivative config space area that
> defaults to ioremap_nocache() (which should provide a sane default
> behaviour) but still allowing architectures on which ioremap_nocache()
> results in posted write transactions to override the function
> call with an arch specific implementation that complies with
> the PCI specifications for configuration transactions.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
> ---
>  arch/alpha/include/asm/io.h | 1 +
>  arch/avr32/include/asm/io.h | 1 +
>  arch/frv/include/asm/io.h   | 1 +
>  arch/ia64/include/asm/io.h  | 1 +
>  arch/x86/include/asm/io.h   | 1 +
>  include/asm-generic/io.h    | 4 ++++
>  6 files changed, 9 insertions(+)
> 
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index ff40491..27379ea 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned long offset,
>  }
>  
>  #define ioremap_uc ioremap_nocache
> +#define ioremap_nopost ioremap_nocache
>  
>  static inline void iounmap(volatile void __iomem *addr)
>  {
> diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h
> index f855646..3f1ced8 100644
> --- a/arch/avr32/include/asm/io.h
> +++ b/arch/avr32/include/asm/io.h
> @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr);
>  #define ioremap_wc ioremap_nocache
>  #define ioremap_wt ioremap_nocache
>  #define ioremap_uc ioremap_nocache
> +#define ioremap_nopost ioremap_nocache
>  
>  #define cached(addr) P1SEGADDR(addr)
>  #define uncached(addr) P2SEGADDR(addr)
> diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h
> index 8062fc7..302fb8c 100644
> --- a/arch/frv/include/asm/io.h
> +++ b/arch/frv/include/asm/io.h
> @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned long physaddr, unsigned l
>  
>  #define ioremap_wc ioremap_nocache
>  #define ioremap_uc ioremap_nocache
> +#define ioremap_nopost ioremap_nocache
>  
>  extern void iounmap(void volatile __iomem *addr);
>  
> diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
> index 5de673a..70a4985 100644
> --- a/arch/ia64/include/asm/io.h
> +++ b/arch/ia64/include/asm/io.h
> @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned lo
>  }
>  #define ioremap_cache ioremap_cache
>  #define ioremap_uc ioremap_nocache
> +#define ioremap_nopost ioremap_nocache
>  
>  
>  /*
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 7afb0e2..50b292f 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
>  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
>  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
>  #define ioremap_uc ioremap_uc
> +#define ioremap_nopost ioremap_nocache

These are all the same as the default from asm-generic.h.  Do we really
need these definitions in alpha, avr32, frv, ia64, x86?

>  extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
>  extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 7ef015e..0e81938 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
>  #endif /* CONFIG_GENERIC_IOMAP */
>  #endif /* CONFIG_HAS_IOPORT_MAP */
>  
> +#ifndef ioremap_nopost
> +#define ioremap_nopost ioremap_nocache
> +#endif
> +
>  #ifndef xlate_dev_kmem_ptr
>  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
>  static inline void *xlate_dev_kmem_ptr(void *addr)
> -- 
> 2.10.0
>
Lorenzo Pieralisi March 28, 2017, 2:45 p.m. UTC | #2
On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
> > Posting") mandate non-posted configuration transactions. As further
> > highlighted in the PCIe specifications (4.0 - Rev0.3, "Ordering
> > Considerations for the Enhanced Configuration Access Mechanism"),
> > through ECAM and ECAM-derivative configuration mechanism, the memory
> > mapped transactions from the host CPU into Configuration Requests on the
> > PCI express fabric may create ordering problems for software because
> > writes to memory address are typically posted transactions (unless the
> > architecture can enforce through virtual address mapping non-posted
> > write transactions behaviour) but writes to Configuration Space are not
> > posted on the PCI express fabric.
> > 
> > Current DT and ACPI host bridge controllers map PCI configuration space
> > (ECAM and ECAM-derivative) into the virtual address space through
> > ioremap() calls, that are non-cacheable device accesses on most
> > architectures, but may provide "bufferable" or "posted" write semantics
> > in architecture like eg ARM/ARM64 that allow ioremap'ed regions writes
> > to be buffered in the bus connecting the host CPU to the PCI fabric;
> > this behaviour, as underlined in the PCIe specifications, may trigger
> > transactions ordering rules and must be prevented.
> > 
> > Introduce a new generic and explicit API to create a memory
> > mapping for ECAM and ECAM-derivative config space area that
> > defaults to ioremap_nocache() (which should provide a sane default
> > behaviour) but still allowing architectures on which ioremap_nocache()
> > results in posted write transactions to override the function
> > call with an arch specific implementation that complies with
> > the PCI specifications for configuration transactions.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
> > ---
> >  arch/alpha/include/asm/io.h | 1 +
> >  arch/avr32/include/asm/io.h | 1 +
> >  arch/frv/include/asm/io.h   | 1 +
> >  arch/ia64/include/asm/io.h  | 1 +
> >  arch/x86/include/asm/io.h   | 1 +
> >  include/asm-generic/io.h    | 4 ++++
> >  6 files changed, 9 insertions(+)
> > 
> > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> > index ff40491..27379ea 100644
> > --- a/arch/alpha/include/asm/io.h
> > +++ b/arch/alpha/include/asm/io.h
> > @@ -300,6 +300,7 @@ static inline void __iomem * ioremap_nocache(unsigned long offset,
> >  }
> >  
> >  #define ioremap_uc ioremap_nocache
> > +#define ioremap_nopost ioremap_nocache
> >  
> >  static inline void iounmap(volatile void __iomem *addr)
> >  {
> > diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h
> > index f855646..3f1ced8 100644
> > --- a/arch/avr32/include/asm/io.h
> > +++ b/arch/avr32/include/asm/io.h
> > @@ -298,6 +298,7 @@ extern void __iounmap(void __iomem *addr);
> >  #define ioremap_wc ioremap_nocache
> >  #define ioremap_wt ioremap_nocache
> >  #define ioremap_uc ioremap_nocache
> > +#define ioremap_nopost ioremap_nocache
> >  
> >  #define cached(addr) P1SEGADDR(addr)
> >  #define uncached(addr) P2SEGADDR(addr)
> > diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h
> > index 8062fc7..302fb8c 100644
> > --- a/arch/frv/include/asm/io.h
> > +++ b/arch/frv/include/asm/io.h
> > @@ -290,6 +290,7 @@ static inline void __iomem *ioremap_fullcache(unsigned long physaddr, unsigned l
> >  
> >  #define ioremap_wc ioremap_nocache
> >  #define ioremap_uc ioremap_nocache
> > +#define ioremap_nopost ioremap_nocache
> >  
> >  extern void iounmap(void volatile __iomem *addr);
> >  
> > diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
> > index 5de673a..70a4985 100644
> > --- a/arch/ia64/include/asm/io.h
> > +++ b/arch/ia64/include/asm/io.h
> > @@ -434,6 +434,7 @@ static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned lo
> >  }
> >  #define ioremap_cache ioremap_cache
> >  #define ioremap_uc ioremap_nocache
> > +#define ioremap_nopost ioremap_nocache
> >  
> >  
> >  /*
> > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > index 7afb0e2..50b292f 100644
> > --- a/arch/x86/include/asm/io.h
> > +++ b/arch/x86/include/asm/io.h
> > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> >  #define ioremap_uc ioremap_uc
> > +#define ioremap_nopost ioremap_nocache
> 
> These are all the same as the default from asm-generic.h.  Do we really
> need these definitions in alpha, avr32, frv, ia64, x86?

Those arches do not include asm-generic.h (I suppose for a good reason)
but a definition is needed anyway if we want code using ioremap_nopost()
to be unconditional. This is one way of doing it, there are others not
sure they are any better, I am open to suggestions.

Thanks,
Lorenzo

> >  extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
> >  extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
> > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > index 7ef015e..0e81938 100644
> > --- a/include/asm-generic/io.h
> > +++ b/include/asm-generic/io.h
> > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> >  #endif /* CONFIG_GENERIC_IOMAP */
> >  #endif /* CONFIG_HAS_IOPORT_MAP */
> >  
> > +#ifndef ioremap_nopost
> > +#define ioremap_nopost ioremap_nocache
> > +#endif
> > +
> >  #ifndef xlate_dev_kmem_ptr
> >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> >  static inline void *xlate_dev_kmem_ptr(void *addr)
> > -- 
> > 2.10.0
> >
Bjorn Helgaas March 30, 2017, 4:47 p.m. UTC | #3
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:

> > >  #define ioremap_uc ioremap_uc
> > > +#define ioremap_nopost ioremap_nocache
> > 
> > These are all the same as the default from asm-generic.h.  Do we really
> > need these definitions in alpha, avr32, frv, ia64, x86?
> 
> Those arches do not include asm-generic.h (I suppose for a good reason)

OK, that explains it.  I'm not at all sure there's a good reason for
those arches not using asm-generic.h, but I haven't looked at the code
to see.

> but a definition is needed anyway if we want code using ioremap_nopost()
> to be unconditional. This is one way of doing it, there are others not
> sure they are any better, I am open to suggestions.
Russell King (Oracle) April 5, 2017, 10:58 a.m. UTC | #4
On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > index 7afb0e2..50b292f 100644
> > > --- a/arch/x86/include/asm/io.h
> > > +++ b/arch/x86/include/asm/io.h
> > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > >  #define ioremap_uc ioremap_uc
> > > +#define ioremap_nopost ioremap_nocache
> > 
> > These are all the same as the default from asm-generic.h.  Do we really
> > need these definitions in alpha, avr32, frv, ia64, x86?
> 
> Those arches do not include asm-generic.h (I suppose for a good reason)
> but a definition is needed anyway if we want code using ioremap_nopost()
> to be unconditional. This is one way of doing it, there are others not
> sure they are any better, I am open to suggestions.

We do have linux/io.h, which should be included in preference to asm/io.h.
linux/io.h has existed for years, but I still see (from time to time)
patches adding drivers that (imho incorrectly) use asm/io.h.

Also, this:

> > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > index 7ef015e..0e81938 100644
> > > --- a/include/asm-generic/io.h
> > > +++ b/include/asm-generic/io.h
> > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > >  #endif /* CONFIG_GENERIC_IOMAP */
> > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > >  
> > > +#ifndef ioremap_nopost
> > > +#define ioremap_nopost ioremap_nocache
> > > +#endif
> > > +
> > >  #ifndef xlate_dev_kmem_ptr
> > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > >  static inline void *xlate_dev_kmem_ptr(void *addr)

could well be located in linux/io.h, which would make it available
everywhere.

I'd suggest one change to this though:

#ifndef ioremap_nopost
static inline void __iomem *ioremap_nopost(resource_size_t offset,
					   unsigned long size)
{
	return ioremap_nocache(offset, size);
}
#endif

This way, if someone forgets to define ioremap_nopost() in their
asm/io.h but provides a definition or extern prototype for
ioremap_nopost(), we end up with a compile error to highlight the
error, rather than the error being hidden by the preprocessor.
Lorenzo Pieralisi April 5, 2017, 12:38 p.m. UTC | #5
On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > index 7afb0e2..50b292f 100644
> > > > --- a/arch/x86/include/asm/io.h
> > > > +++ b/arch/x86/include/asm/io.h
> > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > >  #define ioremap_uc ioremap_uc
> > > > +#define ioremap_nopost ioremap_nocache
> > > 
> > > These are all the same as the default from asm-generic.h.  Do we really
> > > need these definitions in alpha, avr32, frv, ia64, x86?
> > 
> > Those arches do not include asm-generic.h (I suppose for a good reason)
> > but a definition is needed anyway if we want code using ioremap_nopost()
> > to be unconditional. This is one way of doing it, there are others not
> > sure they are any better, I am open to suggestions.
> 
> We do have linux/io.h, which should be included in preference to asm/io.h.
> linux/io.h has existed for years, but I still see (from time to time)
> patches adding drivers that (imho incorrectly) use asm/io.h.
> 
> Also, this:
> 
> > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > index 7ef015e..0e81938 100644
> > > > --- a/include/asm-generic/io.h
> > > > +++ b/include/asm-generic/io.h
> > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > >  
> > > > +#ifndef ioremap_nopost
> > > > +#define ioremap_nopost ioremap_nocache
> > > > +#endif
> > > > +
> > > >  #ifndef xlate_dev_kmem_ptr
> > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> 
> could well be located in linux/io.h, which would make it available
> everywhere.
> 
> I'd suggest one change to this though:
> 
> #ifndef ioremap_nopost
> static inline void __iomem *ioremap_nopost(resource_size_t offset,
> 					   unsigned long size)
> {
> 	return ioremap_nocache(offset, size);
> }
> #endif
> 
> This way, if someone forgets to define ioremap_nopost() in their
> asm/io.h but provides a definition or extern prototype for
> ioremap_nopost(), we end up with a compile error to highlight the
> error, rather than the error being hidden by the preprocessor.

Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
linux/io.h, for whatever reason, does not seem to contain ioremap_*
prototypes; this does not mean we should not add it there but that
would look odd (with all others ioremap_* in asm/io.h), all I am
saying. This stuff requires some clean-up regardless, for the records.

As for the static inline, I did that and that did not make the
kbuild robot happy at all on some arches:

eg: openrisc

>> include/asm-generic/io.h:922:9: error: implicit declaration of
>> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
     return ioremap_nocache(offset, size);

I will have another stab at it since what you put forward makes sense,
I just have to find a way that works across arches.

Thanks !
Lorenzo
Lorenzo Pieralisi April 6, 2017, 10:26 a.m. UTC | #6
On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > > index 7afb0e2..50b292f 100644
> > > > > --- a/arch/x86/include/asm/io.h
> > > > > +++ b/arch/x86/include/asm/io.h
> > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > > >  #define ioremap_uc ioremap_uc
> > > > > +#define ioremap_nopost ioremap_nocache
> > > > 
> > > > These are all the same as the default from asm-generic.h.  Do we really
> > > > need these definitions in alpha, avr32, frv, ia64, x86?
> > > 
> > > Those arches do not include asm-generic.h (I suppose for a good reason)
> > > but a definition is needed anyway if we want code using ioremap_nopost()
> > > to be unconditional. This is one way of doing it, there are others not
> > > sure they are any better, I am open to suggestions.
> > 
> > We do have linux/io.h, which should be included in preference to asm/io.h.
> > linux/io.h has existed for years, but I still see (from time to time)
> > patches adding drivers that (imho incorrectly) use asm/io.h.
> > 
> > Also, this:
> > 
> > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > > index 7ef015e..0e81938 100644
> > > > > --- a/include/asm-generic/io.h
> > > > > +++ b/include/asm-generic/io.h
> > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > > >  
> > > > > +#ifndef ioremap_nopost
> > > > > +#define ioremap_nopost ioremap_nocache
> > > > > +#endif
> > > > > +
> > > > >  #ifndef xlate_dev_kmem_ptr
> > > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> > 
> > could well be located in linux/io.h, which would make it available
> > everywhere.
> > 
> > I'd suggest one change to this though:
> > 
> > #ifndef ioremap_nopost
> > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > 					   unsigned long size)
> > {
> > 	return ioremap_nocache(offset, size);
> > }
> > #endif
> > 
> > This way, if someone forgets to define ioremap_nopost() in their
> > asm/io.h but provides a definition or extern prototype for
> > ioremap_nopost(), we end up with a compile error to highlight the
> > error, rather than the error being hidden by the preprocessor.
> 
> Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
> linux/io.h, for whatever reason, does not seem to contain ioremap_*
> prototypes; this does not mean we should not add it there but that
> would look odd (with all others ioremap_* in asm/io.h), all I am
> saying. This stuff requires some clean-up regardless, for the records.
> 
> As for the static inline, I did that and that did not make the
> kbuild robot happy at all on some arches:
> 
> eg: openrisc
> 
> >> include/asm-generic/io.h:922:9: error: implicit declaration of
> >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
>      return ioremap_nocache(offset, size);

Indeed, the static inline ioremap_nocache() fallback does not work
on all arches (whether I add the fallback in linux/io.h or
asm-generic/io.h is irrelevant), I bump into issues such as the one
reported above.

I could make it:

#ifndef ioremap_nopost
static inline void __iomem *ioremap_nopost(resource_size_t offset,
					   unsigned long size)
{
	return NULL;
}
#endif

which would force arches to define ioremap_nopost() (if they need it).

This is what I *assume* was done for ioremap_uc() in asm-generic/io.h,
but honestly history is hard to follow here.

Thoughts ?

Thanks,
Lorenzo
Russell King (Oracle) April 6, 2017, 10:47 a.m. UTC | #7
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> > eg: openrisc
> > 
> > >> include/asm-generic/io.h:922:9: error: implicit declaration of
> > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
> >      return ioremap_nocache(offset, size);
> 
> Indeed, the static inline ioremap_nocache() fallback does not work
> on all arches (whether I add the fallback in linux/io.h or
> asm-generic/io.h is irrelevant), I bump into issues such as the one
> reported above.

From what I can see:

(a) openrisc does define ioremap_nocache() in its asm/io.h
(b) these do not:

$ grep -L 'ioremap_nocache' arch/*/include/asm/io.h
arch/blackfin/include/asm/io.h
arch/h8300/include/asm/io.h
arch/m68k/include/asm/io.h
arch/score/include/asm/io.h
arch/sparc/include/asm/io.h

Out of those, blackfin, h8300 and score do not define it, whereas m68k
and sparc do in other headers included by asm/io.h.  So it looks like
we have three problem architectures that don't define an ioremap_nocache().

PCI on blackfin depends on BROKEN, so it's not selectable.  From what I
can tell, h8300 and score do not allow PCI to be enabled (but maybe its
buried elsewhere in their Kconfig files, I didn't check.)

So, I think a way around this is to make ioremap_nopost() conditional
on PCI in linux/io.h for the time being - if its scope wants to be
enlarged, then these three architectures would need to have either an
ioremap_nopost() implementation added, or an ioremap_nocache()
implementation.

> I could make it:
> 
> #ifndef ioremap_nopost
> static inline void __iomem *ioremap_nopost(resource_size_t offset,
> 					   unsigned long size)
> {
> 	return NULL;
> }
> #endif
> 
> which would force arches to define ioremap_nopost() (if they need it).
> 
> This is what I *assume* was done for ioremap_uc() in asm-generic/io.h,
> but honestly history is hard to follow here.

If we are going to consider doing that, the correct question we need to be
asking is whether anything will break as a result of this - is there any
existing arch using the code as it stands that will end up being broken
when we switch PCI to use ioremap_nopost(), and we end up using this NULL-
returning default?
Luis Chamberlain April 6, 2017, 10:53 a.m. UTC | #8
On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > > > index 7afb0e2..50b292f 100644
> > > > > > --- a/arch/x86/include/asm/io.h
> > > > > > +++ b/arch/x86/include/asm/io.h
> > > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > > > >  #define ioremap_uc ioremap_uc
> > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > 
> > > > > These are all the same as the default from asm-generic.h.  Do we really
> > > > > need these definitions in alpha, avr32, frv, ia64, x86?
> > > > 
> > > > Those arches do not include asm-generic.h (I suppose for a good reason)
> > > > but a definition is needed anyway if we want code using ioremap_nopost()
> > > > to be unconditional. This is one way of doing it, there are others not
> > > > sure they are any better, I am open to suggestions.
> > > 
> > > We do have linux/io.h, which should be included in preference to asm/io.h.
> > > linux/io.h has existed for years, but I still see (from time to time)
> > > patches adding drivers that (imho incorrectly) use asm/io.h.
> > > 
> > > Also, this:
> > > 
> > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > > > index 7ef015e..0e81938 100644
> > > > > > --- a/include/asm-generic/io.h
> > > > > > +++ b/include/asm-generic/io.h
> > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > > > >  
> > > > > > +#ifndef ioremap_nopost
> > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > > +#endif
> > > > > > +
> > > > > >  #ifndef xlate_dev_kmem_ptr
> > > > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> > > 
> > > could well be located in linux/io.h, which would make it available
> > > everywhere.
> > > 
> > > I'd suggest one change to this though:
> > > 
> > > #ifndef ioremap_nopost
> > > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > > 					   unsigned long size)
> > > {
> > > 	return ioremap_nocache(offset, size);
> > > }
> > > #endif
> > > 
> > > This way, if someone forgets to define ioremap_nopost() in their
> > > asm/io.h but provides a definition or extern prototype for
> > > ioremap_nopost(), we end up with a compile error to highlight the
> > > error, rather than the error being hidden by the preprocessor.
> > 
> > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
> > linux/io.h, for whatever reason, does not seem to contain ioremap_*
> > prototypes; this does not mean we should not add it there but that
> > would look odd (with all others ioremap_* in asm/io.h), all I am
> > saying. This stuff requires some clean-up regardless, for the records.
> > 
> > As for the static inline, I did that and that did not make the
> > kbuild robot happy at all on some arches:
> > 
> > eg: openrisc
> > 
> > >> include/asm-generic/io.h:922:9: error: implicit declaration of
> > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
> >      return ioremap_nocache(offset, size);
> 
> Indeed, the static inline ioremap_nocache() fallback does not work
> on all arches (whether I add the fallback in linux/io.h or
> asm-generic/io.h is irrelevant), I bump into issues such as the one
> reported above.

Its also not *safe* to assume on behalf of all architectures a new ioremap
call is both a good idea and proper.

> I could make it:
> 
> #ifndef ioremap_nopost
> static inline void __iomem *ioremap_nopost(resource_size_t offset,
> 					   unsigned long size)
> {
> 	return NULL;
> }
> #endif
> 
> which would force arches to define ioremap_nopost() (if they need it).

Yes, please do this.

> This is what I *assume* was done for ioremap_uc() in asm-generic/io.h,
> but honestly history is hard to follow here.
> 
> Thoughts ?

Hard to follow ?

I think commit 8c7ea50c010b2 ("x86/mm, asm-generic: Add IOMMU ioremap_uc()
variant default") makes it very clear that historically we have made bad
assumptions and these assumptions are not safe, so the only correct thing we
can do to not stall development is to do what you did above, and each
architecture then can add support for its proper mapping. Its up to each
architecture maintainer to be attentive and review these patches, if they don't
get to it, this will just not work for the new driver that needs it, such is
life, its better than having incorrect defaults spread for all architectures.

The old strategy was very sloppy. Its why I tried to be as clear as possible on
both the commit log and headers about this. If the commit log and headers were
not clear, please help clarify this more somehow in your patches.

  Luis
Lorenzo Pieralisi April 6, 2017, 11:38 a.m. UTC | #9
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote:
> On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > > > > index 7afb0e2..50b292f 100644
> > > > > > > --- a/arch/x86/include/asm/io.h
> > > > > > > +++ b/arch/x86/include/asm/io.h
> > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > > > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > > > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > > > > >  #define ioremap_uc ioremap_uc
> > > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > > 
> > > > > > These are all the same as the default from asm-generic.h.  Do we really
> > > > > > need these definitions in alpha, avr32, frv, ia64, x86?
> > > > > 
> > > > > Those arches do not include asm-generic.h (I suppose for a good reason)
> > > > > but a definition is needed anyway if we want code using ioremap_nopost()
> > > > > to be unconditional. This is one way of doing it, there are others not
> > > > > sure they are any better, I am open to suggestions.
> > > > 
> > > > We do have linux/io.h, which should be included in preference to asm/io.h.
> > > > linux/io.h has existed for years, but I still see (from time to time)
> > > > patches adding drivers that (imho incorrectly) use asm/io.h.
> > > > 
> > > > Also, this:
> > > > 
> > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > > > > index 7ef015e..0e81938 100644
> > > > > > > --- a/include/asm-generic/io.h
> > > > > > > +++ b/include/asm-generic/io.h
> > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > > > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > > > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > > > > >  
> > > > > > > +#ifndef ioremap_nopost
> > > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > > > +#endif
> > > > > > > +
> > > > > > >  #ifndef xlate_dev_kmem_ptr
> > > > > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > > > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> > > > 
> > > > could well be located in linux/io.h, which would make it available
> > > > everywhere.
> > > > 
> > > > I'd suggest one change to this though:
> > > > 
> > > > #ifndef ioremap_nopost
> > > > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > > > 					   unsigned long size)
> > > > {
> > > > 	return ioremap_nocache(offset, size);
> > > > }
> > > > #endif
> > > > 
> > > > This way, if someone forgets to define ioremap_nopost() in their
> > > > asm/io.h but provides a definition or extern prototype for
> > > > ioremap_nopost(), we end up with a compile error to highlight the
> > > > error, rather than the error being hidden by the preprocessor.
> > > 
> > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
> > > linux/io.h, for whatever reason, does not seem to contain ioremap_*
> > > prototypes; this does not mean we should not add it there but that
> > > would look odd (with all others ioremap_* in asm/io.h), all I am
> > > saying. This stuff requires some clean-up regardless, for the records.
> > > 
> > > As for the static inline, I did that and that did not make the
> > > kbuild robot happy at all on some arches:
> > > 
> > > eg: openrisc
> > > 
> > > >> include/asm-generic/io.h:922:9: error: implicit declaration of
> > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
> > >      return ioremap_nocache(offset, size);
> > 
> > Indeed, the static inline ioremap_nocache() fallback does not work
> > on all arches (whether I add the fallback in linux/io.h or
> > asm-generic/io.h is irrelevant), I bump into issues such as the one
> > reported above.
> 
> Its also not *safe* to assume on behalf of all architectures a new ioremap
> call is both a good idea and proper.
> 
> > I could make it:
> > 
> > #ifndef ioremap_nopost
> > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > 					   unsigned long size)
> > {
> > 	return NULL;
> > }
> > #endif
> > 
> > which would force arches to define ioremap_nopost() (if they need it).
> 
> Yes, please do this.

Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell,
having it in linux/io.h is a bit odd given that it would be the only
ioremap_* implementation declared there, I think we need more
consistency here instead of deviating again from what you did.

> > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h,
> > but honestly history is hard to follow here.
> > 
> > Thoughts ?
> 
> Hard to follow ?

I was referring to the whole asm-generic/io.h history.

> I think commit 8c7ea50c010b2 ("x86/mm, asm-generic: Add IOMMU ioremap_uc()
> variant default") makes it very clear that historically we have made bad
> assumptions and these assumptions are not safe, so the only correct thing we
> can do to not stall development is to do what you did above, and each
> architecture then can add support for its proper mapping. Its up to each
> architecture maintainer to be attentive and review these patches, if they don't
> get to it, this will just not work for the new driver that needs it, such is
> life, its better than having incorrect defaults spread for all architectures.
> 
> The old strategy was very sloppy. Its why I tried to be as clear as possible on
> both the commit log and headers about this. If the commit log and headers were
> not clear, please help clarify this more somehow in your patches.

I see your point and I can replicate (probably I will have to patch
microblaze too since some of the PCI drivers patches in this series
that I updated to use ioremap_nopost() will run on it too) what you
did for ioremap_uc() for ioremap_nopost(), I am ok with that, I need
to know if we all are.

Thanks,
Lorenzo
Luis Chamberlain April 6, 2017, 11:59 a.m. UTC | #10
On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> > > > On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> > > > > On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > > > > > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > > > > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > > > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > > > > > index 7afb0e2..50b292f 100644
> > > > > > > > --- a/arch/x86/include/asm/io.h
> > > > > > > > +++ b/arch/x86/include/asm/io.h
> > > > > > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > > > > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > > > > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > > > > > >  #define ioremap_uc ioremap_uc
> > > > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > > > 
> > > > > > > These are all the same as the default from asm-generic.h.  Do we really
> > > > > > > need these definitions in alpha, avr32, frv, ia64, x86?
> > > > > > 
> > > > > > Those arches do not include asm-generic.h (I suppose for a good reason)
> > > > > > but a definition is needed anyway if we want code using ioremap_nopost()
> > > > > > to be unconditional. This is one way of doing it, there are others not
> > > > > > sure they are any better, I am open to suggestions.
> > > > > 
> > > > > We do have linux/io.h, which should be included in preference to asm/io.h.
> > > > > linux/io.h has existed for years, but I still see (from time to time)
> > > > > patches adding drivers that (imho incorrectly) use asm/io.h.
> > > > > 
> > > > > Also, this:
> > > > > 
> > > > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > > > > > index 7ef015e..0e81938 100644
> > > > > > > > --- a/include/asm-generic/io.h
> > > > > > > > +++ b/include/asm-generic/io.h
> > > > > > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > > > > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > > > > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > > > > > >  
> > > > > > > > +#ifndef ioremap_nopost
> > > > > > > > +#define ioremap_nopost ioremap_nocache
> > > > > > > > +#endif
> > > > > > > > +
> > > > > > > >  #ifndef xlate_dev_kmem_ptr
> > > > > > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > > > > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> > > > > 
> > > > > could well be located in linux/io.h, which would make it available
> > > > > everywhere.
> > > > > 
> > > > > I'd suggest one change to this though:
> > > > > 
> > > > > #ifndef ioremap_nopost
> > > > > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > > > > 					   unsigned long size)
> > > > > {
> > > > > 	return ioremap_nocache(offset, size);
> > > > > }
> > > > > #endif
> > > > > 
> > > > > This way, if someone forgets to define ioremap_nopost() in their
> > > > > asm/io.h but provides a definition or extern prototype for
> > > > > ioremap_nopost(), we end up with a compile error to highlight the
> > > > > error, rather than the error being hidden by the preprocessor.
> > > > 
> > > > Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
> > > > linux/io.h, for whatever reason, does not seem to contain ioremap_*
> > > > prototypes; this does not mean we should not add it there but that
> > > > would look odd (with all others ioremap_* in asm/io.h), all I am
> > > > saying. This stuff requires some clean-up regardless, for the records.
> > > > 
> > > > As for the static inline, I did that and that did not make the
> > > > kbuild robot happy at all on some arches:
> > > > 
> > > > eg: openrisc
> > > > 
> > > > >> include/asm-generic/io.h:922:9: error: implicit declaration of
> > > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
> > > >      return ioremap_nocache(offset, size);
> > > 
> > > Indeed, the static inline ioremap_nocache() fallback does not work
> > > on all arches (whether I add the fallback in linux/io.h or
> > > asm-generic/io.h is irrelevant), I bump into issues such as the one
> > > reported above.
> > 
> > Its also not *safe* to assume on behalf of all architectures a new ioremap
> > call is both a good idea and proper.
> > 
> > > I could make it:
> > > 
> > > #ifndef ioremap_nopost
> > > static inline void __iomem *ioremap_nopost(resource_size_t offset,
> > > 					   unsigned long size)
> > > {
> > > 	return NULL;
> > > }
> > > #endif
> > > 
> > > which would force arches to define ioremap_nopost() (if they need it).
> > 
> > Yes, please do this.
> 
> Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell,
> having it in linux/io.h is a bit odd given that it would be the only
> ioremap_* implementation declared there, I think we need more
> consistency here instead of deviating again from what you did.

asm-generic/io.h is the right place for generics which let you override.

> > > This is what I *assume* was done for ioremap_uc() in asm-generic/io.h,
> > > but honestly history is hard to follow here.
> > > 
> > > Thoughts ?
> > 
> > Hard to follow ?
> 
> I was referring to the whole asm-generic/io.h history.
> 
> > I think commit 8c7ea50c010b2 ("x86/mm, asm-generic: Add IOMMU ioremap_uc()
> > variant default") makes it very clear that historically we have made bad
> > assumptions and these assumptions are not safe, so the only correct thing we
> > can do to not stall development is to do what you did above, and each
> > architecture then can add support for its proper mapping. Its up to each
> > architecture maintainer to be attentive and review these patches, if they don't
> > get to it, this will just not work for the new driver that needs it, such is
> > life, its better than having incorrect defaults spread for all architectures.
> > 
> > The old strategy was very sloppy. Its why I tried to be as clear as possible on
> > both the commit log and headers about this. If the commit log and headers were
> > not clear, please help clarify this more somehow in your patches.
> 
> I see your point and I can replicate (probably I will have to patch
> microblaze too since some of the PCI drivers patches in this series
> that I updated to use ioremap_nopost() will run on it too) 

Cool right so if you add support for the archs for the drivers that you know
use this new ioremap then there is no regressions, and then only if a new
driver gets added and an arch needs this they will also need this, but that
is precisely the sort of requirement thought process we want.

> what you
> did for ioremap_uc() for ioremap_nopost(), I am ok with that, I need
> to know if we all are.

Sure. And again, if consensus is built (again) on that strategy please
update the docs to ensure that this is even clearer for the next person.
I thought it was rather clear now but it does not seem so.

  Luis
Russell King (Oracle) April 6, 2017, 12:11 p.m. UTC | #11
On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote:
> On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> > Indeed, the static inline ioremap_nocache() fallback does not work
> > on all arches (whether I add the fallback in linux/io.h or
> > asm-generic/io.h is irrelevant), I bump into issues such as the one
> > reported above.
> 
> Its also not *safe* to assume on behalf of all architectures a new ioremap
> call is both a good idea and proper.

You may be right in general, but not in this case.

Currently, many drivers use plain ioremap() to map this resource.  We
are replacing that existing call - which is known to work in the majority
of cases - with a new call to cater for different semantics required by
an architecture.

Doing a replace of these ioremap() calls with ioremap_nopost() in this
situation, and then having ioremap_nopost() fail is a recipe for causing
lots and lots of regressions.

The only sane approach is to have ioremap_post() default to modelling the
_existing_ behaviour everywhere that it is used.

Requiring it to fail until architecture folk trip over the failure is
totally insane, and I very strongly disagree with you on this.
Luis Chamberlain April 6, 2017, 12:25 p.m. UTC | #12
On Thu, Apr 06, 2017 at 01:11:57PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 06, 2017 at 12:53:12PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> > > Indeed, the static inline ioremap_nocache() fallback does not work
> > > on all arches (whether I add the fallback in linux/io.h or
> > > asm-generic/io.h is irrelevant), I bump into issues such as the one
> > > reported above.
> > 
> > Its also not *safe* to assume on behalf of all architectures a new ioremap
> > call is both a good idea and proper.
> 
> You may be right in general, but not in this case.
> 
> Currently, many drivers use plain ioremap() to map this resource.  We
> are replacing that existing call - which is known to work in the majority
> of cases - with a new call to cater for different semantics required by
> an architecture.
> 
> Doing a replace of these ioremap() calls with ioremap_nopost() in this
> situation, and then having ioremap_nopost() fail is a recipe for causing
> lots and lots of regressions.
> 
> The only sane approach is to have ioremap_post() default to modelling the
> _existing_ behaviour everywhere that it is used.
> 
> Requiring it to fail until architecture folk trip over the failure is
> totally insane, and I very strongly disagree with you on this.

Ah yes, what if with this modulo rule of thumb:

The litmus test then is if an existing set of calls are changed to
use a new ioremap then all archs that support those drivers where the new
call is being added must be modified to also have a correct corresponding
API call ?

This is more work on the new person introducing the new API, and should require
review still on arch maintainers but it seems like a fair compromise.

Then if an API is *new* though then things can move forward without requiring
all archs to add the respective call.

  Luis
Russell King (Oracle) April 6, 2017, 1:07 p.m. UTC | #13
On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote:
> On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote:
> > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell,
> > having it in linux/io.h is a bit odd given that it would be the only
> > ioremap_* implementation declared there, I think we need more
> > consistency here instead of deviating again from what you did.
> 
> asm-generic/io.h is the right place for generics which let you override.

I disagree for two reasons, and I will refer you to Linus' comments back
in 2005 at http://yarchive.net/comp/linux/generic.html

1) asm-generic/io.h has become an ifdef mess, every single definition in
   there is conditional.  The reason for this has happened is that
   architectures can't pick and choose what they want from a single file
   unless something like that is done.  It would be _far_ better to
   split this file up by functional group - eg, ISA IO accessors,
   io(read|write)*(), read|write*(), and so forth.

2) We're at the point where we have various .c files around the kernel
   _directly_ including asm-generic header files, which means the
   use of asm-generic headers is no longer a choice of the architecture.

3) asm-generic/ started out life as the place where example
   implementations of asm/*.h header files were found, and but has
   grown into a place where we dump default implementations.  We had
   a place for default implementations for years, which were the
   linux/*.h headers.  We have now ended up with a mixture of both
   techniques, even for something like io.h, we have the messy
   asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h.
   We have ended up with both linux/io.h and asm-generic/io.h containing
   default definitions.

We've had the rule that where both linux/foo.h and asm/foo.h exist, the
linux/ counterpart is the preferred include file.  That didn't really
happen with asm/io.h due to the number of users that there were, but
when new stuff was added to asm/io.h which we wanted to be generic,
linux/io.h was created to contain that.  This actually pre-dates the
"let's clutter up asm-generic with shared arch stuff" push.

Now, what you say _may_ make sense if we have an
asm-generic/ioremap-nopost.h header which just defines a default
ioremap_nopost.h implementation, and architectures would then be able to
choose whether to include that or not depending on whether they provide
their own implementation.  No ugly ifdefs are necessary with that
approach.

Out of the three choices, I would much rather see the
asm-generic/ioremap-nopost.h approach.  However, the down-side to that
is it means all architectures asm/io.h would need to be touched to
explicitly include that.

What I'm absolutely certain of, though, is that making asm-generic/io.h
even worse by adding yet more conditional stuff to it is not a sane way
forward.
Lorenzo Pieralisi April 6, 2017, 4:21 p.m. UTC | #14
On Thu, Apr 06, 2017 at 02:07:27PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote:
> > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell,
> > > having it in linux/io.h is a bit odd given that it would be the only
> > > ioremap_* implementation declared there, I think we need more
> > > consistency here instead of deviating again from what you did.
> > 
> > asm-generic/io.h is the right place for generics which let you override.
> 
> I disagree for two reasons, and I will refer you to Linus' comments back
> in 2005 at http://yarchive.net/comp/linux/generic.html
> 
> 1) asm-generic/io.h has become an ifdef mess, every single definition in
>    there is conditional.  The reason for this has happened is that
>    architectures can't pick and choose what they want from a single file
>    unless something like that is done.  It would be _far_ better to
>    split this file up by functional group - eg, ISA IO accessors,
>    io(read|write)*(), read|write*(), and so forth.
> 
> 2) We're at the point where we have various .c files around the kernel
>    _directly_ including asm-generic header files, which means the
>    use of asm-generic headers is no longer a choice of the architecture.
> 
> 3) asm-generic/ started out life as the place where example
>    implementations of asm/*.h header files were found, and but has
>    grown into a place where we dump default implementations.  We had
>    a place for default implementations for years, which were the
>    linux/*.h headers.  We have now ended up with a mixture of both
>    techniques, even for something like io.h, we have the messy
>    asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h.
>    We have ended up with both linux/io.h and asm-generic/io.h containing
>    default definitions.
> 
> We've had the rule that where both linux/foo.h and asm/foo.h exist, the
> linux/ counterpart is the preferred include file.  That didn't really
> happen with asm/io.h due to the number of users that there were, but
> when new stuff was added to asm/io.h which we wanted to be generic,
> linux/io.h was created to contain that.  This actually pre-dates the
> "let's clutter up asm-generic with shared arch stuff" push.
> 
> Now, what you say _may_ make sense if we have an
> asm-generic/ioremap-nopost.h header which just defines a default
> ioremap_nopost.h implementation, and architectures would then be able to
> choose whether to include that or not depending on whether they provide
> their own implementation.  No ugly ifdefs are necessary with that
> approach.
> 
> Out of the three choices, I would much rather see the
> asm-generic/ioremap-nopost.h approach.  However, the down-side to that
> is it means all architectures asm/io.h would need to be touched to
> explicitly include that.
> 
> What I'm absolutely certain of, though, is that making asm-generic/io.h
> even worse by adding yet more conditional stuff to it is not a sane way
> forward.

Ok, so:

(1) I can do asm-generic/ioremap-nopost.h, which I assume you want to
    contain something like

static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
{
	return ioremap_nocache(offset, size);
}

Funny bit is that it has to be included by asm*/io.h files _after_
ioremap_nocache has been #defined (that's the reason my approach was
failing miserably even on arches like eg powerpc (see [1] below) that
does have ioremap_nocache), not sure it is going to be very nice to have
an include in the middle of asm*/io.h include files (and I have to patch
all arches which I can do).

Or

(2) I add:

#ifndef ioremap_nopost
static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
{
	return NULL; <-(making it return ioremap_nocache() does not
			work, see [1] for the reason)
}
#endif

in linux/io.h

(3) ioremap_nopost follows Luis' ioremap_uc approach

(1)-(2) bypass completely what Luis did for ioremap_uc(), which implies
that we have yet another way of implementing ioremap_*.

I would like to get this patchset done so if you have an opinion it
is time to state it.

Thanks,
Lorenzo

[1] powerpc build report:

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/config-io-mappings-fix-v3
head:   283a324b549a662346c95c05b08983dd5b83354b
commit: e66b493fe93226c02b1a33355f79db7ed6efe718 [2/23] linux/io.h: add ioremap_nopost remap interface
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout e66b493fe93226c02b1a33355f79db7ed6efe718
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/io.h:28:0,
                    from arch/powerpc/oprofile/op_model_cell.c:29:
   include/linux/io.h: In function 'ioremap_nopost':
   include/linux/io.h:169:9: error: implicit declaration of function 'ioremap_nocache' [-Werror=implicit-function-declaration]
     return ioremap_nocache(offset, size);
            ^~~~~~~~~~~~~~~
>> include/linux/io.h:169:9: error: return makes pointer from integer without a cast [-Werror=int-conversion]
     return ioremap_nocache(offset, size);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +169 include/linux/io.h

   163  }
   164  #endif
   165 
   166  #ifndef ioremap_nopost
   167  static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
   168  {
 > 169           return ioremap_nocache(offset, size);
   170  }
   171  #endif
   172
Russell King (Oracle) April 6, 2017, 4:40 p.m. UTC | #15
On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote:
> Ok, so:
> 
> (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to
>     contain something like
> 
> static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
> {
> 	return ioremap_nocache(offset, size);
> }
> 
> Funny bit is that it has to be included by asm*/io.h files _after_
> ioremap_nocache has been #defined (that's the reason my approach was
> failing miserably even on arches like eg powerpc (see [1] below) that
> does have ioremap_nocache),

PowerPC does have ioremap_nocache() though:

/**
 * ioremap     -   map bus memory into CPU space
...
 * * ioremap_nocache is identical to ioremap
extern void __iomem *ioremap(phys_addr_t address, unsigned long size);
#define ioremap_nocache(addr, size)     ioremap((addr), (size))

and this include file is included very early on in linux/io.h.  I don't
see anything that conditionalises it on anything except __KERNEL__.  So,
the report from 0-day really doesn't make any sense to me.

Do we know how we're ending up in linux/io.h line 169 without having
picked up the ioremap_nocache() definition provided by PowerPC's
asm/io.h ?

> not sure it is going to be very nice to have
> an include in the middle of asm*/io.h include files (and I have to patch
> all arches which I can do).

You mean like we already have to do with this asm-generic/io.h thing in
the ARM io.h header file, because we need to define all the accessors
first, to prevent the asm-generic/io.h thing defining them for us?
Given how asm-generic has headed in this regard, having include files
at all sorts of strange locations within the architecture asm/*.h
header files has become quite normal, unfortunately.

> (2) I add:
> 
> #ifndef ioremap_nopost
> static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
> {
> 	return NULL; <-(making it return ioremap_nocache() does not
> 			work, see [1] for the reason)
> }
> #endif
> 
> in linux/io.h

... which breaks the kernel if ioremap_nopost is missing from the arch
header, and one of the drivers that you're modifying to use this new
ioremap variant happens to be built and used on such an architecture.

> (3) ioremap_nopost follows Luis' ioremap_uc approach

Same problem as (2), as I understand correctly.
Lorenzo Pieralisi April 6, 2017, 5:09 p.m. UTC | #16
On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote:
> > Ok, so:
> > 
> > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to
> >     contain something like
> > 
> > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
> > {
> > 	return ioremap_nocache(offset, size);
> > }
> > 
> > Funny bit is that it has to be included by asm*/io.h files _after_
> > ioremap_nocache has been #defined (that's the reason my approach was
> > failing miserably even on arches like eg powerpc (see [1] below) that
> > does have ioremap_nocache),
> 
> PowerPC does have ioremap_nocache() though:
> 
> /**
>  * ioremap     -   map bus memory into CPU space
> ...
>  * * ioremap_nocache is identical to ioremap
> extern void __iomem *ioremap(phys_addr_t address, unsigned long size);
> #define ioremap_nocache(addr, size)     ioremap((addr), (size))
> 
> and this include file is included very early on in linux/io.h.  I don't
> see anything that conditionalises it on anything except __KERNEL__.  So,
> the report from 0-day really doesn't make any sense to me.
> 
> Do we know how we're ending up in linux/io.h line 169 without having
> picked up the ioremap_nocache() definition provided by PowerPC's
> asm/io.h ?

I will debug it further but I *think* it is because:

eg arch/powerpc/oprofile/op_model_cell.c includes <asm/io.h>

and <asm/io.h> includes <linux/io.h> before ioremap_nocache is defined

> > not sure it is going to be very nice to have
> > an include in the middle of asm*/io.h include files (and I have to patch
> > all arches which I can do).
> 
> You mean like we already have to do with this asm-generic/io.h thing in
> the ARM io.h header file, because we need to define all the accessors
> first, to prevent the asm-generic/io.h thing defining them for us?
> Given how asm-generic has headed in this regard, having include files
> at all sorts of strange locations within the architecture asm/*.h
> header files has become quite normal, unfortunately.

Yes we won't make it any nicer that's for certain, my worry is that
it would end up being even harder to read.

> > (2) I add:
> > 
> > #ifndef ioremap_nopost
> > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
> > {
> > 	return NULL; <-(making it return ioremap_nocache() does not
> > 			work, see [1] for the reason)
> > }
> > #endif
> > 
> > in linux/io.h
> 
> ... which breaks the kernel if ioremap_nopost is missing from the arch
> header, and one of the drivers that you're modifying to use this new
> ioremap variant happens to be built and used on such an architecture.

Yes agree.

> > (3) ioremap_nopost follows Luis' ioremap_uc approach
> 
> Same problem as (2), as I understand correctly.

Agreed. We have to find the lesser evil, that's it.

Thanks !
Lorenzo
Russell King (Oracle) April 6, 2017, 5:19 p.m. UTC | #17
On Thu, Apr 06, 2017 at 06:09:51PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 06, 2017 at 05:40:16PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Apr 06, 2017 at 05:21:56PM +0100, Lorenzo Pieralisi wrote:
> > > Ok, so:
> > > 
> > > (1) I can do asm-generic/ioremap-nopost.h, which I assume you want to
> > >     contain something like
> > > 
> > > static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
> > > {
> > > 	return ioremap_nocache(offset, size);
> > > }
> > > 
> > > Funny bit is that it has to be included by asm*/io.h files _after_
> > > ioremap_nocache has been #defined (that's the reason my approach was
> > > failing miserably even on arches like eg powerpc (see [1] below) that
> > > does have ioremap_nocache),
> > 
> > PowerPC does have ioremap_nocache() though:
> > 
> > /**
> >  * ioremap     -   map bus memory into CPU space
> > ...
> >  * * ioremap_nocache is identical to ioremap
> > extern void __iomem *ioremap(phys_addr_t address, unsigned long size);
> > #define ioremap_nocache(addr, size)     ioremap((addr), (size))
> > 
> > and this include file is included very early on in linux/io.h.  I don't
> > see anything that conditionalises it on anything except __KERNEL__.  So,
> > the report from 0-day really doesn't make any sense to me.
> > 
> > Do we know how we're ending up in linux/io.h line 169 without having
> > picked up the ioremap_nocache() definition provided by PowerPC's
> > asm/io.h ?
> 
> I will debug it further but I *think* it is because:
> 
> eg arch/powerpc/oprofile/op_model_cell.c includes <asm/io.h>
> 
> and <asm/io.h> includes <linux/io.h> before ioremap_nocache is defined

Oh, that's just very wrong.  asm/foo.h should never include linux/foo.h
especially when linux/foo.h already includes asm/foo.h.  I think we
need PowerPC folk to fix this.
Lorenzo Pieralisi April 10, 2017, 2:30 p.m. UTC | #18
On Thu, Apr 06, 2017 at 11:47:37AM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 06, 2017 at 11:26:36AM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 05, 2017 at 01:38:12PM +0100, Lorenzo Pieralisi wrote:
> > > eg: openrisc
> > > 
> > > >> include/asm-generic/io.h:922:9: error: implicit declaration of
> > > >> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
> > >      return ioremap_nocache(offset, size);
> > 
> > Indeed, the static inline ioremap_nocache() fallback does not work
> > on all arches (whether I add the fallback in linux/io.h or
> > asm-generic/io.h is irrelevant), I bump into issues such as the one
> > reported above.
> 
> From what I can see:
> 
> (a) openrisc does define ioremap_nocache() in its asm/io.h
> (b) these do not:
> 
> $ grep -L 'ioremap_nocache' arch/*/include/asm/io.h
> arch/blackfin/include/asm/io.h
> arch/h8300/include/asm/io.h
> arch/m68k/include/asm/io.h
> arch/score/include/asm/io.h
> arch/sparc/include/asm/io.h
> 
> Out of those, blackfin, h8300 and score do not define it, whereas m68k
> and sparc do in other headers included by asm/io.h.  So it looks like
> we have three problem architectures that don't define an ioremap_nocache().
> 
> PCI on blackfin depends on BROKEN, so it's not selectable.  From what I
> can tell, h8300 and score do not allow PCI to be enabled (but maybe its
> buried elsewhere in their Kconfig files, I didn't check.)
> 
> So, I think a way around this is to make ioremap_nopost() conditional
> on PCI in linux/io.h for the time being - if its scope wants to be
> enlarged, then these three architectures would need to have either an
> ioremap_nopost() implementation added, or an ioremap_nocache()
> implementation.

Ok, I implemented asm-generic/ioremap-nopost.h, I do not think we need
a CONFIG_PCI guard (kbuild has not barfed at it, yet), given that
blackfin and h8300 are !MMU and score selects NO_IOMEM.

Patch below is all arches squashed into one commit, I probably have
to split it one per arch which will make this a 50-patch series in
total:

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/commit/?h=pci/config-io-mappings-fix-v3&id=acc0be820c809101e02ab7cb170f802db53af934

If I hear no complaints I will split patch above one per arch and
repost the series shortly (even though I think I should make two series
out of it, one asm-generic/ioremap-nopost.h boilerplate and second
ARM/ARM64 implementation + PCI drivers).

Lorenzo
diff mbox

Patch

diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index ff40491..27379ea 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -300,6 +300,7 @@  static inline void __iomem * ioremap_nocache(unsigned long offset,
 }
 
 #define ioremap_uc ioremap_nocache
+#define ioremap_nopost ioremap_nocache
 
 static inline void iounmap(volatile void __iomem *addr)
 {
diff --git a/arch/avr32/include/asm/io.h b/arch/avr32/include/asm/io.h
index f855646..3f1ced8 100644
--- a/arch/avr32/include/asm/io.h
+++ b/arch/avr32/include/asm/io.h
@@ -298,6 +298,7 @@  extern void __iounmap(void __iomem *addr);
 #define ioremap_wc ioremap_nocache
 #define ioremap_wt ioremap_nocache
 #define ioremap_uc ioremap_nocache
+#define ioremap_nopost ioremap_nocache
 
 #define cached(addr) P1SEGADDR(addr)
 #define uncached(addr) P2SEGADDR(addr)
diff --git a/arch/frv/include/asm/io.h b/arch/frv/include/asm/io.h
index 8062fc7..302fb8c 100644
--- a/arch/frv/include/asm/io.h
+++ b/arch/frv/include/asm/io.h
@@ -290,6 +290,7 @@  static inline void __iomem *ioremap_fullcache(unsigned long physaddr, unsigned l
 
 #define ioremap_wc ioremap_nocache
 #define ioremap_uc ioremap_nocache
+#define ioremap_nopost ioremap_nocache
 
 extern void iounmap(void volatile __iomem *addr);
 
diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
index 5de673a..70a4985 100644
--- a/arch/ia64/include/asm/io.h
+++ b/arch/ia64/include/asm/io.h
@@ -434,6 +434,7 @@  static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned lo
 }
 #define ioremap_cache ioremap_cache
 #define ioremap_uc ioremap_nocache
+#define ioremap_nopost ioremap_nocache
 
 
 /*
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7afb0e2..50b292f 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -171,6 +171,7 @@  static inline unsigned int isa_virt_to_bus(volatile void *address)
 extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
 extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
 #define ioremap_uc ioremap_uc
+#define ioremap_nopost ioremap_nocache
 
 extern void __iomem *ioremap_cache(resource_size_t offset, unsigned long size);
 extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, unsigned long prot_val);
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015e..0e81938 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -915,6 +915,10 @@  extern void ioport_unmap(void __iomem *p);
 #endif /* CONFIG_GENERIC_IOMAP */
 #endif /* CONFIG_HAS_IOPORT_MAP */
 
+#ifndef ioremap_nopost
+#define ioremap_nopost ioremap_nocache
+#endif
+
 #ifndef xlate_dev_kmem_ptr
 #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
 static inline void *xlate_dev_kmem_ptr(void *addr)