Re: Odd pci_iounmap() declaration rules..

[Helge Deller <deller@gmx.de>]

* Linus Torvalds <torvalds@linux-foundation.org>:
> 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 */