Re: [Patch V4 29/42] x86, irq: introduce two helper functions to support irqdomain map operation

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Mon, Jun 09, 2014 at 04:19:58PM +0800, Jiang Liu wrote:
> Currently there are multiple entries to program IOAPIC pins, such as
> io_apic_setup_irq_pin_once(), io_apic_set_pci_routing() and
> setup_IO_APIC_irq_extra() etc.
> 
> This patch introduces two functions to help consolidate the code to
> program IOAPIC pins. Function mp_set_pin_attr() is used to optionally
> set trigger, polarity and NUMA node property for an IOAPIC pin.
> If mp_set_pin_attr() is not invoked for a pin, the default configuration
> from BIOS will be used.
> 
> Function mp_irqdomain_map() is an common implementation of irqdomain map()
> operation. It figures out attribures for pin and then actually programs
> the IOAPIC pin. We hope this will be the only entrance for programming
> IOAPIC pin.
> 
> And the flow will:
> 1) caller such as xxx_pci_irq_enable figures out pin attributes.
> 2) Invoke mp_set_pin_attr() to set attributes for a pin. If the pin has
>    already bin programmed,  mp_set_pin_attr() will aslo detects attribute
>    confictions.
> 3) Invoke mp_map_pin_to_irq()
> 3.1) If IRQ has already been assigned, return irq_find_mapping()
> 3.2) Else irq_create_mapping()
> 		->irq_domain_associate()
> 			->mp_irqdomain_map()
> 				->io_apic_setup_irq_pin()
> 
> So every pin will only programmed once by mp_irqdomain_map(), so we
> could kill io_apic_setup_irq_pin_once(), io_apic_set_pci_routing() and
> setup_IO_APIC_irq_extra() etc.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  arch/x86/include/asm/io_apic.h |    5 ++
>  arch/x86/kernel/apic/io_apic.c |   99 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index 3e4bea3a52b1..c53587868590 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -173,7 +173,9 @@ enum ioapic_domain_type {
>  };
>  
>  struct device_node;
> +struct irq_domain;
>  struct irq_domain_ops;
> +
>  struct ioapic_domain_cfg {
>  	enum ioapic_domain_type		type;
>  	const struct irq_domain_ops	*ops;
> @@ -192,6 +194,9 @@ extern u32 mp_pin_to_gsi(int ioapic, int pin);
>  extern int mp_map_gsi_to_irq(u32 gsi, unsigned int flags);
>  extern void __init mp_register_ioapic(int id, u32 address, u32 gsi_base,
>  				      struct ioapic_domain_cfg *cfg);
> +extern int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
> +			    irq_hw_number_t hwirq);
> +extern int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node);
>  extern void __init pre_init_apic_IRQ0(void);
>  
>  extern void mp_save_irq(struct mpc_intsrc *m);
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 9c5f70699a80..61aae90f7e23 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -87,6 +87,14 @@ static DEFINE_RAW_SPINLOCK(vector_lock);
>  static DEFINE_MUTEX(ioapic_mutex);
>  static unsigned int ioapic_dynirq_base;
>  
> +struct mp_pin_info {
> +	int trigger;
> +	int polarity;
> +	int node;
> +	int set;
> +	u32 count;
> +};
> +
>  static struct ioapic {
>  	/*
>  	 * # of IRQ routing registers
> @@ -102,6 +110,7 @@ static struct ioapic {
>  	struct mp_ioapic_gsi  gsi_config;
>  	struct ioapic_domain_cfg irqdomain_cfg;
>  	struct irq_domain *irqdomain;
> +	struct mp_pin_info *pin_info;
>  	DECLARE_BITMAP(pin_programmed, MP_MAX_IOAPIC_PIN + 1);
>  } ioapics[MAX_IO_APICS];
>  
> @@ -147,6 +156,11 @@ static inline int mp_init_irq_at_boot(int ioapic, int irq)
>  	return ioapic == 0 || (irq >= 0 && irq < nr_legacy_irqs());
>  }
>  
> +static inline struct mp_pin_info *mp_pin_info(int ioapic_idx, int pin)
> +{
> +	return ioapics[ioapic_idx].pin_info + pin;
> +}
> +
>  static inline struct irq_domain *mp_ioapic_irqdomain(int ioapic)
>  {
>  	return ioapics[ioapic].irqdomain;
> @@ -1006,6 +1020,7 @@ static int mp_map_pin_to_irq(u32 gsi, int idx, int ioapic, int pin,
>  {
>  	int irq;
>  	struct irq_domain *domain = mp_ioapic_irqdomain(ioapic);
> +	struct mp_pin_info *info = mp_pin_info(ioapic, pin);
>  
>  	/*
>  	 * Don't use irqdomain to manage ISA IRQs because there may be
> @@ -1034,6 +1049,13 @@ static int mp_map_pin_to_irq(u32 gsi, int idx, int ioapic, int pin,
>  	irq = irq_find_mapping(domain, pin);
>  	if (irq <= 0 && (flags & IOAPIC_MAP_ALLOC))
>  		irq = alloc_irq_from_domain(domain, gsi, pin);
> +
> +	if (flags & IOAPIC_MAP_ALLOC) {
> +		if (irq > 0)
> +			info->count++;
> +		else if (info->count == 0)
> +			info->set = 0;
> +	}
>  	mutex_unlock(&ioapic_mutex);
>  
>  	return irq > 0 ? irq : -1;
> @@ -2923,18 +2945,27 @@ out:
>  
>  static int mp_irqdomain_create(int ioapic)
>  {
> +	size_t size;
>  	int hwirqs = mp_ioapic_pin_count(ioapic);
>  	struct ioapic *ip = &ioapics[ioapic];
>  	struct ioapic_domain_cfg *cfg = &ip->irqdomain_cfg;
>  	struct mp_ioapic_gsi *gsi_cfg = mp_ioapic_gsi_routing(ioapic);
>  
> +	size = sizeof(struct mp_pin_info) * mp_ioapic_pin_count(ioapic);
> +	ip->pin_info = kzalloc(size, GFP_KERNEL);
> +	if (!ip->pin_info)
> +		return -ENOMEM;
> +
>  	if (cfg->type == IOAPIC_DOMAIN_INVALID)
>  		return 0;
>  
>  	ip->irqdomain = irq_domain_add_linear(cfg->dev, hwirqs, cfg->ops,
>  					      (void *)(long)ioapic);
> -	if(!ip->irqdomain)
> +	if(!ip->irqdomain) {
> +		kfree(ip->pin_info);
> +		ip->pin_info = NULL;
>  		return -ENOMEM;
> +	}
>  
>  	if (cfg->type == IOAPIC_DOMAIN_LEGACY ||
>  	    cfg->type == IOAPIC_DOMAIN_STRICT)
> @@ -3898,6 +3929,72 @@ void __init mp_register_ioapic(int id, u32 address, u32 gsi_base,
>  	nr_ioapics++;
>  }
>  
> +int mp_irqdomain_map(struct irq_domain *domain, unsigned int virq,
> +		     irq_hw_number_t hwirq)
> +{
> +	int ioapic = (int)(long)domain->host_data;
> +	struct mp_pin_info *info = mp_pin_info(ioapic, hwirq);
> +	struct io_apic_irq_attr attr;
> +
> +	/*
> +	 * Skip the timer IRQ if there's a quirk handler installed and if it
> +	 * returns 1:
> +	 */
> +	if (apic->multi_timer_check &&
> +	    apic->multi_timer_check(ioapic, virq))
> +		return 0;
> +
> +	/* Get default attribute if not set by caller yet */
> +	if (!info->set) {
> +		u32 gsi = mp_pin_to_gsi(ioapic, hwirq);
> +
> +		if (acpi_get_override_irq(gsi, &info->trigger,
> +					  &info->polarity) < 0) {

Sadly this seems to break LPSS ACPI enumerated devices.

Before this change /proc/interrupts say:

   0:         14          0          0          0   IO-APIC-edge      timer
   1:         10          0          0          0   IO-APIC-edge      i8042
   4:         79          0          0          0   IO-APIC-edge      serial
   5:         52          0          0          0   IO-APIC-fasteoi   mmc0
   6:          0          0          0          0   IO-APIC-fasteoi   dw_dmac
   7:          0          0          0          0   IO-APIC-fasteoi   INT3432:00, INT3433:00
   8:          1          0          0          0   IO-APIC-edge      rtc0
   9:        174          0          0          0   IO-APIC-fasteoi   acpi
  57:        476          0          0          0   PCI-MSI-edge      xhci_hcd
  58:         17          0          0          0   PCI-MSI-edge      snd_hda_intel

After the change it looks like this:

  0:         14          0          0          0   IO-APIC-edge      timer
  1:         10          0          0          0   IO-APIC-edge      i8042
  4:         64          0          0          0   IO-APIC-edge      serial
  5:          0          0          0          0   IO-APIC-edge      mmc0
  6:          0          0          0          0   IO-APIC-edge      dw_dmac
  7:          0          0          0          0   IO-APIC-edge      INT3432:00, INT3433:00
  8:          1          0          0          0   IO-APIC-edge      rtc0
  9:        173          0          0          0   IO-APIC-fasteoi   acpi
 41:        471          0          0          0   PCI-MSI-edge      xhci_hcd
 42:         17          0          0          0   PCI-MSI-edge      snd_hda_intel

Notice the interrupt triggering of IRQs 5, 6, and 7 has changed from level to
edge. I also see this printed on the console:

[    1.676685] Failed to set pin attr for GSI6
[    1.691708] Failed to set pin attr for GSI7
[    1.706750] Failed to set pin attr for GSI7
[    1.721768] Failed to set pin attr for GSI13
[    1.736765] Failed to set pin attr for GSI5
[    1.838342] Failed to set pin attr for GSI6

Any ideas how to get that fixed?

We used to handle this in drivers/acpi/resource.c:acpi_dev_get_irqresource() so
that only IRQ() and IRQNoFlags() ACPI resources resort calling
acpi_get_override_irq(). Now that doesn't help anymore.

> +			/*
> +			 * PCI interrupts are always polarity one level
> +			 * triggered.
> +			 */
> +			info->trigger = 1;
> +			info->polarity = 1;
> +		}
> +		info->node = NUMA_NO_NODE;
> +		info->set = 1;
> +	}
> +	set_io_apic_irq_attr(&attr, ioapic, hwirq, info->trigger,
> +			     info->polarity);
> +
> +	return io_apic_setup_irq_pin(virq, info->node, &attr);
> +}