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
