ACPI/PCI Hotplug: acpiphp cleanup

PATCH Superseded Alex Chiang 2 participants 8 messages Jul 10, 2009 – Jul 14, 2009

Subsystems (3)

ACPI PCI ACPI component architecture (acpica)

lore

Share & tools

lore

Alex Chiang

author

2009-07-10 19:42 UTC #0 lore raw

cover ACPI/PCI Hotplug: acpiphp cleanup

Hi Len, Jesse,

I'd like to have this patch series considered for the current release because it clarifies some usage of the new interface acpi_get_pci_dev() that was also introduced in this release cycle.

The short story is, acpi_get_pci_dev() doesn't always return a struct pci_dev, because a PCI root bridge may not have an associated pci_dev on platforms with non-materialized root bridges.

See this patch for more:

	http://patchwork.kernel.org/patch/32613/

I had to export acpi_pci_root to accomplish my goal, but it did result in a nice net cleanup in acpiphp.

It should probably go through Jesse's tree, but needs Len's ACK for the acpi_pci_root bit.

Thanks.

/ac

---

Alex Chiang (3):
ACPI: export acpi_pci_root and friends
PCI Hotplug: acpiphp: find bridges the easy way
PCI Hotplug: convert acpi_pci_detect_ejectable() to take an acpi_handle

 drivers/acpi/pci_root.c            |   17 +------
 drivers/pci/hotplug/acpi_pcihp.c   |    9 +---
 drivers/pci/hotplug/acpiphp_glue.c |   92 +++++++++---------------------------
 drivers/pci/hotplug/pciehp_acpi.c  |    5 +-
 include/acpi/acpi_bus.h            |   14 +++++
 include/linux/pci_hotplug.h        |    2 -
 6 files changed, 47 insertions(+), 92 deletions(-)

Alex Chiang

author

2009-07-10 19:42 UTC #1 lore raw

PATCH 1/3 ACPI: export acpi_pci_root and friends

We can simplify ACPI drivers if we can tell whether a handle is an ACPI PCI root or not.

Cc Bjorn Helgaas <[email protected]>

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

---

 drivers/acpi/pci_root.c |   17 ++---------------
 include/acpi/acpi_bus.h |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 15 deletions(-)

drivers/acpi/pci_root.c +2 ~~-15~~

index 55b5b90..31b961c 100644
@@ -61,20 +61,6 @@ static struct acpi_driver acpi_pci_root_driver = {
 		},
 };
 
-struct acpi_pci_root {
-	struct list_head node;
-	struct acpi_device *device;
-	struct pci_bus *bus;
-	u16 segment;
-	u8 bus_nr;
-
-	u32 osc_support_set;	/* _OSC state of support bits */
-	u32 osc_control_set;	/* _OSC state of control bits */
-	u32 osc_control_qry;	/* the latest _OSC query result */
-
-	u32 osc_queried:1;	/* has _OSC control been queried? */
-};
-
 static LIST_HEAD(acpi_pci_roots);
 
 static struct acpi_pci_driver *sub_driver;

@@ -317,7 +303,7 @@ static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
 	return status;
 }
 
-static struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
+struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 {
 	struct acpi_pci_root *root;

@@ -327,6 +313,7 @@ static struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 	}
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(acpi_pci_find_root);
 
 struct acpi_handle_node {
 	struct list_head node;

include/acpi/acpi_bus.h +14 -0

index c65e4ce..21f7152 100644
@@ -369,10 +369,24 @@ int register_acpi_bus_type(struct acpi_bus_type *);
 int unregister_acpi_bus_type(struct acpi_bus_type *);
 struct device *acpi_get_physical_device(acpi_handle);
 
+struct acpi_pci_root {
+	struct list_head node;
+	struct acpi_device * device;
+	struct acpi_pci_id id;
+	struct pci_bus *bus;
+
+	u32 osc_support_set;	/* _OSC state of support bits */
+	u32 osc_control_set;	/* _OSC state of control bits */
+	u32 osc_control_qry;	/* the latest _OSC query result */
+
+	u32 osc_queried:1;	/* has _OSC control been queried? */
+};
+
 /* helper */
 acpi_handle acpi_get_child(acpi_handle, acpi_integer);
 int acpi_is_root_bridge(acpi_handle);
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);
+struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
 #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
 
 #ifdef CONFIG_PM_SLEEP

