Re: [PATCH 02/10] x86: fix out of order of gsi - full

From: Yinghai Lu
Date: Mon Mar 22 2010 - 15:47:37 EST

Next message: Avi Kivity: "Re: [RFC] Unify KVM kernel-space and user-space code into a singleproject"
Previous message: Avi Kivity: "Re: [RFC] Unify KVM kernel-space and user-space code into a singleproject"
In reply to: Thomas Gleixner: "Re: [PATCH 02/10] x86: fix out of order of gsi - full"
Next in thread: Yinghai Lu: "[PATCH 05/10] x86: use vector_desc instead of vector_irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 03/22/2010 04:14 AM, Thomas Gleixner wrote:
> 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

ok

>
>> @@ -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 ?
>
looks *irq = gsi_to_irq(gsi) should already do the work for us.
>> #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 ?

yes. offset for non legacy irq.

>
>> +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;

ok

>
>> + 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

ok

>
>> +{
>> + 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

yes.

should adjust that function position.

arch/x86/kernel/acpi/boot.c: plat_gsi = mp_register_gsi(dev, gsi, trigger, polarity);
arch/x86/kernel/acpi/boot.c:int mp_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)

>
>> {
>> 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.

ok

>
>> 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 ?

it is 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.

ok another patch.

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Avi Kivity: "Re: [RFC] Unify KVM kernel-space and user-space code into a singleproject"
Previous message: Avi Kivity: "Re: [RFC] Unify KVM kernel-space and user-space code into a singleproject"
In reply to: Thomas Gleixner: "Re: [PATCH 02/10] x86: fix out of order of gsi - full"
Next in thread: Yinghai Lu: "[PATCH 05/10] x86: use vector_desc instead of vector_irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]