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

[Jiang Liu <jiang.liu@linux.intel.com>]

Hi Mika,
	I'm out of office until next Tuesday. Will handle this
when back to work. Could you please help to take a look at
mp_set_gsi_attr() to check whether it could be used to resolve
this issue.
Regards!
Gerry

On 2014/8/22 0:57, Mika Westerberg wrote:
> On Thu, Aug 21, 2014 at 05:17:29PM +0300, Mika Westerberg wrote:
>> 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.
> 
> There's also a bugzilla bug filed about this here where touchpad stopped
> working:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=82601
>