| Message ID | 20240111085540.7740-6-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Regather scattered PCI-Code | expand |
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. > + * 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) > + */ > +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. The kernel-doc addition could possibly also move there since it isn't related to the unification. > { > -#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? It seems like implementing iomem_is_ioport() for the other arches would be straightforward and if done first, could make this patch look tidier. 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. > + */ > +#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 >
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 > > >
On Fri, Jan 26, 2024, at 14:59, Philipp Stanner wrote: > 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 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 This one definitely takes some work to untangle, it's selected by two powerpc platforms, but one doesn't actually need it any more, and the other one uses it only for non-PCI devices. I think the other architectures are easier to change and do fix real bugs, but it's probably best done one at a time. Arnd
On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote: > 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: > ... > > > -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. > > > + */ > > > +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. > > > > 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()? I had in mind the patch that fixes the pci_iounmap() #ifdef problem, which (if you split it out from 1/5) would be a relatively trivial patch. Or the kernel-doc addition could be its own separate patch. The point is that this unification patch is fairly complicated, so anything we can do to move things unrelated to unification elsewhere makes this one easier to review. > > 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. It looks like iomem_is_ioport() is basically the guards in pci_iounmap() implementations that, if true, prevent calling iounmap(), so it it seems like they should be trivial, e.g., return !__is_mmio(addr); # alpha return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr); # microblaze Unless they're significantly more complicated than that, I don't see the point of deferring them. > > > + */ > > > +#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 > > > > > >
On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote: > On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote: > > 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: > > ... > > > > > -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. > > > > + */ > > > > +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. > > > > > > 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()? > > I had in mind the patch that fixes the pci_iounmap() #ifdef problem, > which (if you split it out from 1/5) would be a relatively trivial > patch. Or the kernel-doc addition could be its own separate patch. > The point is that this unification patch is fairly complicated, so > anything we can do to move things unrelated to unification elsewhere > makes this one easier to review. I think it should be a separate patch, then, as it doesn't belong by 100% to any of the patches here. If I had to pick one, I'd have included the docu into patch #2 or #3. Let's make it a separate one, following as a 6th patch in this series > > > > 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. > > It looks like iomem_is_ioport() is basically the guards in > pci_iounmap() implementations that, if true, prevent calling > iounmap(), so it it seems like they should be trivial, e.g., > > return !__is_mmio(addr); # alpha > > return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm > > return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr); > # microblaze > > Unless they're significantly more complicated than that, I don't see > the point of deferring them. Have you seen Arnd's reply from Friday? Cleaning up Powerpc's use of lib/iomap.c will be significantly more complicated. This series' purpose actually always has been to move PCI functions to where they belong, i.e. from lib/ to drivers/pci. I originally didn't want to touch pci_iounmap(), since I deemed it too complicated. Arnd pushed for unifying it. Anyways, investing much more time into this is beyond my time budget. I only started this series to have a cleaner basis to do the devres functions. So my suggestions is that we either go with this cleanup here, which improves the situation at least somewhat, or we simply drop patch #5 and leave pci_iounmap() as the last pci_ function in lib/ 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 > > > > > > > > > >
On Mon, Jan 29, 2024 at 11:43:34AM +0100, Philipp Stanner wrote: > On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote: > > On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote: > > > 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: > > > ... > > > > > > > -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. > > > > > + */ > > > > > +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. > > > > > > > > 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()? > > > > I had in mind the patch that fixes the pci_iounmap() #ifdef problem, > > which (if you split it out from 1/5) would be a relatively trivial > > patch. Or the kernel-doc addition could be its own separate patch. > > The point is that this unification patch is fairly complicated, so > > anything we can do to move things unrelated to unification elsewhere > > makes this one easier to review. > > I think it should be a separate patch, then, as it doesn't belong by > 100% to any of the patches here. If I had to pick one, I'd have > included the docu into patch #2 or #3. > > Let's make it a separate one, following as a 6th patch in this series Sounds good. > > > > 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. > > > > It looks like iomem_is_ioport() is basically the guards in > > pci_iounmap() implementations that, if true, prevent calling > > iounmap(), so it it seems like they should be trivial, e.g., > > > > return !__is_mmio(addr); # alpha > > > > return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm > > > > return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr); > > # microblaze > > > > Unless they're significantly more complicated than that, I don't see > > the point of deferring them. > ... > This series' purpose actually always has been to move PCI functions to > where they belong, i.e. from lib/ to drivers/pci. > I originally didn't want to touch pci_iounmap(), since I deemed it too > complicated. Arnd pushed for unifying it. > > Anyways, investing much more time into this is beyond my time budget. I > only started this series to have a cleaner basis to do the devres > functions. > > So my suggestions is that we either go with this cleanup here, which > improves the situation at least somewhat, or we simply drop patch #5 > and leave pci_iounmap() as the last pci_ function in lib/ OK, let's drop #5 for now. It definitely seems like where we should go in the future, but I think it will make more sense when we can include a few of the simple conversions that will show how it all fits together. Bjorn
diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c index 91285fcff1ba..b7faf22ec8f5 100644 --- a/drivers/pci/iomap.c +++ 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. + * 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. + */ +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) { -#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); } EXPORT_SYMBOL(pci_iounmap); diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index bac63e874c7b..58c7bf4080da 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1129,11 +1129,34 @@ extern void ioport_unmap(void __iomem *p); #endif /* CONFIG_GENERIC_IOMAP */ #endif /* CONFIG_HAS_IOPORT_MAP */ -#ifndef CONFIG_GENERIC_IOMAP +/* + * TODO: + * remove this once all architectures replaced their pci_iounmap() with + * a custom implementation of iomem_is_ioport(). + */ #ifndef pci_iounmap +#define pci_iounmap pci_iounmap #define ARCH_WANTS_GENERIC_PCI_IOUNMAP +#endif /* pci_iounmap */ + +/* + * This function is a helper only needed for the generic pci_iounmap(). + * It's provided here if the architecture does not provide its own version. + */ +#ifndef iomem_is_ioport +#define iomem_is_ioport iomem_is_ioport +static inline bool iomem_is_ioport(void __iomem *addr_raw) +{ +#ifdef CONFIG_HAS_IOPORT + uintptr_t start = (uintptr_t)PCI_IOBASE; + uintptr_t addr = (uintptr_t)addr_raw; + + if (addr >= start && addr < start + IO_SPACE_LIMIT) + return true; #endif -#endif + return false; +} +#endif /* iomem_is_ioport */ #ifndef xlate_dev_mem_ptr #define xlate_dev_mem_ptr xlate_dev_mem_ptr diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h index 196087a8126e..2cdc6988a102 100644 --- a/include/asm-generic/iomap.h +++ b/include/asm-generic/iomap.h @@ -110,6 +110,27 @@ static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size) } #endif +/* + * 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 lib/iomap.c 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 the version from lib/iomap.c to x86 specific code. Then, remove + * this ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT-mechanism. + */ +#ifdef CONFIG_GENERIC_IOMAP +#ifndef iomem_is_ioport +#define iomem_is_ioport iomem_is_ioport +bool iomem_is_ioport(void __iomem *addr); +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT +#endif /* iomem_is_ioport */ +#endif /* CONFIG_GENERIC_IOMAP */ + #include <asm-generic/pci_iomap.h> #endif diff --git a/lib/iomap.c b/lib/iomap.c index 4f8b31baa575..eb9a879ebf42 100644 --- a/lib/iomap.c +++ b/lib/iomap.c @@ -418,12 +418,26 @@ EXPORT_SYMBOL(ioport_map); EXPORT_SYMBOL(ioport_unmap); #endif /* CONFIG_HAS_IOPORT_MAP */ -#ifdef CONFIG_PCI -/* Hide the details if this is a MMIO or PIO address space and just do what - * you expect in the correct way. */ -void pci_iounmap(struct pci_dev *dev, void __iomem * 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. + */ +#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 */
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. Suggested-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Philipp Stanner <pstanner@redhat.com> Reviewed-by: Arnd Bergmann <arnd@arndb.de> --- drivers/pci/iomap.c | 46 +++++++++++++------------------------ include/asm-generic/io.h | 27 ++++++++++++++++++++-- include/asm-generic/iomap.h | 21 +++++++++++++++++ lib/iomap.c | 28 ++++++++++++++++------ 4 files changed, 83 insertions(+), 39 deletions(-)