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

[Bjorn Helgaas]

On Mon, Jan 29, 2024 at 11:43:34AM +0100, Philipp Stanner wrote:
> 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

Sounds good.

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

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

OK, let's drop #5 for now.  It definitely seems like where we should
go in the future, but I think it will make more sense when we can
include a few of the simple conversions that will show how it all fits
together.

Bjorn