| Message ID | 20231120215945.52027-6-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Regather scattered PCI-Code | expand |
On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: > lib/iomap.c contains one of the definitions of pci_iounmap(). The > current comment above this out-of-place function does not clarify WHY > the function is defined here. > > Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c > clarifies that in a far better way. > > Extend the existing comment with an excerpt from Linus's and hint at the > other implementation in drivers/pci/iomap.c > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> I think instead of explaining why the code is so complicated here, I'd prefer to make it more logical and not have to explain it. We should be able to define a generic version like void pci_iounmap(struct pci_dev *dev, void __iomem * addr) { #ifdef CONFIG_HAS_IOPORT if (iomem_is_ioport(addr)) { ioport_unmap(addr); return; } #endif iounmap(addr) } and then define iomem_is_ioport() in lib/iomap.c for x86, while defining it in asm-generic/io.h for the rest, with an override in asm/io.h for those architectures that need a custom inb(). Note that with ia64 gone, GENERIC_IOMAP is not at all generic any more and could just move it to x86 or name it something else. This is what currently uses it: arch/hexagon/Kconfig: select GENERIC_IOMAP arch/um/Kconfig: select GENERIC_IOMAP These have no port I/O at all, so it doesn't do anything. arch/m68k/Kconfig: select GENERIC_IOMAP on m68knommu, the default implementation from asm-generic/io.h as the same effect as GENERIC_IOMAP but is more efficient. On classic m68k, GENERIC_IOMAP does not do what it is meant to because I/O ports on ISA devices have port numbers above PIO_OFFSET. Also they don't have PCI. arch/mips/Kconfig: select GENERIC_IOMAP This looks completely bogus because it sets PIO_RESERVED to 0 and always uses the mmio part of lib/iomap.c. arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP This is only used for two platforms: cell and powernv, though on Cell it no longer does anything after the commit f4981a00636 ("powerpc: Remove the celleb support"); I think the entire io_workarounds code now be folded back into spider_pci.c if we wanted to. The PowerNV LPC support does seem to still rely on it. This tries to do the exact same thing as lib/logic_pio.c for Huawei arm64 servers. I suspect that neither of them does it entirely correctly since the powerpc side appears to just override any non-LPC PIO support while the arm64 side is missing the ioread/iowrite support. Arnd
On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote: > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: > > lib/iomap.c contains one of the definitions of pci_iounmap(). The > > current comment above this out-of-place function does not clarify > > WHY > > the function is defined here. > > > > Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c > > clarifies that in a far better way. > > > > Extend the existing comment with an excerpt from Linus's and hint > > at the > > other implementation in drivers/pci/iomap.c > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > I think instead of explaining why the code is so complicated > here, I'd prefer to make it more logical and not have to > explain it. > > We should be able to define a generic version like > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > { > #ifdef CONFIG_HAS_IOPORT > if (iomem_is_ioport(addr)) { > ioport_unmap(addr); > return; > } > #endif > iounmap(addr) > } And where would you like such a function to reside? drivers/pci/iomap.c? P. > > and then define iomem_is_ioport() in lib/iomap.c for x86, > while defining it in asm-generic/io.h for the rest, > with an override in asm/io.h for those architectures > that need a custom inb(). > > Note that with ia64 gone, GENERIC_IOMAP is not at all > generic any more and could just move it to x86 or name > it something else. This is what currently uses it: > > arch/hexagon/Kconfig: select GENERIC_IOMAP > arch/um/Kconfig: select GENERIC_IOMAP > > These have no port I/O at all, so it doesn't do anything. > > arch/m68k/Kconfig: select GENERIC_IOMAP > > on m68knommu, the default implementation from asm-generic/io.h > as the same effect as GENERIC_IOMAP but is more efficient. > On classic m68k, GENERIC_IOMAP does not do what it is > meant to because I/O ports on ISA devices have port > numbers above PIO_OFFSET. Also they don't have PCI. > > arch/mips/Kconfig: select GENERIC_IOMAP > > This looks completely bogus because it sets PIO_RESERVED > to 0 and always uses the mmio part of lib/iomap.c. > > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP > > This is only used for two platforms: cell and powernv, > though on Cell it no longer does anything after the > commit f4981a00636 ("powerpc: Remove the celleb support"); > I think the entire io_workarounds code now be folded > back into spider_pci.c if we wanted to. > > The PowerNV LPC support does seem to still rely on it. > This tries to do the exact same thing as lib/logic_pio.c > for Huawei arm64 servers. I suspect that neither of them > does it entirely correctly since the powerpc side appears > to just override any non-LPC PIO support while the arm64 > side is missing the ioread/iowrite support. > > Arnd >
On Tue, Nov 21, 2023, at 15:38, Philipp Stanner wrote: > On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote: >> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: >> We should be able to define a generic version like >> >> void pci_iounmap(struct pci_dev *dev, void __iomem * addr) >> { >> #ifdef CONFIG_HAS_IOPORT >> if (iomem_is_ioport(addr)) { >> ioport_unmap(addr); >> return; >> } >> #endif >> iounmap(addr) >> } > > And where would you like such a function to reside? > drivers/pci/iomap.c? Yes, I think that would be the logical place. It could also be an inline function but that's not great on architectures that don't also have iomem_is_ioport() inline. Arnd
Hi Arnd, On 11/21/23 11:03, Arnd Bergmann wrote: > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: >> lib/iomap.c contains one of the definitions of pci_iounmap(). The >> current comment above this out-of-place function does not clarify WHY >> the function is defined here. >> >> Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c >> clarifies that in a far better way. >> >> Extend the existing comment with an excerpt from Linus's and hint at the >> other implementation in drivers/pci/iomap.c >> >> Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > I think instead of explaining why the code is so complicated > here, I'd prefer to make it more logical and not have to > explain it. > > We should be able to define a generic version like > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) Let's shed some light on the different config options related to this. To me it looks like GENERIC_IOMAP always implies GENERIC_PCI_IOMAP. lib/iomap.c contains a definition of pci_iounmap() since it uses the common IO_COND() macro. This definitions wins if GENERIC_IOMAP was selected. lib/pci_iomap.c contains another definition of pci_iounmap() which is guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple definitions in case either GENERIC_IOMAP is set or the architecture already defined pci_iounmap(). What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP producing an empty definition of pci_iounmap() though [1]? And more generally, is there any other (subtle) logic behind this? [1] https://elixir.bootlin.com/linux/latest/source/lib/pci_iomap.c#L167 > { > #ifdef CONFIG_HAS_IOPORT > if (iomem_is_ioport(addr)) { > ioport_unmap(addr); > return; > } > #endif > iounmap(addr) > } > > and then define iomem_is_ioport() in lib/iomap.c for x86, > while defining it in asm-generic/io.h for the rest, > with an override in asm/io.h for those architectures > that need a custom inb(). So, that would be similar to IO_COND(), right? What would we need inb() for in this context? - Danilo > > Note that with ia64 gone, GENERIC_IOMAP is not at all > generic any more and could just move it to x86 or name > it something else. This is what currently uses it: > > arch/hexagon/Kconfig: select GENERIC_IOMAP > arch/um/Kconfig: select GENERIC_IOMAP > > These have no port I/O at all, so it doesn't do anything. > > arch/m68k/Kconfig: select GENERIC_IOMAP > > on m68knommu, the default implementation from asm-generic/io.h > as the same effect as GENERIC_IOMAP but is more efficient. > On classic m68k, GENERIC_IOMAP does not do what it is > meant to because I/O ports on ISA devices have port > numbers above PIO_OFFSET. Also they don't have PCI. > > arch/mips/Kconfig: select GENERIC_IOMAP > > This looks completely bogus because it sets PIO_RESERVED > to 0 and always uses the mmio part of lib/iomap.c. > > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP > > This is only used for two platforms: cell and powernv, > though on Cell it no longer does anything after the > commit f4981a00636 ("powerpc: Remove the celleb support"); > I think the entire io_workarounds code now be folded > back into spider_pci.c if we wanted to. > > The PowerNV LPC support does seem to still rely on it. > This tries to do the exact same thing as lib/logic_pio.c > for Huawei arm64 servers. I suspect that neither of them > does it entirely correctly since the powerpc side appears > to just override any non-LPC PIO support while the arm64 > side is missing the ioread/iowrite support. > > Arnd >
Hi, On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote: > Hi Arnd, > > On 11/21/23 11:03, Arnd Bergmann wrote: > > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: > > > lib/iomap.c contains one of the definitions of pci_iounmap(). The > > > current comment above this out-of-place function does not clarify > > > WHY > > > the function is defined here. > > > > > > Linus's detailed comment above pci_iounmap() in > > > drivers/pci/iomap.c > > > clarifies that in a far better way. > > > > > > Extend the existing comment with an excerpt from Linus's and hint > > > at the > > > other implementation in drivers/pci/iomap.c > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > I think instead of explaining why the code is so complicated > > here, I'd prefer to make it more logical and not have to > > explain it. > > > > We should be able to define a generic version like > > > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > > Let's shed some light on the different config options related to > this. > > To me it looks like GENERIC_IOMAP always implies GENERIC_PCI_IOMAP. > > lib/iomap.c contains a definition of pci_iounmap() since it uses the > common IO_COND() macro. This definitions wins if GENERIC_IOMAP was > selected. Yes. So it seems the only way the implementation in lib/pci_iomap.c can ever win is when someone selects GENERIC_PCI_IOMAP *without* selecting GENERIC_IOMAP. > > lib/pci_iomap.c contains another definition of pci_iounmap() which is > guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple > definitions > in case either GENERIC_IOMAP is set or the architecture already > defined > pci_iounmap(). To clarify that, here's the relevant excerpt from include/asm- generic/io.h: #ifndef CONFIG_GENERIC_IOMAP #ifndef pci_iounmap #define ARCH_WANTS_GENERIC_PCI_IOUNMAP #endif #endif > > What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP > producing > an empty definition of pci_iounmap() though [1]? > > And more generally, is there any other (subtle) logic behind this? That's indeed also very hard to understand for me, because you'd expect that if pci_iomap() exists (and does something), pci_iounmap() should also exist and, of course, unmapp the memory again. From include/asm-generic/io.h: #ifdef CONFIG_HAS_IOPORT_MAP #ifndef CONFIG_GENERIC_IOMAP #ifndef ioport_map #define ioport_map ioport_map static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) { port &= IO_SPACE_LIMIT; return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; } #define ARCH_HAS_GENERIC_IOPORT_MAP #endif As far as I understand the logic, an empty pci_iounmap() is generated (and used?) in lib/pci_iounmap.c if: * CONFIG_HAS_IOPORT_MAP has not been defined * CONFIG_GENERIC_IOMAP has been defined (makes sense, then we use the one from lib/iomap.c anyways) * ioport_map has been defined by someone other than asm-generic/io.h Regarding the last point, a number of architectures define their own ioport_map(): arch/alpha/kernel/io.c, line 684 (as a function) arch/arc/include/asm/io.h, line 27 (as a function) arch/arm/mm/iomap.c, line 19 (as a function) arch/m68k/include/asm/kmap.h, line 60 (as a function) arch/parisc/lib/iomap.c, line 504 (as a function) arch/powerpc/kernel/iomap.c, line 14 (as a function) arch/s390/include/asm/io.h, line 38 (as a function) arch/sh/kernel/ioport.c, line 24 (as a function) arch/sparc/lib/iomap.c, line 10 (as a function) I grepped through those archs and as I see it, none of those specify an empty pci_iomap() that could be a counterpart to the potentially empty pci_iounmap() in lib/pci_iomap.c All of them use the generic pci_iomap.c, which can _never_ be empty. Perhaps when the functions returning always NULL in include/asm- generic/pci_iomap.h were to be used...? But I think they should never be used, because then pci_iomap.c wins. Arnds seems to agree with that, because he pointed out that these functions are now surplus relicts in his reply to my Patch Nr.1: > From what I can tell looking at the header, I think we can > just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)" > bit entirely, as it no longer serves the purpose it originally > had. So it seems that the empty unmap-function in pci_iomap.c is the left- over counterpart of those mapping functions always returning NULL. @Arnd: Your code draft void pci_iounmap(struct pci_dev *dev, void __iomem * addr) { #ifdef CONFIG_HAS_IOPORT if (iomem_is_ioport(addr)) { ioport_unmap(addr); return; } #endif iounmap(addr) } seems to agree with that: There will never be the need for an empty function that does nothing. Correct? P. > > [1] > https://elixir.bootlin.com/linux/latest/source/lib/pci_iomap.c#L167 > > > { > > #ifdef CONFIG_HAS_IOPORT > > if (iomem_is_ioport(addr)) { > > ioport_unmap(addr); > > return; > > } > > #endif > > iounmap(addr) > > } > > > > and then define iomem_is_ioport() in lib/iomap.c for x86, > > while defining it in asm-generic/io.h for the rest, > > with an override in asm/io.h for those architectures > > that need a custom inb(). > > So, that would be similar to IO_COND(), right? What would we need > inb() for > in this context? > > - Danilo > > > > > Note that with ia64 gone, GENERIC_IOMAP is not at all > > generic any more and could just move it to x86 or name > > it something else. This is what currently uses it: > > > > arch/hexagon/Kconfig: select GENERIC_IOMAP > > arch/um/Kconfig: select GENERIC_IOMAP > > > > These have no port I/O at all, so it doesn't do anything. > > > > arch/m68k/Kconfig: select GENERIC_IOMAP > > > > on m68knommu, the default implementation from asm-generic/io.h > > as the same effect as GENERIC_IOMAP but is more efficient. > > On classic m68k, GENERIC_IOMAP does not do what it is > > meant to because I/O ports on ISA devices have port > > numbers above PIO_OFFSET. Also they don't have PCI. > > > > arch/mips/Kconfig: select GENERIC_IOMAP > > > > This looks completely bogus because it sets PIO_RESERVED > > to 0 and always uses the mmio part of lib/iomap.c. > > > > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP > > > > This is only used for two platforms: cell and powernv, > > though on Cell it no longer does anything after the > > commit f4981a00636 ("powerpc: Remove the celleb support"); > > I think the entire io_workarounds code now be folded > > back into spider_pci.c if we wanted to. > > > > The PowerNV LPC support does seem to still rely on it. > > This tries to do the exact same thing as lib/logic_pio.c > > for Huawei arm64 servers. I suspect that neither of them > > does it entirely correctly since the powerpc side appears > > to just override any non-LPC PIO support while the arm64 > > side is missing the ioread/iowrite support. > > > > Arnd > > >
Hi again, so I thought about this for a while and want to ask some follow up questions in addition to those by Danilo in the other mail. (btw, -CC Herbert Xu, since the mailserver is bouncing) On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote: > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: > > lib/iomap.c contains one of the definitions of pci_iounmap(). The > > current comment above this out-of-place function does not clarify > > WHY > > the function is defined here. > > > > Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c > > clarifies that in a far better way. > > > > Extend the existing comment with an excerpt from Linus's and hint > > at the > > other implementation in drivers/pci/iomap.c > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > I think instead of explaining why the code is so complicated > here, I'd prefer to make it more logical and not have to > explain it. > > We should be able to define a generic version like > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > { > #ifdef CONFIG_HAS_IOPORT > if (iomem_is_ioport(addr)) { > ioport_unmap(addr); > return; > } > #endif > iounmap(addr) > } ACK, I think this makes sense – if we agree (as in the other thread) that we never need an empty pci_iounmap(). > > and then define iomem_is_ioport() in lib/iomap.c for x86, Wait a minute. lib/ should never contain architecture-specific code, should it? I assume your argument is that we write a default version of iomem_is_ioport(), that could, in theory, be used by many architectures, but ultimately only x86 will use that default. > while defining it in asm-generic/io.h for the rest, So we're not talking about the function prototypes here, but about the actual implementation that should reside in asm-generic/io.h, aye? Because the prototype obviously always has to be identical. > with an override in asm/io.h for those architectures > that need a custom inb(). So like this in ARCH/include/asm/io.h: #define iomem_is_ioport iomem_is_ioport bool iomem_is_ioport(...); and in include/asm-generic/io.h: #ifndef iomem_is_ioport bool iomem_is_ioport(...); correct? Still, as Danilo has asked in his email, the question about how inb() is related to it still stands > > Note that with ia64 gone, GENERIC_IOMAP is not at all > generic any more and could just move it to x86 or name > it something else. This is what currently uses it: > > arch/hexagon/Kconfig: select GENERIC_IOMAP > arch/um/Kconfig: select GENERIC_IOMAP > > These have no port I/O at all, so it doesn't do anything. > > arch/m68k/Kconfig: select GENERIC_IOMAP > > on m68knommu, the default implementation from asm-generic/io.h > as the same effect as GENERIC_IOMAP but is more efficient. > On classic m68k, GENERIC_IOMAP does not do what it is > meant to because I/O ports on ISA devices have port > numbers above PIO_OFFSET. Also they don't have PCI. > > arch/mips/Kconfig: select GENERIC_IOMAP > > This looks completely bogus because it sets PIO_RESERVED > to 0 and always uses the mmio part of lib/iomap.c. > > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP > > This is only used for two platforms: cell and powernv, > though on Cell it no longer does anything after the > commit f4981a00636 ("powerpc: Remove the celleb support"); > I think the entire io_workarounds code now be folded > back into spider_pci.c if we wanted to. > > The PowerNV LPC support does seem to still rely on it. > This tries to do the exact same thing as lib/logic_pio.c > for Huawei arm64 servers. I suspect that neither of them > does it entirely correctly since the powerpc side appears > to just override any non-LPC PIO support while the arm64 > side is missing the ioread/iowrite support. I think by now I get what the issue with GENERIC_IOMAP is. But do you want me to do something about GENERIC_IOMAP (besides the things directly related to the PCI functionality I'm touching) for you to approve of a modified version of this patch series? P. > > Arnd >
On Wed, Nov 29, 2023, at 13:40, Philipp Stanner wrote: > On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote: >> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote: >> We should be able to define a generic version like >> >> void pci_iounmap(struct pci_dev *dev, void __iomem * addr) >> { >> #ifdef CONFIG_HAS_IOPORT >> if (iomem_is_ioport(addr)) { >> ioport_unmap(addr); >> return; >> } >> #endif >> iounmap(addr) >> } > > ACK, I think this makes sense – if we agree (as in the other thread) > that we never need an empty pci_iounmap(). > >> >> and then define iomem_is_ioport() in lib/iomap.c for x86, > > Wait a minute. > lib/ should never contain architecture-specific code, should it? I > assume your argument is that we write a default version of > iomem_is_ioport(), that could, in theory, be used by many > architectures, but ultimately only x86 will use that default. I would hope that eventually we can build lib/iomap.c only on x86, as everything else can live without it. >> while defining it in asm-generic/io.h for the rest, > > So we're not talking about the function prototypes here, but about the > actual implementation that should reside in asm-generic/io.h, aye? > Because the prototype obviously always has to be identical. It could live in lib/pci_iomap.c or in include/asm-generic/pci_iomap.h, it doesn't really matter since the definition is trivial. asm-generic/io.h is probably not the right place, unless we want to merge all of asm-generic/pci_iomap.h into asm-generic/io.h. We could do that now that all architectures include asm-generic/io.h and that includes asm-generic/pci_iomap.h unconditionally. >> with an override in asm/io.h for those architectures >> that need a custom inb(). > > So like this in ARCH/include/asm/io.h: > > #define iomem_is_ioport iomem_is_ioport > bool iomem_is_ioport(...); > > and in include/asm-generic/io.h: > > #ifndef iomem_is_ioport > bool iomem_is_ioport(...); > > correct? Yes. >> arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP >> >> This is only used for two platforms: cell and powernv, >> though on Cell it no longer does anything after the >> commit f4981a00636 ("powerpc: Remove the celleb support"); >> I think the entire io_workarounds code now be folded >> back into spider_pci.c if we wanted to. >> >> The PowerNV LPC support does seem to still rely on it. >> This tries to do the exact same thing as lib/logic_pio.c >> for Huawei arm64 servers. I suspect that neither of them >> does it entirely correctly since the powerpc side appears >> to just override any non-LPC PIO support while the arm64 >> side is missing the ioread/iowrite support. > > I think by now I get what the issue with GENERIC_IOMAP is. But do you > want me to do something about GENERIC_IOMAP (besides the things > directly related to the PCI functionality I'm touching) for you to > approve of a modified version of this patch series? It would be nice to clean up some of the architectures that incorrectly select it at the moment, but that can be a separate series if you want to get this one done first, or I can take a look myself. Arnd
On Wed, Nov 29, 2023, at 11:16, Philipp Stanner wrote: > On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote: >> >> lib/pci_iomap.c contains another definition of pci_iounmap() which is >> guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple >> definitions >> in case either GENERIC_IOMAP is set or the architecture already >> defined >> pci_iounmap(). > > To clarify that, here's the relevant excerpt from include/asm- > generic/io.h: > > #ifndef CONFIG_GENERIC_IOMAP > #ifndef pci_iounmap > #define ARCH_WANTS_GENERIC_PCI_IOUNMAP > #endif > #endif Right, this was added fairly recently in an effort to unify the architectures that can share a simple implementation based on the way that modern PCI host bridges on non-x86 work. >> What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP >> producing >> an empty definition of pci_iounmap() though [1]? >> >> And more generally, is there any other (subtle) logic behind this? > > That's indeed also very hard to understand for me, because you'd expect > that if pci_iomap() exists (and does something), pci_iounmap() should > also exist and, of course, unmapp the memory again. Right, I think that was a leak introduced in 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all") that should be fixed like --- a/lib/pci_iomap.c +++ b/lib/pci_iomap.c @@ -170,8 +170,8 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p) if (addr >= start && addr < start + IO_SPACE_LIMIT) return; - iounmap(p); #endif + iounmap(p); } EXPORT_SYMBOL(pci_iounmap); i.e. architectures without port I/O just call iounmap() but those that support the normal ioport_map() have to skip iounmap() for ports in that special PIO range. > Regarding the last point, a number of architectures define their own > ioport_map(): > > arch/alpha/kernel/io.c, line 684 (as a function) > arch/arc/include/asm/io.h, line 27 (as a function) > arch/arm/mm/iomap.c, line 19 (as a function) > arch/m68k/include/asm/kmap.h, line 60 (as a function) > arch/parisc/lib/iomap.c, line 504 (as a function) > arch/powerpc/kernel/iomap.c, line 14 (as a function) > arch/s390/include/asm/io.h, line 38 (as a function) > arch/sh/kernel/ioport.c, line 24 (as a function) > arch/sparc/lib/iomap.c, line 10 (as a function) > > I grepped through those archs and as I see it, none of those specify an > empty pci_iomap() that could be a counterpart to the potentially empty > pci_iounmap() in lib/pci_iomap.c I'm trying to unwind what you are saying here, and there are two separate issues: - an empty unmap() function still makes sense if the map() function just returns a usable pointer like the asm-generic version of ioport_map(), it only has to be non-empty if the map function allocates a resource that has to be freed later, like the page table entries for most ioremap() implementations. - pci_iounmap() in lib/pci_iomap.c being empty is probably just a bug >> From what I can tell looking at the header, I think we can >> just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)" >> bit entirely, as it no longer serves the purpose it originally >> had. > > So it seems that the empty unmap-function in pci_iomap.c is the left- > over counterpart of those mapping functions always returning NULL. no > @Arnd: > Your code draft > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > { > #ifdef CONFIG_HAS_IOPORT > if (iomem_is_ioport(addr)) { > ioport_unmap(addr); > return; > } > #endif > iounmap(addr) > } > > seems to agree with that: There will never be the need for an empty > function that does nothing. Correct? Agreed, while arch/sparc/ currently has an empty pci_iounmap(), that is just because the normal iounmap() on that architecture is also empty, given that all MMIO memory is always mapped. >> > { >> > #ifdef CONFIG_HAS_IOPORT >> > if (iomem_is_ioport(addr)) { >> > ioport_unmap(addr); >> > return; >> > } >> > #endif >> > iounmap(addr) >> > } >> > >> > and then define iomem_is_ioport() in lib/iomap.c for x86, >> > while defining it in asm-generic/io.h for the rest, >> > with an override in asm/io.h for those architectures >> > that need a custom inb(). >> >> So, that would be similar to IO_COND(), right? What would we need >> inb() for in this context? In general, any architecture that has a custom inb() also needs a custom ioport_map() and iomem_is_ioport() in this scheme, while the "normal" architectures like arm/arm64 and riscv should be able to just use the asm-generic version. IO_COND() is really specific to those architectures that rely on the rather misnamed GENERIC_IOMAP for implementing ioport_map(). Arnd
diff --git a/lib/iomap.c b/lib/iomap.c index 4f8b31baa575..647aac8ea3e3 100644 --- a/lib/iomap.c +++ b/lib/iomap.c @@ -419,8 +419,17 @@ 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. */ +/* + * Hide the details if this is a MMIO or PIO address space and just do what + * you expect in the correct way. + * + * 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. + * + * For more details see also the pci_iounmap() implementation in + * drivers/pci/iomap.c + */ void pci_iounmap(struct pci_dev *dev, void __iomem * addr) { IO_COND(addr, /* nothing */, iounmap(addr));
lib/iomap.c contains one of the definitions of pci_iounmap(). The current comment above this out-of-place function does not clarify WHY the function is defined here. Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c clarifies that in a far better way. Extend the existing comment with an excerpt from Linus's and hint at the other implementation in drivers/pci/iomap.c Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- lib/iomap.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)