Re: [PATCH v2 4/4] lib, pci: unify generic pci_iounmap()

[Philipp Stanner <pstanner@redhat.com>]

On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> > The implementation of pci_iounmap() is currently scattered over two
> > files, drivers/pci/iounmap.c and lib/iomap.c. Additionally,
> > architectures can define their own version.
> > 
> > Besides one unified version being desirable in the first place, the
> > old
> > version in drivers/pci/iounmap.c contained a bug and could leak
> > memory
> > mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> > should not
> > have guarded iounmap(p); in addition to the preceding code.
> > 
> > To have only one version, it's necessary to create a helper
> > function,
> > iomem_is_ioport(), that tells pci_iounmap() whether the passed
> > address
> > points to an ioport or normal memory.
> > 
> > iomem_is_ioport() can be provided through three different ways:
> >   1. The architecture itself provides it.
> >   2. As a default version in include/asm-generic/io.h for those
> >      architectures that don't use CONFIG_GENERIC_IOMAP, but also
> > don't
> >      provide their own version of iomem_is_ioport().
> >   3. As a default version in lib/iomap.c for those architectures
> > that
> >      define and use CONFIG_GENERIC_IOMAP (currently, only x86
> > really
> >      uses the functions in lib/iomap.c)
> 
> I would count 3 as a special case of 1 here.

ACK

> 
> > Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> > Provide the function iomem_is_ioport() in include/asm-generic/io.h
> > and
> > lib/iomap.c.
> > 
> > Remove the CONFIG_GENERIC_IOMAP guard around
> > ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set
> > CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the
> > function.
> > 
> > Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make
> > sense of it all")
> > Suggested-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> 
> Looks good overall. It would be nice to go further than this
> and replace all the custom pci_iounmap() variants with custom
> iomem_is_ioport() implementations, but that can be a follow-up
> along with removing the incorrect or useless 'select GENERIC_IOMAP'
> parts.

Yes, let's schedule that for a follow up. The way my project plans
sound currently, it's likely that I'll stay close to PCI for the next
months anyways, so it's likely we'll get an opportunity to pick this up
on the run

> 
> >                 return;
> > -       iounmap(p);
> > +       }
> >  #endif
> > +       iounmap(addr);
> >  }
> 
> I think the bugfix should be a separate patch so we can backport
> it to stable kernels.

ACK, good idea

> 
> > +#ifndef CONFIG_GENERIC_IOMAP
> > +static inline bool iomem_is_ioport(void __iomem *addr)
> > +{
> > +       unsigned long port = (unsigned long __force)addr;
> > +
> > +       // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE
> > into account
> > +       // similar as in ioport_map() ?
> > +
> > +       if (port > MMIO_UPPER_LIMIT)
> > +               return false;
> > +
> > +       return true;
> > +}
> 
> This has to have the exact logic that was present in the
> old pci_iounmap(). For the default version that is currently
> in lib/pci_iomap.c, this means something along the linens of

OK, I see, so iomem_is_ioport() takes the form derived from
lib/pci_iomap.c for asm-generic/io.h, and the form of lib/iomap.c for
the one in lib/iomap.c (obviously)

> 
> static inline bool struct iomem_is_ioport(void __iomem *p)
> {
> #ifdef CONFIG_HAS_IOPORT
>         uintptr_t start = (uintptr_t) PCI_IOBASE;
>         uintptr_t addr = (uintptr_t) p;
> 
>         if (addr >= start && addr < start + IO_SPACE_LIMIT)
>                 return true;
> #endif
>         return false;
> }
> 
> > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be
> > used. 
> > */
> > +bool iomem_is_ioport(void __iomem *addr);
> > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
> 
> I'm not sure what this macro is for, since it appears to
> do the opposite of what its name suggests: rather than
> provide the generic version of iomem_is_ioport(), it
> skips that and provides a custom one to go with lib/iomap.c

Hmmm well now it's getting tricky.

This else-branch is the one where CONFIG_GENERIC_IOMAP is actually set.

I think we're running into the "generic not being generic now that IA64
has died" problem you were hinting at.

If we build for x86 and have CONFIG_GENERIC set, only then do we want
iomem_is_ioport() from lib/iomap.c. So the macro serves avoiding a
collision between symbols. Because lib/iomap.c might be compiled even
if someone else already has defined iomem_is_ioport().
I also don't like it, but it was the least bad solution I could come up
with
Suggestions?


P.

> 
>      Arnd
>