Alex Chiang

author

2009-07-10 19:42 UTC #2 lore raw

PATCH 2/3 PCI Hotplug: acpiphp: find bridges the easy way

In reply to Alex Chiang

Instead of constantly evaluating _ADR and _SEG over and over again, let's simplify our lives by using:

acpi_pci_find_root() for root bridges
acpi_get_pci_dev() for p2p bridges

This change eliminates some copy 'n paste code and also allows us to simplify some internal interfaces.

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

---

 drivers/pci/hotplug/acpiphp_glue.c |  108 ++++++++++++------------------------
 1 files changed, 37 insertions(+), 71 deletions(-)

drivers/pci/hotplug/acpiphp_glue.c +37 ~~-71~~

index 0cb0f83..de4afc6 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);
+		pbus = pdev->subordinate;
+		pci_dev_put(pdev);
+	}
+	return pbus;
+}
+
 /* 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)

@@ -261,14 +276,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 
 /* see if it's worth looking at this bridge */
-static int detect_ejectable_slots(struct pci_bus *pbus)
+static int detect_ejectable_slots(acpi_handle handle)
 {
-	int found = acpi_pci_detect_ejectable(pbus);
+	int found;
+	struct pci_bus *pbus;
+
+	pbus = pci_bus_from_handle(handle);
+	found = acpi_pci_detect_ejectable(pbus);
 	if (!found) {
-		acpi_handle bridge_handle = acpi_pci_get_bridge_handle(pbus);
-		if (!bridge_handle)
-			return 0;
-		acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge_handle, (u32)1,
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
 				    is_pci_dock_device, (void *)&found, NULL);
 	}
 	return found;

@@ -399,9 +415,10 @@ static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
 
 
 /* allocate and initialize host bridge data structure */
