| Message ID | 20231201121622.16343-5-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Regather scattered PCI-Code | expand |
On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote: > The implementation of pci_iounmap() is currently scattered over two > files, drivers/pci/iounmap.c and lib/iomap.c. Additionally, > architectures can define their own version. > > Besides one unified version being desirable in the first place, the old > version in drivers/pci/iounmap.c contained a bug and could leak memory > mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP should not > have guarded iounmap(p); in addition to the preceding code. > > 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 three different ways: > 1. The architecture itself provides it. > 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(). > 3. As a default version in lib/iomap.c for those architectures that > define and use CONFIG_GENERIC_IOMAP (currently, only x86 really > uses the functions in lib/iomap.c) I would count 3 as a special case of 1 here. > Create a unified version of pci_iounmap() in drivers/pci/iomap.c. > Provide the function iomem_is_ioport() in include/asm-generic/io.h and > lib/iomap.c. > > 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. > > Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make > sense of it all") > Suggested-by: Arnd Bergmann <arnd@kernel.org> > Signed-off-by: Philipp Stanner <pstanner@redhat.com> Looks good overall. It would be nice to go further than this and replace all the custom pci_iounmap() variants with custom iomem_is_ioport() implementations, but that can be a follow-up along with removing the incorrect or useless 'select GENERIC_IOMAP' parts. > return; > - iounmap(p); > + } > #endif > + iounmap(addr); > } I think the bugfix should be a separate patch so we can backport it to stable kernels. > +#ifndef CONFIG_GENERIC_IOMAP > +static inline bool iomem_is_ioport(void __iomem *addr) > +{ > + unsigned long port = (unsigned long __force)addr; > + > + // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE into account > + // similar as in ioport_map() ? > + > + if (port > MMIO_UPPER_LIMIT) > + return false; > + > + return true; > +} This has to have the exact logic that was present in the old pci_iounmap(). For the default version that is currently in lib/pci_iomap.c, this means something along the linens of static inline bool struct iomem_is_ioport(void __iomem *p) { #ifdef CONFIG_HAS_IOPORT uintptr_t start = (uintptr_t) PCI_IOBASE; uintptr_t addr = (uintptr_t) p; if (addr >= start && addr < start + IO_SPACE_LIMIT) return true; #endif return false; } > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used. > */ > +bool iomem_is_ioport(void __iomem *addr); > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT I'm not sure what this macro is for, since it appears to do the opposite of what its name suggests: rather than provide the generic version of iomem_is_ioport(), it skips that and provides a custom one to go with lib/iomap.c Arnd
On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote: > On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote: > > The implementation of pci_iounmap() is currently scattered over two > > files, drivers/pci/iounmap.c and lib/iomap.c. Additionally, > > architectures can define their own version. > > > > Besides one unified version being desirable in the first place, the > > old > > version in drivers/pci/iounmap.c contained a bug and could leak > > memory > > mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP > > should not > > have guarded iounmap(p); in addition to the preceding code. > > > > 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 three different ways: > > 1. The architecture itself provides it. > > 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(). > > 3. As a default version in lib/iomap.c for those architectures > > that > > define and use CONFIG_GENERIC_IOMAP (currently, only x86 > > really > > uses the functions in lib/iomap.c) > > I would count 3 as a special case of 1 here. ACK > > > Create a unified version of pci_iounmap() in drivers/pci/iomap.c. > > Provide the function iomem_is_ioport() in include/asm-generic/io.h > > and > > lib/iomap.c. > > > > 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. > > > > Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make > > sense of it all") > > Suggested-by: Arnd Bergmann <arnd@kernel.org> > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > Looks good overall. It would be nice to go further than this > and replace all the custom pci_iounmap() variants with custom > iomem_is_ioport() implementations, but that can be a follow-up > along with removing the incorrect or useless 'select GENERIC_IOMAP' > parts. Yes, let's schedule that for a follow up. The way my project plans sound currently, it's likely that I'll stay close to PCI for the next months anyways, so it's likely we'll get an opportunity to pick this up on the run > > > return; > > - iounmap(p); > > + } > > #endif > > + iounmap(addr); > > } > > I think the bugfix should be a separate patch so we can backport > it to stable kernels. ACK, good idea > > > +#ifndef CONFIG_GENERIC_IOMAP > > +static inline bool iomem_is_ioport(void __iomem *addr) > > +{ > > + unsigned long port = (unsigned long __force)addr; > > + > > + // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE > > into account > > + // similar as in ioport_map() ? > > + > > + if (port > MMIO_UPPER_LIMIT) > > + return false; > > + > > + return true; > > +} > > This has to have the exact logic that was present in the > old pci_iounmap(). For the default version that is currently > in lib/pci_iomap.c, this means something along the linens of OK, I see, so iomem_is_ioport() takes the form derived from lib/pci_iomap.c for asm-generic/io.h, and the form of lib/iomap.c for the one in lib/iomap.c (obviously) > > static inline bool struct iomem_is_ioport(void __iomem *p) > { > #ifdef CONFIG_HAS_IOPORT > uintptr_t start = (uintptr_t) PCI_IOBASE; > uintptr_t addr = (uintptr_t) p; > > if (addr >= start && addr < start + IO_SPACE_LIMIT) > return true; > #endif > return false; > } > > > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be > > used. > > */ > > +bool iomem_is_ioport(void __iomem *addr); > > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT > > I'm not sure what this macro is for, since it appears to > do the opposite of what its name suggests: rather than > provide the generic version of iomem_is_ioport(), it > skips that and provides a custom one to go with lib/iomap.c Hmmm well now it's getting tricky. This else-branch is the one where CONFIG_GENERIC_IOMAP is actually set. I think we're running into the "generic not being generic now that IA64 has died" problem you were hinting at. If we build for x86 and have CONFIG_GENERIC set, only then do we want iomem_is_ioport() from lib/iomap.c. So the macro serves avoiding a collision between symbols. Because lib/iomap.c might be compiled even if someone else already has defined iomem_is_ioport(). I also don't like it, but it was the least bad solution I could come up with Suggestions? P. > > Arnd >
On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote: > On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote: >> >> static inline bool struct iomem_is_ioport(void __iomem *p) >> { >> #ifdef CONFIG_HAS_IOPORT >> uintptr_t start = (uintptr_t) PCI_IOBASE; >> uintptr_t addr = (uintptr_t) p; >> >> if (addr >= start && addr < start + IO_SPACE_LIMIT) >> return true; >> #endif >> return false; >> } >> >> > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be >> > used. >> > */ >> > +bool iomem_is_ioport(void __iomem *addr); >> > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT >> >> I'm not sure what this macro is for, since it appears to >> do the opposite of what its name suggests: rather than >> provide the generic version of iomem_is_ioport(), it >> skips that and provides a custom one to go with lib/iomap.c > > Hmmm well now it's getting tricky. > > This else-branch is the one where CONFIG_GENERIC_IOMAP is actually set. > > I think we're running into the "generic not being generic now that IA64 > has died" problem you were hinting at. > > If we build for x86 and have CONFIG_GENERIC set, only then do we want > iomem_is_ioport() from lib/iomap.c. So the macro serves avoiding a > collision between symbols. Because lib/iomap.c might be compiled even > if someone else already has defined iomem_is_ioport(). > I also don't like it, but it was the least bad solution I could come up > with > Suggestions? The best I can think of is to move the lib/iomap.c version of iomem_is_ioport() to include/asm-generic/iomap.h with an #ifndef iomem_is_ioport / #define iomem_is_ioport check around it. This file is only included on parisc, alpha, sh and when CONFIG_GENERIC_IOMAP is set. The default version in asm-generic/io.h then just needs one more #ifdef iomem_is_ioport check around it. Arnd
On Fri, 2023-12-01 at 22:32 +0100, Arnd Bergmann wrote: > On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote: > > On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote: > > > > > > static inline bool struct iomem_is_ioport(void __iomem *p) > > > { > > > #ifdef CONFIG_HAS_IOPORT > > > uintptr_t start = (uintptr_t) PCI_IOBASE; > > > uintptr_t addr = (uintptr_t) p; > > > > > > if (addr >= start && addr < start + IO_SPACE_LIMIT) > > > return true; > > > #endif > > > return false; > > > } > > > > > > > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will > > > > be > > > > used. > > > > */ > > > > +bool iomem_is_ioport(void __iomem *addr); > > > > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT > > > > > > I'm not sure what this macro is for, since it appears to > > > do the opposite of what its name suggests: rather than > > > provide the generic version of iomem_is_ioport(), it > > > skips that and provides a custom one to go with lib/iomap.c > > > > Hmmm well now it's getting tricky. > > > > This else-branch is the one where CONFIG_GENERIC_IOMAP is actually > > set. > > > > I think we're running into the "generic not being generic now that > > IA64 > > has died" problem you were hinting at. > > > > If we build for x86 and have CONFIG_GENERIC set, only then do we > > want > > iomem_is_ioport() from lib/iomap.c. So the macro serves avoiding a > > collision between symbols. Because lib/iomap.c might be compiled > > even > > if someone else already has defined iomem_is_ioport(). > > I also don't like it, but it was the least bad solution I could > > come up > > with > > Suggestions? > > The best I can think of is to move the lib/iomap.c version > of iomem_is_ioport() to include/asm-generic/iomap.h with > an #ifndef iomem_is_ioport / #define iomem_is_ioport > check around it. This file is only included on parisc, alpha, > sh and when CONFIG_GENERIC_IOMAP is set. My implementation from lib/iomap.c basically just throws away the IO_COND macro and does the checks manually: #if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT) bool iomem_is_ioport(void __iomem *addr) { unsigned long port = (unsigned long __force)addr; if (port > PIO_OFFSET && port < PIO_RESERVED) return true; return false; } #endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */ So we'd also need PIO_OFFSET and PIO_RESERVED, which are not present in asm-generic/iomap.h. Sure, we could move them there or into a common header. But I'm not sure if that is wise, meaning: is it really better than the ugly WANTS_GENERIC_IOMEM... macro? Our entire goal in this series is, after all, to simplify the implementation. P. > > The default version in asm-generic/io.h then just needs > one more #ifdef iomem_is_ioport check around it. > > Arnd >
On Fri, Dec 1, 2023, at 22:56, Philipp Stanner wrote: > On Fri, 2023-12-01 at 22:32 +0100, Arnd Bergmann wrote: >> On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote: >> The best I can think of is to move the lib/iomap.c version >> of iomem_is_ioport() to include/asm-generic/iomap.h with >> an #ifndef iomem_is_ioport / #define iomem_is_ioport >> check around it. This file is only included on parisc, alpha, >> sh and when CONFIG_GENERIC_IOMAP is set. > > My implementation from lib/iomap.c basically just throws away the > IO_COND macro and does the checks manually: > > #if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT) > bool iomem_is_ioport(void __iomem *addr) > { > unsigned long port = (unsigned long __force)addr; > > if (port > PIO_OFFSET && port < PIO_RESERVED) > return true; > > return false; > } > #endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */ > > So we'd also need PIO_OFFSET and PIO_RESERVED, which are not present in > asm-generic/iomap.h. > > Sure, we could move them there or into a common header. But I'm not > sure if that is wise, meaning: is it really better than the ugly > WANTS_GENERIC_IOMEM... macro? > > Our entire goal in this series is, after all, to simplify the > implementation. Right, in that case it's probably better to leave it in lib/iomap.c, just keep the ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT definition in include/asm-generic/iomap.h as well then and keep it out of the normal asm-generic/io.h path. Arnd
diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c index 0a9d503ba533..cb7f86371b7d 100644 --- a/drivers/pci/iomap.c +++ b/drivers/pci/iomap.c @@ -135,42 +135,24 @@ 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. + * Generic version of pci_iounmap() that is used if the architecture does not + * provide its own version. * - * 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. + * If you have special rules for your architecture, you need to implement your + * own pci_iounmap() in ARCH/asm/io.h that knows the rules for where and how IO + * vs MEM get mapped. */ #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP) -void pci_iounmap(struct pci_dev *dev, void __iomem *p) +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) +#ifdef CONFIG_HAS_IOPORT + if (iomem_is_ioport(addr)) { + ioport_unmap(addr); return; - iounmap(p); + } #endif + iounmap(addr); } EXPORT_SYMBOL(pci_iounmap); diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index bac63e874c7b..4177d6b97e0b 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1129,11 +1129,42 @@ extern void ioport_unmap(void __iomem *p); #endif /* CONFIG_GENERIC_IOMAP */ #endif /* CONFIG_HAS_IOPORT_MAP */ -#ifndef CONFIG_GENERIC_IOMAP #ifndef pci_iounmap +#define pci_iounmap pci_iounmap #define ARCH_WANTS_GENERIC_PCI_IOUNMAP -#endif -#endif +#endif /* pci_iounmap */ + + +/* + * This function is a helper only needed for the generic pci_iounmap(). + * It's provided here if the architecture does not select GENERIC_IOMAP and does + * not provide its own version. + */ +#ifdef CONFIG_HAS_IOPORT +#ifndef iomem_is_ioport /* i.e., if the architecture hasn't defined its own. */ +#define iomem_is_ioport iomem_is_ioport + +#ifndef CONFIG_GENERIC_IOMAP +static inline bool iomem_is_ioport(void __iomem *addr) +{ + unsigned long port = (unsigned long __force)addr; + + // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE into account + // similar as in ioport_map() ? + + if (port > MMIO_UPPER_LIMIT) + return false; + + return true; +} +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used. */ +bool iomem_is_ioport(void __iomem *addr); +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT +#endif /* CONFIG_GENERIC_IOMAP */ + +#endif /* iomem_is_ioport */ +#endif /* CONFIG_HAS_IOPORT */ + #ifndef xlate_dev_mem_ptr #define xlate_dev_mem_ptr xlate_dev_mem_ptr diff --git a/lib/iomap.c b/lib/iomap.c index 4f8b31baa575..adaf6246f892 100644 --- a/lib/iomap.c +++ b/lib/iomap.c @@ -418,12 +418,14 @@ 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 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/iounmap.c and lib/iomap.c. Additionally, architectures can define their own version. Besides one unified version being desirable in the first place, the old version in drivers/pci/iounmap.c contained a bug and could leak memory mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP should not have guarded iounmap(p); in addition to the preceding code. 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 three different ways: 1. The architecture itself provides it. 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(). 3. As a default version in lib/iomap.c for those architectures that define and use CONFIG_GENERIC_IOMAP (currently, only x86 really uses the functions in lib/iomap.c) Create a unified version of pci_iounmap() in drivers/pci/iomap.c. Provide the function iomem_is_ioport() in include/asm-generic/io.h and lib/iomap.c. 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. Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all") Suggested-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/pci/iomap.c | 40 +++++++++++----------------------------- include/asm-generic/io.h | 37 ++++++++++++++++++++++++++++++++++--- lib/iomap.c | 16 +++++++++------- 3 files changed, 54 insertions(+), 39 deletions(-)