| Message ID | 1371660979-21588-3-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jun 19, 2013 at 06:56:10PM +0200, Thomas Petazzoni wrote: [...] > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index 2c10752..4bc0c8f 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -30,19 +30,35 @@ static int pci_msi_enable = 1; > > /* Arch hooks */ > > -#ifndef arch_msi_check_device > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) > +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > +{ > + return -EINVAL; > +} > + > +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > +{ > + return default_setup_msi_irq(dev, desc); > +} > + > +void default_teardown_msi_irq(unsigned int irq) > +{ > +} > + > +void __weak arch_teardown_msi_irq(unsigned int irq) > +{ > + return default_teardown_msi_irq(irq); > +} > + > +int default_msi_check_device(struct pci_dev *dev, int nvec, int type) > { > return 0; > } > -#endif I don't think keeping the default_*() for these three is necessary, given that they don't do anything and therefore no architecture is likely to call them when overriding. Thierry
Dear Thierry Reding, On Thu, 20 Jun 2013 20:57:08 +0200, Thierry Reding wrote: > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > > index 2c10752..4bc0c8f 100644 > > --- a/drivers/pci/msi.c > > +++ b/drivers/pci/msi.c > > @@ -30,19 +30,35 @@ static int pci_msi_enable = 1; > > > > /* Arch hooks */ > > > > -#ifndef arch_msi_check_device > > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) > > +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > > +{ > > + return -EINVAL; > > +} > > + > > +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > > +{ > > + return default_setup_msi_irq(dev, desc); > > +} > > + > > +void default_teardown_msi_irq(unsigned int irq) > > +{ > > +} > > + > > +void __weak arch_teardown_msi_irq(unsigned int irq) > > +{ > > + return default_teardown_msi_irq(irq); > > +} > > + > > +int default_msi_check_device(struct pci_dev *dev, int nvec, int type) > > { > > return 0; > > } > > -#endif > > I don't think keeping the default_*() for these three is necessary, > given that they don't do anything and therefore no architecture is > likely to call them when overriding. Ok. I was just keeping them for the sake of consistency with the other calls that do have a default behavior, but if it's considered not useful, I'll get rid of them in v4. Thanks for your comments! Thomas
On Thu, Jun 20, 2013 at 12:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jun 19, 2013 at 06:56:10PM +0200, Thomas Petazzoni wrote: > [...] >> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c >> index 2c10752..4bc0c8f 100644 >> --- a/drivers/pci/msi.c >> +++ b/drivers/pci/msi.c >> @@ -30,19 +30,35 @@ static int pci_msi_enable = 1; >> >> /* Arch hooks */ >> >> -#ifndef arch_msi_check_device >> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) >> +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) >> +{ >> + return -EINVAL; >> +} >> + >> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) >> +{ >> + return default_setup_msi_irq(dev, desc); >> +} >> + >> +void default_teardown_msi_irq(unsigned int irq) >> +{ >> +} >> + >> +void __weak arch_teardown_msi_irq(unsigned int irq) >> +{ >> + return default_teardown_msi_irq(irq); >> +} >> + >> +int default_msi_check_device(struct pci_dev *dev, int nvec, int type) >> { >> return 0; >> } >> -#endif > > I don't think keeping the default_*() for these three is necessary, > given that they don't do anything and therefore no architecture is > likely to call them when overriding. I agree; the whole point of __weak is to provide a "default" implementation, so I hope you can just remove any empty default_*() functions and fold the others into the arch_*() functions directly. If there actually are callbacks from arch-specific strong functions back to the stuff in the default_*() functions, that suggests that we should refactor and rename that bit of functionality. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Bjorn Helgaas, On Mon, 24 Jun 2013 19:52:45 -0600, Bjorn Helgaas wrote: > I agree; the whole point of __weak is to provide a "default" > implementation, so I hope you can just remove any empty default_*() > functions and fold the others into the arch_*() functions directly. > > If there actually are callbacks from arch-specific strong functions > back to the stuff in the default_*() functions, that suggests that we > should refactor and rename that bit of functionality. There is one such case, as highlighted in the commit log of this patch: """ This is needed because the x86 Xen implementation of teardown_msi_irqs() needs to do some work, and then call the default implementation of this hook. """ Any suggestion on how to solve this particular case? Thanks, Thomas
On Tue, Jun 25, 2013 at 3:55 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Bjorn Helgaas, > > On Mon, 24 Jun 2013 19:52:45 -0600, Bjorn Helgaas wrote: > >> I agree; the whole point of __weak is to provide a "default" >> implementation, so I hope you can just remove any empty default_*() >> functions and fold the others into the arch_*() functions directly. >> >> If there actually are callbacks from arch-specific strong functions >> back to the stuff in the default_*() functions, that suggests that we >> should refactor and rename that bit of functionality. > > There is one such case, as highlighted in the commit log of this patch: > > """ > This is needed because the x86 Xen implementation of > teardown_msi_irqs() needs to do some work, and then call the default > implementation of this hook. > """ > > Any suggestion on how to solve this particular case? Nope. Just leave it as-is for that case and consolidate what you can. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Bjorn Helgaas, On Tue, 25 Jun 2013 10:20:33 -0600, Bjorn Helgaas wrote: > > Any suggestion on how to solve this particular case? > > Nope. Just leave it as-is for that case and consolidate what you can. So I keep the weak arch_*() function and the strong default_*() for this particular hook, and for all the other hooks, I only keep a weak arch_*() function. Is this what you suggest for now? Best regards, Thomas
On Tue, Jun 25, 2013 at 10:44 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Bjorn Helgaas, > > On Tue, 25 Jun 2013 10:20:33 -0600, Bjorn Helgaas wrote: > >> > Any suggestion on how to solve this particular case? >> >> Nope. Just leave it as-is for that case and consolidate what you can. > > So I keep the weak arch_*() function and the strong default_*() for > this particular hook, and for all the other hooks, I only keep a weak > arch_*() function. Is this what you suggest for now? Yep, that sounds good. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h index b8e24fd..031f4c1 100644 --- a/arch/mips/include/asm/pci.h +++ b/arch/mips/include/asm/pci.h @@ -137,11 +137,6 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) return channel ? 15 : 14; } -#ifdef CONFIG_CPU_CAVIUM_OCTEON -/* MSI arch hook for OCTEON */ -#define arch_setup_msi_irqs arch_setup_msi_irqs -#endif - extern char * (*pcibios_plat_setup)(char *str); #ifdef CONFIG_OF diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h index 6653f27..95145a1 100644 --- a/arch/powerpc/include/asm/pci.h +++ b/arch/powerpc/include/asm/pci.h @@ -113,11 +113,6 @@ extern int pci_domain_nr(struct pci_bus *bus); /* Decide whether to display the domain number in /proc */ extern int pci_proc_domain(struct pci_bus *bus); -/* MSI arch hooks */ -#define arch_setup_msi_irqs arch_setup_msi_irqs -#define arch_teardown_msi_irqs arch_teardown_msi_irqs -#define arch_msi_check_device arch_msi_check_device - struct vm_area_struct; /* Map a range of PCI memory or I/O space for a device into user space */ int pci_mmap_page_range(struct pci_dev *pdev, struct vm_area_struct *vma, diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 6c18012..8641e8d 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -21,10 +21,6 @@ void pci_iounmap(struct pci_dev *, void __iomem *); int pci_domain_nr(struct pci_bus *); int pci_proc_domain(struct pci_bus *); -/* MSI arch hooks */ -#define arch_setup_msi_irqs arch_setup_msi_irqs -#define arch_teardown_msi_irqs arch_teardown_msi_irqs - #define ZPCI_BUS_NR 0 /* default bus number */ #define ZPCI_DEVFN 0 /* default device number */ diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index d9e9e6c..8c61de0 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -100,29 +100,6 @@ static inline void early_quirks(void) { } extern void pci_iommu_alloc(void); #ifdef CONFIG_PCI_MSI -/* MSI arch specific hooks */ -static inline int x86_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) -{ - return x86_msi.setup_msi_irqs(dev, nvec, type); -} - -static inline void x86_teardown_msi_irqs(struct pci_dev *dev) -{ - x86_msi.teardown_msi_irqs(dev); -} - -static inline void x86_teardown_msi_irq(unsigned int irq) -{ - x86_msi.teardown_msi_irq(irq); -} -static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq) -{ - x86_msi.restore_msi_irqs(dev, irq); -} -#define arch_setup_msi_irqs x86_setup_msi_irqs -#define arch_teardown_msi_irqs x86_teardown_msi_irqs -#define arch_teardown_msi_irq x86_teardown_msi_irq -#define arch_restore_msi_irqs x86_restore_msi_irqs /* implemented in arch/x86/kernel/apic/io_apic. */ struct msi_desc; int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); @@ -130,11 +107,6 @@ void native_teardown_msi_irq(unsigned int irq); void native_restore_msi_irqs(struct pci_dev *dev, int irq); int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, unsigned int irq_base, unsigned int irq_offset); -/* default to the implementation in drivers/lib/msi.c */ -#define HAVE_DEFAULT_MSI_TEARDOWN_IRQS -#define HAVE_DEFAULT_MSI_RESTORE_IRQS -void default_teardown_msi_irqs(struct pci_dev *dev); -void default_restore_msi_irqs(struct pci_dev *dev, int irq); #else #define native_setup_msi_irqs NULL #define native_teardown_msi_irq NULL diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c index 45a14db..a2b189c 100644 --- a/arch/x86/kernel/x86_init.c +++ b/arch/x86/kernel/x86_init.c @@ -116,6 +116,27 @@ struct x86_msi_ops x86_msi = { .setup_hpet_msi = default_setup_hpet_msi, }; +/* MSI arch specific hooks */ +int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) +{ + return x86_msi.setup_msi_irqs(dev, nvec, type); +} + +void arch_teardown_msi_irqs(struct pci_dev *dev) +{ + x86_msi.teardown_msi_irqs(dev); +} + +void arch_teardown_msi_irq(unsigned int irq) +{ + x86_msi.teardown_msi_irq(irq); +} + +void arch_restore_msi_irqs(struct pci_dev *dev, int irq) +{ + x86_msi.restore_msi_irqs(dev, irq); +} + struct x86_io_apic_ops x86_io_apic_ops = { .init = native_io_apic_init_mappings, .read = native_io_apic_read, diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 2c10752..4bc0c8f 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -30,19 +30,35 @@ static int pci_msi_enable = 1; /* Arch hooks */ -#ifndef arch_msi_check_device -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) +{ + return -EINVAL; +} + +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) +{ + return default_setup_msi_irq(dev, desc); +} + +void default_teardown_msi_irq(unsigned int irq) +{ +} + +void __weak arch_teardown_msi_irq(unsigned int irq) +{ + return default_teardown_msi_irq(irq); +} + +int default_msi_check_device(struct pci_dev *dev, int nvec, int type) { return 0; } -#endif -#ifndef arch_setup_msi_irqs -# define arch_setup_msi_irqs default_setup_msi_irqs -# define HAVE_DEFAULT_MSI_SETUP_IRQS -#endif +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type) +{ + return default_msi_check_device(dev, nvec, type); +} -#ifdef HAVE_DEFAULT_MSI_SETUP_IRQS int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) { struct msi_desc *entry; @@ -65,14 +81,12 @@ int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) return 0; } -#endif -#ifndef arch_teardown_msi_irqs -# define arch_teardown_msi_irqs default_teardown_msi_irqs -# define HAVE_DEFAULT_MSI_TEARDOWN_IRQS -#endif +int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) +{ + return default_setup_msi_irqs(dev, nvec, type); +} -#ifdef HAVE_DEFAULT_MSI_TEARDOWN_IRQS void default_teardown_msi_irqs(struct pci_dev *dev) { struct msi_desc *entry; @@ -86,14 +100,12 @@ void default_teardown_msi_irqs(struct pci_dev *dev) arch_teardown_msi_irq(entry->irq + i); } } -#endif -#ifndef arch_restore_msi_irqs -# define arch_restore_msi_irqs default_restore_msi_irqs -# define HAVE_DEFAULT_MSI_RESTORE_IRQS -#endif +void __weak arch_teardown_msi_irqs(struct pci_dev *dev) +{ + return default_teardown_msi_irqs(dev); +} -#ifdef HAVE_DEFAULT_MSI_RESTORE_IRQS void default_restore_msi_irqs(struct pci_dev *dev, int irq) { struct msi_desc *entry; @@ -111,7 +123,11 @@ void default_restore_msi_irqs(struct pci_dev *dev, int irq) if (entry) write_msi_msg(irq, &entry->msg); } -#endif + +void __weak arch_restore_msi_irqs(struct pci_dev *dev, int irq) +{ + return default_restore_msi_irqs(dev, irq); +} static void msi_set_enable(struct pci_dev *dev, int enable) { diff --git a/include/linux/msi.h b/include/linux/msi.h index 20c2d6d..e792a3f 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -50,12 +50,25 @@ struct msi_desc { }; /* - * The arch hook for setup up msi irqs + * 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. In turn, such architecture + * specific code may be interested in calling back into the default + * implementation of those operations, in which case the non-weak + * default_*() variants can be used. */ 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); int arch_msi_check_device(struct pci_dev* dev, int nvec, int type); +void arch_restore_msi_irqs(struct pci_dev *dev, int irq); + +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc); +void default_teardown_msi_irq(unsigned int irq); +int default_msi_check_device(struct pci_dev *dev, int nvec, int type); +int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); +void default_teardown_msi_irqs(struct pci_dev *dev); +void default_restore_msi_irqs(struct pci_dev *dev, int irq); #endif /* LINUX_MSI_H */
Until now, the MSI architecture-specific functions could be overloaded using a fairly complex set of #define and compile-time conditionals. In order to prepare for the introduction of the msi_chip infrastructure, it is desirable to switch all those functions to use the 'weak' mechanism. This commit converts all the architecture that were overidding those MSI functions to use the new strategy. The new strategy consists of defining two functions for each hook: * One function is called default_<something>() and is not weak. It contains the default implementation of the hook. * One function is called arch_<something>() and is weak. The default implementation (in drivers/pci/msi.c) simply calls the corresponding default_<something>() function. The idea is that architecture-specific code can re-implement its own, non-weak, version of arch_<something>(). And to achieve this, it may wish to call back into the default implementation provided by the PCI core, which is possible thanks to the default_<something>() function. This is needed because the x86 Xen implementation of teardown_msi_irqs() needs to do some work, and then call the default implementation of this hook. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- arch/mips/include/asm/pci.h | 5 ---- arch/powerpc/include/asm/pci.h | 5 ---- arch/s390/include/asm/pci.h | 4 --- arch/x86/include/asm/pci.h | 28 -------------------- arch/x86/kernel/x86_init.c | 21 +++++++++++++++ drivers/pci/msi.c | 58 +++++++++++++++++++++++++++--------------- include/linux/msi.h | 15 ++++++++++- 7 files changed, 72 insertions(+), 64 deletions(-)