Regather scattered PCI-Code

pci_iounmap() in lib/pci_iomap.c is supposed to check whether an address is within ioport-range IF the config specifies that ioports exist. If so, the port should be unmapped with ioport_unmap(). If not, it's a generic MMIO address that has to be passed to iounmap().

The bugs are:
1. ioport_unmap() is missing entirely, so this function will never actually unmap a port.

2. the #ifdef for the ioport-ranges accidentally also guards iounmap(), potentially compiling an empty function. This would cause the mapping to be leaked.

Implement the missing call to ioport_unmap().

Move the guard so that iounmap() will always be part of the function.

CC <[email protected]> # v5.15+

Fixes 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all")

Reported-by Danilo Krummrich <[email protected]>

Suggested-by Arnd Bergmann <[email protected]>

Signed-off-by Philipp Stanner <[email protected]>

Reviewed-by Arnd Bergmann <[email protected]>

---
 lib/pci_iomap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

lib/pci_iomap.c +4 -2

index ce39ce9f3526..6e144b017c48 100644
@@ -168,10 +168,12 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 	uintptr_t start = (uintptr_t) PCI_IOBASE;
 	uintptr_t addr = (uintptr_t) p;
 
-	if (addr >= start && addr < start + IO_SPACE_LIMIT)
+	if (addr >= start && addr < start + IO_SPACE_LIMIT) {
+		ioport_unmap(p);
 		return;
-	iounmap(p);
+	}
 #endif
+	iounmap(p);
 }
 EXPORT_SYMBOL(pci_iounmap);
 

Signature · 1 line

2.43.0

Quoted 11 lines · Philipp Stanner

pci_iounmap() in lib/pci_iomap.c is supposed to check whether an address
is within ioport-range IF the config specifies that ioports exist. If
so, the port should be unmapped with ioport_unmap(). If not, it's a generic MMIO address that has to be passed to iounmap().

The bugs are:
  1. ioport_unmap() is missing entirely, so this function will never
     actually unmap a port.

The preceding comment suggests that in this default implementation, the ioport does not need unmapping, and it wasn't something it was supposed to do but just failed to do:

 * 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.

Almost all ioport_unmap() implementations are empty, so in most cases it's a no-op (parisc is an exception).

I'm happy to add the ioport_unmap() even just for symmetry, but if we do, I think we should update or remove that comment.

Quoted 5 lines

  2. the #ifdef for the ioport-ranges accidentally also guards iounmap(), potentially compiling an empty function. This would cause the mapping to be leaked.

Quoted 4 lines

Implement the missing call to ioport_unmap().

Move the guard so that iounmap() will always be part of the function.

I think we should fix this bug in a separate patch because the ioport_unmap() is much more subtle and doesn't need to be complicated with this fix.

Quoted 5 lines

CC <[email protected]> # v5.15+

Fixes 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make

sense of it all")

Reported-by Danilo Krummrich <[email protected]>

Is there a URL we can include for Danilo's report?  I found
https://lore.kernel.org/all/[email protected]/
,
but I'm not sure that's the right part of the conversation.

Quoted 31 lines

Suggested-by Arnd Bergmann <[email protected]>

Signed-off-by Philipp Stanner <[email protected]>

Reviewed-by Arnd Bergmann <[email protected]>

---
 lib/pci_iomap.c | 6 ++++--

 1 file changed, 4 insertions(+), 2 deletions(-)

lib/pci_iomap.c +0 -0

index ce39ce9f3526..6e144b017c48 100644
@@ -168,10 +168,12 @@ void pci_iounmap(struct pci_dev *dev, void
 __iomem *p)
         uintptr_t start = (uintptr_t) PCI_IOBASE;
         uintptr_t addr = (uintptr_t) p;

-       if (addr >= start && addr < start + IO_SPACE_LIMIT) +       if (addr >= start && addr < start + IO_SPACE_LIMIT) { +               ioport_unmap(p);
                return;
-       iounmap(p);
+       }
 #endif
+       iounmap(p);
 }
 EXPORT_SYMBOL(pci_iounmap);

Signature · 1 line

2.43.0

This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not belong to lib/ because it is not generic infrastructure.

Move the file to drivers/pci/ and implement the necessary changes to Makefiles and Kconfigs.
...

drivers/pci/Kconfig +4 -0

@@ -13,6 +13,11 @@ config FORCE_PCI
 	select HAVE_PCI
 	select PCI
 
