On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote: > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote: > > The implementation of pci_iounmap() is currently scattered over two > > files, drivers/pci/iomap.c and lib/iomap.c. Additionally, > > architectures can define their own version. > > > > To have only one version, it's necessary to create a helper > > function, > > iomem_is_ioport(), that tells pci_iounmap() whether the passed > > address > > points to an ioport or normal memory. > > > > iomem_is_ioport() can be provided through two different ways: > > 1. The architecture itself provides it. As of today, the version > > coming from lib/iomap.c de facto is the x86-specific version > > and > > comes into play when CONFIG_GENERIC_IOMAP is selected. This > > rather > > confusing naming is an artifact left by the removal of IA64. > > 2. As a default version in include/asm-generic/io.h for those > > architectures that don't use CONFIG_GENERIC_IOMAP, but also > > don't > > provide their own version of iomem_is_ioport(). > > > > Once all architectures that support ports provide > > iomem_is_ioport(), the > > arch-specific definitions for pci_iounmap() can be removed and the > > archs > > can use the generic implementation, instead. > > > > Create a unified version of pci_iounmap() in drivers/pci/iomap.c. > > Provide the function iomem_is_ioport() in include/asm-generic/io.h > > (generic) and lib/iomap.c ("pseudo-generic" for x86). > > > > Remove the CONFIG_GENERIC_IOMAP guard around > > ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set > > CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the > > function. > > > > Add TODOs for follow-up work on the "generic is not generic but > > x86-specific"-Problem. > > ... > > > +++ b/drivers/pci/iomap.c > > @@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev > > *dev, int bar, unsigned long maxlen) > > EXPORT_SYMBOL_GPL(pci_iomap_wc); > > > > /* > > - * pci_iounmap() somewhat illogically comes from lib/iomap.c for > > the > > - * CONFIG_GENERIC_IOMAP case, because that's the code that knows > > about > > - * the different IOMAP ranges. > > + * This check is still necessary due to legacy reasons. > > * > > - * But if the architecture does not use the generic iomap code, > > and if > > - * it has _not_ defined it's own private pci_iounmap function, we > > define > > - * it here. > > - * > > - * NOTE! This default implementation assumes that if the > > architecture > > - * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping > > will > > - * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT > > [, > > - * and does not need unmapping with 'ioport_unmap()'. > > - * > > - * If you have different rules for your architecture, you need to > > - * implement your own pci_iounmap() that knows the rules for where > > - * and how IO vs MEM get mapped. > > - * > > - * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic > > comes > > - * from legacy <asm-generic/io.h> header file behavior. In > > particular, > > - * it would seem to make sense to do the iounmap(p) for the non- > > IO-space > > - * case here regardless, but that's not what the old header file > > code > > - * did. Probably incorrectly, but this is meant to be bug-for-bug > > - * compatible. > > Moving this comment update to the patch that adds the ioport_unmap() > call would make that patch more consistent and simplify this patch. The bugfix from patch #1 you mean. I can take care of that when splitting that patch as you suggested > > > + * TODO: Have all architectures that provide their own > > pci_iounmap() provide > > + * iomem_is_ioport() instead. Remove this #if afterwards. > > */ > > #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP) > > > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p) > > +/** > > + * pci_iounmap - Unmapp a mapping > > + * @dev: PCI device the mapping belongs to > > + * @addr: start address of the mapping > > + * > > + * Unmapp a PIO or MMIO mapping. > > s/Unmapp/Unmap/ (twice) OK > > > + */ > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) > > Maybe move the "p" to "addr" rename to the patch that fixes the > pci_iounmap() #ifdef problem, since that's a trivial change that > already has to do with handling both PIO and MMIO? Then this patch > would be a little more focused. OK > > The kernel-doc addition could possibly also move there since it isn't > related to the unification. You mean the one from my devres-patch-series? Or documentation specifically about pci_iounmap()? > > > { > > -#ifdef ARCH_HAS_GENERIC_IOPORT_MAP > > - uintptr_t start = (uintptr_t) PCI_IOBASE; > > - uintptr_t addr = (uintptr_t) p; > > - > > - if (addr >= start && addr < start + IO_SPACE_LIMIT) { > > - ioport_unmap(p); > > +#ifdef CONFIG_HAS_IOPORT_MAP > > + if (iomem_is_ioport(addr)) { > > + ioport_unmap(addr); > > return; > > } > > #endif > > - iounmap(p); > > + > > + iounmap(addr); > > } > > > + * If CONFIG_GENERIC_IOMAP is selected and the architecture does > > NOT provide its > > + * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that > > the generic > > + * version from asm-generic/io.h is NOT used and instead the > > second "generic" > > + * version from this file here is used. > > + * > > + * There are currently two generic versions because of a difficult > > cleanup > > + * process. Namely, the version in lib/iomap.c once was really > > generic when IA64 > > + * still existed. Today, it's only really used by x86. > > + * > > + * TODO: Move this function to x86-specific code. > > Some of these TODOs look fairly simple. Are they actually hard, or > could they just be done now? If they were simple from my humble POV I would have implemented them. The information about the x86-specficity is from Arnd Bergmann, the header-maintainer. I myself am not that sure how much work it would be to move the entire lib/iomap.c file to x86. At least some (possibley "dead") hooks to it still exist, for example here: arch/powerpc/platforms/Kconfig # L.189 > > It seems like implementing iomem_is_ioport() for the other arches > would be straightforward and if done first, could make this patch > look > tidier. That would be the cleanest solution. But the cleaner you want to be, the more time you have to spend ;) I can take another look and see if I could do that with reasonable effort. Otherwise I'd go for: > Or if the TODOs can't be done now, maybe the iomem_is_ioport() > addition could be done as a separate patch to make the unification > more obvious. sic Thx, P. > > > + */ > > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT) > > +bool iomem_is_ioport(void __iomem *addr) > > { > > - IO_COND(addr, /* nothing */, iounmap(addr)); > > + unsigned long port = (unsigned long __force)addr; > > + > > + if (port > PIO_OFFSET && port < PIO_RESERVED) > > + return true; > > + > > + return false; > > } > > -EXPORT_SYMBOL(pci_iounmap); > > -#endif /* CONFIG_PCI */ > > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */ > > -- > > 2.43.0 > > >