From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754244Ab0CVLOj (ORCPT ); Mon, 22 Mar 2010 07:14:39 -0400 Received: from www.tglx.de ([62.245.132.106]:46401 "EHLO www.tglx.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754263Ab0CVLOh (ORCPT ); Mon, 22 Mar 2010 07:14:37 -0400 Date: Mon, 22 Mar 2010 12:14:15 +0100 (CET) From: Thomas Gleixner To: Yinghai Lu cc: Ingo Molnar , "H. Peter Anvin" , Andrew Morton , "Eric W. Biederman" , Jesse Barnes , linux-kernel@vger.kernel.org, Thomas Renninger , Suresh Siddha , len.brown@intel.com Subject: Re: [PATCH 02/10] x86: fix out of order of gsi - full In-Reply-To: <1269221770-9667-3-git-send-email-yinghai@kernel.org> Message-ID: References: <1269221770-9667-1-git-send-email-yinghai@kernel.org> <1269221770-9667-3-git-send-email-yinghai@kernel.org> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 21 Mar 2010, Yinghai Lu wrote: > +extern int gsi_delta; Is this really necessary ? See below. > +int gsi_to_irq(unsigned int gsi); unsigned int please > +unsigned int irq_to_gsi(int irq); Ditto > @@ -446,11 +448,11 @@ void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger) > > int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > { > - *irq = gsi; > + *irq = gsi_to_irq(gsi); > > #ifdef CONFIG_X86_IO_APIC > if (acpi_irq_model == ACPI_IRQ_MODEL_IOAPIC) > - setup_IO_APIC_irq_extra(gsi); > + setup_IO_APIC_irq_extra(gsi, irq); Please do not propagate that pointer. *irq = setup_IO_APIC_irq_extra(gsi); Also these changes have a total lack of comments. Why do we modify *irq here depending on the setup_IO_APIC_irq_extra() results ? > #endif > > return 0; > @@ -914,6 +916,40 @@ static void save_mp_irq(struct mpc_intsrc *m) > panic("Max # of irq sources exceeded!!\n"); > } > > +/* By default isa irqs are identity mapped to gsis */ > +static unsigned int isa_irq_to_gsi[NR_IRQS_LEGACY] = { > + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 > +}; > + > +int gsi_delta; Please make this static and provide a function to modify it from probe_ioapic_i8259(). Also the variable name is horrible. What's the delta here ? It's an offset, right ? > +int gsi_to_irq(unsigned int gsi) unsigned int > +{ > + unsigned int irq = gsi; > + unsigned int i; > + > + irq += gsi_delta; Please make that: unsigned int i, irq = gsi + gsi_delta; > + for (i = 0; i < NR_IRQS_LEGACY; i++) { > + if (isa_irq_to_gsi[i] == gsi) { > + irq = i; > + break; > + } > + } > + > + return irq; > +} > + > +unsigned int irq_to_gsi(int irq) unsigned int please > +{ > + unsigned int gsi; > + > + if (irq < NR_IRQS_LEGACY) > + gsi = isa_irq_to_gsi[irq]; > + else > + gsi = irq - gsi_delta; > + > + return gsi; > +} > + > void __init mp_override_legacy_irq(u8 bus_irq, u8 polarity, u8 trigger, u32 gsi) > { > int ioapic; > @@ -945,6 +981,8 @@ void __init mp_override_legacy_irq(u8 bus_irq, u8 polarity, u8 trigger, u32 gsi) > mp_irq.dstirq = pin; /* INTIN# */ > > save_mp_irq(&mp_irq); > + > + isa_irq_to_gsi[bus_irq] = gsi; > } > > void __init mp_config_acpi_legacy_irqs(void) > @@ -974,7 +1012,7 @@ void __init mp_config_acpi_legacy_irqs(void) > /* > * Locate the IOAPIC that manages the ISA IRQs (0-15). > */ > - ioapic = mp_find_ioapic(0); > + ioapic = mp_find_ioapic(irq_to_gsi(0)); > if (ioapic < 0) > return; > dstapic = mp_ioapics[ioapic].apicid; > @@ -1057,6 +1095,7 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) Why is mp_register_gsi global ? It's only used in arch/x86/kernel/acpi/boot.c > { > int ioapic; > int ioapic_pin; > + int irq; irq is unsigned int. > struct io_apic_irq_attr irq_attr; > > if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) > @@ -1079,11 +1118,12 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) > gsi = ioapic_renumber_irq(ioapic, gsi); > #endif > > + irq = gsi_to_irq(gsi); > if (ioapic_pin > MP_MAX_IOAPIC_PIN) { > printk(KERN_ERR "Invalid reference to IOAPIC pin " > "%d-%d\n", mp_ioapics[ioapic].apicid, > ioapic_pin); > - return gsi; > + return irq; > } > > if (enable_update_mptable) > @@ -1092,9 +1132,9 @@ int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) > set_io_apic_irq_attr(&irq_attr, ioapic, ioapic_pin, > trigger == ACPI_EDGE_SENSITIVE ? 0 : 1, > polarity == ACPI_ACTIVE_HIGH ? 0 : 1); > - io_apic_set_pci_routing(dev, gsi, &irq_attr); > + io_apic_set_pci_routing(dev, irq, &irq_attr); > > - return gsi; > + return irq; > } > > /* > @@ -1151,8 +1191,10 @@ static int __init acpi_parse_madt_ioapic_entries(void) > * If BIOS did not supply an INT_SRC_OVR for the SCI > * pretend we got one so we can set the SCI flags. > */ > - if (!acpi_sci_override_gsi) > - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0); > + if (!acpi_sci_override_gsi) { > + int irq = gsi_to_irq(acpi_gbl_FADT.sci_interrupt); unsigned int. New line between variable declaration and code. > + acpi_sci_ioapic_setup(irq, acpi_gbl_FADT.sci_interrupt, 0, 0); > + } > > /* Fill in identity legacy mappings where no override */ > mp_config_acpi_legacy_irqs(); > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c > index a917fdf..61b59ef 100644 > --- a/arch/x86/kernel/apic/io_apic.c > +++ b/arch/x86/kernel/apic/io_apic.c > @@ -97,6 +97,8 @@ int mp_irq_entries; > /* GSI interrupts */ > static int nr_irqs_gsi = NR_IRQS_LEGACY; > > +static int boot_ioapic_idx; > + > #if defined (CONFIG_MCA) || defined (CONFIG_EISA) > int mp_bus_id_to_type[MAX_MP_BUSSES]; > #endif > @@ -1032,7 +1034,7 @@ static inline int irq_trigger(int idx) > int (*ioapic_renumber_irq)(int ioapic, int irq); > static int pin_2_irq(int idx, int apic, int pin) > { > - int irq, i; > + int irq; Can we please do a cleanup of irq variables to use unsigned int. That should be done as a separate patch. > int bus = mp_irqs[idx].srcbus; > > /* > @@ -1044,18 +1046,28 @@ static int pin_2_irq(int idx, int apic, int pin) > if (test_bit(bus, mp_bus_not_pci)) { > irq = mp_irqs[idx].srcbusirq; Can we simply return mp_irqs[idx].srcbusirq here and get rid of the else path ident level ? > } else { > - /* > - * PCI IRQs are mapped in order > - */ > - i = irq = 0; > - while (i < apic) > - irq += nr_ioapic_registers[i++]; > - irq += pin; > + unsigned int gsi; New line please > + if (!acpi_ioapic) { > + int i; > + /* > + * PCI IRQs are mapped in order > + */ > + i = gsi = 0; > + while (i < apic) > + gsi += nr_ioapic_registers[i++]; > + gsi += pin; > + } else > + gsi = pin + mp_gsi_routing[apic].gsi_base; > + > +#ifdef CONFIG_X86_32 > /* > * For MPS mode, so far only needed by ES7000 platform > */ > if (ioapic_renumber_irq) > - irq = ioapic_renumber_irq(apic, irq); > + gsi = ioapic_renumber_irq(apic, gsi); > +#endif > + > + irq = gsi_to_irq(gsi); > } > > #ifdef CONFIG_X86_32 > @@ -1505,9 +1517,10 @@ static void __init setup_IO_APIC_irqs(void) > struct irq_cfg *cfg; > int node = cpu_to_node(boot_cpu_id); > > + apic_id = boot_ioapic_idx; > + > apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n"); > > - for (apic_id = 0; apic_id < nr_ioapics; apic_id++) > for (pin = 0; pin < nr_ioapic_registers[apic_id]; pin++) { > idx = find_irq_entry(apic_id, pin, mp_INT); > if (idx == -1) { > @@ -1529,9 +1542,6 @@ static void __init setup_IO_APIC_irqs(void) > > irq = pin_2_irq(idx, apic_id, pin); > > - if ((apic_id > 0) && (irq > 16)) > - continue; > - > /* > * Skip the timer IRQ if there's a quirk handler > * installed and if it returns 1: > @@ -1565,7 +1575,7 @@ static void __init setup_IO_APIC_irqs(void) > * but could not use acpi_register_gsi() > * like some special sci in IBM x3330 > */ > -void setup_IO_APIC_irq_extra(u32 gsi) > +void setup_IO_APIC_irq_extra(u32 gsi, unsigned int *pirq) > { > int apic_id = 0, pin, idx, irq; > int node = cpu_to_node(boot_cpu_id); > @@ -1585,6 +1595,7 @@ void setup_IO_APIC_irq_extra(u32 gsi) > return; > > irq = pin_2_irq(idx, apic_id, pin); > + *pirq = irq; > #ifdef CONFIG_SPARSE_IRQ > desc = irq_to_desc(irq); > if (desc) > @@ -2028,6 +2039,30 @@ void __init enable_IO_APIC(void) > clear_IO_APIC(); > } > > +static void __init probe_ioapic_i8259(void) > +{ > + /* probe boot ioapic idx */ > + boot_ioapic_idx = ioapic_i8259.apic; boot_ioapic_idx is an apic id. Why is the variable name suggesting it's an index ? > + if (boot_ioapic_idx < 0) > + boot_ioapic_idx = find_isa_irq_apic(0, mp_INT); > +#ifdef CONFIG_ACPI > + if (!acpi_disabled && acpi_ioapic && boot_ioapic_idx < 0) > + boot_ioapic_idx = mp_find_ioapic(irq_to_gsi(0)); > +#endif > + if (boot_ioapic_idx < 0) > + boot_ioapic_idx = 0; > + > +#ifdef CONFIG_ACPI > + if (mp_gsi_routing[boot_ioapic_idx].gsi_base) { > + gsi_delta = NR_IRQS_LEGACY; > + nr_irqs_gsi += NR_IRQS_LEGACY; > + printk(KERN_DEBUG "new nr_irqs_gsi: %d\n", nr_irqs_gsi); > + } > +#endif > + > + printk(KERN_INFO "boot_ioapic_idx: %d\n", boot_ioapic_idx); > +} > + > /* > * Not an __init, needed by the reboot code > */ > @@ -3045,7 +3080,7 @@ static inline void __init check_timer(void) > legacy_pic->chip->unmask(0); > } > if (disable_timer_pin_1 > 0) > - clear_IO_APIC_pin(0, pin1); > + clear_IO_APIC_pin(apic1, pin1); How is this change related to this patch ? It looks more like an independent fix. Thanks, tglx