+# select this to provide a generic PCI iomap,
+# without PCI itself having to be defined
+config GENERIC_PCI_IOMAP
+	bool
 

Quoted 23 lines · Philipp Stanner

This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not
belong to lib/ because it is not generic infrastructure.

Move the file to drivers/pci/ and implement the necessary changes to
Makefiles and Kconfigs.
...

drivers/pci/Kconfig +0 -0

@@ -13,6 +13,11 @@ config FORCE_PCI

        select HAVE_PCI
        select PCI

drivers/pci/Kconfig +4 -0

+# select this to provide a generic PCI iomap,
+# without PCI itself having to be defined
+config GENERIC_PCI_IOMAP
+       bool

drivers/pci/iomap.c renamed from lib/pci_iomap.c +0 -0

@@ -9,7 +9,6 @@

Quoted 4 lines

 #include <linux/export.h>

-#ifdef CONFIG_PCI

IIUC, in the case where CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI was not set, pci_iomap.c was compiled but produced no code because the entire file was wrapped with this #ifdef.

But after this patch, it looks like pci_iomap_range(), pci_iomap_wc_range(), etc., *will* be compiled?

Is that what you intend, or did I miss something?

Bjorn

Quoted 33 lines · Bjorn Helgaas

Quoted 23 lines · Philipp Stanner

This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not
belong to lib/ because it is not generic infrastructure.

Move the file to drivers/pci/ and implement the necessary changes to
Makefiles and Kconfigs.
...

drivers/pci/Kconfig +0 -0

@@ -13,6 +13,11 @@ config FORCE_PCI

        select HAVE_PCI
        select PCI

drivers/pci/Kconfig +4 -0

+# select this to provide a generic PCI iomap,
+# without PCI itself having to be defined
+config GENERIC_PCI_IOMAP
+       bool

drivers/pci/iomap.c renamed from lib/pci_iomap.c +0 -0

@@ -9,7 +9,6 @@

Quoted 4 lines

 #include <linux/export.h>

-#ifdef CONFIG_PCI

IIUC, in the case where CONFIG_GENERIC_PCI_IOMAP=y but CONFIG_PCI was not set, pci_iomap.c was compiled but produced no code because the entire file was wrapped with this #ifdef.

Quoted 4 lines

But after this patch, it looks like pci_iomap_range(), pci_iomap_wc_range(), etc., *will* be compiled?

Is that what you intend, or did I miss something?

They *will* be compiled when BOTH, CONFIG_PCI and
CONFIG_GENERIC_PCI_IOMAP have been set.

The implementation of pci_iounmap() is currently scattered over two files, drivers/pci/iomap.c and lib/iomap.c. Additionally, architectures can define their own version.

To have only one version, it's necessary to create a helper function, iomem_is_ioport(), that tells pci_iounmap() whether the passed address points to an ioport or normal memory.

iomem_is_ioport() can be provided through two different ways:
1. The architecture itself provides it. As of today, the version coming from lib/iomap.c de facto is the x86-specific version and comes into play when CONFIG_GENERIC_IOMAP is selected. This rather confusing naming is an artifact left by the removal of IA64.
2. As a default version in include/asm-generic/io.h for those architectures that don't use CONFIG_GENERIC_IOMAP, but also don't provide their own version of iomem_is_ioport().

Once all architectures that support ports provide iomem_is_ioport(), the arch-specific definitions for pci_iounmap() can be removed and the archs can use the generic implementation, instead.

Create a unified version of pci_iounmap() in drivers/pci/iomap.c. Provide the function iomem_is_ioport() in include/asm-generic/io.h (generic) and lib/iomap.c ("pseudo-generic" for x86).

Remove the CONFIG_GENERIC_IOMAP guard around ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the function.

Add TODOs for follow-up work on the "generic is not generic but x86-specific"-Problem.
...

+++ b/drivers/pci/iomap.c

drivers/pci/iomap.c +1 -12

@@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
 EXPORT_SYMBOL_GPL(pci_iomap_wc);
 
 /*
- * pci_iounmap() somewhat illogically comes from lib/iomap.c for the
- * CONFIG_GENERIC_IOMAP case, because that's the code that knows about
- * the different IOMAP ranges.
+ * This check is still necessary due to legacy reasons.
  *
- * But if the architecture does not use the generic iomap code, and if
- * it has _not_ defined it's own private pci_iounmap function, we define
- * it here.
- *
- * NOTE! This default implementation assumes that if the architecture
- * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
- * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
- * and does not need unmapping with 'ioport_unmap()'.
- *

drivers/pci/iomap.c +5 -7

 {
-#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
-	uintptr_t start = (uintptr_t) PCI_IOBASE;
-	uintptr_t addr = (uintptr_t) p;
-
-	if (addr >= start && addr < start + IO_SPACE_LIMIT) {
-		ioport_unmap(p);
+#ifdef CONFIG_HAS_IOPORT_MAP
+	if (iomem_is_ioport(addr)) {
+		ioport_unmap(addr);
 		return;
 	}
 #endif
-	iounmap(p);
+
+	iounmap(addr);
 }
 

+ */

