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)
+ return true;
+
+ return false;
}
-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
--
2.43.0Hi 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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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
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, 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
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>
© 2016 - 2025 Red Hat, Inc.