-static void add_host_bridge(acpi_handle *handle, struct pci_bus *pci_bus)
+static void add_host_bridge(acpi_handle *handle)
 {
 	struct acpiphp_bridge *bridge;
+	struct acpi_pci_root *root = acpi_pci_find_root(handle);
 
 	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
 	if (bridge == NULL)

@@ -410,7 +427,7 @@ static void add_host_bridge(acpi_handle *handle, struct pci_bus *pci_bus)
 	bridge->type = BRIDGE_TYPE_HOST;
 	bridge->handle = handle;
 
-	bridge->pci_bus = pci_bus;
+	bridge->pci_bus = root->bus;
 
 	spin_lock_init(&bridge->res_lock);

@@ -419,7 +436,7 @@ static void add_host_bridge(acpi_handle *handle, struct pci_bus *pci_bus)
 
 
 /* allocate and initialize PCI-to-PCI bridge data structure */
-static void add_p2p_bridge(acpi_handle *handle, struct pci_dev *pci_dev)
+static void add_p2p_bridge(acpi_handle *handle)
 {
 	struct acpiphp_bridge *bridge;

@@ -433,8 +450,8 @@ static void add_p2p_bridge(acpi_handle *handle, struct pci_dev *pci_dev)
 	bridge->handle = handle;
 	config_p2p_bridge_flags(bridge);
 
-	bridge->pci_dev = pci_dev_get(pci_dev);
-	bridge->pci_bus = pci_dev->subordinate;
+	bridge->pci_dev = acpi_get_pci_dev(handle);
+	bridge->pci_bus = bridge->pci_dev->subordinate;
 	if (!bridge->pci_bus) {
 		err("This is not a PCI-to-PCI bridge!\n");
 		goto err;

@@ -451,7 +468,7 @@ static void add_p2p_bridge(acpi_handle *handle, struct pci_dev *pci_dev)
 	init_bridge_misc(bridge);
 	return;
  err:
-	pci_dev_put(pci_dev);
+	pci_dev_put(bridge->pci_dev);
 	kfree(bridge);
 	return;
 }

@@ -462,39 +479,21 @@ static acpi_status
 find_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	acpi_status status;
-	acpi_handle dummy_handle;
-	unsigned long long tmp;
-	int device, function;
 	struct pci_dev *dev;
-	struct pci_bus *pci_bus = context;
-
-	status = acpi_get_handle(handle, "_ADR", &dummy_handle);
-	if (ACPI_FAILURE(status))
-		return AE_OK;		/* continue */
-
-	status = acpi_evaluate_integer(handle, "_ADR", NULL, &tmp);
-	if (ACPI_FAILURE(status)) {
-		dbg("%s: _ADR evaluation failure\n", __func__);
-		return AE_OK;
-	}
-
-	device = (tmp >> 16) & 0xffff;
-	function = tmp & 0xffff;
-
-	dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
 
+	dev = acpi_get_pci_dev(handle);
 	if (!dev || !dev->subordinate)
 		goto out;
 
 	/* check if this bridge has ejectable slots */
-	if ((detect_ejectable_slots(dev->subordinate) > 0)) {
+	if ((detect_ejectable_slots(handle) > 0)) {
 		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
-		add_p2p_bridge(handle, dev);
+		add_p2p_bridge(handle);
 	}
 
 	/* search P2P bridges under this p2p bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     find_p2p_bridge, dev->subordinate, NULL);
+				     find_p2p_bridge, NULL, NULL);
 	if (ACPI_FAILURE(status))
 		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);

@@ -509,9 +508,7 @@ static int add_bridge(acpi_handle handle)
 {
 	acpi_status status;
 	unsigned long long tmp;
-	int seg, bus;
 	acpi_handle dummy_handle;
-	struct pci_bus *pci_bus;
 
 	/* if the bridge doesn't have _STA, we assume it is always there */
 	status = acpi_get_handle(handle, "_STA", &dummy_handle);

@@ -526,36 +523,15 @@ static int add_bridge(acpi_handle handle)
 			return 0;
 	}
 
-	/* get PCI segment number */
-	status = acpi_evaluate_integer(handle, "_SEG", NULL, &tmp);
-
-	seg = ACPI_SUCCESS(status) ? tmp : 0;
-
-	/* get PCI bus number */
-	status = acpi_evaluate_integer(handle, "_BBN", NULL, &tmp);
-
-	if (ACPI_SUCCESS(status)) {
-		bus = tmp;
-	} else {
-		warn("can't get bus number, assuming 0\n");
-		bus = 0;
-	}
-
-	pci_bus = pci_find_bus(seg, bus);
-	if (!pci_bus) {
-		err("Can't find bus %04x:%02x\n", seg, bus);
-		return 0;
-	}
-
 	/* check if this bridge has ejectable slots */
-	if (detect_ejectable_slots(pci_bus) > 0) {
+	if (detect_ejectable_slots(handle) > 0) {
 		dbg("found PCI host-bus bridge with hot-pluggable slots\n");
-		add_host_bridge(handle, pci_bus);
+		add_host_bridge(handle);
 	}
 
 	/* search P2P bridges under this host bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
-				     find_p2p_bridge, pci_bus, NULL);
+				     find_p2p_bridge, NULL, NULL);
 
 	if (ACPI_FAILURE(status))
 		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);

@@ -1387,16 +1363,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 +1371,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;
 }

Alex Chiang

author

2009-07-10 19:42 UTC #3 lore raw

PATCH 3/3 PCI Hotplug: convert acpi_pci_detect_ejectable() to take an acpi_handle

In reply to Alex Chiang

acpi_pci_detect_ejectable() goes through effort to convert its struct pci_bus arg to an acpi_handle, but every time we use this interface, we already have the handle available.

So let's just use the handle instead of converting back and forth.

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

---

 drivers/pci/hotplug/acpi_pcihp.c   |    9 +++------
 drivers/pci/hotplug/acpiphp_glue.c |   32 +++++++++++---------------------
 drivers/pci/hotplug/pciehp_acpi.c  |    5 +++--
 include/linux/pci_hotplug.h        |    2 +-
 4 files changed, 18 insertions(+), 30 deletions(-)

drivers/pci/hotplug/acpi_pcihp.c +3 -6

index eb15958..80af75c 100644
@@ -500,18 +500,15 @@ check_hotplug(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 /**
  * acpi_pci_detect_ejectable - check if the PCI bus has ejectable slots
- * @pbus - PCI bus to scan
+ * @handle - handle of the PCI bus to scan
  *
  * Returns 1 if the PCI bus has ACPI based ejectable slots, 0 otherwise.
  */
-int acpi_pci_detect_ejectable(struct pci_bus *pbus)
+int acpi_pci_detect_ejectable(acpi_handle handle)
 {
-	acpi_handle handle;
 	int found = 0;
 
-	if (!(handle = acpi_pci_get_bridge_handle(pbus)))
-		return 0;
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
 			    check_hotplug, (void *)&found, NULL);
 	return found;
 }

drivers/pci/hotplug/acpiphp_glue.c +11 ~~-21~~

index de4afc6..680c336 100644
@@ -62,21 +62,6 @@ 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);
-		pbus = pdev->subordinate;
-		pci_dev_put(pdev);
-	}
-	return pbus;
-}
-
 /* 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)

@@ -278,11 +263,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 /* see if it's worth looking at this bridge */
 static int detect_ejectable_slots(acpi_handle handle)
 {
-	int found;
-	struct pci_bus *pbus;
-
-	pbus = pci_bus_from_handle(handle);
-	found = acpi_pci_detect_ejectable(pbus);
+	int found = acpi_pci_detect_ejectable(handle);
 	if (!found) {
 		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
 				    is_pci_dock_device, (void *)&found, NULL);

@@ -1363,7 +1344,16 @@ 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_bus *bus = pci_bus_from_handle(handle);
+	struct pci_bus *bus;
+
+	if (acpi_is_root_bridge(handle)) {
+		struct acpi_pci_root *root = acpi_pci_find_root(handle);
+		bus = root->bus;
+	} else {
+		struct pci_dev *pdev = acpi_get_pci_dev(handle);
+		bus = pdev->subordinate;
+		pci_dev_put(pdev);
+	}
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);