lib/iomap.c +9 -3

+#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 */

Signature · 1 line

2.43.0

Quoted 77 lines · Philipp Stanner

The implementation of pci_iounmap() is currently scattered over two files, drivers/pci/iomap.c and lib/iomap.c. Additionally, architectures can define their own version.

To have only one version, it's necessary to create a helper function,
iomem_is_ioport(), that tells pci_iounmap() whether the passed address
points to an ioport or normal memory.

iomem_is_ioport() can be provided through two different ways:
  1. The architecture itself provides it. As of today, the version coming from lib/iomap.c de facto is the x86-specific version and
     comes into play when CONFIG_GENERIC_IOMAP is selected. This rather
     confusing naming is an artifact left by the removal of IA64.
  2. As a default version in include/asm-generic/io.h for those architectures that don't use CONFIG_GENERIC_IOMAP, but also don't
     provide their own version of iomem_is_ioport().

Once all architectures that support ports provide iomem_is_ioport(), the
arch-specific definitions for pci_iounmap() can be removed and the archs
can use the generic implementation, instead.

Create a unified version of pci_iounmap() in drivers/pci/iomap.c. Provide the function iomem_is_ioport() in include/asm-generic/io.h (generic) and lib/iomap.c ("pseudo-generic" for x86).

Remove the CONFIG_GENERIC_IOMAP guard around ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the function.

Add TODOs for follow-up work on the "generic is not generic but x86-specific"-Problem.
...

+++ b/drivers/pci/iomap.c

(quoted diff) +0 -0

@@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev
 *dev, int bar, unsigned long maxlen)
  EXPORT_SYMBOL_GPL(pci_iomap_wc);

 /*
- * pci_iounmap() somewhat illogically comes from lib/iomap.c for the
- * CONFIG_GENERIC_IOMAP case, because that's the code that knows about
- * the different IOMAP ranges.
+ * This check is still necessary due to legacy reasons. *
- * But if the architecture does not use the generic iomap code, and if
- * it has _not_ defined it's own private pci_iounmap function, we define
- * it here.
- *
- * NOTE! This default implementation assumes that if the architecture
- * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
- * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
- * and does not need unmapping with 'ioport_unmap()'.
- *
- * If you have different rules for your architecture, you need to
- * implement your own pci_iounmap() that knows the rules for where
- * and how IO vs MEM get mapped.
- *
- * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic comes
- * from legacy <asm-generic/io.h> header file behavior. In particular,
- * it would seem to make sense to do the iounmap(p) for the non-

Quoted 5 lines

IO-space
- * case here regardless, but that's not what the old header file code
- * did. Probably incorrectly, but this is meant to be bug-for-bug
- * compatible.

Moving this comment update to the patch that adds the ioport_unmap() call would make that patch more consistent and simplify this patch.

Quoted 7 lines

+ * 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)

drivers/pci/iomap.c void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen) · line 150

Quoted 7 lines

drivers/pci/iomap.c +6 -1

-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+/**
+ * pci_iounmap - Unmapp a mapping
+ * @dev: PCI device the mapping belongs to
+ * @addr: start address of the mapping
+ *
+ * Unmapp a PIO or MMIO mapping.

s/Unmapp/Unmap/ (twice)

Quoted 3 lines

+ */
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)

Maybe move the "p" to "addr" rename to the patch that fixes the pci_iounmap() #ifdef problem, since that's a trivial change that already has to do with handling both PIO and MMIO?  Then this patch would be a little more focused.

The kernel-doc addition could possibly also move there since it isn't related to the unification.

