| Message ID | 20200821002948.285988229@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | x86, PCI, XEN, genirq ...: Prepare for device MSI | expand |
On Fri, Aug 21, 2020 at 02:24:54AM +0200, Thomas Gleixner wrote: > If an architecture does not require the MSI setup/teardown fallback > functions, then allow them to be replaced by stub functions which emit a > warning. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org Acked-by: Bjorn Helgaas <bhelgaas@google.com> Question/comment below. > --- > drivers/pci/Kconfig | 3 +++ > drivers/pci/msi.c | 3 ++- > include/linux/msi.h | 31 ++++++++++++++++++++++++++----- > 3 files changed, 31 insertions(+), 6 deletions(-) > > --- 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_DISABLE_ARCH_FALLBACKS > + bool > + > config PCI_QUIRKS > default y > bool "Enable PCI quirk workarounds" if EXPERT > --- 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 > > +#ifndef CONFIG_PCI_MSI_DISABLE_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_DISABLE_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. > + * > + * They can be replaced by stubs with warnings via > + * CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS when the architecture fully > + * utilizes direct irqdomain based setup. Do you expect *all* arches to eventually use direct irqdomain setup? And in that case, to remove the config option? If not, it seems like it'd be nicer to have the burden on the arches that need/want to use arch-specific code instead of on the arches that do things generically. > */ > +#ifndef 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 Tue, Aug 25 2020 at 15:07, Bjorn Helgaas wrote: >> + * 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. >> + * >> + * They can be replaced by stubs with warnings via >> + * CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS when the architecture fully >> + * utilizes direct irqdomain based setup. > > Do you expect *all* arches to eventually use direct irqdomain setup? Ideally that happens some day. We have five left when x86 is converted: IA64, MIPS, POWERPC, S390, SPARC IA64 is unlikely to be fixed, but might be solved naturally by removal. For the others I don't know, but it's not on the horizon anytime soon I fear. > And in that case, to remove the config option? Yes, and all the code which depends on it. > If not, it seems like it'd be nicer to have the burden on the arches > that need/want to use arch-specific code instead of on the arches that > do things generically. Right, but they still share the common code there and some of them provide only parts of the weak callbacks. I'm not sure whether it's a good idea to copy all of this into each affected architecture. Or did you just mean that those architectures should select CONFIG_I_WANT_THE CRUFT instead of opting out on the fully irq domain based ones? Thanks, tglx
On Tue, Aug 25, 2020 at 11:28:30PM +0200, Thomas Gleixner wrote: > On Tue, Aug 25 2020 at 15:07, Bjorn Helgaas wrote: > >> + * 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. > >> + * > >> + * They can be replaced by stubs with warnings via > >> + * CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS when the architecture fully > >> + * utilizes direct irqdomain based setup. > > If not, it seems like it'd be nicer to have the burden on the arches > > that need/want to use arch-specific code instead of on the arches that > > do things generically. > > Right, but they still share the common code there and some of them > provide only parts of the weak callbacks. I'm not sure whether it's a > good idea to copy all of this into each affected architecture. > > Or did you just mean that those architectures should select > CONFIG_I_WANT_THE CRUFT instead of opting out on the fully irq domain > based ones? Yes, that was my real question -- can we confine the cruft in the crufty arches? If not, no big deal. Bjorn
On Tue, Aug 25 2020 at 16:35, Bjorn Helgaas wrote: > On Tue, Aug 25, 2020 at 11:28:30PM +0200, Thomas Gleixner wrote: >> >> Or did you just mean that those architectures should select >> CONFIG_I_WANT_THE CRUFT instead of opting out on the fully irq domain >> based ones? > > Yes, that was my real question -- can we confine the cruft in the > crufty arches? If not, no big deal. Should be doable. Let me try. Thanks, tglx
On Tue, Aug 25 2020 at 23:40, Thomas Gleixner wrote: > On Tue, Aug 25 2020 at 16:35, Bjorn Helgaas wrote: >> On Tue, Aug 25, 2020 at 11:28:30PM +0200, Thomas Gleixner wrote: >>> >>> Or did you just mean that those architectures should select >>> CONFIG_I_WANT_THE CRUFT instead of opting out on the fully irq domain >>> based ones? >> >> Yes, that was my real question -- can we confine the cruft in the >> crufty arches? If not, no big deal. > > Should be doable. Let me try. Bah. There is more cruft. The weak implementation has another way to go sideways via msi_controller::setup_irq[s] and msi_controller::teardown_irq drivers/pci/controller/pci-tegra.c drivers/pci/controller/pcie-rcar-host.c drivers/pci/controller/pcie-xilinx.c Groan....
--- 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_DISABLE_ARCH_FALLBACKS + bool + config PCI_QUIRKS default y bool "Enable PCI quirk workarounds" if EXPERT --- 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 +#ifndef CONFIG_PCI_MSI_DISABLE_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_DISABLE_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. + * + * They can be replaced by stubs with warnings via + * CONFIG_PCI_MSI_DISABLE_ARCH_FALLBACKS when the architecture fully + * utilizes direct irqdomain based setup. */ +#ifndef 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 {
If an architecture does not require the MSI setup/teardown fallback functions, then allow them to be replaced by stub functions which emit a warning. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org --- drivers/pci/Kconfig | 3 +++ drivers/pci/msi.c | 3 ++- include/linux/msi.h | 31 ++++++++++++++++++++++++++----- 3 files changed, 31 insertions(+), 6 deletions(-)