drivers/pci/hotplug/pciehp_acpi.c +3 -2

index 9604801..0c6aa99 100644
@@ -47,7 +47,7 @@ int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
 {
 	if (slot_detection_mode != PCIEHP_DETECT_ACPI)
 		return 0;
-	if (acpi_pci_detect_ejectable(dev->subordinate))
+	if (acpi_pci_detect_ejectable(dev->dev.archdata.acpi_handle))
 		return 0;
 	return -ENODEV;
 }

@@ -94,7 +94,8 @@ static int __init dummy_probe(struct pcie_device *dev)
 			dup_slot_id++;
 	}
 	list_add_tail(&slot->slot_list, &dummy_slots);
-	if (!acpi_slot_detected && acpi_pci_detect_ejectable(pbus))
+	if (!acpi_slot_detected &&
+		acpi_pci_detect_ejectable(pbus->self->dev.archdata.acpi_handle))
 		acpi_slot_detected = 1;
 	return -ENODEV;         /* dummy driver always returns error */
 }

include/linux/pci_hotplug.h +1 -1

index 4391741..43394fc 100644
@@ -230,7 +230,7 @@ extern acpi_status acpi_get_hp_params_from_firmware(struct pci_bus *bus,
 				struct hotplug_params *hpp);
 int acpi_get_hp_hw_control_from_firmware(struct pci_dev *dev, u32 flags);
 int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