Quoted 19 lines

 {

drivers/pci/iomap.c +3 -6

-#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
-       uintptr_t start = (uintptr_t) PCI_IOBASE;
-       uintptr_t addr = (uintptr_t) p;
-
-       if (addr >= start && addr < start + IO_SPACE_LIMIT) {
-               ioport_unmap(p);
+#ifdef CONFIG_HAS_IOPORT_MAP
+       if (iomem_is_ioport(addr)) {
+               ioport_unmap(addr);

                return;
        }
 #endif
-       iounmap(p);
+
+       iounmap(addr);
 }

Quoted 15 lines

+ * If CONFIG_GENERIC_IOMAP is selected and the architecture does NOT provide its
+ * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that the generic
+ * version from asm-generic/io.h is NOT used and instead the second "generic"
+ * version from this file here is used.
+ *
+ * There are currently two generic versions because of a difficult cleanup
+ * process. Namely, the version in lib/iomap.c once was really generic when IA64
+ * still existed. Today, it's only really used by x86.
+ *
+ * TODO: Move this function to x86-specific code.

Some of these TODOs look fairly simple.  Are they actually hard, or could they just be done now?

It seems like implementing iomem_is_ioport() for the other arches would be straightforward and if done first, could make this patch look
tidier.

Or if the TODOs can't be done now, maybe the iomem_is_ioport() addition could be done as a separate patch to make the unification more obvious.

Quoted 19 lines

+ */

lib/iomap.c +2 -0

+#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;

lib/iomap.c +5 -0

+
+       if (port > PIO_OFFSET && port < PIO_RESERVED)
+               return true;
+
+       return false;

 }

lib/iomap.c +1 -2

-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */

Signature · 1 line

2.43.0

Quoted 4 lines · Bjorn Helgaas

On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:

The kernel-doc addition could possibly also move there since it isn't related to the unification.

You mean the one from my devres-patch-series? Or documentation specifically about pci_iounmap()?

Quoted 35 lines

Quoted 19 lines

 {

drivers/pci/iomap.c +3 -6

-#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
-       uintptr_t start = (uintptr_t) PCI_IOBASE;
-       uintptr_t addr = (uintptr_t) p;
-
-       if (addr >= start && addr < start + IO_SPACE_LIMIT) {
-               ioport_unmap(p);
+#ifdef CONFIG_HAS_IOPORT_MAP
+       if (iomem_is_ioport(addr)) {
+               ioport_unmap(addr);

                return;
        }
 #endif
-       iounmap(p);
+
+       iounmap(addr);
 }

Quoted 15 lines

+ * 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.

Quoted 2 lines

Some of these TODOs look fairly simple.  Are they actually hard, or could they just be done now?

If they were simple from my humble POV I would have implemented them. The information about the x86-specficity is from Arnd Bergmann, the header-maintainer.

I myself am not that sure how much work it would be to move the entire lib/iomap.c file to x86. At least some (possibley "dead") hooks to it still exist, for example here:
arch/powerpc/platforms/Kconfig # L.189

Quoted 1 line · Bjorn Helgaas

On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:

...

Quoted 15 lines

drivers/pci/iomap.c void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen) · line 152

Quoted 9 lines

drivers/pci/iomap.c +8 -1

-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+/**
+ * pci_iounmap - Unmapp a mapping
+ * @dev: PCI device the mapping belongs to
+ * @addr: start address of the mapping
+ *
+ * Unmapp a PIO or MMIO mapping.
+ */
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)

Maybe move the "p" to "addr" rename to the patch that fixes the pci_iounmap() #ifdef problem, since that's a trivial change that already has to do with handling both PIO and MMIO?  Then this patch would be a little more focused.

Quoted 2 lines

The kernel-doc addition could possibly also move there since it isn't related to the unification.

You mean the one from my devres-patch-series? Or documentation specifically about pci_iounmap()?

Quoted 4 lines

It seems like implementing iomem_is_ioport() for the other arches would be straightforward and if done first, could make this patch look
tidier.

That would be the cleanest solution. But the cleaner you want to be, the more time you have to spend ;)
I can take another look and see if I could do that with reasonable effort.
Otherwise I'd go for:

Quoted 3 lines

Or if the TODOs can't be done now, maybe the iomem_is_ioport() addition could be done as a separate patch to make the unification more obvious.

Quoted 19 lines

Quoted 18 lines

+ */

lib/iomap.c +2 -0

+#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;

lib/iomap.c +5 -0

+
+       if (port > PIO_OFFSET && port < PIO_RESERVED)
+               return true;
+
+       return false;

 }

lib/iomap.c +1 -2

-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */

Signature · 1 line

2.43.0

Quoted 20 lines · Philipp Stanner

Quoted 1 line · Bjorn Helgaas

On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:

...

Quoted 16 lines

