ACPI/PCI: export acpi_pci_root to find pci_bus correctly

We cannot simply call acpi_get_pci_dev() on any random ACPI handle and hope that it works, because a PCI root bridge may not have an associated struct pci_dev.

This is allowed per the PCI specification, and is referred to as a non-materialized bridge.

So, depending on the type of PCI bridge that the handle points to, use the appropriate interface to return the struct pci_bus correctly.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +3 -0

index 0cb0f83..fa4658b 100644
@@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
 
+static struct pci_bus *pci_bus_from_handle(acpi_handle handle)
+{
+	struct pci_bus *pbus;

drivers/pci/hotplug/acpiphp_glue.c +3 -11

+}
+
 /* callback routine to check for the existence of a pci dock device */
 static acpi_status
 is_pci_dock_device(acpi_handle handle, u32 lvl, void *context, void **rv)

@@ -1387,16 +1402,7 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
 /* Program resources in newly inserted bridge */
 static int acpiphp_configure_bridge (acpi_handle handle)
 {
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
-	dev = acpi_get_pci_dev(handle);
-	if (!dev) {
-		err("cannot get PCI domain and bus number for bridge\n");
-		return -EINVAL;
-	}
-
-	bus = dev->bus;
+	struct pci_bus *bus = pci_bus_from_handle(handle);
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);

@@ -1404,7 +1410,6 @@ static int acpiphp_configure_bridge (acpi_handle handle)
 	acpiphp_set_hpp_values(handle, bus);
 	pci_enable_bridges(bus);
 	acpiphp_configure_ioapics(handle);
-	pci_dev_put(dev);
 	return 0;
 }

Quoted 33 lines

We cannot simply call acpi_get_pci_dev() on any random ACPI handle and hope that it works, because a PCI root bridge may not have an associated struct pci_dev.

This is allowed per the PCI specification, and is referred to as a non-materialized bridge.

So, depending on the type of PCI bridge that the handle points to, use the appropriate interface to return the struct pci_bus correctly.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +9 -0

index 0cb0f83..fa4658b 100644
@@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
 
+static struct pci_bus *pci_bus_from_handle(acpi_handle handle)
+{
+	struct pci_bus *pbus;
+
+	if (acpi_is_root_bridge(handle)) {
+		struct acpi_pci_root *root = acpi_pci_find_root(handle);
+		pbus = root->bus;
+	} else {
+		struct pci_dev *pdev = acpi_get_pci_dev(handle);

drivers/pci/hotplug/acpiphp_glue.c static void acpiphp_sanitize_bus(struct pci_bus *bus); · line 77

Quoted 4 lines

drivers/pci/hotplug/acpiphp_glue.c +4 -0

+		pbus = pdev->subordinate;
+		pci_dev_put(pdev);
+	}
+	return pbus;

I worry that acpi_is_root_bridge() merely checks the device IDs of "handle", which isn't quite the same as checking whether the pci_root driver has claimed "handle".

Are you confident that it's safe to move the pci_dev_put(), so it is released before configuring the bridge?

What do you think about something like this (even though the get/put is still a little clunky):

	struct pci_dev *dev = NULL;

	root = acpi_pci_find_root(handle);
	if (root)
		bus = root->bus;
	else {
		dev = acpi_get_pci_dev(handle);
		if (dev)
			bus = pdev->subordinate;
		else {
			err("cannot get PCI domain and bus number for bridge\n");
			return -EINVAL;

		}
	}

	pci_bus_size_bridges(bus);
	...
	if (dev)
		pci_dev_put(dev);
	return 0;

* Bjorn Helgaas <[email protected]>:

Quoted 34 lines · Alex Chiang

Quoted 33 lines

We cannot simply call acpi_get_pci_dev() on any random ACPI handle and hope that it works, because a PCI root bridge may not have an associated struct pci_dev.

This is allowed per the PCI specification, and is referred to as a non-materialized bridge.

So, depending on the type of PCI bridge that the handle points to, use the appropriate interface to return the struct pci_bus correctly.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +9 -0

index 0cb0f83..fa4658b 100644
@@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
 
+static struct pci_bus *pci_bus_from_handle(acpi_handle handle)
+{
+	struct pci_bus *pbus;
+
+	if (acpi_is_root_bridge(handle)) {
+		struct acpi_pci_root *root = acpi_pci_find_root(handle);
+		pbus = root->bus;
+	} else {
+		struct pci_dev *pdev = acpi_get_pci_dev(handle);

Quoted 8 lines

drivers/pci/hotplug/acpiphp_glue.c static void acpiphp_sanitize_bus(struct pci_bus *bus); · line 77

Quoted 4 lines

drivers/pci/hotplug/acpiphp_glue.c +4 -0

+		pbus = pdev->subordinate;
+		pci_dev_put(pdev);
+	}
+	return pbus;

I worry that acpi_is_root_bridge() merely checks the device IDs of "handle", which isn't quite the same as checking whether the pci_root driver has claimed "handle".

Hm, I understand this concern in a theoretical sense, but could you explain more of what you're thinking about, and give me a concrete example of something that might go wrong here?

Quoted 20 lines

* Bjorn Helgaas <[email protected]>:

Quoted 10 lines · Alex Chiang

Quoted 9 lines

drivers/pci/hotplug/acpiphp_glue.c +9 -0

+static struct pci_bus *pci_bus_from_handle(acpi_handle handle)
+{
+	struct pci_bus *pbus;
+
+	if (acpi_is_root_bridge(handle)) {
+		struct acpi_pci_root *root = acpi_pci_find_root(handle);
+		pbus = root->bus;
+	} else {
+		struct pci_dev *pdev = acpi_get_pci_dev(handle);

Quoted 8 lines

drivers/pci/hotplug/acpiphp_glue.c static void acpiphp_sanitize_bus(struct pci_bus *bus); · line 77

Quoted 4 lines

drivers/pci/hotplug/acpiphp_glue.c +4 -0

+		pbus = pdev->subordinate;
+		pci_dev_put(pdev);
+	}
+	return pbus;

I worry that acpi_is_root_bridge() merely checks the device IDs of "handle", which isn't quite the same as checking whether the pci_root driver has claimed "handle".

Quoted 3 lines

Hm, I understand this concern in a theoretical sense, but could you explain more of what you're thinking about, and give me a concrete example of something that might go wrong here?

My concern is only theoretical -- I could imagine a PNP0A03 device in the namespace (so acpi_is_root_bridge() is true) that has not been claimed by the pci_root driver (so acpi_pci_find_root() returns NULL).

I don't think this will happen in practice because pci_root can't be a module, but it's easier to analyze with just one check, since you can learn everything you need from acpi_pci_find_root() without also depending on acpi_is_root_bridge().