-int acpi_pci_detect_ejectable(struct pci_bus *pbus);
+int acpi_pci_detect_ejectable(acpi_handle handle);
 #endif
 #endif

Jesse Barnes

2009-07-14 19:27 UTC #4 lore raw

In reply to Alex Chiang

Quoted 12 lines · On Fri, 10 Jul 2009 13:42:33 -0600 Alex Chiang <[email protected]> wrote:

Hi Len, Jesse,
I'd like to have this patch series considered for the current release because it clarifies some usage of the new interface acpi_get_pci_dev() that was also introduced in this release cycle.
The short story is, acpi_get_pci_dev() doesn't always return a struct pci_dev, because a PCI root bridge may not have an associated pci_dev on platforms with non-materialized root bridges.
See this patch for more:

Quoted 7 lines

	http://patchwork.kernel.org/patch/32613/
I had to export acpi_pci_root to accomplish my goal, but it did result in a nice net cleanup in acpiphp.
It should probably go through Jesse's tree, but needs Len's ACK for the acpi_pci_root bit.

Haven't heard from Len yet, but I'm a bit nervous about adding this to for-linus. We already have a fix upstream for the acpi_get_pci_dev issue right? So there's no hurry on this set of (nice) cleanups is there?

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Alex Chiang

author

2009-07-14 20:04 UTC #5 lore raw

* Jesse Barnes <[email protected]>:

Quoted 4 lines

Haven't heard from Len yet, but I'm a bit nervous about adding this to for-linus. We already have a fix upstream for the acpi_get_pci_dev issue right? So there's no hurry on this set of (nice) cleanups is there?

The problem is that I introduced an assumption in acpiphp_configure_bridge, which calls acpi_get_pci_dev. That call may fail on systems with non-materialized PCI root bridges.

Fixing that assumption was addressed by this hunk in patch 2/3:

@@ -1387,16 +1363,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);

Now, my systems cannot do PCI root bridge hotplug, so that line where we call acpi_get_pci_dev() won't cause us to fail, but I'm concerned about machines in the wild that:

a) can do PCI root bridge hotplug
b) have non-materialized PCI root bridges

Maybe that is a small set of machines...

I agree that this series is on the large side for -rc4 and beyond, but I'd prefer to fix it now.

The thing is, a "minimal" fix wouldn't save us that much, since we'd still have to export acpi_pci_root stuff. We could maybe drop patch 3/3 which makes the final diffstat look like this:

 drivers/acpi/pci_root.c            |   17 +-----
 include/acpi/acpi_bus.h            |   14 +++++
 drivers/pci/hotplug/acpiphp_glue.c |  108 ++++++++++++------------------------
 3 files changed, 53 insertions(+), 86 deletions(-)

Hm, actually, I can actually make that even smaller by separating out the interface change.

Would you like me to do that and resubmit? I'd still need an ACK from Len re: acpi_pci_root...

Thanks.

/ac

Jesse Barnes

2009-07-14 20:32 UTC #6 lore raw

Quoted 65 lines · On Tue, 14 Jul 2009 14:04:31 -0600 Alex Chiang <[email protected]> wrote:

* Jesse Barnes <[email protected]>:
Quoted 4 lines
Haven't heard from Len yet, but I'm a bit nervous about adding this to for-linus. We already have a fix upstream for the acpi_get_pci_dev issue right? So there's no hurry on this set of (nice) cleanups is there?
The problem is that I introduced an assumption in acpiphp_configure_bridge, which calls acpi_get_pci_dev. That call may fail on systems with non-materialized PCI root bridges.
Fixing that assumption was addressed by this hunk in patch 2/3:
(quoted diff) +0 -0
@@ -1387,16 +1363,7 @@ static void acpiphp_sanitize_bus(struct
pci_bus *bus) /* Program resources in newly inserted bridge */
static int acpiphp_configure_bridge (acpi_handle handle) {
(quoted diff) +0 -6
-       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");
(quoted diff) +1 -4
-               return -EINVAL;
-       }
-
-       bus = dev->bus;
+       struct pci_bus *bus = pci_bus_from_handle(handle);
 
        pci_bus_size_bridges(bus);
        pci_bus_assign_resources(bus);
