Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

[Philipp Stanner]

On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote:
> On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote:
> > On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> > ...
> 
> > > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > > > +/**
> > > > + * pci_iounmap - Unmapp a mapping
> > > > + * @dev: PCI device the mapping belongs to
> > > > + * @addr: start address of the mapping
> > > > + *
> > > > + * Unmapp a PIO or MMIO mapping.
> > > > + */
> > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> > > 
> > > Maybe move the "p" to "addr" rename to the patch that fixes the
> > > pci_iounmap() #ifdef problem, since that's a trivial change that
> > > already has to do with handling both PIO and MMIO?  Then this
> > > patch
> > > would be a little more focused.
> > > 
> > > The kernel-doc addition could possibly also move there since it
> > > isn't
> > > related to the unification.
> > 
> > You mean the one from my devres-patch-series? Or documentation
> > specifically about pci_iounmap()?
> 
> I had in mind the patch that fixes the pci_iounmap() #ifdef problem,
> which (if you split it out from 1/5) would be a relatively trivial
> patch.  Or the kernel-doc addition could be its own separate patch.
> The point is that this unification patch is fairly complicated, so
> anything we can do to move things unrelated to unification elsewhere
> makes this one easier to review.

I think it should be a separate patch, then, as it doesn't belong by
100% to any of the patches here. If I had to pick one, I'd have
included the docu into patch #2 or #3.

Let's make it a separate one, following as a 6th patch in this series

> 
> > > It seems like implementing iomem_is_ioport() for the other arches
> > > would be straightforward and if done first, could make this patch
> > > look
> > > tidier.
> > 
> > That would be the cleanest solution. But the cleaner you want to
> > be,
> > the more time you have to spend ;)
> > I can take another look and see if I could do that with reasonable
> > effort.
> > Otherwise I'd go for:
> > 
> > > Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> > > addition could be done as a separate patch to make the
> > > unification
> > > more obvious.
> 
> It looks like iomem_is_ioport() is basically the guards in
> pci_iounmap() implementations that, if true, prevent calling
> iounmap(), so it it seems like they should be trivial, e.g.,
> 
>   return !__is_mmio(addr); # alpha
> 
>   return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm
> 
>   return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);
> # microblaze
> 
> Unless they're significantly more complicated than that, I don't see
> the point of deferring them.

Have you seen Arnd's reply from Friday?
Cleaning up Powerpc's use of lib/iomap.c will be significantly more
complicated.

This series' purpose actually always has been to move PCI functions to
where they belong, i.e. from lib/ to drivers/pci.
I originally didn't want to touch pci_iounmap(), since I deemed it too
complicated. Arnd pushed for unifying it.

Anyways, investing much more time into this is beyond my time budget. I
only started this series to have a cleaner basis to do the devres
functions.

So my suggestions is that we either go with this cleanup here, which
improves the situation at least somewhat, or we simply drop patch #5
and leave pci_iounmap() as the last pci_ function in lib/


P.

> 
> > > > + */
> > > > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> > > > +bool iomem_is_ioport(void __iomem *addr)
> > > >  {
> > > > -       IO_COND(addr, /* nothing */, iounmap(addr));
> > > > +       unsigned long port = (unsigned long __force)addr;
> > > > +
> > > > +       if (port > PIO_OFFSET && port < PIO_RESERVED)
> > > > +               return true;
> > > > +
> > > > +       return false;
> > > >  }
> > > > -EXPORT_SYMBOL(pci_iounmap);
> > > > -#endif /* CONFIG_PCI */
> > > > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> > > > -- 
> > > > 2.43.0
> > > > 
> > > 
> > 
>