| Message ID | 20170411122923.6285-5-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote: > +static inline void __iomem *ioremap_nopost(phys_addr_t offset, > size_t size) > +{ > + return ioremap_nocache(offset, size); > +} > + No this is wrong as I explained. This is a semantic that simply *cannot* be generically provided accross architectures as a mapping attribute. The solution to your problem lies elsewhere. Cheers, Ben.
On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote: > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset, > > size_t size) > > +{ > > + return ioremap_nocache(offset, size); > > +} > > + > > No this is wrong as I explained. > > This is a semantic that simply *cannot* be generically provided accross > architectures as a mapping attribute. I agree that a default implementation does not make much sense. The only solution to this, if we want the ioremap_nopost to be made available to generic code (and drivers - ie DT PCI host bridge drivers on ARM/ARM64 are not arch code), is to make the ioremap_nopost() call return NULL unless overriden by arch code that can provide its semantics. Thanks, Lorenzo
On Tue, 2017-04-11 at 15:31 +0100, Lorenzo Pieralisi wrote: > > This is a semantic that simply *cannot* be generically provided accross > > architectures as a mapping attribute. > > I agree that a default implementation does not make much sense. The > only solution to this, if we want the ioremap_nopost to be made available > to generic code (and drivers - ie DT PCI host bridge drivers on ARM/ARM64 > are not arch code), is to make the ioremap_nopost() call return NULL > unless overriden by arch code that can provide its semantics. That would be a better option. You might be able to implement a fallback, for example by having the config ops do a read back from the bridge. Ben.
On Wed, Apr 12, 2017 at 09:14:07AM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-11 at 15:31 +0100, Lorenzo Pieralisi wrote: > > > This is a semantic that simply *cannot* be generically provided accross > > > architectures as a mapping attribute. > > > > I agree that a default implementation does not make much sense. The > > only solution to this, if we want the ioremap_nopost to be made available > > to generic code (and drivers - ie DT PCI host bridge drivers on ARM/ARM64 > > are not arch code), is to make the ioremap_nopost() call return NULL > > unless overriden by arch code that can provide its semantics. > > That would be a better option. I think that's what I will implement which basically means I will replace the default: static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return ioremap_nocache(offset, size); } with static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) { return NULL; } If an arch can override ioremap_nopost() with sensible mapping attributes (or whatever make it enforceable) it does, if it can't any ioremap_nopost() usage will result in a mapping failure, if you see any other viable solution please shout. Thanks, Lorenzo
On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote: > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset, > > size_t size) > > +{ > > + return ioremap_nocache(offset, size); > > +} > > + > > No this is wrong as I explained. > > This is a semantic that simply *cannot* be generically provided accross > architectures as a mapping attribute. > > The solution to your problem lies elsewhere. I disagree. Sure, it may not be supportable across all architectures, but we're not introducing something brand new here. What we're trying to do is to fix some _existing_ drivers that are already using ioremap() to map the PCI IO and configuration spaces. Maybe it would help if those drivers were part of this patch set, rather than the patch set just introducing a whole new architecture interface without really showing where the problem drivers are. The issue here is that if we make this new ioremap_nopost() fail by returning NULL if an architecture does not provide an implementation, then these drivers are going to start failing on such architectures. It is only safe to do that where we know for certain that the drivers are not used - but I don't think that's the case here (it's difficult to tell, because no drivers are updated in this series, so we don't know which are going to be affected.) So, the question is: - is it better to provide a default implementation which provides the functionality that existing drivers are _already_ using, albiet not entirely correctly. or: - is it better to break drivers on architectures when they're converted to fix up this issue. What, I suppose, we could do is not bother with a default implementation, but instead litter drivers with: +#ifdef ioremap_post + base = ioremap_post(...); +#else base = ioremap(...); +#endif which gets around your objection - not providing a default that's weaker than required, but also not breaking the drivers. The problem is that we end up littering drivers with ifdefs. However, I'm willing to bet that the architectures that you're saying will not be able to support the weaker semantic won't have any need to use these drivers, or if they do, the drivers will need the method of accessing stuff like PCI IO and configuration spaces radically altered. So, maybe the best solution is to not provide any kind of default implementation, add a HAVE_IOREMAP_POST Kconfig symbol, have architectures select that when they do provide ioremap_post(), and make the drivers depend on that (so we don't end up trying to build these drivers on architectures where they can never work.) Down side to that is reduced build coverage.
On Wed, Apr 12, 2017 at 12:20:22PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote: > > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote: > > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset, > > > size_t size) > > > +{ > > > + return ioremap_nocache(offset, size); > > > +} > > > + > > > > No this is wrong as I explained. > > > > This is a semantic that simply *cannot* be generically provided accross > > architectures as a mapping attribute. > > > > The solution to your problem lies elsewhere. > > I disagree. Sure, it may not be supportable across all architectures, > but we're not introducing something brand new here. > > What we're trying to do is to fix some _existing_ drivers that are > already using ioremap() to map the PCI IO and configuration spaces. > Maybe it would help if those drivers were part of this patch set, > rather than the patch set just introducing a whole new architecture > interface without really showing where the problem drivers are. > > The issue here is that if we make this new ioremap_nopost() fail by > returning NULL if an architecture does not provide an implementation, > then these drivers are going to start failing on such architectures. > > It is only safe to do that where we know for certain that the drivers > are not used - but I don't think that's the case here (it's difficult > to tell, because no drivers are updated in this series, so we don't > know which are going to be affected.) > > So, the question is: > > - is it better to provide a default implementation which provides the > functionality that existing drivers are _already_ using, albiet not > entirely correctly. > > or: > > - is it better to break drivers on architectures when they're converted > to fix up this issue. > > What, I suppose, we could do is not bother with a default implementation, > but instead litter drivers with: > > +#ifdef ioremap_post > + base = ioremap_post(...); > +#else > base = ioremap(...); > +#endif > > which gets around your objection - not providing a default that's weaker > than required, but also not breaking the drivers. The problem is that > we end up littering drivers with ifdefs. > > However, I'm willing to bet that the architectures that you're saying > will not be able to support the weaker semantic won't have any need to > use these drivers, or if they do, the drivers will need the method of > accessing stuff like PCI IO and configuration spaces radically altered. > > So, maybe the best solution is to not provide any kind of default > implementation, add a HAVE_IOREMAP_POST Kconfig symbol, have architectures > select that when they do provide ioremap_post(), and make the drivers > depend on that (so we don't end up trying to build these drivers on > architectures where they can never work.) Down side to that is reduced > build coverage. I can do that yes, which already means I have to know if eg microblaze (drivers/pci/host/pcie-xilinx.c) can provide a mapping with nonposted writes semantics, otherwise it is a dead-end. Another option would be going back to what v1 did, namely, to implement a pci_remap_cfgspace() interface (it is the _nopost() suffix that stirred debate - nobody would object to having a default pci_remap_cfgspace() implementation that defaults to ioremap_nocache(), I know Bjorn does not like it to be PCI specific, just adding an option on the table to make progress). Thanks, Lorenzo
On Tue, Apr 18, 2017 at 10:49 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Wed, Apr 12, 2017 at 12:20:22PM +0100, Russell King - ARM Linux wrote: >> On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote: >> > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote: >> > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset, >> > > size_t size) >> > > +{ >> > > + return ioremap_nocache(offset, size); >> > > +} >> > > + >> > >> > No this is wrong as I explained. >> > >> > This is a semantic that simply *cannot* be generically provided accross >> > architectures as a mapping attribute. >> > >> > The solution to your problem lies elsewhere. >> >> I disagree. Sure, it may not be supportable across all architectures, >> but we're not introducing something brand new here. >> >> What we're trying to do is to fix some _existing_ drivers that are >> already using ioremap() to map the PCI IO and configuration spaces. >> Maybe it would help if those drivers were part of this patch set, >> rather than the patch set just introducing a whole new architecture >> interface without really showing where the problem drivers are. >> >> The issue here is that if we make this new ioremap_nopost() fail by >> returning NULL if an architecture does not provide an implementation, >> then these drivers are going to start failing on such architectures. >> >> It is only safe to do that where we know for certain that the drivers >> are not used - but I don't think that's the case here (it's difficult >> to tell, because no drivers are updated in this series, so we don't >> know which are going to be affected.) >> >> So, the question is: >> >> - is it better to provide a default implementation which provides the >> functionality that existing drivers are _already_ using, albiet not >> entirely correctly. >> >> or: >> >> - is it better to break drivers on architectures when they're converted >> to fix up this issue. >> >> What, I suppose, we could do is not bother with a default implementation, >> but instead litter drivers with: >> >> +#ifdef ioremap_post >> + base = ioremap_post(...); >> +#else >> base = ioremap(...); >> +#endif >> >> which gets around your objection - not providing a default that's weaker >> than required, but also not breaking the drivers. The problem is that >> we end up littering drivers with ifdefs. >> >> However, I'm willing to bet that the architectures that you're saying >> will not be able to support the weaker semantic won't have any need to >> use these drivers, or if they do, the drivers will need the method of >> accessing stuff like PCI IO and configuration spaces radically altered. >> >> So, maybe the best solution is to not provide any kind of default >> implementation, add a HAVE_IOREMAP_POST Kconfig symbol, have architectures >> select that when they do provide ioremap_post(), and make the drivers >> depend on that (so we don't end up trying to build these drivers on >> architectures where they can never work.) Down side to that is reduced >> build coverage. > > I can do that yes, which already means I have to know if eg microblaze > (drivers/pci/host/pcie-xilinx.c) can provide a mapping with nonposted > writes semantics, otherwise it is a dead-end. > > Another option would be going back to what v1 did, namely, to implement > a pci_remap_cfgspace() interface (it is the _nopost() suffix that stirred > debate - nobody would object to having a default pci_remap_cfgspace() > implementation that defaults to ioremap_nocache(), I know Bjorn does not > like it to be PCI specific, just adding an option on the table to make > progress). The reason I objected to pci_remap_cfgspace() was that I thought ioremap_nopost() was implementable across arches. That turned out not to be true, so I'm fine with calling it pci_remap_cfgspace(). Maybe it's worth a note in the default implementation that arches should override it with a non-postable version if they can. Bjorn
On Tue, 2017-04-18 at 16:49 +0100, Lorenzo Pieralisi wrote: > I can do that yes, which already means I have to know if eg microblaze > (drivers/pci/host/pcie-xilinx.c) can provide a mapping with nonposted > writes semantics, otherwise it is a dead-end. > > Another option would be going back to what v1 did, namely, to implement > a pci_remap_cfgspace() interface (it is the _nopost() suffix that stirred > debate - nobody would object to having a default pci_remap_cfgspace() > implementation that defaults to ioremap_nocache(), I know Bjorn does not > like it to be PCI specific, just adding an option on the table to make > progress). Well, it boils down again to the fact that a mapping attribute isn't sufficient. Let's say I'm microblaze and I can't do non-posted mapping. Then the Host Bridge driver needs to *know* that so it can implement a different workaround, such as reading back from some bridge register after every config write which ensures the previous write reached its destination for example, or whatever other IP specific mechanism. Cheers, Ben.
diff --git a/include/asm-generic/ioremap-nopost.h b/include/asm-generic/ioremap-nopost.h new file mode 100644 index 0000000..015911f --- /dev/null +++ b/include/asm-generic/ioremap-nopost.h @@ -0,0 +1,9 @@ +#ifndef __ASM_GENERIC_IOREMAP_NOPOST_H +#define __ASM_GENERIC_IOREMAP_NOPOST_H + +static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size) +{ + return ioremap_nocache(offset, size); +} + +#endif /* __ASM_GENERIC_IOREMAP_NOPOST_H */
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 (and a corresponding generic asm-generic header file) that defaults to ioremap_nocache() (which should provide a sane default behaviour) and can be included by all architectures that do not require an arch specific memory mapping for ioremap_nopost() to guarantee non-posted writes behaviour. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Luis R. Rodriguez <mcgrof@kernel.org> --- include/asm-generic/ioremap-nopost.h | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 include/asm-generic/ioremap-nopost.h