On Wed, Nov 05, 2014 at 10:44:25PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 05, 2014 01:22:52 PM Bjorn Helgaas wrote:
> > ...
> > The acpi_pci_get_bridge_handle(struct pci_bus *) interface niggles at me a
> > little because I don't think there's any concept of an ACPI device for a
> > PCI *bus*, so it doesn't seem like a very good fit to say "find the handle
> > for this bus". But that's for later.
>
> To me it does what it says: Get me the handle of the bridge leading to
> this bus.
Yeah, I know, it's a great name because it does *exactly* what the name
says. I'm just wondering if there's a nicer way to express what the caller
needs.
Two of the three callers start with a pci_dev, look up a pci_bus to pass
in, and get back a handle corresponding to a pci_host_bridge or a pci_dev
(or a NULL). It seems a little cluttered because the pci_bus is only
incidental and the caller doesn't care about it at all except for passing
it to acpi_pci_get_bridge_handle().
It's relatively common to start with a pci_dev and look for an ACPI handle
that corresponds to that device or the closest enclosing scope, so maybe
there should be a way to do that directly.
For pci_get_hp_params(), I think the current code is actually slightly
buggy because we don't look for _HPP/_HPX on the device itself; we only
look at the bridges upstream from it.
What I had in mind was something like the following (untested and not for
application).
Bjorn
diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 876ccc620440..5e95df56b8ae 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -116,20 +116,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
string = (struct acpi_buffer){ ACPI_ALLOCATE_BUFFER, NULL };
}
- handle = ACPI_HANDLE(&pdev->dev);
- if (!handle) {
- /*
- * This hotplug controller was not listed in the ACPI name
- * space at all. Try to get acpi handle of parent pci bus.
- */
- struct pci_bus *pbus;
- for (pbus = pdev->bus; pbus; pbus = pbus->parent) {
- handle = acpi_pci_get_bridge_handle(pbus);
- if (handle)
- break;
- }
- }
-
+ /*
+ * We did not find _OSC on the host bridge, so look for any
+ * enclosing device with an OSHP method.
+ */
+ handle = pci_find_acpi_handle(pdev);
while (handle) {
acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
dbg("Trying to get hotplug control for %s \n",
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 6ebf8edc5f3c..3b3f0720fff0 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -246,14 +246,8 @@ int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp)
{
acpi_status status;
acpi_handle handle, phandle;
- struct pci_bus *pbus;
- handle = NULL;
- for (pbus = dev->bus; pbus; pbus = pbus->parent) {
- handle = acpi_pci_get_bridge_handle(pbus);
- if (handle)
- break;
- }
+ handle = pci_find_acpi_handle(dev);
/*
* _HPP settings apply to all child buses, until another _HPP is
@@ -279,6 +273,33 @@ int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp)
}
EXPORT_SYMBOL_GPL(pci_get_hp_params);
+/*
+ * Search for an ACPI handle. The PCI device itself may have one, or an
+ * upstream device (either a PCI-to-PCI bridge or a PCI host bridge) may
+ * have one.
+ */
+acpi_handle pci_find_acpi_handle(struct pci_dev *pdev)
+{
+ struct device *dev;
+ acpi_handle handle;
+ struct pci_bus *bus;
+
+ dev = &pdev->dev;
+ handle = ACPI_HANDLE(dev);
+ while (!handle) {
+ pdev = pci_physfn(pdev);
+ bus = pdev->bus;
+ if (pci_is_root_bus(bus))
+ dev = bus->bridge;
+ else {
+ pdev = bus->self;
+ dev = &pdev->dev;
+ }
+ handle = ACPI_HANDLE(dev);
+ }
+ return handle;
+}
+
/**
* pci_acpi_wake_bus - Root bus wakeup notification fork function.
* @work: Work item to handle.
--
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
