| Message ID | 20200826112333.992429909@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | x86, PCI, XEN, genirq ...: Prepare for device MSI | expand |
On Wed, Aug 26 2020 at 13:17, Thomas Gleixner wrote: > + * If CONFIG_PCI_MSI_ARCH_FALLBACKS is not selected they are replaced by > + * stubs with warnings. > */ > +#ifdef CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS Groan, I obviously failed to pull that back from the test box where I fixed it. That wants to be: +#ifdef CONFIG_PCI_MSI_ARCH_FALLBACKS Doing five things at once does not work well
On Wed, 26 Aug 2020 12:17:02 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > The arch_.*_msi_irq[s] fallbacks are compiled in whether an architecture > requires them or not. Architectures which are fully utilizing hierarchical > irq domains should never call into that code. > > It's not only architectures which depend on that by implementing one or > more of the weak functions, there is also a bunch of drivers which relies > on the weak functions which invoke msi_controller::setup_irq[s] and > msi_controller::teardown_irq. > > Make the architectures and drivers which rely on them select them in Kconfig > and if not selected replace them by stub functions which emit a warning and > fail the PCI/MSI interrupt allocation. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > V2: Make the architectures (and drivers) which need the fallbacks select them > and not the other way round (Bjorn). > --- > arch/ia64/Kconfig | 1 + > arch/mips/Kconfig | 1 + > arch/powerpc/Kconfig | 1 + > arch/s390/Kconfig | 1 + > arch/sparc/Kconfig | 1 + > arch/x86/Kconfig | 1 + > drivers/pci/Kconfig | 3 +++ > drivers/pci/controller/Kconfig | 3 +++ > drivers/pci/msi.c | 3 ++- > include/linux/msi.h | 31 ++++++++++++++++++++++++++----- > 10 files changed, 40 insertions(+), 6 deletions(-) > [...] > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -41,6 +41,7 @@ config PCI_TEGRA > bool "NVIDIA Tegra PCIe controller" > depends on ARCH_TEGRA || COMPILE_TEST > depends on PCI_MSI_IRQ_DOMAIN > + select PCI_MSI_ARCH_FALLBACKS > help > Say Y here if you want support for the PCIe host controller found > on NVIDIA Tegra SoCs. > @@ -67,6 +68,7 @@ config PCIE_RCAR_HOST > bool "Renesas R-Car PCIe host controller" > depends on ARCH_RENESAS || COMPILE_TEST > depends on PCI_MSI_IRQ_DOMAIN > + select PCI_MSI_ARCH_FALLBACKS > help > Say Y here if you want PCIe controller support on R-Car SoCs in host > mode. > @@ -103,6 +105,7 @@ config PCIE_XILINX_CPM > bool "Xilinx Versal CPM host bridge support" > depends on ARCH_ZYNQMP || COMPILE_TEST > select PCI_HOST_COMMON > + select PCI_MSI_ARCH_FALLBACKS This guy actually doesn't implement MSIs at all (it seems to delegate them to an ITS present in the system, if I read the DT binding correctly). However its older brother from the same silicon dealer seems to need it. The patchlet below should fix it. diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index 9ad13919bcaa..f56ff049d469 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -96,6 +96,7 @@ config PCI_HOST_GENERIC config PCIE_XILINX bool "Xilinx AXI PCIe host bridge support" + select PCI_MSI_ARCH_FALLBACKS depends on OF || COMPILE_TEST help Say 'Y' here if you want kernel to support the Xilinx AXI PCIe @@ -105,7 +106,6 @@ config PCIE_XILINX_CPM bool "Xilinx Versal CPM host bridge support" depends on ARCH_ZYNQMP || COMPILE_TEST select PCI_HOST_COMMON - select PCI_MSI_ARCH_FALLBACKS help Say 'Y' here if you want kernel support for the Xilinx Versal CPM host bridge. With that fixed, Acked-by: Marc Zyngier <maz@kernel.org> M.
On Wed, Aug 26 2020 at 22:14, Marc Zyngier wrote: > On Wed, 26 Aug 2020 12:17:02 +0100, > Thomas Gleixner <tglx@linutronix.de> wrote: >> @@ -103,6 +105,7 @@ config PCIE_XILINX_CPM >> bool "Xilinx Versal CPM host bridge support" >> depends on ARCH_ZYNQMP || COMPILE_TEST >> select PCI_HOST_COMMON >> + select PCI_MSI_ARCH_FALLBACKS > > This guy actually doesn't implement MSIs at all (it seems to delegate > them to an ITS present in the system, if I read the DT binding > correctly). However its older brother from the same silicon dealer > seems to need it. The patchlet below should fix it. Gah, at some point my eyes went squared and I lost track..
[+cc Rob, cover https://lore.kernel.org/r/20200826111628.794979401@linutronix.de/ this https://lore.kernel.org/r/20200826112333.992429909@linutronix.de/] On Wed, Aug 26, 2020 at 01:17:02PM +0200, Thomas Gleixner wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > The arch_.*_msi_irq[s] fallbacks are compiled in whether an architecture > requires them or not. Architectures which are fully utilizing hierarchical > irq domains should never call into that code. > > It's not only architectures which depend on that by implementing one or > more of the weak functions, there is also a bunch of drivers which relies > on the weak functions which invoke msi_controller::setup_irq[s] and > msi_controller::teardown_irq. > > Make the architectures and drivers which rely on them select them in Kconfig > and if not selected replace them by stub functions which emit a warning and > fail the PCI/MSI interrupt allocation. Sorry, I really don't understand this, so these are probably stupid questions. If CONFIG_PCI_MSI_ARCH_FALLBACKS is defined, we will supply implementations of: arch_setup_msi_irq arch_teardown_msi_irq arch_setup_msi_irqs arch_teardown_msi_irqs default_teardown_msi_irqs # non-weak You select CONFIG_PCI_MSI_ARCH_FALLBACKS for ia64, mips, powerpc, s390, sparc, and x86. I see that all of those arches implement at least one of the functions above. But x86 doesn't and I can't figure out why it needs to select CONFIG_PCI_MSI_ARCH_FALLBACKS. I assume there's a way to convert these arches to hierarchical irq domains so they wouldn't need this at all? Is there a sample conversion to look at? And I can't figure out what's special about tegra, rcar, and xilinx that makes them need it as well. Is there something I could grep for to identify them? Is there a way to convert them so they don't need it? > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -193,17 +193,38 @@ void pci_msi_mask_irq(struct irq_data *d > void pci_msi_unmask_irq(struct irq_data *data); > > /* > - * The arch hooks to setup up msi irqs. Those functions are > - * implemented as weak symbols so that they /can/ be overriden by > - * architecture specific code if needed. > + * The arch hooks to setup up msi irqs. Default functions are implemented s/msi/MSI/ to match the one below. > + * as weak symbols so that they /can/ be overriden by architecture specific > + * code if needed. These hooks must be enabled by the architecture or by > + * drivers which depend on them via msi_controller based MSI handling.
On Thu, Aug 27, 2020 at 01:20:40PM -0500, Bjorn Helgaas wrote: [...] > And I can't figure out what's special about tegra, rcar, and xilinx > that makes them need it as well. Is there something I could grep for > to identify them? Is there a way to convert them so they don't need > it? I think DT binding and related firmware support are needed to setup the MSI IRQ domains correctly, there is nothing special about tegra, rcar and xilinx AFAIK (well, all native host controllers MSI handling is *special* just to be polite but let's gloss over this for the time being). struct msi_controller, to answer the first question. I have doubts about pci_mvebu too, they do allocate an msi_controller but without methods so it looks pretty much useless. Hyper-V code too seems questionable, maybe there is room for more clean-ups. Lorenzo > > --- a/include/linux/msi.h > > +++ b/include/linux/msi.h > > @@ -193,17 +193,38 @@ void pci_msi_mask_irq(struct irq_data *d > > void pci_msi_unmask_irq(struct irq_data *data); > > > > /* > > - * The arch hooks to setup up msi irqs. Those functions are > > - * implemented as weak symbols so that they /can/ be overriden by > > - * architecture specific code if needed. > > + * The arch hooks to setup up msi irqs. Default functions are implemented > > s/msi/MSI/ to match the one below. > > > + * as weak symbols so that they /can/ be overriden by architecture specific > > + * code if needed. These hooks must be enabled by the architecture or by > > + * drivers which depend on them via msi_controller based MSI handling.
On Fri, Aug 28, 2020 at 12:21:42PM +0100, Lorenzo Pieralisi wrote: > On Thu, Aug 27, 2020 at 01:20:40PM -0500, Bjorn Helgaas wrote: > > [...] > > > And I can't figure out what's special about tegra, rcar, and xilinx > > that makes them need it as well. Is there something I could grep for > > to identify them? Is there a way to convert them so they don't need > > it? > > I think DT binding and related firmware support are needed to setup the > MSI IRQ domains correctly, there is nothing special about tegra, rcar > and xilinx AFAIK (well, all native host controllers MSI handling is > *special* just to be polite but let's gloss over this for the time > being). > > struct msi_controller, to answer the first question. > > I have doubts about pci_mvebu too, they do allocate an msi_controller > but without methods so it looks pretty much useless. Oh, I did once know things about mvebu.. I suspect the msi controller pointer assignment is dead code at this point. The only implementation of MSI with that PCI root port is drivers/irqchip/irq-armada-370-xp.c which looks like it uses irq_domain. Actually looks like things are very close to eliminating msi_controller. This is dead code, can't find a setter for hw_pci->msi_ctrl: arch/arm/include/asm/mach/pci.h: struct msi_controller *msi_ctrl; arch/arm/kernel/bios32.c: bridge->msi = hw->msi_ctrl; This is probably just copying NULL from one place to another: drivers/pci/controller/pci-mvebu.c: struct msi_controller *msi; These need conversion to irq_domain (right?): drivers/pci/controller/pci-hyperv.c: struct msi_controller msi_chip; drivers/pci/controller/pci-tegra.c: struct msi_controller chip; drivers/pci/controller/pcie-rcar-host.c: struct msi_controller chip; drivers/pci/controller/pcie-xilinx.c:static struct msi_controller xilinx_pcie_msi_chip = { Then the stuff in drivers/pci/msi.c can go away. So the arch_setup_msi_irq/etc is not really an arch hook, but some infrastructure to support those 4 PCI root port drivers. Jason
Hi Jason, On 2020-08-28 13:19, Jason Gunthorpe wrote: > On Fri, Aug 28, 2020 at 12:21:42PM +0100, Lorenzo Pieralisi wrote: >> On Thu, Aug 27, 2020 at 01:20:40PM -0500, Bjorn Helgaas wrote: >> >> [...] >> >> > And I can't figure out what's special about tegra, rcar, and xilinx >> > that makes them need it as well. Is there something I could grep for >> > to identify them? Is there a way to convert them so they don't need >> > it? >> >> I think DT binding and related firmware support are needed to setup >> the >> MSI IRQ domains correctly, there is nothing special about tegra, rcar >> and xilinx AFAIK (well, all native host controllers MSI handling is >> *special* just to be polite but let's gloss over this for the time >> being). >> >> struct msi_controller, to answer the first question. >> >> I have doubts about pci_mvebu too, they do allocate an msi_controller >> but without methods so it looks pretty much useless. > > Oh, I did once know things about mvebu.. > > I suspect the msi controller pointer assignment is dead code at this > point. The only implementation of MSI with that PCI root port is > drivers/irqchip/irq-armada-370-xp.c which looks like it uses > irq_domain. > > Actually looks like things are very close to eliminating > msi_controller. > > This is dead code, can't find a setter for hw_pci->msi_ctrl: > > arch/arm/include/asm/mach/pci.h: struct msi_controller > *msi_ctrl; > arch/arm/kernel/bios32.c: bridge->msi = > hw->msi_ctrl; > > This is probably just copying NULL from one place to another: > > drivers/pci/controller/pci-mvebu.c: struct msi_controller *msi; > > These need conversion to irq_domain (right?): > > drivers/pci/controller/pci-hyperv.c: struct msi_controller msi_chip; > drivers/pci/controller/pci-tegra.c: struct msi_controller chip; > drivers/pci/controller/pcie-rcar-host.c: struct msi_controller > chip; > drivers/pci/controller/pcie-xilinx.c:static struct msi_controller > xilinx_pcie_msi_chip = { > > Then the stuff in drivers/pci/msi.c can go away. > > So the arch_setup_msi_irq/etc is not really an arch hook, but some > infrastructure to support those 4 PCI root port drivers. I happen to have a *really old* patch addressing Tegra [1], which I was never able to test (no HW). Rebasing it shouldn't be too hard, and maybe you can find someone internally willing to give it a spin? That'd be a good start towards the removal of this cruft. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/kill-msi-controller&id=83b3602fcee7972b9d549ed729b56ec28de16081
On Fri, Aug 28, 2020 at 01:47:59PM +0100, Marc Zyngier wrote: > > So the arch_setup_msi_irq/etc is not really an arch hook, but some > > infrastructure to support those 4 PCI root port drivers. > > I happen to have a *really old* patch addressing Tegra [1], which > I was never able to test (no HW). Rebasing it shouldn't be too hard, > and maybe you can find someone internally willing to give it a spin? Sure, that helps a bunch, I will ask internally if someone in that BU can take a look. Thanks, Jason
On 2020-08-28 13:54, Jason Gunthorpe wrote: > On Fri, Aug 28, 2020 at 01:47:59PM +0100, Marc Zyngier wrote: > >> > So the arch_setup_msi_irq/etc is not really an arch hook, but some >> > infrastructure to support those 4 PCI root port drivers. >> >> I happen to have a *really old* patch addressing Tegra [1], which >> I was never able to test (no HW). Rebasing it shouldn't be too hard, >> and maybe you can find someone internally willing to give it a spin? > > Sure, that helps a bunch, I will ask internally if someone in that BU > can take a look. I just rebased[1] the patch onto -rc2, and provided a TODO list in the commit message. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/tegra-msi
On Thu, Aug 27 2020 at 13:20, Bjorn Helgaas wrote: > On Wed, Aug 26, 2020 at 01:17:02PM +0200, Thomas Gleixner wrote: >> Make the architectures and drivers which rely on them select them in Kconfig >> and if not selected replace them by stub functions which emit a warning and >> fail the PCI/MSI interrupt allocation. > > Sorry, I really don't understand this, so these are probably stupid > questions. > > If CONFIG_PCI_MSI_ARCH_FALLBACKS is defined, we will supply > implementations of: > > arch_setup_msi_irq > arch_teardown_msi_irq > arch_setup_msi_irqs > arch_teardown_msi_irqs > default_teardown_msi_irqs # non-weak > > You select CONFIG_PCI_MSI_ARCH_FALLBACKS for ia64, mips, powerpc, > s390, sparc, and x86. I see that all of those arches implement at > least one of the functions above. But x86 doesn't and I can't figure > out why it needs to select CONFIG_PCI_MSI_ARCH_FALLBACKS. X86 still has them at that point in the series and the next patch removes them. I wanted to have the warnings in place before doing so. > I assume there's a way to convert these arches to hierarchical irq > domains so they wouldn't need this at all? Is there a sample > conversion to look at? For a quick and dirty step it's pretty much the wrapper I used for XEN and then make sure that the msi_domain pointer is populated is pci_device::device. > And I can't figure out what's special about tegra, rcar, and xilinx > that makes them need it as well. Those are old drivers from the time where ARM did not use hierarchical irq domains and nobody cared to fix them up. > Is there something I could grep for > to identify them? git grep arch_setup_msi_irq git grep arch_teardown_msi_irq > Is there a way to convert them so they don't need it? Sure, it just needs some work and probably hardware to test. Thanks, tglx
On Wed, 2020-08-26 at 13:17 +0200, Thomas Gleixner wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > The arch_.*_msi_irq[s] fallbacks are compiled in whether an architecture > requires them or not. Architectures which are fully utilizing hierarchical > irq domains should never call into that code. > > It's not only architectures which depend on that by implementing one or > more of the weak functions, there is also a bunch of drivers which relies > on the weak functions which invoke msi_controller::setup_irq[s] and > msi_controller::teardown_irq. > > Make the architectures and drivers which rely on them select them in Kconfig > and if not selected replace them by stub functions which emit a warning and > fail the PCI/MSI interrupt allocation. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Today's linux-next will have some warnings on s390x: .config: https://gitlab.com/cailca/linux-mm/-/blob/master/s390.config WARNING: unmet direct dependencies detected for PCI_MSI_ARCH_FALLBACKS Depends on [n]: PCI [=n] Selected by [y]: - S390 [=y] WARNING: unmet direct dependencies detected for PCI_MSI_ARCH_FALLBACKS Depends on [n]: PCI [=n] Selected by [y]: - S390 [=y] > --- > V2: Make the architectures (and drivers) which need the fallbacks select them > and not the other way round (Bjorn). > --- > arch/ia64/Kconfig | 1 + > arch/mips/Kconfig | 1 + > arch/powerpc/Kconfig | 1 + > arch/s390/Kconfig | 1 + > arch/sparc/Kconfig | 1 + > arch/x86/Kconfig | 1 + > drivers/pci/Kconfig | 3 +++ > drivers/pci/controller/Kconfig | 3 +++ > drivers/pci/msi.c | 3 ++- > include/linux/msi.h | 31 ++++++++++++++++++++++++++----- > 10 files changed, 40 insertions(+), 6 deletions(-) > > --- a/arch/ia64/Kconfig > +++ b/arch/ia64/Kconfig > @@ -56,6 +56,7 @@ config IA64 > select NEED_DMA_MAP_STATE > select NEED_SG_DMA_LENGTH > select NUMA if !FLATMEM > + select PCI_MSI_ARCH_FALLBACKS > default y > help > The Itanium Processor Family is Intel's 64-bit successor to > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -86,6 +86,7 @@ config MIPS > select MODULES_USE_ELF_REL if MODULES > select MODULES_USE_ELF_RELA if MODULES && 64BIT > select PERF_USE_VMALLOC > + select PCI_MSI_ARCH_FALLBACKS > select RTC_LIB > select SYSCTL_EXCEPTION_TRACE > select VIRT_TO_BUS > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -246,6 +246,7 @@ config PPC > select OLD_SIGACTION if PPC32 > select OLD_SIGSUSPEND > select PCI_DOMAINS if PCI > + select PCI_MSI_ARCH_FALLBACKS > select PCI_SYSCALL if PCI > select PPC_DAWR if PPC64 > select RTC_LIB > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -185,6 +185,7 @@ config S390 > select OLD_SIGSUSPEND3 > select PCI_DOMAINS if PCI > select PCI_MSI if PCI > + select PCI_MSI_ARCH_FALLBACKS > select SPARSE_IRQ > select SYSCTL_EXCEPTION_TRACE > select THREAD_INFO_IN_TASK > --- a/arch/sparc/Kconfig > +++ b/arch/sparc/Kconfig > @@ -43,6 +43,7 @@ config SPARC > select GENERIC_STRNLEN_USER > select MODULES_USE_ELF_RELA > select PCI_SYSCALL if PCI > + select PCI_MSI_ARCH_FALLBACKS > select ODD_RT_SIGACTION > select OLD_SIGSUSPEND > select CPU_NO_EFFICIENT_FFS > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -225,6 +225,7 @@ config X86 > select NEED_SG_DMA_LENGTH > select PCI_DOMAINS if PCI > select PCI_LOCKLESS_CONFIG if PCI > + select PCI_MSI_ARCH_FALLBACKS > select PERF_EVENTS > select RTC_LIB > select RTC_MC146818_LIB > --- a/drivers/pci/Kconfig > +++ b/drivers/pci/Kconfig > @@ -56,6 +56,9 @@ config PCI_MSI_IRQ_DOMAIN > depends on PCI_MSI > select GENERIC_MSI_IRQ_DOMAIN > > +config PCI_MSI_ARCH_FALLBACKS > + bool > + > config PCI_QUIRKS > default y > bool "Enable PCI quirk workarounds" if EXPERT > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -41,6 +41,7 @@ config PCI_TEGRA > bool "NVIDIA Tegra PCIe controller" > depends on ARCH_TEGRA || COMPILE_TEST > depends on PCI_MSI_IRQ_DOMAIN > + select PCI_MSI_ARCH_FALLBACKS > help > Say Y here if you want support for the PCIe host controller found > on NVIDIA Tegra SoCs. > @@ -67,6 +68,7 @@ config PCIE_RCAR_HOST > bool "Renesas R-Car PCIe host controller" > depends on ARCH_RENESAS || COMPILE_TEST > depends on PCI_MSI_IRQ_DOMAIN > + select PCI_MSI_ARCH_FALLBACKS > help > Say Y here if you want PCIe controller support on R-Car SoCs in host > mode. > @@ -103,6 +105,7 @@ config PCIE_XILINX_CPM > bool "Xilinx Versal CPM host bridge support" > depends on ARCH_ZYNQMP || COMPILE_TEST > select PCI_HOST_COMMON > + select PCI_MSI_ARCH_FALLBACKS > help > Say 'Y' here if you want kernel support for the > Xilinx Versal CPM host bridge. > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -58,8 +58,8 @@ static void pci_msi_teardown_msi_irqs(st > #define pci_msi_teardown_msi_irqs arch_teardown_msi_irqs > #endif > > +#ifdef CONFIG_PCI_MSI_ARCH_FALLBACKS > /* Arch hooks */ > - > int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > { > struct msi_controller *chip = dev->bus->msi; > @@ -132,6 +132,7 @@ void __weak arch_teardown_msi_irqs(struc > { > return default_teardown_msi_irqs(dev); > } > +#endif /* CONFIG_PCI_MSI_ARCH_FALLBACKS */ > > static void default_restore_msi_irq(struct pci_dev *dev, int irq) > { > --- a/include/linux/msi.h > +++ b/include/linux/msi.h > @@ -193,17 +193,38 @@ void pci_msi_mask_irq(struct irq_data *d > void pci_msi_unmask_irq(struct irq_data *data); > > /* > - * The arch hooks to setup up msi irqs. Those functions are > - * implemented as weak symbols so that they /can/ be overriden by > - * architecture specific code if needed. > + * The arch hooks to setup up msi irqs. Default functions are implemented > + * as weak symbols so that they /can/ be overriden by architecture specific > + * code if needed. These hooks must be enabled by the architecture or by > + * drivers which depend on them via msi_controller based MSI handling. > + * > + * If CONFIG_PCI_MSI_ARCH_FALLBACKS is not selected they are replaced by > + * stubs with warnings. > */ > +#ifdef CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS > int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc); > void arch_teardown_msi_irq(unsigned int irq); > int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); > void arch_teardown_msi_irqs(struct pci_dev *dev); > -void arch_restore_msi_irqs(struct pci_dev *dev); > - > void default_teardown_msi_irqs(struct pci_dev *dev); > +#else > +static inline int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int > type) > +{ > + WARN_ON_ONCE(1); > + return -ENODEV; > +} > + > +static inline void arch_teardown_msi_irqs(struct pci_dev *dev) > +{ > + WARN_ON_ONCE(1); > +} > +#endif > + > +/* > + * The restore hooks are still available as they are useful even > + * for fully irq domain based setups. Courtesy to XEN/X86. > + */ > +void arch_restore_msi_irqs(struct pci_dev *dev); > void default_restore_msi_irqs(struct pci_dev *dev); > > struct msi_controller { >
On Fri, Sep 25, 2020 at 09:54:52AM -0400, Qian Cai wrote: > On Wed, 2020-08-26 at 13:17 +0200, Thomas Gleixner wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > > > The arch_.*_msi_irq[s] fallbacks are compiled in whether an architecture > > requires them or not. Architectures which are fully utilizing hierarchical > > irq domains should never call into that code. > > > > It's not only architectures which depend on that by implementing one or > > more of the weak functions, there is also a bunch of drivers which relies > > on the weak functions which invoke msi_controller::setup_irq[s] and > > msi_controller::teardown_irq. > > > > Make the architectures and drivers which rely on them select them in Kconfig > > and if not selected replace them by stub functions which emit a warning and > > fail the PCI/MSI interrupt allocation. > > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > Today's linux-next will have some warnings on s390x: > > .config: https://gitlab.com/cailca/linux-mm/-/blob/master/s390.config > > WARNING: unmet direct dependencies detected for PCI_MSI_ARCH_FALLBACKS > Depends on [n]: PCI [=n] > Selected by [y]: > - S390 [=y] > > WARNING: unmet direct dependencies detected for PCI_MSI_ARCH_FALLBACKS > Depends on [n]: PCI [=n] > Selected by [y]: > - S390 [=y] > Yes, as well as on mips and sparc which also don't FORCE_PCI. This seems to work for s390: diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index b0b7acf07eb8..41136fbe909b 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -192,3 +192,3 @@ config S390 select PCI_MSI if PCI - select PCI_MSI_ARCH_FALLBACKS + select PCI_MSI_ARCH_FALLBACKS if PCI select SET_FS
On Sat, Sep 26 2020 at 14:38, Vasily Gorbik wrote: > On Fri, Sep 25, 2020 at 09:54:52AM -0400, Qian Cai wrote: > Yes, as well as on mips and sparc which also don't FORCE_PCI. > This seems to work for s390: > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > index b0b7acf07eb8..41136fbe909b 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -192,3 +192,3 @@ config S390 > select PCI_MSI if PCI > - select PCI_MSI_ARCH_FALLBACKS > + select PCI_MSI_ARCH_FALLBACKS if PCI > select SET_FS lemme fix that for all of them ...
--- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -56,6 +56,7 @@ config IA64 select NEED_DMA_MAP_STATE select NEED_SG_DMA_LENGTH select NUMA if !FLATMEM + select PCI_MSI_ARCH_FALLBACKS default y help The Itanium Processor Family is Intel's 64-bit successor to --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -86,6 +86,7 @@ config MIPS select MODULES_USE_ELF_REL if MODULES select MODULES_USE_ELF_RELA if MODULES && 64BIT select PERF_USE_VMALLOC + select PCI_MSI_ARCH_FALLBACKS select RTC_LIB select SYSCTL_EXCEPTION_TRACE select VIRT_TO_BUS --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -246,6 +246,7 @@ config PPC select OLD_SIGACTION if PPC32 select OLD_SIGSUSPEND select PCI_DOMAINS if PCI + select PCI_MSI_ARCH_FALLBACKS select PCI_SYSCALL if PCI select PPC_DAWR if PPC64 select RTC_LIB --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -185,6 +185,7 @@ config S390 select OLD_SIGSUSPEND3 select PCI_DOMAINS if PCI select PCI_MSI if PCI + select PCI_MSI_ARCH_FALLBACKS select SPARSE_IRQ select SYSCTL_EXCEPTION_TRACE select THREAD_INFO_IN_TASK --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -43,6 +43,7 @@ config SPARC select GENERIC_STRNLEN_USER select MODULES_USE_ELF_RELA select PCI_SYSCALL if PCI + select PCI_MSI_ARCH_FALLBACKS select ODD_RT_SIGACTION select OLD_SIGSUSPEND select CPU_NO_EFFICIENT_FFS --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -225,6 +225,7 @@ config X86 select NEED_SG_DMA_LENGTH select PCI_DOMAINS if PCI select PCI_LOCKLESS_CONFIG if PCI + select PCI_MSI_ARCH_FALLBACKS select PERF_EVENTS select RTC_LIB select RTC_MC146818_LIB --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -56,6 +56,9 @@ config PCI_MSI_IRQ_DOMAIN depends on PCI_MSI select GENERIC_MSI_IRQ_DOMAIN +config PCI_MSI_ARCH_FALLBACKS + bool + config PCI_QUIRKS default y bool "Enable PCI quirk workarounds" if EXPERT --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -41,6 +41,7 @@ config PCI_TEGRA bool "NVIDIA Tegra PCIe controller" depends on ARCH_TEGRA || COMPILE_TEST depends on PCI_MSI_IRQ_DOMAIN + select PCI_MSI_ARCH_FALLBACKS help Say Y here if you want support for the PCIe host controller found on NVIDIA Tegra SoCs. @@ -67,6 +68,7 @@ config PCIE_RCAR_HOST bool "Renesas R-Car PCIe host controller" depends on ARCH_RENESAS || COMPILE_TEST depends on PCI_MSI_IRQ_DOMAIN + select PCI_MSI_ARCH_FALLBACKS help Say Y here if you want PCIe controller support on R-Car SoCs in host mode. @@ -103,6 +105,7 @@ config PCIE_XILINX_CPM bool "Xilinx Versal CPM host bridge support" depends on ARCH_ZYNQMP || COMPILE_TEST select PCI_HOST_COMMON + select PCI_MSI_ARCH_FALLBACKS help Say 'Y' here if you want kernel support for the Xilinx Versal CPM host bridge. --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -58,8 +58,8 @@ static void pci_msi_teardown_msi_irqs(st #define pci_msi_teardown_msi_irqs arch_teardown_msi_irqs #endif +#ifdef CONFIG_PCI_MSI_ARCH_FALLBACKS /* Arch hooks */ - int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) { struct msi_controller *chip = dev->bus->msi; @@ -132,6 +132,7 @@ void __weak arch_teardown_msi_irqs(struc { return default_teardown_msi_irqs(dev); } +#endif /* CONFIG_PCI_MSI_ARCH_FALLBACKS */ static void default_restore_msi_irq(struct pci_dev *dev, int irq) { --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -193,17 +193,38 @@ void pci_msi_mask_irq(struct irq_data *d void pci_msi_unmask_irq(struct irq_data *data); /* - * The arch hooks to setup up msi irqs. Those functions are - * implemented as weak symbols so that they /can/ be overriden by - * architecture specific code if needed. + * The arch hooks to setup up msi irqs. Default functions are implemented + * as weak symbols so that they /can/ be overriden by architecture specific + * code if needed. These hooks must be enabled by the architecture or by + * drivers which depend on them via msi_controller based MSI handling. + * + * If CONFIG_PCI_MSI_ARCH_FALLBACKS is not selected they are replaced by + * stubs with warnings. */ +#ifdef CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc); void arch_teardown_msi_irq(unsigned int irq); int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); void arch_teardown_msi_irqs(struct pci_dev *dev); -void arch_restore_msi_irqs(struct pci_dev *dev); - void default_teardown_msi_irqs(struct pci_dev *dev); +#else +static inline int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) +{ + WARN_ON_ONCE(1); + return -ENODEV; +} + +static inline void arch_teardown_msi_irqs(struct pci_dev *dev) +{ + WARN_ON_ONCE(1); +} +#endif + +/* + * The restore hooks are still available as they are useful even + * for fully irq domain based setups. Courtesy to XEN/X86. + */ +void arch_restore_msi_irqs(struct pci_dev *dev); void default_restore_msi_irqs(struct pci_dev *dev); struct msi_controller {