| Message ID | 20170327094954.7162-3-lorenzo.pieralisi@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 >
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 > >
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.
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.
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
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
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?
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
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
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
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.
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
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.
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
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.
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
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.
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 --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)
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(+)