Re: Odd pci_iounmap() declaration rules..

Re: Odd pci_iounmap() declaration rules..

[Nathan Chancellor]

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

Re: Odd pci_iounmap() declaration rules..

[Linus Torvalds]

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

Re: Odd pci_iounmap() declaration rules..

[Linus Torvalds]

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

Re: Odd pci_iounmap() declaration rules..

[Linus Torvalds]

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

Re: Odd pci_iounmap() declaration rules..

[Nathan Chancellor]

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

Re: Odd pci_iounmap() declaration rules..

[Guenter Roeck]

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