drivers/pci/iomap.c void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen) · line 152

Quoted 9 lines

drivers/pci/iomap.c +8 -1

-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+/**
+ * pci_iounmap - Unmapp a mapping
+ * @dev: PCI device the mapping belongs to
+ * @addr: start address of the mapping
+ *
+ * Unmapp a PIO or MMIO mapping.
+ */
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)

Maybe move the "p" to "addr" rename to the patch that fixes the pci_iounmap() #ifdef problem, since that's a trivial change that already has to do with handling both PIO and MMIO?  Then this patch
would be a little more focused.

Quoted 6 lines

Quoted 3 lines

The kernel-doc addition could possibly also move there since it isn't
related to the unification.

You mean the one from my devres-patch-series? Or documentation specifically about pci_iounmap()?

I had in mind the patch that fixes the pci_iounmap() #ifdef problem, which (if you split it out from 1/5) would be a relatively trivial patch.  Or the kernel-doc addition could be its own separate patch. The point is that this unification patch is fairly complicated, so anything we can do to move things unrelated to unification elsewhere makes this one easier to review.

Quoted 6 lines

Quoted 5 lines

It seems like implementing iomem_is_ioport() for the other arches would be straightforward and if done first, could make this patch look
tidier.

Quoted 11 lines

That would be the cleanest solution. But the cleaner you want to be,
the more time you have to spend ;)
I can take another look and see if I could do that with reasonable effort.
Otherwise I'd go for:

Quoted 4 lines

Or if the TODOs can't be done now, maybe the iomem_is_ioport() addition could be done as a separate patch to make the unification
more obvious.

It looks like iomem_is_ioport() is basically the guards in pci_iounmap() implementations that, if true, prevent calling iounmap(), so it it seems like they should be trivial, e.g.,

  return !__is_mmio(addr); # alpha

  return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm

  return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);

# microblaze

Unless they're significantly more complicated than that, I don't see the point of deferring them.

Quoted 21 lines

Quoted 20 lines

Quoted 19 lines

+ */

lib/iomap.c +2 -0

+#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;

lib/iomap.c +5 -0

+
+       if (port > PIO_OFFSET && port < PIO_RESERVED)
+               return true;
+
+       return false;

 }

lib/iomap.c +1 -2

-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */

Signature · 1 line

2.43.0

Quoted 28 lines · Bjorn Helgaas

Quoted 20 lines · Philipp Stanner

Quoted 1 line · Bjorn Helgaas

On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:

...

Quoted 16 lines

drivers/pci/iomap.c void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen) · line 152

Quoted 9 lines

drivers/pci/iomap.c +8 -1

-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+/**
+ * pci_iounmap - Unmapp a mapping
+ * @dev: PCI device the mapping belongs to
+ * @addr: start address of the mapping
+ *
+ * Unmapp a PIO or MMIO mapping.
+ */
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)

Maybe move the "p" to "addr" rename to the patch that fixes the pci_iounmap() #ifdef problem, since that's a trivial change that already has to do with handling both PIO and MMIO?  Then this patch
would be a little more focused.

Quoted 6 lines

Quoted 3 lines

The kernel-doc addition could possibly also move there since it isn't
related to the unification.

You mean the one from my devres-patch-series? Or documentation specifically about pci_iounmap()?

Quoted 6 lines

I had in mind the patch that fixes the pci_iounmap() #ifdef problem, which (if you split it out from 1/5) would be a relatively trivial patch.  Or the kernel-doc addition could be its own separate patch. The point is that this unification patch is fairly complicated, so anything we can do to move things unrelated to unification elsewhere makes this one easier to review.

I think it should be a separate patch, then, as it doesn't belong by 100% to any of the patches here. If I had to pick one, I'd have included the docu into patch #2 or #3.

Let's make it a separate one, following as a 6th patch in this series

Quoted 23 lines

Quoted 5 lines

Quoted 4 lines

It seems like implementing iomem_is_ioport() for the other arches would be straightforward and if done first, could make this patch look
tidier.

Quoted 11 lines

That would be the cleanest solution. But the cleaner you want to be,
the more time you have to spend ;)
I can take another look and see if I could do that with reasonable effort.
Otherwise I'd go for:

Quoted 4 lines

Or if the TODOs can't be done now, maybe the iomem_is_ioport() addition could be done as a separate patch to make the unification
more obvious.

It looks like iomem_is_ioport() is basically the guards in pci_iounmap() implementations that, if true, prevent calling iounmap(), so it it seems like they should be trivial, e.g.,

  return !__is_mmio(addr); # alpha

