| Message ID | 20231204123834.29247-6-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Regather scattered PCI-Code | expand |
On Mon, 2023-12-04 at 13:38 +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-spcific"-Problem. > > Suggested-by: Arnd Bergmann <arnd@kernel.org> > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/pci/iomap.c | 46 ++++++++++++----------------------- > -- > include/asm-generic/io.h | 27 ++++++++++++++++++++-- > include/asm-generic/iomap.h | 21 +++++++++++++++++ > lib/iomap.c | 28 ++++++++++++++++------ > 4 files changed, 82 insertions(+), 40 deletions(-) > > diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c > index 91285fcff1ba..439ba2e9710f 100644 > --- a/drivers/pci/iomap.c > +++ b/drivers/pci/iomap.c > @@ -135,44 +135,28 @@ 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); > + 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) by the way: Reading that again my instinctive feeling would be that it should be port >= PIO_OFFSET. This is, however, the exactly copied logic from the IO_COND macro in lib/iomap.c. Is it possible that this macro contains a bug here? P. > + return true; > + > + return false; > } > -EXPORT_SYMBOL(pci_iounmap); > -#endif /* CONFIG_PCI */ > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
On Mon, Dec 4, 2023, at 14:39, Philipp Stanner wrote: > On Mon, 2023-12-04 at 13:38 +0100, Philipp Stanner wrote: >> + */ >> +#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) > > by the way: > Reading that again my instinctive feeling would be that it should be > port >= PIO_OFFSET. > > This is, however, the exactly copied logic from the IO_COND macro in > lib/iomap.c. Is it possible that this macro contains a bug here? Right, I think that would make more sense, but there is no practical difference as long as I/O port 0 is always unused, which is at least true on x86 because of PCIBIOS_MIN_IO. Commit bb356ddb78b2 ("RISC-V: PCI: Avoid handing out address 0 to devices") describes how setting PCIBIOS_MIN_IO to 0 caused other problems. I would just leave the logic consistent with IO_COND here, or maybe use IO_COND() directly, like IO_COND(addr, return true, /* nothing */); return false; Arnd
On Mon, Dec 4, 2023, at 13:38, 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-spcific"-Problem. > > Suggested-by: Arnd Bergmann <arnd@kernel.org> > Signed-off-by: Philipp Stanner <pstanner@redhat.com> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
On Mon, 2023-12-04 at 14:50 +0100, Arnd Bergmann wrote: > On Mon, Dec 4, 2023, at 14:39, Philipp Stanner wrote: > > On Mon, 2023-12-04 at 13:38 +0100, Philipp Stanner wrote: > > > > + */ > > > +#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) > > > > by the way: > > Reading that again my instinctive feeling would be that it should > > be > > port >= PIO_OFFSET. > > > > This is, however, the exactly copied logic from the IO_COND macro > > in > > lib/iomap.c. Is it possible that this macro contains a bug here? > > Right, I think that would make more sense, but there is no > practical difference as long as I/O port 0 is always unused, > which is at least true on x86 because of PCIBIOS_MIN_IO. > Commit bb356ddb78b2 ("RISC-V: PCI: Avoid handing out address > 0 to devices") describes how setting PCIBIOS_MIN_IO to 0 > caused other problems. Ok, makes sense. But should we then adjust iomem_is_ioport() in asm-generic/io.h, as well, so that it matches IO_COND()'s behavior? It currently does this: uintptr_t start = (uintptr_t)PCI_IOBASE; uintptr_t addr = (uintptr_t)addr_raw; if (addr >= start && addr < start + IO_SPACE_LIMIT) return true; and if the architecture does not set PCI_IOBASE, then it's set per default to 0, as well. So we have two inconsistent definitons > > I would just leave the logic consistent with IO_COND here, > or maybe use IO_COND() directly, like > > IO_COND(addr, return true, /* nothing */); > return false; I considered using it to increase consistency. However, I valued improved readability more. Considering that the mid-term goal is to move it to x86 anyways I'd like to leave it that way for now. P. > > Arnd >
On Mon, Dec 4, 2023, at 15:09, Philipp Stanner wrote: > On Mon, 2023-12-04 at 14:50 +0100, Arnd Bergmann wrote: >> On Mon, Dec 4, 2023, at 14:39, Philipp Stanner wrote: >> > On Mon, 2023-12-04 at 13:38 +0100, Philipp Stanner wrote: > > Ok, makes sense. > > But should we then adjust iomem_is_ioport() in asm-generic/io.h, as > well, so that it matches IO_COND()'s behavior? > > It currently does this: > > uintptr_t start = (uintptr_t)PCI_IOBASE; > uintptr_t addr = (uintptr_t)addr_raw; > > if (addr >= start && addr < start + IO_SPACE_LIMIT) > return true; > > and if the architecture does not set PCI_IOBASE, then it's set per > default to 0, as well. > > So we have two inconsistent definitons No, I would also keep the logic here, since it makes more sense and the inconsistency is only for the corner case that doesn't hit in practice. The PCI_IOBASE==0 case should never happen here, as that doesn't work with the generic inb(). I think the only target left that has I/O ports but doesn't set PCI_IOBASE at all is sparc, but that is special in a number of ways. Arnd
Hi Philipp, kernel test robot noticed the following build errors: [auto build test ERROR on pci/next] [also build test ERROR on pci/for-linus arnd-asm-generic/master kees/for-next/pstore kees/for-next/kspp linus/master v6.7-rc4 next-20231205] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-pci_iomap-c-fix-cleanup-bugs-in-pci_iounmap/20231204-204128 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/20231204123834.29247-6-pstanner%40redhat.com patch subject: [PATCH v3 5/5] lib, pci: unify generic pci_iounmap() config: openrisc-virt_defconfig (https://download.01.org/0day-ci/archive/20231205/202312051813.09WbvusW-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051813.09WbvusW-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312051813.09WbvusW-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/pci/iomap.c: In function 'pci_iounmap': >> drivers/pci/iomap.c:155:17: error: implicit declaration of function 'ioport_unmap'; did you mean 'devm_ioport_unmap'? [-Werror=implicit-function-declaration] 155 | ioport_unmap(addr); | ^~~~~~~~~~~~ | devm_ioport_unmap cc1: some warnings being treated as errors vim +155 drivers/pci/iomap.c 144 145 /** 146 * pci_iounmap - Unmapp a mapping 147 * @dev: PCI device the mapping belongs to 148 * @addr: start address of the mapping 149 * 150 * Unmapp a PIO or MMIO mapping. 151 */ 152 void pci_iounmap(struct pci_dev *dev, void __iomem *addr) 153 { 154 if (iomem_is_ioport(addr)) { > 155 ioport_unmap(addr); 156 return; 157 } 158 159 iounmap(addr); 160 } 161 EXPORT_SYMBOL(pci_iounmap); 162
Alright, so it seems that not all architectures provide ioport_unmap(). So I'll provide yet another preprocessor guard in v4. Wohooo, we love them... P. On Tue, 2023-12-05 at 18:44 +0800, kernel test robot wrote: > Hi Philipp, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on pci/next] > [also build test ERROR on pci/for-linus arnd-asm-generic/master > kees/for-next/pstore kees/for-next/kspp linus/master v6.7-rc4 next- > 20231205] > [If your patch is applied to the wrong git tree, kindly drop us a > note. > And when submitting patch, we suggest to use '--base' as documented > in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: > https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-pci_iomap-c-fix-cleanup-bugs-in-pci_iounmap/20231204-204128 > base: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next > patch link: > https://lore.kernel.org/r/20231204123834.29247-6-pstanner%40redhat.com > patch subject: [PATCH v3 5/5] lib, pci: unify generic pci_iounmap() > config: openrisc-virt_defconfig > (https://download.01.org/0day-ci/archive/20231205/202312051813.09Wbvu > sW-lkp@intel.com/config) > compiler: or1k-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20231205/202312051813.09Wbvu > sW-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new > version of > the same patch/commit), kindly add following tags > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202312051813.09WbvusW-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > drivers/pci/iomap.c: In function 'pci_iounmap': > > > drivers/pci/iomap.c:155:17: error: implicit declaration of > > > function 'ioport_unmap'; did you mean 'devm_ioport_unmap'? [- > > > Werror=implicit-function-declaration] > 155 | ioport_unmap(addr); > | ^~~~~~~~~~~~ > | devm_ioport_unmap > cc1: some warnings being treated as errors > > > vim +155 drivers/pci/iomap.c > > 144 > 145 /** > 146 * pci_iounmap - Unmapp a mapping > 147 * @dev: PCI device the mapping belongs to > 148 * @addr: start address of the mapping > 149 * > 150 * Unmapp a PIO or MMIO mapping. > 151 */ > 152 void pci_iounmap(struct pci_dev *dev, void __iomem *addr) > 153 { > 154 if (iomem_is_ioport(addr)) { > > 155 ioport_unmap(addr); > 156 return; > 157 } > 158 > 159 iounmap(addr); > 160 } > 161 EXPORT_SYMBOL(pci_iounmap); > 162 >
On Tue, Dec 5, 2023, at 15:34, Philipp Stanner wrote: > Alright, so it seems that not all architectures provide ioport_unmap(). > So I'll provide yet another preprocessor guard in v4. Wohooo, we love > them... Right, I think CONFIG_HAS_IOPORT_MAP is the symbol you need to check here. There are a few targets that have inb/outb but can't map them to __iomem pointers for some reason, as well as more that have neither CONFIG_HAS_IOPORT nor CONFIG_HAS_IOPORT_MAP. Arnd
diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c index 91285fcff1ba..439ba2e9710f 100644 --- a/drivers/pci/iomap.c +++ b/drivers/pci/iomap.c @@ -135,44 +135,28 @@ 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); + 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-spcific"-Problem. Suggested-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/pci/iomap.c | 46 ++++++++++++------------------------- include/asm-generic/io.h | 27 ++++++++++++++++++++-- include/asm-generic/iomap.h | 21 +++++++++++++++++ lib/iomap.c | 28 ++++++++++++++++------ 4 files changed, 82 insertions(+), 40 deletions(-)