Subject: Re: [PATCHv8 01/10] PCI: use weak functions for MSI arch-specific functions

From: Benjamin Herrenschmidt <[email-masked]>
Date: Fri, 09 Aug 2013 08:39:27 +1000

On Fri, 2013-08-09 at 00:17 +0200, Thomas Petazzoni wrote:
> 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 architectures that
> were overidding those MSI functions to use the new strategy.
> 
> Note that we keep two separate, non-weak, functions
> default_teardown_msi_irqs() and default_restore_msi_irqs() for the
> default behavior of the arch_teardown_msi_irqs() and
> arch_restore_msi_irqs(), as the default behavior is needed by x86 PCI
> code.

Looks good, I'll give it a quick spin to make sure there is no
accidental breakage on ppc today.

> Signed-off-by: Thomas Petazzoni <[email-masked]>
> Acked-by: Bjorn Helgaas <[email-masked]>
> Tested-by: Daniel Price <[email-masked]>
> Tested-by: Thierry Reding <[email-masked]>

Acked-by: Benjamin Herrenschmidt <[email-masked]>

> Cc: Paul Mackerras <[email-masked]>
> Cc: [email-masked]
> Cc: Martin Schwidefsky <[email-masked]>
> Cc: Heiko Carstens <[email-masked]>
> Cc: [email-masked]
> Cc: [email-masked]
> Cc: Thomas Gleixner <[email-masked]>
> Cc: Ingo Molnar <[email-masked]>
> Cc: H. Peter Anvin <[email-masked]>
> Cc: [email-masked]
> Cc: Russell King <[email-masked]>
> Cc: Tony Luck <[email-masked]>
> Cc: Fenghua Yu <[email-masked]>
> Cc: [email-masked]
> Cc: Ralf Baechle <[email-masked]>
> Cc: [email-masked]
> Cc: David S. Miller <[email-masked]>
> Cc: [email-masked]
> Cc: Chris Metcalf <[email-masked]>
> ---
>  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              | 48 +++++++++++++++++++++---------------------
>  include/linux/msi.h            |  8 ++++++-
>  7 files changed, 52 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h
> index fa8e0aa..f194c08 100644
> --- a/arch/mips/include/asm/pci.h
> +++ b/arch/mips/include/asm/pci.h
> @@ -136,11 +136,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 6e577ba..262b91b 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 5f24c71..4c374a9 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 aca7578..823c386 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -30,20 +30,21 @@ 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 __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>  {
> -	return 0;
> +	return -EINVAL;
>  }
> -#endif
>  
> -#ifndef arch_setup_msi_irqs
> -# define arch_setup_msi_irqs default_setup_msi_irqs
> -# define HAVE_DEFAULT_MSI_SETUP_IRQS
> -#endif
> +void __weak arch_teardown_msi_irq(unsigned int irq)
> +{
> +}
>  
> -#ifdef HAVE_DEFAULT_MSI_SETUP_IRQS
> -int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +{
> +	return 0;
> +}
> +
> +int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
>  	struct msi_desc *entry;
>  	int ret;
> @@ -65,14 +66,11 @@ 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
>  
> -#ifdef HAVE_DEFAULT_MSI_TEARDOWN_IRQS
> +/*
> + * We have a default implementation available as a separate non-weak
> + * function, as it is used by the Xen x86 PCI code
> + */
>  void default_teardown_msi_irqs(struct pci_dev *dev)
>  {
>  	struct msi_desc *entry;
> @@ -89,14 +87,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;
> @@ -114,7 +110,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 ee66f3a..271dfd1 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -51,12 +51,18 @@ 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.
>   */
>  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);
> +
> +void default_teardown_msi_irqs(struct pci_dev *dev);
> +void default_restore_msi_irqs(struct pci_dev *dev, int irq);
>  
>  #endif /* LINUX_MSI_H */