Quoted 7 lines

  return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm

  return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);

# microblaze

Unless they're significantly more complicated than that, I don't see the point of deferring them.

...
This series' purpose actually always has been to move PCI functions to where they belong, i.e. from lib/ to drivers/pci.
I originally didn't want to touch pci_iounmap(), since I deemed it too complicated. Arnd pushed for unifying it.

Anyways, investing much more time into this is beyond my time budget. I only started this series to have a cleaner basis to do the devres functions.

So my suggestions is that we either go with this cleanup here, which improves the situation at least somewhat, or we simply drop patch #5 and leave pci_iounmap() as the last pci_ function in lib/

Second Resend. Would be cool if someone could tell me what I'll have to do so we can get this merged.

@Stable-Kernel:
You receive this patch series because its first patch fixes leaks in PCI.

Quoted 3 lines · Philipp Stanner

Second Resend. Would be cool if someone could tell me what I'll have to
do so we can get this merged.

I don't even know who'd merge it, but um doesn't seem appropriate...

Quoted 5 lines

@Stable-Kernel:
You receive this patch series because its first patch fixes leaks in
PCI.

Too early for that, stable just ignores stuff before it hits mainline.

johannes

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Philipp Stanner

Second Resend. Would be cool if someone could tell me what I'll have to
do so we can get this merged.

I don't even know who'd merge it, but um doesn't seem appropriate...

UM isn't a recipent, I'd guess it's some mail filter which might make it appear that way :)

The lists I sent this to are [email protected], [email protected], [email protected], [email protected]

Anyways, PCI is for sure who should merge this, since it's 100% about PCI.

Second Resend. Would be cool if someone could tell me what I'll have to do so we can get this merged. This is blocking the followup work I've got in the pipe

Philipp Stanner (5):
lib/pci_iomap.c: fix cleanup bugs in pci_iounmap()
lib: move pci_iomap.c to drivers/pci/
lib: move pci-specific devres code to drivers/pci/
pci: move devres code from pci.c to devres.c lib, pci: unify generic pci_iounmap()

 MAINTAINERS                            |   1 -
 drivers/pci/Kconfig                    |   5 +
 drivers/pci/Makefile                   |   3 +-
 drivers/pci/devres.c                   | 450 +++++++++++++++++++++++++
 lib/pci_iomap.c => drivers/pci/iomap.c |  49 +--
 drivers/pci/pci.c                      | 249 --------------
 drivers/pci/pci.h                      |  24 ++
 include/asm-generic/io.h               |  27 +-
 include/asm-generic/iomap.h            |  21 ++
 lib/Kconfig                            |   3 -
 lib/Makefile                           |   1 -
 lib/devres.c                           | 208 +-----------
 lib/iomap.c                            |  28 +-
 13 files changed, 566 insertions(+), 503 deletions(-)
 create mode 100644 drivers/pci/devres.c
 rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)

Quoted 5 lines · Philipp Stanner

Second Resend. Would be cool if someone could tell me what I'll have to
do so we can get this merged. This is blocking the followup work I've
got in the pipe

This seems PCI-focused, and I'll look at merging this after v6.8-rc1 is tagged and the merge window closes (probably Jan 21).  Then I'll rebase it to v6.8-rc1, tidy the subject lines to look like the rest of drivers/pci/, etc.

Quoted 25 lines

Philipp Stanner (5):
  lib/pci_iomap.c: fix cleanup bugs in pci_iounmap()
  lib: move pci_iomap.c to drivers/pci/
  lib: move pci-specific devres code to drivers/pci/
  pci: move devres code from pci.c to devres.c lib, pci: unify generic pci_iounmap()

 MAINTAINERS                            |   1 - drivers/pci/Kconfig                    |   5 + drivers/pci/Makefile                   |   3 +- drivers/pci/devres.c                   | 450
+++++++++++++++++++++++++
 lib/pci_iomap.c => drivers/pci/iomap.c |  49 +-- drivers/pci/pci.c                      | 249 -------------- drivers/pci/pci.h                      |  24 ++ include/asm-generic/io.h               |  27 +- include/asm-generic/iomap.h            |  21 ++ lib/Kconfig                            |   3 - lib/Makefile                           |   1 - lib/devres.c                           | 208 +----------- lib/iomap.c                            |  28 +-

 13 files changed, 566 insertions(+), 503 deletions(-)

 create mode 100644 drivers/pci/devres.c rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)