Re: [PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
On Thu, Apr 8, 2021 at 2:01 PM Hector Martin wrote: > On 08/04/2021 06.03, Will Deacon wrote: > >> I would rewrite above as > >> > >> void __iomem *ret; > >> > >> ret = ioremap_np(offset, size); > >> if (ret) > >>return ret; > >> > >> return ioremap(offset, size); > > > > Looks like it might be one of those rare occasions where the GCC ternary if > > extension thingy comes in handy: > > > > return ioremap_np(offset, size) ?: ioremap(offset, size); > > Today I learned that this one is kosher in kernel code. Handy! Let's go > with that. It depends on the maintainer. Greg, for example, doesn't like this. I have no strong opinion (I use both variants on case-by-case basis), though I think in headers better to spell out all conditionals clearly. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
On 08/04/2021 06.03, Will Deacon wrote: I would rewrite above as void __iomem *ret; ret = ioremap_np(offset, size); if (ret) return ret; return ioremap(offset, size); Looks like it might be one of those rare occasions where the GCC ternary if extension thingy comes in handy: return ioremap_np(offset, size) ?: ioremap(offset, size); Today I learned that this one is kosher in kernel code. Handy! Let's go with that. Acked-by: Will Deacon Thanks! -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
Re: [PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
On Wed, Apr 07, 2021 at 04:27:42PM +0300, Andy Shevchenko wrote: > On Fri, Apr 2, 2021 at 12:07 PM Hector Martin wrote: > > > > Now that we have ioremap_np(), we can make pci_remap_cfgspace() default > > to it, falling back to ioremap() on platforms where it is not available. > > > > Remove the arm64 implementation, since that is now redundant. Future > > cleanups should be able to do the same for other arches, and eventually > > make the generic pci_remap_cfgspace() unconditional. > > ... > > > + void __iomem *ret = ioremap_np(offset, size); > > + > > + if (!ret) > > + ret = ioremap(offset, size); > > + > > + return ret; > > Usually negative conditions are worse for cognitive functions of human beings. > (On top of that some patterns are applied) > > I would rewrite above as > > void __iomem *ret; > > ret = ioremap_np(offset, size); > if (ret) > return ret; > > return ioremap(offset, size); Looks like it might be one of those rare occasions where the GCC ternary if extension thingy comes in handy: return ioremap_np(offset, size) ?: ioremap(offset, size); but however it's done, the logic looks good to me and thanks Hector for updating this: Acked-by: Will Deacon Will
Re: [PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
On Fri, Apr 2, 2021 at 12:07 PM Hector Martin wrote: > > Now that we have ioremap_np(), we can make pci_remap_cfgspace() default > to it, falling back to ioremap() on platforms where it is not available. > > Remove the arm64 implementation, since that is now redundant. Future > cleanups should be able to do the same for other arches, and eventually > make the generic pci_remap_cfgspace() unconditional. ... > + void __iomem *ret = ioremap_np(offset, size); > + > + if (!ret) > + ret = ioremap(offset, size); > + > + return ret; Usually negative conditions are worse for cognitive functions of human beings. (On top of that some patterns are applied) I would rewrite above as void __iomem *ret; ret = ioremap_np(offset, size); if (ret) return ret; return ioremap(offset, size); -- With Best Regards, Andy Shevchenko
[PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
Now that we have ioremap_np(), we can make pci_remap_cfgspace() default
to it, falling back to ioremap() on platforms where it is not available.
Remove the arm64 implementation, since that is now redundant. Future
cleanups should be able to do the same for other arches, and eventually
make the generic pci_remap_cfgspace() unconditional.
Signed-off-by: Hector Martin
---
arch/arm64/include/asm/io.h | 10 --
include/linux/io.h | 21 +
2 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 953b8703af60..7fd836bea7eb 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -171,16 +171,6 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr,
size_t size);
#define ioremap_wc(addr, size) __ioremap((addr), (size),
__pgprot(PROT_NORMAL_NC))
#define ioremap_np(addr, size) __ioremap((addr), (size),
__pgprot(PROT_DEVICE_nGnRnE))
-/*
- * PCI configuration space mapping function.
- *
- * The PCI specification disallows posted write configuration transactions.
- * Add an arch specific pci_remap_cfgspace() definition that is implemented
- * through nGnRnE device memory attribute as recommended by the ARM v8
- * Architecture reference manual Issue A.k B2.8.2 "Device memory".
- */
-#define pci_remap_cfgspace(addr, size) __ioremap((addr), (size),
__pgprot(PROT_DEVICE_nGnRnE))
-
/*
* io{read,write}{16,32,64}be() macros
*/
diff --git a/include/linux/io.h b/include/linux/io.h
index d718354ed3e1..6f6b9233f2c3 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -82,20 +82,25 @@ void devm_memunmap(struct device *dev, void *addr);
#ifdef CONFIG_PCI
/*
* The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
- * Posting") mandate non-posted configuration transactions. There is
- * no ioremap API in the kernel that can guarantee non-posted write
- * semantics across arches so provide a default implementation for
- * mapping PCI config space that defaults to ioremap(); arches
- * should override it if they have memory mapping implementations that
- * guarantee non-posted writes semantics to make the memory mapping
- * compliant with the PCI specification.
+ * Posting") mandate non-posted configuration transactions. This default
+ * implementation attempts to use the ioremap_np() API to provide this
+ * on arches that support it, and falls back to ioremap() on those that
+ * don't. Overriding this function is deprecated; arches that properly
+ * support non-posted accesses should implement ioremap_np() instead, which
+ * this default implementation can then use to return mappings compliant with
+ * the PCI specification.
*/
#ifndef pci_remap_cfgspace
#define pci_remap_cfgspace pci_remap_cfgspace
static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
size_t size)
{
- return ioremap(offset, size);
+ void __iomem *ret = ioremap_np(offset, size);
+
+ if (!ret)
+ ret = ioremap(offset, size);
+
+ return ret;
}
#endif
#endif
--
2.30.0
