| Message ID | 20231201121622.16343-1-pstanner@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Regather scattered PCI-Code | expand |
On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote: > > Arnd has suggested that architectures defining a custom inb() need their > own iomem_is_ioport(), as well. I've grepped for inb() and found the > following list of archs that define their own: > - alpha > - arm > - m68k <-- > - parisc > - powerpc > - sh > - sparc > - x86 <-- > > All of those have their own definitons of pci_iounmap(). Therefore, they > don't need our generic version in the first place and, thus, also need > no iomem_is_ioport(). What I meant of course is that they should define iomem_is_ioport() in order to drop the custom pci_iounmap() and have only one remaining definition of that function left. The one special case that I missed the last time is s390, which does not use GENERIC_PCI_IOMAP and will just require a separate copy of pci_iounmap() to go along with the is custom pci_iomap(). > The two exceptions are x86 and m68k. The former uses lib/iomap.c through > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard). > > So as I see it, only m68k WOULD need its own custom definition of > iomem_is_ioport(). But as I understand it it doesn't because it uses the > one from asm-generic/pci_iomap.h ?? At the moment, m68k gets the pci_iounmap() from lib/iomap.c if PCI is enabled for coldfire, but that incorrectly calls iounmap() on PCI_IO_PA if it gets passed a PIO address. The version from asm-generic/io.h should fix this. For classic m68k, there is no PCI, so nothing calls pci_iounmap(). > I wasn't entirely sure how to deal with the address ranges for the > generic implementation in asm-generic/io.h. It's marked with a TODO. > Input appreciated. I commented on the function directly. To clarify, I think we should be able to directly turn each pci_iounmap() definition into a iomem_is_ioport() definition by keeping the logic unchanged and just return 'true' for the PIO variant or 'false' for the MMIO version. > I removed the guard around define pci_iounmap in asm-generic/io.h. An > alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and > CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no > collision however, because generic pci_iounmap() from > drivers/pci/iomap.c will only get pulled in when > CONFIG_GENERIC_PCI_IOMAP is actually set. The "#define pci_iomap" can be removed entirely I think. Arnd
On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote: > On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote: > > > > Arnd has suggested that architectures defining a custom inb() need > > their > > own iomem_is_ioport(), as well. I've grepped for inb() and found > > the > > following list of archs that define their own: > > - alpha > > - arm > > - m68k <-- > > - parisc > > - powerpc > > - sh > > - sparc > > - x86 <-- > > > > All of those have their own definitons of pci_iounmap(). Therefore, > > they > > don't need our generic version in the first place and, thus, also > > need > > no iomem_is_ioport(). > > What I meant of course is that they should define iomem_is_ioport() > in order to drop the custom pci_iounmap() and have only one remaining > definition of that function left. Ah, gotcha! Yes, that would be neat. Would also allow for droping ARCH_WANTS_GENERIC_PCI_IOUNMAP. > > The one special case that I missed the last time is s390, which > does not use GENERIC_PCI_IOMAP and will just require a separate > copy of pci_iounmap() to go along with the is custom pci_iomap(). > > > The two exceptions are x86 and m68k. The former uses lib/iomap.c > > through > > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous > > discussion > > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard). > > > > So as I see it, only m68k WOULD need its own custom definition of > > iomem_is_ioport(). But as I understand it it doesn't because it > > uses the > > one from asm-generic/pci_iomap.h ?? > > At the moment, m68k gets the pci_iounmap() from lib/iomap.c > if PCI is enabled for coldfire, but that incorrectly calls > iounmap() on PCI_IO_PA if it gets passed a PIO address. > > The version from asm-generic/io.h should fix this. So, to be sure: m68k will use the generic iomem_is_ioport() despite defining its own inb()? > > For classic m68k, there is no PCI, so nothing calls pci_iounmap(). > > > I wasn't entirely sure how to deal with the address ranges for the > > generic implementation in asm-generic/io.h. It's marked with a > > TODO. > > Input appreciated. > > I commented on the function directly. To clarify, I think we should > be able to directly turn each pci_iounmap() definition into > a iomem_is_ioport() definition by keeping the logic unchanged > and just return 'true' for the PIO variant or 'false' for the MMIO > version. > > > I removed the guard around define pci_iounmap in asm-generic/io.h. > > An > > alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP > > and > > CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no > > collision however, because generic pci_iounmap() from > > drivers/pci/iomap.c will only get pulled in when > > CONFIG_GENERIC_PCI_IOMAP is actually set. > > The "#define pci_iomap" can be removed entirely I think. I also think it can, because first arch/asm/io.h includes asm- generic/io.h. I was just wondering why many other functions in asm-generic/io.h always define their own names.. It's obviously very hard to test which config will break, so I thought it's better safe than sorry here P. > > Arnd >
On Fri, Dec 1, 2023, at 20:09, Philipp Stanner wrote: > On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote: >> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote: >> >> The one special case that I missed the last time is s390, which >> does not use GENERIC_PCI_IOMAP and will just require a separate >> copy of pci_iounmap() to go along with the is custom pci_iomap(). >> >> > The two exceptions are x86 and m68k. The former uses lib/iomap.c >> > through >> > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous >> > discussion >> > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard). >> > >> > So as I see it, only m68k WOULD need its own custom definition of >> > iomem_is_ioport(). But as I understand it it doesn't because it >> > uses the >> > one from asm-generic/pci_iomap.h ?? >> >> At the moment, m68k gets the pci_iounmap() from lib/iomap.c >> if PCI is enabled for coldfire, but that incorrectly calls >> iounmap() on PCI_IO_PA if it gets passed a PIO address. >> >> The version from asm-generic/io.h should fix this. > > So, to be sure: m68k will use the generic iomem_is_ioport() despite > defining its own inb()? It depends, as m68k has two separate asm/io.h implementations: - arch/m68k/include/asm/io_no.h uses the default inb() from asm-generic/io.h, so it should use the asm-generic version of iomem_is_ioport(). - arch/m68k/include/asm/io_mm.h is rather special when it comes to inb()/outb(), but since there is no PCI, I would just use the default iomem_is_ioport() because it doesn't matter as long as there are no callers. If we ever need a working iomem_is_ioport() here, it would need the same special cases as isa_itb(). >> The "#define pci_iomap" can be removed entirely I think. > > I also think it can, because first arch/asm/io.h includes asm- > generic/io.h. > I was just wondering why many other functions in asm-generic/io.h > always define their own names.. > > It's obviously very hard to test which config will break, so I thought > it's better safe than sorry here I'm fairly sure it's not actually needed, but since the entire file does it, there is probably no harm keeping it consistent for the next added function. This is one more thing to maybe clean up eventually in the future, possibly as part of moving the contents of asm-generic/io.h into linux/io.h, which is something I'd like to do now that all architectures finally started using the asm-generic version. Arnd