Odd pci_iounmap() declaration rules..

* Odd pci_iounmap() declaration rules..
@ 2021-09-18 20:38 Linus Torvalds
  2021-09-19 17:04 ` Helge Deller
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2021-09-18 20:38 UTC (permalink / raw)
  To: Arnd Bergmann, Helge Deller, Guenter Roeck, Ulrich Teichert; +Cc: linux-arch

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

                     Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread