Re: [PATCH] ACPI, PCI: Get PRT entry during acpi_pci_enable_irq()

[Bjorn Helgaas]

On Thu, Feb 14, 2013 at 5:50 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Tue, Feb 12, 2013 at 12:22 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> On Tuesday, February 12, 2013 11:11:23 AM Yinghai Lu wrote:
>>> Peter Hurley found "irq 18 nobody cared" with pci-next, and dmesg has
>>>
>>> [    8.983246] pci 0000:00:1e.0: can't derive routing for PCI INT A
>>> [    8.983600] snd_ctxfi 0000:09:02.0: PCI INT A: no GSI - using ISA IRQ 5
>>>
>>> bisect to
>>> | commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
>>> |     PCI: Put pci_dev in device tree as early as possible
>>>
>>> It turns out we need to call acpi_pci_irq_add_prt() after the pci bridges
>>> are scanned.
>>>
>>> Bjorn said:
>>>  The bus number binding means acpi_pci_irq_add_prt() has to happen
>>>  after enumerating everything below a bridge, and it will prevent us
>>>  from doing any bus number reassignment for hotplug.
>>>
>>>  I think we should remove the bus numbers from the cached _PRT (or
>>>  maybe even remove the _PRT caching completely).  When we enable a PCI
>>>  device's IRQ, we should search up the PCI device tree looking for a
>>>  _PRT associated with each node, and applying normal PCI bridge
>>>  swizzling when we don't find a _PRT.  I think this can be done without
>>>  using PCI bus numbers at all.
>>>
>>> So here we try to remove _PRT caching completely.
>>>
>>> -v2: check !handle early.
>>>
>>> Reported-and-tested-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>>> Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>>> ---
>>>  drivers/acpi/pci_irq.c      |   95 +++++++++++++++++---------------------------
>>>  drivers/acpi/pci_root.c     |   18 --------
>>>  drivers/pci/pci-acpi.c      |   24 -----------
>>>  include/acpi/acpi_drivers.h |    5 --
>>>  4 files changed, 38 insertions(+), 104 deletions(-)
>
> Bjorn,
>
> Can you put this one into pci/next?

I'm not sure what this patch is based on or what the best way to merge
it is.  It doesn't apply cleanly to my next or
pci/yinghai-root-bus-hotplug branches.

I did apply it manually on top of pci/yinghai-root-bus-hotplug to try
it out, but we need to tweak the messages a little bit.

Previously we printed "ACPI: PCI Interrupt Routing Table [%s._PRT]"
once when loading it, which was fine.  Now we print it every time we
look at a _PRT, which is too much because it isn't really adding any
information.

We also print "ACPI Exception: AE_NOT_FOUND, Evaluating _PRT
[AE_NOT_FOUND] (20121018/pci_irq-259)" if we find ACPI nodes without
_PRTs, which we shouldn't do, because that's a common and normal
situation.

Bjorn

>>> Index: linux-2.6/drivers/acpi/pci_irq.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/acpi/pci_irq.c
>>> +++ linux-2.6/drivers/acpi/pci_irq.c
>>> @@ -53,9 +53,6 @@ struct acpi_prt_entry {
>>>       u32                     index;          /* GSI, or link _CRS index */
>>>  };
>>>
>>> -static LIST_HEAD(acpi_prt_list);
>>> -static DEFINE_SPINLOCK(acpi_prt_lock);
>>> -
>>>  static inline char pin_name(int pin)
>>>  {
>>>       return 'A' + pin - 1;
>>> @@ -65,28 +62,6 @@ static inline char pin_name(int pin)
>>>                           PCI IRQ Routing Table (PRT) Support
>>>     -------------------------------------------------------------------------- */
>>>
>>> -static struct acpi_prt_entry *acpi_pci_irq_find_prt_entry(struct pci_dev *dev,
>>> -                                                       int pin)
>>> -{
>>> -     struct acpi_prt_entry *entry;
>>> -     int segment = pci_domain_nr(dev->bus);
>>> -     int bus = dev->bus->number;
>>> -     int device = PCI_SLOT(dev->devfn);
>>> -
>>> -     spin_lock(&acpi_prt_lock);
>>> -     list_for_each_entry(entry, &acpi_prt_list, list) {
>>> -             if ((segment == entry->id.segment)
>>> -                 && (bus == entry->id.bus)
>>> -                 && (device == entry->id.device)
>>> -                 && (pin == entry->pin)) {
>>> -                     spin_unlock(&acpi_prt_lock);
>>> -                     return entry;
>>> -             }
>>> -     }
>>> -     spin_unlock(&acpi_prt_lock);
>>> -     return NULL;
>>> -}
>>> -
>>>  /* http://bugzilla.kernel.org/show_bug.cgi?id=4773 */
>>>  static const struct dmi_system_id medion_md9580[] = {
>>>       {
>>> @@ -184,11 +159,19 @@ static void do_prt_fixups(struct acpi_pr
>>>       }
>>>  }
>>>
>>> -static int acpi_pci_irq_add_entry(acpi_handle handle, int segment, int bus,
>>> -                               struct acpi_pci_routing_table *prt)
>>> +static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
>>> +                               int pin, struct acpi_pci_routing_table *prt,
>>> +                               struct acpi_prt_entry **entry_ptr)
>>>  {
>>> +     int segment = pci_domain_nr(dev->bus);
>>> +     int bus = dev->bus->number;
>>> +     int device = PCI_SLOT(dev->devfn);
>>>       struct acpi_prt_entry *entry;
>>>
>>> +     if (((prt->address >> 16) & 0xffff) != device ||
>>> +         prt->pin + 1 != pin)
>>> +             return -ENODEV;
>>> +
>>>       entry = kzalloc(sizeof(struct acpi_prt_entry), GFP_KERNEL);
>>>       if (!entry)
>>>               return -ENOMEM;
>>> @@ -237,26 +220,33 @@ static int acpi_pci_irq_add_entry(acpi_h
>>>                             entry->id.device, pin_name(entry->pin),
>>>                             prt->source, entry->index));
>>>
>>> -     spin_lock(&acpi_prt_lock);
>>> -     list_add_tail(&entry->list, &acpi_prt_list);
>>> -     spin_unlock(&acpi_prt_lock);
>>> +     *entry_ptr = entry;
>>>
>>>       return 0;
>>>  }
>>>
>>> -int acpi_pci_irq_add_prt(acpi_handle handle, int segment, int bus)
>>> +static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev,
>>> +                       int pin, struct acpi_prt_entry **entry_ptr)
>>>  {
>>>       acpi_status status;
>>>       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>>>       struct acpi_pci_routing_table *entry;
>>> +     acpi_handle handle = NULL;
>>> +
>>> +     if (dev->bus->bridge)
>>> +             handle = ACPI_HANDLE(dev->bus->bridge);
>>> +
>>> +     if (!handle)
>>> +             return -ENODEV;
>>>
>>>       /* 'handle' is the _PRT's parent (root bridge or PCI-PCI bridge) */
>>>       status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>>>       if (ACPI_FAILURE(status))
>>>               return -ENODEV;
>>>
>>> -     printk(KERN_DEBUG "ACPI: PCI Interrupt Routing Table [%s._PRT]\n",
>>> -            (char *) buffer.pointer);
>>> +     dev_printk(KERN_DEBUG, &dev->dev,
>>> +                     "ACPI: PCI Interrupt Routing Table [%s._PRT]\n",
>>> +                    (char *) buffer.pointer);
>>>
>>>       kfree(buffer.pointer);
>>>
>>> @@ -273,7 +263,9 @@ int acpi_pci_irq_add_prt(acpi_handle han
>>>
>>>       entry = buffer.pointer;
>>>       while (entry && (entry->length > 0)) {
>>> -             acpi_pci_irq_add_entry(handle, segment, bus, entry);
>>> +             if (!acpi_pci_irq_check_entry(handle, dev, pin,
>>> +                                              entry, entry_ptr))
>>> +                     break;
>>>               entry = (struct acpi_pci_routing_table *)
>>>                   ((unsigned long)entry + entry->length);
>>>       }
>>> @@ -282,23 +274,6 @@ int acpi_pci_irq_add_prt(acpi_handle han
>>>       return 0;
>>>  }
>>>
>>> -void acpi_pci_irq_del_prt(int segment, int bus)
>>> -{
>>> -     struct acpi_prt_entry *entry, *tmp;
>>> -
>>> -     printk(KERN_DEBUG
>>> -            "ACPI: Delete PCI Interrupt Routing Table for %04x:%02x\n",
>>> -            segment, bus);
>>> -     spin_lock(&acpi_prt_lock);
>>> -     list_for_each_entry_safe(entry, tmp, &acpi_prt_list, list) {
>>> -             if (segment == entry->id.segment && bus == entry->id.bus) {
>>> -                     list_del(&entry->list);
>>> -                     kfree(entry);
>>> -             }
>>> -     }
>>> -     spin_unlock(&acpi_prt_lock);
>>> -}
>>> -
>>>  /* --------------------------------------------------------------------------
>>>                            PCI Interrupt Routing Support
>>>     -------------------------------------------------------------------------- */
>>> @@ -359,12 +334,13 @@ static int acpi_reroute_boot_interrupt(s
>>>
>>>  static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin)
>>>  {
>>> -     struct acpi_prt_entry *entry;
>>> +     struct acpi_prt_entry *entry = NULL;
>>>       struct pci_dev *bridge;
>>>       u8 bridge_pin, orig_pin = pin;
>>> +     int ret;
>>>
>>> -     entry = acpi_pci_irq_find_prt_entry(dev, pin);
>>> -     if (entry) {
>>> +     ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
>>> +     if (!ret && entry) {
>>>  #ifdef CONFIG_X86_IO_APIC
>>>               acpi_reroute_boot_interrupt(dev, entry);
>>>  #endif /* CONFIG_X86_IO_APIC */
>>> @@ -373,7 +349,7 @@ static struct acpi_prt_entry *acpi_pci_i
>>>               return entry;
>>>       }
>>>
>>> -     /*
>>> +     /*
>>>        * Attempt to derive an IRQ for this device from a parent bridge's
>>>        * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge).
>>>        */
>>> @@ -393,8 +369,8 @@ static struct acpi_prt_entry *acpi_pci_i
>>>                       pin = bridge_pin;
>>>               }
>>>
>>> -             entry = acpi_pci_irq_find_prt_entry(bridge, pin);
>>> -             if (entry) {
>>> +             ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
>>> +             if (!ret && entry) {
>>>                       ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>>                                        "Derived GSI for %s INT %c from %s\n",
>>>                                        pci_name(dev), pin_name(orig_pin),
>>> @@ -470,6 +446,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>>>                       dev_warn(&dev->dev, "PCI INT %c: no GSI\n",
>>>                                pin_name(pin));
>>>               }
>>> +
>>>               return 0;
>>>       }
>>>
>>> @@ -477,6 +454,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>>>       if (rc < 0) {
>>>               dev_warn(&dev->dev, "PCI INT %c: failed to register GSI\n",
>>>                        pin_name(pin));
>>> +             kfree(entry);
>>>               return rc;
>>>       }
>>>       dev->irq = rc;
>>> @@ -491,6 +469,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>>>               (triggering == ACPI_LEVEL_SENSITIVE) ? "level" : "edge",
>>>               (polarity == ACPI_ACTIVE_LOW) ? "low" : "high", dev->irq);
>>>
>>> +     kfree(entry);
>>>       return 0;
>>>  }
>>>
>>> @@ -513,6 +492,8 @@ void acpi_pci_irq_disable(struct pci_dev
>>>       else
>>>               gsi = entry->index;
>>>
>>> +     kfree(entry);
>>> +
>>>       /*
>>>        * TBD: It might be worth clearing dev->irq by magic constant
>>>        * (e.g. PCI_UNDEFINED_IRQ).
>>> Index: linux-2.6/drivers/acpi/pci_root.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/acpi/pci_root.c
>>> +++ linux-2.6/drivers/acpi/pci_root.c
>>> @@ -413,7 +413,6 @@ static int acpi_pci_root_add(struct acpi
>>>       acpi_status status;
>>>       int result;
>>>       struct acpi_pci_root *root;
>>> -     acpi_handle handle;
>>>       struct acpi_pci_driver *driver;
>>>       u32 flags, base_flags;
>>>       bool is_osc_granted = false;
>>> @@ -468,16 +467,6 @@ static int acpi_pci_root_add(struct acpi
>>>              acpi_device_name(device), acpi_device_bid(device),
>>>              root->segment, &root->secondary);
>>>
>>> -     /*
>>> -      * PCI Routing Table
>>> -      * -----------------
>>> -      * Evaluate and parse _PRT, if exists.
>>> -      */
>>> -     status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
>>> -     if (ACPI_SUCCESS(status))
>>> -             result = acpi_pci_irq_add_prt(device->handle, root->segment,
>>> -                                           root->secondary.start);
>>> -
>>>       root->mcfg_addr = acpi_pci_root_get_mcfg_addr(device->handle);
>>>
>>>       /*
>>> @@ -602,7 +591,6 @@ out_del_root:
>>>       list_del(&root->node);
>>>       mutex_unlock(&acpi_pci_root_lock);
>>>
>>> -     acpi_pci_irq_del_prt(root->segment, root->secondary.start);
>>>  end:
>>>       kfree(root);
>>>       return result;
>>> @@ -610,8 +598,6 @@ end:
>>>
>>>  static void acpi_pci_root_remove(struct acpi_device *device)
>>>  {
>>> -     acpi_status status;
>>> -     acpi_handle handle;
>>>       struct acpi_pci_root *root = acpi_driver_data(device);
>>>       struct acpi_pci_driver *driver;
>>>
>>> @@ -626,10 +612,6 @@ static void acpi_pci_root_remove(struct
>>>       device_set_run_wake(root->bus->bridge, false);
>>>       pci_acpi_remove_bus_pm_notifier(device);
>>>
>>> -     status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
>>> -     if (ACPI_SUCCESS(status))
>>> -             acpi_pci_irq_del_prt(root->segment, root->secondary.start);
>>> -
>>>       pci_remove_root_bus(root->bus);
>>>
>>>       mutex_lock(&acpi_pci_root_lock);
>>> Index: linux-2.6/drivers/pci/pci-acpi.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/pci/pci-acpi.c
>>> +++ linux-2.6/drivers/pci/pci-acpi.c
>>> @@ -307,25 +307,6 @@ static void pci_acpi_setup(struct device
>>>       struct pci_dev *pci_dev = to_pci_dev(dev);
>>>       acpi_handle handle = ACPI_HANDLE(dev);
>>>       struct acpi_device *adev;
>>> -     acpi_status status;
>>> -     acpi_handle dummy;
>>> -
>>> -     /*
>>> -      * Evaluate and parse _PRT, if exists.  This code allows parsing of
>>> -      * _PRT objects within the scope of non-bridge devices.  Note that
>>> -      * _PRTs within the scope of a PCI bridge assume the bridge's
>>> -      * subordinate bus number.
>>> -      *
>>> -      * TBD: Can _PRTs exist within the scope of non-bridge PCI devices?
>>> -      */
>>> -     status = acpi_get_handle(handle, METHOD_NAME__PRT, &dummy);
>>> -     if (ACPI_SUCCESS(status)) {
>>> -             unsigned char bus;
>>> -
>>> -             bus = pci_dev->subordinate ?
>>> -                     pci_dev->subordinate->number : pci_dev->bus->number;
>>> -             acpi_pci_irq_add_prt(handle, pci_domain_nr(pci_dev->bus), bus);
>>> -     }
>>>
>>>       if (acpi_bus_get_device(handle, &adev) || !adev->wakeup.flags.valid)
>>>               return;
>>> @@ -340,7 +321,6 @@ static void pci_acpi_setup(struct device
>>>
>>>  static void pci_acpi_cleanup(struct device *dev)
>>>  {
>>> -     struct pci_dev *pci_dev = to_pci_dev(dev);
>>>       acpi_handle handle = ACPI_HANDLE(dev);
>>>       struct acpi_device *adev;
>>>
>>> @@ -349,10 +329,6 @@ static void pci_acpi_cleanup(struct devi
>>>               device_set_run_wake(dev, false);
>>>               pci_acpi_remove_pm_notifier(adev);
>>>       }
>>> -
>>> -     if (pci_dev->subordinate)
>>> -             acpi_pci_irq_del_prt(pci_domain_nr(pci_dev->bus),
>>> -                                  pci_dev->subordinate->number);
>>>  }
>>>
>>>  static struct acpi_bus_type acpi_pci_bus = {
>>> Index: linux-2.6/include/acpi/acpi_drivers.h
>>> ===================================================================
>>> --- linux-2.6.orig/include/acpi/acpi_drivers.h
>>> +++ linux-2.6/include/acpi/acpi_drivers.h
>>> @@ -90,11 +90,6 @@ int acpi_pci_link_allocate_irq(acpi_hand
>>>                              int *polarity, char **name);
>>>  int acpi_pci_link_free_irq(acpi_handle handle);
>>>
>>> -/* ACPI PCI Interrupt Routing (pci_irq.c) */
>>> -
>>> -int acpi_pci_irq_add_prt(acpi_handle handle, int segment, int bus);
>>> -void acpi_pci_irq_del_prt(int segment, int bus);
>>> -
>>>  /* ACPI PCI Device Binding (pci_bind.c) */
>>>
>>>  struct pci_bus;
>> --
>> I speak only for myself.
>> Rafael J. Wysocki, Intel Open Source Technology Center.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/