Regather scattered PCI-Code

-#ifdef CONFIG_PCI
/**

Quoted 3 lines · Philipp Stanner

-#ifdef CONFIG_PCI
 /**

You should not remove the #ifdef here, it probably results in a build failure when CONFIG_GENERIC_PCI_IOMAP is set and GENERIC_PCI is not.

Alternatively you could use Kconfig or Makefile logic to prevent the file from being built without CONFIG_PCI.

   Arnd

Quoted 8 lines · Arnd Bergmann

Quoted 3 lines · Philipp Stanner

-#ifdef CONFIG_PCI
 /**

You should not remove the #ifdef here, it probably results in a build failure when CONFIG_GENERIC_PCI_IOMAP is set and GENERIC_PCI is not.

CONFIG_PCI you mean.
Yes, that results in a build failure. That's what the Intel bots have reminded me of subtly before, which is why I:

Quoted 3 lines

Alternatively you could use Kconfig or Makefile logic to prevent the file from being built without CONFIG_PCI.

did exactly that in this very patch:

The pcim_*() functions in lib/devres.c are guarded by an #ifdef CONFIG_PCI and, thus, don't belong to this file. They are only ever used for pci and are not generic infrastructure.

Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c. Adjust the Makefile.

Suggested-by Danilo Krummrich <[email protected]>

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

---
 drivers/pci/Makefile |   2 +-
 drivers/pci/devres.c | 207 ++++++++++++++++++++++++++++++++++++++++++
 lib/devres.c         | 208 +------------------------------------------

Quoted 9 lines · Philipp Stanner

The pcim_*() functions in lib/devres.c are guarded by an #ifdef CONFIG_PCI and, thus, don't belong to this file. They are only ever used
for pci and are not generic infrastructure.

Move all pcim_*() functions in lib/devres.c to
drivers/pci/devres.c.
Adjust the Makefile.

Quoted 8 lines

Suggested-by Danilo Krummrich <[email protected]>

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

---
 drivers/pci/Makefile |   2 +-
 drivers/pci/devres.c | 207
++++++++++++++++++++++++++++++++++++++++++
 lib/devres.c         | 208 +--------------------------------------
----

I still think this should go into drivers/pci/pci_iomap.c along with the other functions.

     Arnd

The devres functions have different compile rules than the iomap functions have.

I would dislike it very much to need yet another preprocessor instruction, especially if we're talking about #ifdef PCI within the very PCI driver.

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 <[email protected]>

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

Quoted 18 lines · Philipp Stanner

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.

Quoted 11 lines

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.

Quoted 11 lines

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.

Quoted 4 lines

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

sense of it all")

Suggested-by Arnd Bergmann <[email protected]>

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

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.

Quoted 7 lines

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

I think the bugfix should be a separate patch so we can backport it to stable kernels.

Quoted 11 lines

include/asm-generic/io.h +6 -0

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

include/asm-generic/io.h extern void ioport_unmap(void __iomem *p); · line 1159

Quoted 4 lines

+               return false;

include/asm-generic/io.h +3 -0

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

include/asm-generic/io.h extern void ioport_unmap(void __iomem *p); · line 1162

Quoted 5 lines

+#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used.
*/

include/asm-generic/io.h +2 -0

+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

Quoted 13 lines · Arnd Bergmann

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;
}

Quoted 10 lines

include/asm-generic/io.h extern void ioport_unmap(void __iomem *p); · line 1162

Quoted 5 lines

+#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used.
*/

include/asm-generic/io.h +2 -0

+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?

Quoted 35 lines · Philipp Stanner

Quoted 13 lines · Arnd Bergmann

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;
}

Quoted 11 lines

include/asm-generic/io.h extern void ioport_unmap(void __iomem *p); · line 1162

Quoted 6 lines

+#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be
used.
*/

include/asm-generic/io.h +2 -0

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

Quoted 10 lines

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

Quoted 7 lines · Arnd Bergmann

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.

Arnd has suggested that architectures defining a custom inb() need their own iomem_is_ioport(), as well. I've grepped for inb() and found the following list of archs that define their own:
- alpha
- arm
- m68k <--
- parisc
- powerpc
- sh
- sparc
- x86 <--

All of those have their own definitons of pci_iounmap(). Therefore, they don't need our generic version in the first place and, thus, also need no iomem_is_ioport().

The two exceptions are x86 and m68k. The former uses lib/iomap.c through CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).

So as I see it, only m68k WOULD need its own custom definition of iomem_is_ioport(). But as I understand it it doesn't because it uses the one from asm-generic/pci_iomap.h ??

I wasn't entirely sure how to deal with the address ranges for the generic implementation in asm-generic/io.h. It's marked with a TODO. Input appreciated.

I removed the guard around define pci_iounmap in asm-generic/io.h. An alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no collision however, because generic pci_iounmap() from drivers/pci/iomap.c will only get pulled in when CONFIG_GENERIC_PCI_IOMAP is actually set.

Quoted 15 lines · Philipp Stanner

Arnd has suggested that architectures defining a custom inb() need their
own iomem_is_ioport(), as well. I've grepped for inb() and found the
following list of archs that define their own:
  - alpha
  - arm
  - m68k <--
  - parisc
  - powerpc
  - sh
  - sparc
  - x86 <--

Quoted 5 lines

All of those have their own definitons of pci_iounmap(). Therefore, they
don't need our generic version in the first place and, thus, also need
no iomem_is_ioport().

What I meant of course is that they should define iomem_is_ioport() in order to drop the custom pci_iounmap() and have only one remaining definition of that function left.

The one special case that I missed the last time is s390, which does not use GENERIC_PCI_IOMAP and will just require a separate copy of pci_iounmap() to go along with the is custom pci_iomap().

Quoted 10 lines

The two exceptions are x86 and m68k. The former uses lib/iomap.c through
CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion
(thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).

So as I see it, only m68k WOULD need its own custom definition of iomem_is_ioport(). But as I understand it it doesn't because it uses the
one from asm-generic/pci_iomap.h ??

At the moment, m68k gets the pci_iounmap() from lib/iomap.c if PCI is enabled for coldfire, but that incorrectly calls iounmap() on PCI_IO_PA if it gets passed a PIO address.

The version from asm-generic/io.h should fix this.

For classic m68k, there is no PCI, so nothing calls pci_iounmap().

Quoted 4 lines

I wasn't entirely sure how to deal with the address ranges for the generic implementation in asm-generic/io.h. It's marked with a TODO.
Input appreciated.

I commented on the function directly. To clarify, I think we should be able to directly turn each pci_iounmap() definition into a iomem_is_ioport() definition by keeping the logic unchanged and just return 'true' for the PIO variant or 'false' for the MMIO version.

Quoted 8 lines

I removed the guard around define pci_iounmap in asm-generic/io.h. An
alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and
CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no collision however, because generic pci_iounmap() from drivers/pci/iomap.c will only get pulled in when CONFIG_GENERIC_PCI_IOMAP is actually set.

The "#define pci_iomap" can be removed entirely I think.

     Arnd

Quoted 17 lines · Arnd Bergmann

On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:

The one special case that I missed the last time is s390, which does not use GENERIC_PCI_IOMAP and will just require a separate copy of pci_iounmap() to go along with the is custom pci_iomap().

Quoted 10 lines

The two exceptions are x86 and m68k. The former uses lib/iomap.c through
CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion
(thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).

So as I see it, only m68k WOULD need its own custom definition of iomem_is_ioport(). But as I understand it it doesn't because it uses the
one from asm-generic/pci_iomap.h ??

Quoted 5 lines

At the moment, m68k gets the pci_iounmap() from lib/iomap.c if PCI is enabled for coldfire, but that incorrectly calls iounmap() on PCI_IO_PA if it gets passed a PIO address.

The version from asm-generic/io.h should fix this.

So, to be sure: m68k will use the generic iomem_is_ioport() despite defining its own inb()?

Quoted 1 line

The "#define pci_iomap" can be removed entirely I think.

I also think it can, because first arch/asm/io.h includes asm- generic/io.h.
I was just wondering why many other functions in asm-generic/io.h always define their own names..

It's obviously very hard to test which config will break, so I thought it's better safe than sorry here