Now, my systems cannot do PCI root bridge hotplug, so that line where we call acpi_get_pci_dev() won't cause us to fail, but I'm concerned about machines in the wild that:
a) can do PCI root bridge hotplug
b) have non-materialized PCI root bridges
Maybe that is a small set of machines...
I agree that this series is on the large side for -rc4 and beyond, but I'd prefer to fix it now.
The thing is, a "minimal" fix wouldn't save us that much, since we'd still have to export acpi_pci_root stuff. We could maybe drop patch 3/3 which makes the final diffstat look like this:
 drivers/acpi/pci_root.c            |   17 +-----
 include/acpi/acpi_bus.h            |   14 +++++
 drivers/pci/hotplug/acpiphp_glue.c |  108
++++++++++++------------------------ 3 files changed, 53 insertions(+), 86 deletions(-)
Hm, actually, I can actually make that even smaller by separating out the interface change.
Would you like me to do that and resubmit? I'd still need an ACK from Len re: acpi_pci_root...
Thanks.
/ac

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Jesse Barnes

2009-07-14 20:33 UTC #7 lore raw

In reply to Alex Chiang

Quoted 51 lines · On Tue, 14 Jul 2009 14:04:31 -0600 Alex Chiang <[email protected]> wrote:

* Jesse Barnes <[email protected]>:
Quoted 4 lines
Haven't heard from Len yet, but I'm a bit nervous about adding this to for-linus. We already have a fix upstream for the acpi_get_pci_dev issue right? So there's no hurry on this set of (nice) cleanups is there?
The problem is that I introduced an assumption in acpiphp_configure_bridge, which calls acpi_get_pci_dev. That call may fail on systems with non-materialized PCI root bridges.
Fixing that assumption was addressed by this hunk in patch 2/3:
(quoted diff) +0 -0
@@ -1387,16 +1363,7 @@ static void acpiphp_sanitize_bus(struct
pci_bus *bus) /* Program resources in newly inserted bridge */
static int acpiphp_configure_bridge (acpi_handle handle) {
(quoted diff) +0 -6
-       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");
(quoted diff) +1 -4
-               return -EINVAL;
-       }
-
-       bus = dev->bus;
+       struct pci_bus *bus = pci_bus_from_handle(handle);
 
        pci_bus_size_bridges(bus);
        pci_bus_assign_resources(bus);
Now, my systems cannot do PCI root bridge hotplug, so that line where we call acpi_get_pci_dev() won't cause us to fail, but I'm concerned about machines in the wild that:
a) can do PCI root bridge hotplug
b) have non-materialized PCI root bridges
Maybe that is a small set of machines...
I agree that this series is on the large side for -rc4 and beyond, but I'd prefer to fix it now.
The thing is, a "minimal" fix wouldn't save us that much, since we'd still have to export acpi_pci_root stuff. We could maybe drop patch 3/3 which makes the final diffstat look like this:
 drivers/acpi/pci_root.c            |   17 +-----
 include/acpi/acpi_bus.h            |   14 +++++
 drivers/pci/hotplug/acpiphp_glue.c |  108

Quoted 8 lines

++++++++++++------------------------ 3 files changed, 53 insertions(+), 86 deletions(-)
Hm, actually, I can actually make that even smaller by separating out the interface change.
Would you like me to do that and resubmit? I'd still need an ACK from Len re: acpi_pci_root...

How about I actually send a reply this time...

No you don't need to re-submit; assuming Len comes back quickly we can put this into my for-linus branch. Len, we just need your ack to apply this series.

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center