linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Odd pci_iounmap() declaration rules..
@ 2021-09-18 20:38 Linus Torvalds
  2021-09-19 17:04 ` Helge Deller
  0 siblings, 1 reply; 9+ 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] 9+ messages in thread

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

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


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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-19 17:04 ` Helge Deller
@ 2021-09-19 18:05   ` Linus Torvalds
  2021-09-19 21:28     ` Nathan Chancellor
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2021-09-19 18:05 UTC (permalink / raw)
  To: Helge Deller
  Cc: Arnd Bergmann, Guenter Roeck, Ulrich Teichert, linux-arch,
	James Bottomley

On Sun, Sep 19, 2021 at 10:04 AM Helge Deller <deller@gmx.de> wrote:
>
> Can you test if it fixes your alpha build (with GENERIC_PCI_IOMAP=y) as
> well?

Yup. With this I can do that "enable GENERIC_PCI_IOMAP
unconditionally" thing on alpha, and the one off EISA/PCI driver now
builds cleanly without PCI.

I applied it directly (along with the alpha patch to GENERIC_PCI_IOMAP).

I have now looked at a number of drivers and architectures that I had
happily forgotten _all_ about long long ago.

It's been kind of fun, but I sure can't claim it has been really _productive_.

Thanks,

               Linus

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-19 18:05   ` Linus Torvalds
@ 2021-09-19 21:28     ` Nathan Chancellor
  2021-09-19 22:27       ` Linus Torvalds
  0 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2021-09-19 21:28 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Helge Deller, Arnd Bergmann, Guenter Roeck, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Sun, Sep 19, 2021 at 11:05:35AM -0700, Linus Torvalds wrote:
> On Sun, Sep 19, 2021 at 10:04 AM Helge Deller <deller@gmx.de> wrote:
> >
> > Can you test if it fixes your alpha build (with GENERIC_PCI_IOMAP=y) as
> > well?
> 
> Yup. With this I can do that "enable GENERIC_PCI_IOMAP
> unconditionally" thing on alpha, and the one off EISA/PCI driver now
> builds cleanly without PCI.
> 
> I applied it directly (along with the alpha patch to GENERIC_PCI_IOMAP).
> 
> I have now looked at a number of drivers and architectures that I had
> happily forgotten _all_ about long long ago.
> 
> It's been kind of fun, but I sure can't claim it has been really _productive_.

Commit 9caea0007601 ("parisc: Declare pci_iounmap() parisc version only
when CONFIG_PCI enabled") causes the following build error on arm64 with
Fedora's config, which CKI initially reported:

https://src.fedoraproject.org/rpms/kernel/raw/rawhide/f/kernel-aarch64-fedora.config
https://lore.kernel.org/r/cki.E3FB2299E5.UQ0I0LMEXJ@redhat.com/
https://arr-cki-prod-datawarehouse-public.s3.amazonaws.com/datawarehouse-public/2021/09/19/373372721/build_aarch64_redhat%3A1603258426/build.log

In file included from ./arch/arm64/include/asm/io.h:185,
                 from ./include/linux/io.h:13,
                 from ./include/acpi/acpi_io.h:5,
                 from ./include/linux/acpi.h:35,
                 from ./include/acpi/apei.h:9,
                 from ./include/acpi/ghes.h:5,
                 from ./include/linux/arm_sdei.h:8,
                 from arch/arm64/kernel/asm-offsets.c:10:
./include/asm-generic/io.h:1059:21: error: static declaration of 'pci_iounmap' follows non-static declaration
 1059 | #define pci_iounmap pci_iounmap
      |                     ^~~~~~~~~~~
./include/asm-generic/io.h:1060:20: note: in expansion of macro 'pci_iounmap'
 1060 | static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
      |                    ^~~~~~~~~~~
In file included from ./include/asm-generic/io.h:19,
                 from ./arch/arm64/include/asm/io.h:185,
                 from ./include/linux/io.h:13,
                 from ./include/acpi/acpi_io.h:5,
                 from ./include/linux/acpi.h:35,
                 from ./include/acpi/apei.h:9,
                 from ./include/acpi/ghes.h:5,
                 from ./include/linux/arm_sdei.h:8,
                 from arch/arm64/kernel/asm-offsets.c:10:
./include/asm-generic/pci_iomap.h:21:13: note: previous declaration of 'pci_iounmap' with type 'void(struct pci_dev *, void *)'
   21 | extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
      |             ^~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:121: arch/arm64/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1219: prepare0] Error 2
make[1]: Target 'all' not remade because of errors.
make: *** [Makefile:350: __build_one_by_one] Error 2
make: Target 'olddefconfig' not remade because of errors.
make: Target 'all' not remade because of errors.

Sorry, I do not have time at the moment to look at it (family Sunday and
whatnot) but I wanted to be sure you were aware of it.

Cheers,
Nathan

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-19 21:28     ` Nathan Chancellor
@ 2021-09-19 22:27       ` Linus Torvalds
  2021-09-19 22:44         ` Linus Torvalds
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2021-09-19 22:27 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Helge Deller, Arnd Bergmann, Guenter Roeck, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Sun, Sep 19, 2021 at 2:28 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Commit 9caea0007601 ("parisc: Declare pci_iounmap() parisc version only
> when CONFIG_PCI enabled") causes the following build error on arm64 with
> Fedora's config, which CKI initially reported:

Ok, so I spent a lot of time trying to figure out what the heck is going on.

And while this is really *REALLY* confusing code, and nobody should
use it, I think the fix is this oneliner:

  diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
  index e93375c710b9..692e964e56b4 100644
  --- a/include/asm-generic/io.h
  +++ b/include/asm-generic/io.h
  @@ -1047,7 +1047,7 @@ extern void ioport_unmap(void __iomem *p);
   #endif /* CONFIG_GENERIC_IOMAP */
   #endif /* CONFIG_HAS_IOPORT_MAP */

  -#ifndef CONFIG_GENERIC_IOMAP
  +#ifndef CONFIG_GENERIC_PCI_IOMAP
   struct pci_dev;
   extern void __iomem *pci_iomap(struct pci_dev *dev, int bar,
unsigned long max);

let me go test, I do have an arm64 build environment for this all, but
for that commit I had only done parisc, alpha and x86-64.

           Linus

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-19 22:27       ` Linus Torvalds
@ 2021-09-19 22:44         ` Linus Torvalds
  2021-09-20  0:44           ` Linus Torvalds
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2021-09-19 22:44 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Helge Deller, Arnd Bergmann, Guenter Roeck, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Sun, Sep 19, 2021 at 3:27 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>   --- a/include/asm-generic/io.h
>   +++ b/include/asm-generic/io.h
>   @@ -1047,7 +1047,7 @@ extern void ioport_unmap(void __iomem *p);
>   -#ifndef CONFIG_GENERIC_IOMAP
>   +#ifndef CONFIG_GENERIC_PCI_IOMAP
>
> let me go test, I do have an arm64 build environment for this all, but
> for that commit I had only done parisc, alpha and x86-64.

Hah. That conditional makes a lot more sense that way, but doing that
sensible thing exposes more oddities.

In particular, both arm and arm64 do

        select GENERIC_PCI_IOMAP

so they get the bog-standard pci_iomap() from lib/pci_iomap.c. All
good and sensible.

But is there a pci_iounmap() in lib/pci_iomap.c? No. The default
pci_iounmap() is in lib/iomap.c, which arm and arm64 do *not* use,
because they don't have GENERIC_IOMAP.

So those architectures depended on the header file copy, and with that
sane oneliner, the compile part of the build works fine, but then it
ends with

    undefined reference to `pci_iounmap'

because that didn't work out at all.

The fix seems to be to just move that odd code from the header file to
lib/pci_iomap.c, and that should make it all JustWork(tm).

That would at least make some of this make more sense. But this is all
a maze of random odd architecture things, so I'll have to look some
more at it.

            Linus

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-19 22:44         ` Linus Torvalds
@ 2021-09-20  0:44           ` Linus Torvalds
  2021-09-20 14:30             ` Nathan Chancellor
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2021-09-20  0:44 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Helge Deller, Arnd Bergmann, Guenter Roeck, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Sun, Sep 19, 2021 at 3:44 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The fix seems to be to just move that odd code from the header file to
> lib/pci_iomap.c, and that should make it all JustWork(tm).

I'm not 100% happy about the end result, and in particular I think
that the new generic pci_iounmap() function for the
ARCH_WANTS_GENERIC_PCI_IOUNMAP case should do the "iounmap(p)" thing
even if ARCH_HAS_GENERIC_IOPORT_MAP wasn't true, but I tried to keep
the old rules, even if they seemed broken.

arm and arm64 build for me, as did sparc64 and alpha. At least in the
configs I tested.

And the code _does_ make a bit more sense than it used to. It still
has crazy corners, but moving the pci_iounmap() code out of the header
file should make it easier to fix up in the future.

          Linus

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-20  0:44           ` Linus Torvalds
@ 2021-09-20 14:30             ` Nathan Chancellor
  2021-09-20 15:31               ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2021-09-20 14:30 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Helge Deller, Arnd Bergmann, Guenter Roeck, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Sun, Sep 19, 2021 at 05:44:03PM -0700, Linus Torvalds wrote:
> On Sun, Sep 19, 2021 at 3:44 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The fix seems to be to just move that odd code from the header file to
> > lib/pci_iomap.c, and that should make it all JustWork(tm).
> 
> I'm not 100% happy about the end result, and in particular I think
> that the new generic pci_iounmap() function for the
> ARCH_WANTS_GENERIC_PCI_IOUNMAP case should do the "iounmap(p)" thing
> even if ARCH_HAS_GENERIC_IOPORT_MAP wasn't true, but I tried to keep
> the old rules, even if they seemed broken.
> 
> arm and arm64 build for me, as did sparc64 and alpha. At least in the
> configs I tested.
> 
> And the code _does_ make a bit more sense than it used to. It still
> has crazy corners, but moving the pci_iounmap() code out of the header
> file should make it easier to fix up in the future.

Thanks, I can confirm that all of my build tests pass without any
issues now.

Cheers,
Nathan

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

* Re: Odd pci_iounmap() declaration rules..
  2021-09-20 14:30             ` Nathan Chancellor
@ 2021-09-20 15:31               ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2021-09-20 15:31 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Linus Torvalds, Helge Deller, Arnd Bergmann, Ulrich Teichert,
	linux-arch, James Bottomley, llvm

On Mon, Sep 20, 2021 at 07:30:15AM -0700, Nathan Chancellor wrote:
> On Sun, Sep 19, 2021 at 05:44:03PM -0700, Linus Torvalds wrote:
> > On Sun, Sep 19, 2021 at 3:44 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > The fix seems to be to just move that odd code from the header file to
> > > lib/pci_iomap.c, and that should make it all JustWork(tm).
> > 
> > I'm not 100% happy about the end result, and in particular I think
> > that the new generic pci_iounmap() function for the
> > ARCH_WANTS_GENERIC_PCI_IOUNMAP case should do the "iounmap(p)" thing
> > even if ARCH_HAS_GENERIC_IOPORT_MAP wasn't true, but I tried to keep
> > the old rules, even if they seemed broken.
> > 
> > arm and arm64 build for me, as did sparc64 and alpha. At least in the
> > configs I tested.
> > 
> > And the code _does_ make a bit more sense than it used to. It still
> > has crazy corners, but moving the pci_iounmap() code out of the header
> > file should make it easier to fix up in the future.
> 
> Thanks, I can confirm that all of my build tests pass without any
> issues now.
> 
I still see build failures for sparc64:allnoconfig and sparc64:tinyconfig.

Guenter

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

end of thread, other threads:[~2021-09-20 15:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-18 20:38 Odd pci_iounmap() declaration rules Linus Torvalds
2021-09-19 17:04 ` Helge Deller
2021-09-19 18:05   ` Linus Torvalds
2021-09-19 21:28     ` Nathan Chancellor
2021-09-19 22:27       ` Linus Torvalds
2021-09-19 22:44         ` Linus Torvalds
2021-09-20  0:44           ` Linus Torvalds
2021-09-20 14:30             ` Nathan Chancellor
2021-09-20 15:31               ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).