Re: Odd pci_iounmap() declaration rules..

* Linus Torvalds [off-list ref]:

So I was looking at a patch by Ulrich that reportedly got the alpha
'Jensen' platform compile going again, and while it didn't work for
me, a slightly modified one did get it mostly working. See commit
cc9d3aaa5331 ("alpha: make 'Jensen' IO functions build again") for
entirely irrelevant details.

Anyway, that particular case doesn't really matter, but the Jensen
configuration is somewhat interesting in that it doesn't have
CONFIG_PCI (and alpha doesn't do CONFIG_PM), and it turns out that
breaks some other things.  Some of them are just random driver issues,
not a big deal.

But one particular case is odd: "pci_iounmap()" behavior is all kinds of crazy.

Now, to put that in perspective, look at pci_iomap(), and it's
actually fairly normal, and handled in
include/asm-generic/pci_iomap.h, with a fairly sane

  #ifdef CONFIG_PCI
  .. real declaration ..
  #elif defined(CONFIG_GENERIC_PCI_IOMAP)
  .. empty declaration ..
  #endif

although you can kind of see some oddity there if you look at the
declaration of the __pci_ioport_map() thing for the special case of
port remapping. Whatever. On the whole, you have that "declare for
PCI, otherwise have empty declarations so that non-PCI systems can
still build cleanly".

Now, alpha makes the mistake of making that GENERIC_PCI_IOMAP thing
conditional on having PCI at all:

        select GENERIC_PCI_IOMAP if PCI

which then means that the "non-PCI systems can still build cleanly"
doesn't actually end up working, but whatever.  It does point out that
maybe that

  #elif defined(CONFIG_GENERIC_PCI_IOMAP)

should perhaps just be a #else. Because it's kind of silly to only
have those empty declarations if PCI is _not_ enabled, but
GENERIC_PCI_IOMAP is enabled. Most architectures seem to just select
GENERIC_PCI_IOMAP unconditionally, and it also gets selected magically
for you if you pick GENERIC_IOMAP.

So it's all a bit illogical, and slightyl complicated, but it doesn't
seem to be a huge deal.

What is *entirely* illogical is the state of "pci_iounmap()", though.

You'd think that it would mirror pci_iomap(), wouldn't you? The two go
literally hand-in-hand and pair up, after all.

But no. Not at all.

pci_iounmap() is decared not in include/asm-generic/pci_iomap.h
together with pci_iomap(), but in include/asm-generic/iomap.h.

And it has a different #ifdef too, doing

  #ifdef CONFIG_PCI
  .. delcaration..
  #elif defined(CONFIG_GENERIC_IOMAP)
  .. empty implementation ..
  #endif

which makes _no_ sense. Except it seems to be paired with this one in
asm-generic/io.h (!!):

  #ifndef CONFIG_GENERIC_IOMAP
  .. declaration  for pci_iomap() ..

  #ifndef pci_iounmap
  #define pci_iounmap pci_iounmap
  static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
  {
        __pci_ioport_unmap(p);
  }
  #endif
  #endif /* CONFIG_GENERIC_IOMAP */

which really makes no sense at all.

So there's GENERIC_IOMAP, and there is GENERIC_PCI_IOMAP, and the
_former_ modifies the behavior of "pci_iounmap()", but the _latter_
(more logically) modifies the behavior of "pci_iomap()".

I think this may at least partly just be a mistake. See commit
97a29d59fc22 ("[PARISC] fix compile break caused by iomap: make
IOPORT/PCI mapping functions conditional") which added those two
different CONFIG_ tests. The different config option kind of makes
sense in the context of which header file the declaration was in, but
I think _that_ in turn was just an older confusing mistake.

I'd like to fix some of the alpha issues by just making alpha do

        select GENERIC_PCI_IOMAP

unconditionally, so that if PCI isn't enabled, it gets the default
empty implementation.

But that doesn't work right now, because of the crazy situation with
pci_iounmap().

I think the right fix would be to(),

 (a) move pci_iounmap() declaration to
include/asm-generic/pci_iomap.h, and use the sane GENERIC_PCI_IOMAP
config option for it (or even better, remove that #elif entirely and
make it just #else

 (b) if you want to make your own pci_iounmap(), you just implement
your own one, and don't play games in the header file.

Hmm? Comments? Added random people who have been involved in this
(commit 97a29d59fc22 is from James, replaced him with Helge instead).

I do agree.

The attached patch implements (a) and (b) and builds and boots cleanly
on my non-PCI machine with CONFIG_PCI=n and GENERIC_PCI_IOMAP=y and I
assume it fixes your alpha build as well.

I assume the problem arised from the fact that pci_iounmap() was defined
on parisc unconditionally even for CONFIG_PCI=n.

Can you test if it fixes your alpha build (with GENERIC_PCI_IOMAP=y) as
well?

Helge
---
From 6f2f2855d05ca056481dbc446802826f70247b4c Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Sun, 19 Sep 2021 18:45:45 +0200
Subject: [PATCH] parisc: Declare pci_iounmap() parisc version only when
 CONFIG_PCI enabled

Linus noticed odd declaration rules for pci_iounmap() in iomap.h and
pci_iomap.h, where it was dependend on either
CONFIG_NO_GENERIC_PCI_IOPORT_MAP or CONFIG_GENERIC_IOMAP when CONFIG_PCI
was disabled.

Testing on parisc seems to indicate that we need pci_iounmap() only when
CONFIG_PCI is enabled, so the declaration of pci_iounmap() can be moved
cleanly into pci_iomap.h in sync with the declarations of pci_iomap().

Signed-off-by: Helge Deller <deller@gmx.de>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 97a29d59fc22 ("[PARISC] fix compile break caused by iomap: make IOPORT/PCI mapping functions conditional")

diff --git a/arch/parisc/lib/iomap.c b/arch/parisc/lib/iomap.c
index f03adb1999e7..367f6397bda7 100644
--- a/arch/parisc/lib/iomap.c
+++ b/arch/parisc/lib/iomap.c

@@ -513,12 +513,15 @@ void ioport_unmap(void __iomem *addr)
 	}
 }

+#ifdef CONFIG_PCI
 void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
 {
 	if (!INDIRECT_ADDR(addr)) {
 		iounmap(addr);
 	}
 }
+EXPORT_SYMBOL(pci_iounmap);
+#endif

 EXPORT_SYMBOL(ioread8);
 EXPORT_SYMBOL(ioread16);

@@ -544,4 +547,3 @@ EXPORT_SYMBOL(iowrite16_rep);
 EXPORT_SYMBOL(iowrite32_rep);
 EXPORT_SYMBOL(ioport_map);
 EXPORT_SYMBOL(ioport_unmap);
-EXPORT_SYMBOL(pci_iounmap);

diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 9b3eb6d86200..08237ae8b840 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h

@@ -110,16 +110,6 @@ static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)
 }
 #endif

-#ifdef CONFIG_PCI
-/* Destroy a virtual mapping cookie for a PCI BAR (memory or IO) */
-struct pci_dev;
-extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
-#elif defined(CONFIG_GENERIC_IOMAP)
-struct pci_dev;
-static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
-{ }
-#endif
-
 #include <asm-generic/pci_iomap.h>

 #endif

diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index df636c6d8e6c..5a2f9bf53384 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h

@@ -18,6 +18,7 @@ extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
 extern void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
 					unsigned long offset,
 					unsigned long maxlen);
+extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
 /* Create a virtual mapping cookie for a port on a given PCI device.
  * Do not call this directly, it exists to make it easier for architectures
  * to override */

@@ -50,6 +51,8 @@ static inline void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
 {
 	return NULL;
 }
+static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
+{ }
 #endif

 #endif /* __ASM_GENERIC_PCI_IOMAP_H */