lkml.org 
[lkml]   [2009]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4] x86/pci: update pirq_enable_irq to setup io apic routing -v2

    * Yinghai Lu <yinghai@kernel.org> wrote:

    > so we could set io apic routing only when enable device irq.
    >
    > also could make setup_IO_APIC_irqs and setup_ioapic_dest only handle
    > first ioapic...
    >
    > v2: remove one one not needed style change.
    > merge the patch only setup io_apic for acpi on in setup_IO_APIC_irqs
    >
    > [ Impact: make mptable irq enable more like acpi is used, and numa_irq_desc could get correct node when acpi=off ]
    >
    > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    >
    > ---
    > arch/x86/kernel/apic/io_apic.c | 148 ++++++++++++++++++++---------------------
    > arch/x86/pci/irq.c | 84 ++++++++---------------
    > 2 files changed, 103 insertions(+), 129 deletions(-)

    Ok, i guess this makes sense with acpi-less bootup modes.

    There's hefty impact on io_apic.c and pci/irq.c as well. The io-apic
    code already has a fair amount of changes queued up. Jesse: do you
    have pci/irq.c changes queued up? If you agree with the patch, how
    should we handle this?

    A few mostly cosmetic comments:

    > +#ifdef CONFIG_ACPI
    > + if (!acpi_disabled && acpi_ioapic) {
    > + apic_id = mp_find_ioapic(0);
    > + if (apic_id < 0)
    > + apic_id = 0;
    > + }
    > +#endif

    that should go into a helper. I suspect.

    >
    > - idx = find_irq_entry(apic_id, pin, mp_INT);
    > - if (idx == -1) {
    > - if (!notcon) {
    > - notcon = 1;
    > - apic_printk(APIC_VERBOSE,
    > - KERN_DEBUG " %d-%d",
    > - mp_ioapics[apic_id].apicid, pin);
    > - } else
    > - apic_printk(APIC_VERBOSE, " %d-%d",
    > - mp_ioapics[apic_id].apicid, pin);
    > - continue;
    > - }
    > - if (notcon) {
    > + for (pin = 0; pin < nr_ioapic_registers[apic_id]; pin++) {
    > + idx = find_irq_entry(apic_id, pin, mp_INT);
    > + if (idx == -1) {
    > + if (!notcon) {
    > + notcon = 1;
    > apic_printk(APIC_VERBOSE,
    > - " (apicid-pin) not connected\n");
    > - notcon = 0;
    > - }
    > + KERN_DEBUG " %d-%d",
    > + mp_ioapics[apic_id].apicid, pin);
    > + } else
    > + apic_printk(APIC_VERBOSE, " %d-%d",
    > + mp_ioapics[apic_id].apicid, pin);
    > + continue;
    > + }
    > + if (notcon) {
    > + apic_printk(APIC_VERBOSE,
    > + " (apicid-pin) not connected\n");
    > + notcon = 0;
    > + }
    >
    > - irq = pin_2_irq(idx, apic_id, pin);
    > + irq = pin_2_irq(idx, apic_id, pin);
    >
    > - /*
    > - * 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(apic_id, irq))
    > - continue;
    > -
    > - desc = irq_to_desc_alloc_node(irq, node);
    > - if (!desc) {
    > - printk(KERN_INFO "can not get irq_desc for %d\n", irq);
    > - continue;
    > - }
    > - cfg = desc->chip_data;
    > - add_pin_to_irq_node(cfg, node, apic_id, pin);
    > + /*
    > + * 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(apic_id, irq))
    > + continue;
    >
    > - setup_IO_APIC_irq(apic_id, pin, irq, desc,
    > - irq_trigger(idx), irq_polarity(idx));
    > + desc = irq_to_desc_alloc_node(irq, node);
    > + if (!desc) {
    > + printk(KERN_INFO "can not get irq_desc for %d\n", irq);
    > + continue;
    > }
    > + cfg = desc->chip_data;
    > + add_pin_to_irq_node(cfg, node, apic_id, pin);
    > + set_bit(pin, mp_ioapic_routing[apic_id].pin_programmed);
    > + setup_IO_APIC_irq(apic_id, pin, irq, desc,
    > + irq_trigger(idx), irq_polarity(idx));
    > }

    this loop has become quite large now - could its body be moved into
    a helper inline function?

    > +#ifdef CONFIG_ACPI
    > + if (!acpi_disabled && acpi_ioapic) {
    > + ioapic = mp_find_ioapic(0);
    > + if (ioapic < 0)
    > + ioapic = 0;
    > + }
    > +#endif

    Needs a helper too?

    > Index: linux-2.6/arch/x86/pci/irq.c
    > ===================================================================
    > --- linux-2.6.orig/arch/x86/pci/irq.c
    > +++ linux-2.6/arch/x86/pci/irq.c
    > @@ -889,6 +889,9 @@ static int pcibios_lookup_irq(struct pci
    > return 0;
    > }
    >
    > + if (io_apic_assign_pci_irqs)
    > + return 0;
    > +
    > /* Find IRQ routing entry */
    >
    > if (!pirq_table)
    > @@ -1039,63 +1042,15 @@ static void __init pcibios_fixup_irqs(vo
    > pirq_penalty[dev->irq]++;
    > }
    >
    > + if (io_apic_assign_pci_irqs)
    > + return;
    > +
    > dev = NULL;
    > while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
    > pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
    > if (!pin)
    > continue;
    >
    > -#ifdef CONFIG_X86_IO_APIC
    > - /*
    > - * Recalculate IRQ numbers if we use the I/O APIC.
    > - */
    > - if (io_apic_assign_pci_irqs) {
    > - int irq;
    > - int ioapic = -1, ioapic_pin = -1;
    > - int triggering, polarity;
    > -
    > - /*
    > - * interrupt pins are numbered starting from 1
    > - */
    > - irq = IO_APIC_get_PCI_irq_vector(dev->bus->number,
    > - PCI_SLOT(dev->devfn), pin - 1,
    > - &ioapic, &ioapic_pin,
    > - &triggering, &polarity);
    > - /*
    > - * Busses behind bridges are typically not listed in the
    > - * MP-table. In this case we have to look up the IRQ
    > - * based on the parent bus, parent slot, and pin number.
    > - * The SMP code detects such bridged busses itself so we
    > - * should get into this branch reliably.
    > - */
    > - if (irq < 0 && dev->bus->parent) {
    > - /* go back to the bridge */
    > - struct pci_dev *bridge = dev->bus->self;
    > - int bus;
    > -
    > - pin = pci_swizzle_interrupt_pin(dev, pin);
    > - bus = bridge->bus->number;
    > - irq = IO_APIC_get_PCI_irq_vector(bus,
    > - PCI_SLOT(bridge->devfn),
    > - pin - 1,
    > - &ioapic, &ioapic_pin,
    > - &triggering, &polarity);
    > - if (irq >= 0)
    > - dev_warn(&dev->dev,
    > - "using bridge %s INT %c to "
    > - "get IRQ %d\n",
    > - pci_name(bridge),
    > - 'A' + pin - 1, irq);
    > - }
    > - if (irq >= 0) {
    > - dev_info(&dev->dev,
    > - "PCI->APIC IRQ transform: INT %c "
    > - "-> IRQ %d\n",
    > - 'A' + pin - 1, irq);
    > - dev->irq = irq;
    > - }
    > - }
    > -#endif

    nice!

    > /*
    > * Still no IRQ? Try to lookup one...
    > */
    > @@ -1190,6 +1145,19 @@ int __init pcibios_irq_init(void)
    > pcibios_enable_irq = pirq_enable_irq;
    >
    > pcibios_fixup_irqs();
    > +
    > + if (io_apic_assign_pci_irqs && pci_routeirq) {
    > + struct pci_dev *dev = NULL;
    > + /*
    > + * PCI IRQ routing is set up by pci_enable_device(), but we
    > + * also do it here in case there are still broken drivers that
    > + * don't use pci_enable_device().
    > + */
    > + printk(KERN_INFO "PCI: Routing PCI interrupts for all devices because \"pci=routeirq\" specified\n");
    > + for_each_pci_dev(dev)
    > + pirq_enable_irq(dev);
    > + }
    > +
    > return 0;
    > }
    >
    > @@ -1220,13 +1188,17 @@ void pcibios_penalize_isa_irq(int irq, i
    > static int pirq_enable_irq(struct pci_dev *dev)
    > {
    > u8 pin;
    > - struct pci_dev *temp_dev;
    >
    > pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
    > - if (pin && !pcibios_lookup_irq(dev, 1) && !dev->irq) {
    > + if (pin && !pcibios_lookup_irq(dev, 1)) {
    > char *msg = "";
    >
    > + if (!io_apic_assign_pci_irqs && dev->irq)
    > + return 0;
    > +
    > if (io_apic_assign_pci_irqs) {
    > +#ifdef CONFIG_X86_IO_APIC
    > + struct pci_dev *temp_dev;
    > int irq;
    > int ioapic = -1, ioapic_pin = -1;
    > int triggering, polarity;
    > @@ -1261,12 +1233,16 @@ static int pirq_enable_irq(struct pci_de
    > }
    > dev = temp_dev;
    > if (irq >= 0) {
    > + io_apic_set_pci_routing(&dev->dev, ioapic,
    > + ioapic_pin, irq,
    > + triggering, polarity);
    > + dev->irq = irq;
    > dev_info(&dev->dev, "PCI->APIC IRQ transform: "
    > "INT %c -> IRQ %d\n", 'A' + pin - 1, irq);
    > - dev->irq = irq;
    > return 0;
    > } else
    > msg = "; probably buggy MP table";
    > +#endif
    > } else if (pci_probe & PCI_BIOS_IRQ_SCAN)
    > msg = "";
    > else

    What a [pre-existing] maze of if else if else. Now also burdened
    with an #ifdef. New helper(s) needed?

    Ingo


    \
     
     \ /
      Last update: 2009-05-06 14:49    [W:5.523 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site