Skip to contentSkip to conversation

PCI root bridge: get bus info from _CRS

PATCH In review Bjorn Helgaas 3 participants 8 messages Jun 18, 2009 – Jun 20, 2009

Subsystems (2)
ACPI PCI
lore
Share & tools
lore

Bjorn Helgaas

author
#0 lore raw

cover PCI root bridge: get bus info from _CRS

The main patch in this series changes the way we discover the downstream bus number of PCI root bridges. The end result should be the same as the current code (as far as I know, this doesn't fix any bugs). But I believe this follows the intent of the spec better, makes Linux work more like Windows, and removes false accusations Linux makes against the BIOS.

---

Bjorn Helgaas (5):
ACPI: pci_root: remove unused dev/fn information
ACPI: pci_root: simplify list traversals
ACPI: pci_root: use driver data rather than list lookup
ACPI: pci_root: simplify acpi_pci_root_add() control flow
ACPI: pci_root: check _CRS, then _BBN for downstream bus number

 drivers/acpi/pci_root.c |  185 ++++++++++++++---------------------------------
 1 files changed, 57 insertions(+), 128 deletions(-)
author
#1 lore raw

PATCH 1/5 ACPI: pci_root: check _CRS, then _BBN for downstream bus number

To find a host bridge's downstream bus number, we currently look at _BBN first. If _BBN returns a bus number we've already seen, we conclude that _BBN was wrong and look for a bus number in _CRS.

However, the spec[1] (figure 5-5 and the example in sec 9.12.1) and an ACPI FAQ[2] suggest that the OS should use _CRS to discover the bus number range, and that _BBN is really intended to bootstrap _CRS methods that reference PCI opregions.

This patch makes us always look at _CRS first. If _CRS doesn't supply a bus number, we look at _BBN. If _BBN doesn't exist, we default to zero. This makes the behavior consistent regardless of device discovery order. Previously, if A and B had duplicate _BBNs and we found A first, we'd only look at B's _CRS, whereas if we found B first, we'd only look at A's _CRS.

I'm told that Windows discovers host bridge bus numbers using _CRS, so it should be fairly safe to rely on this BIOS functionality.

This patch also removes two misleading messages: we printed the "Wrong _BBN value, reboot and use option 'pci=noacpi'" message before looking at _CRS, so we would likely find the bus number in _CRS, the system would work fine, and the user would be confused. The "PCI _CRS %d overrides _BBN 0" message incorrectly assumes _BBN was zero, and it's useless anyway because we print the segment/bus number a few lines later.

References:

    [1] http://www.acpi.info/DOWNLOADS/ACPIspec30b.pdf
    [2] http://www.acpi.info/acpi_faq.htm _BBN/_CRS discussion
    http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWAR05005_WinHEC05.ppt (slide 17)
    http://bugzilla.kernel.org/show_bug.cgi?id=1662 ASUS PR-DLS
    http://bugzilla.kernel.org/show_bug.cgi?id=1127 ASUS PR-DLSW
    http://bugzilla.kernel.org/show_bug.cgi?id=1741 ASUS PR-DLS533
Signed-off-by Bjorn Helgaas <[email protected]>
Reviewed-by Alex Chiang <[email protected]>
CC Shaohua Li <[email protected]>
CC Kenji Kaneshige <[email protected]>
---
 drivers/acpi/pci_root.c |   54 ++++++++++++++---------------------------------
 1 files changed, 16 insertions(+), 38 deletions(-)
drivers/acpi/pci_root.c +16 -38
index f341b07..7847732 100644
@@ -470,12 +470,12 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	int result = 0;
 	struct acpi_pci_root *root = NULL;
-	struct acpi_pci_root *tmp;
 	acpi_status status = AE_OK;
 	unsigned long long value = 0;
 	acpi_handle handle = NULL;
 	struct acpi_device *child;
 	u32 flags, base_flags;
+	int bus;
 
 
 	if (!device)

@@ -523,46 +523,24 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	/* 
 	 * Bus
 	 * ---
-	 * Obtained via _BBN, if exists, otherwise assumed to be zero (0).
+	 * Check _CRS first, then _BBN.  If no _BBN, default to zero.
 	 */
-	status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN, NULL,
-				       &value);
-	switch (status) {
-	case AE_OK:
-		root->id.bus = (u16) value;
-		break;
-	case AE_NOT_FOUND:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Assuming bus 0 (no _BBN)\n"));
-		root->id.bus = 0;
-		break;
-	default:
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BBN"));
-		result = -ENODEV;
-		goto end;
-	}
-
-	/* Some systems have wrong _BBN */
-	list_for_each_entry(tmp, &acpi_pci_roots, node) {
-		if ((tmp->id.segment == root->id.segment)
-		    && (tmp->id.bus == root->id.bus)) {
-			int bus = 0;
-			acpi_status status;
-
-			printk(KERN_ERR PREFIX
-				    "Wrong _BBN value, reboot"
-				    " and use option 'pci=noacpi'\n");
-
-			status = try_get_root_bridge_busnr(device->handle, &bus);
-			if (ACPI_FAILURE(status))
-				break;
-			if (bus != root->id.bus) {
-				printk(KERN_INFO PREFIX
-				       "PCI _CRS %d overrides _BBN 0\n", bus);
-				root->id.bus = bus;
-			}
-			break;
+	status = try_get_root_bridge_busnr(device->handle, &bus);
+	if (ACPI_SUCCESS(status))
+		root->id.bus = bus;
+	else {
+		status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN,					       NULL, &value);
+		if (ACPI_SUCCESS(status))
+			root->id.bus = (u16) value;
+		else if (status == AE_NOT_FOUND)
+			root->id.bus = 0;
+		else {
+			ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BBN"));
+			result = -ENODEV;
+			goto end;
 		}
 	}
+
 	/*
 	 * Device & Function
 	 * -----------------
#2 lore raw

Re: [PATCH 1/5] ACPI: pci_root: check _CRS, then _BBN for downstream bus number

Quoted 18 lines · Bjorn Helgaas

To find a host bridge's downstream bus number, we currently look at _BBN first. If _BBN returns a bus number we've already seen, we conclude that _BBN was wrong and look for a bus number in _CRS.

However, the spec[1] (figure 5-5 and the example in sec 9.12.1) and an ACPI FAQ[2] suggest that the OS should use _CRS to discover the bus number range, and that _BBN is really intended to bootstrap _CRS methods that reference PCI opregions.

This patch makes us always look at _CRS first. If _CRS doesn't supply a bus number, we look at _BBN. If _BBN doesn't exist, we default to zero. This makes the behavior consistent regardless of device discovery order. Previously, if A and B had duplicate _BBNs and we found A first, we'd only look at B's _CRS, whereas if we found B first, we'd only look at A's _CRS.

I'm told that Windows discovers host bridge bus numbers using _CRS, so it should be fairly safe to rely on this BIOS functionality.

Quoted 6 lines

This patch also removes two misleading messages: we printed the "Wrong _BBN value, reboot and use option 'pci=noacpi'" message before looking at _CRS, so we would likely find the bus number in _CRS, the system would work fine, and the user would be confused. The "PCI _CRS %d overrides _BBN 0" message incorrectly assumes _BBN was zero, and it's useless anyway because we print the segment/bus number a few lines later.

Though it might be very stupid question, can I ask you a question?

Is minimum value of PCI bus number range reported by _CRS always equal to the bus number of the PCI root bus? I glanced over the specs, but I could not confirm it.

Thanks,
Kenji Kaneshige

Quoted 100 lines

References:

    [1] http://www.acpi.info/DOWNLOADS/ACPIspec30b.pdf
    [2] http://www.acpi.info/acpi_faq.htm _BBN/_CRS discussion
    http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWAR05005_WinHEC05.ppt (slide 17)
    http://bugzilla.kernel.org/show_bug.cgi?id=1662 ASUS PR-DLS
    http://bugzilla.kernel.org/show_bug.cgi?id=1127 ASUS PR-DLSW
    http://bugzilla.kernel.org/show_bug.cgi?id=1741 ASUS PR-DLS533
Signed-off-by Bjorn Helgaas <[email protected]>
Reviewed-by Alex Chiang <[email protected]>
CC Shaohua Li <[email protected]>
CC Kenji Kaneshige <[email protected]>
---
 drivers/acpi/pci_root.c |   54 ++++++++++++++---------------------------------
 1 files changed, 16 insertions(+), 38 deletions(-)
drivers/acpi/pci_root.c +16 -38
index f341b07..7847732 100644
@@ -470,12 +470,12 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	int result = 0;
 	struct acpi_pci_root *root = NULL;
-	struct acpi_pci_root *tmp;
 	acpi_status status = AE_OK;
 	unsigned long long value = 0;
 	acpi_handle handle = NULL;
 	struct acpi_device *child;
 	u32 flags, base_flags;
+	int bus;
 
 
 	if (!device)

@@ -523,46 +523,24 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	/* 
 	 * Bus
 	 * ---
-	 * Obtained via _BBN, if exists, otherwise assumed to be zero (0).
+	 * Check _CRS first, then _BBN.  If no _BBN, default to zero.
 	 */
-	status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN, NULL,
-				       &value);
-	switch (status) {
-	case AE_OK:
-		root->id.bus = (u16) value;
-		break;
-	case AE_NOT_FOUND:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Assuming bus 0 (no _BBN)\n"));
-		root->id.bus = 0;
-		break;
-	default:
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BBN"));
-		result = -ENODEV;
-		goto end;
-	}
-
-	/* Some systems have wrong _BBN */
-	list_for_each_entry(tmp, &acpi_pci_roots, node) {
-		if ((tmp->id.segment == root->id.segment)
-		    && (tmp->id.bus == root->id.bus)) {
-			int bus = 0;
-			acpi_status status;
-
-			printk(KERN_ERR PREFIX
-				    "Wrong _BBN value, reboot"
-				    " and use option 'pci=noacpi'\n");
-
-			status = try_get_root_bridge_busnr(device->handle, &bus);
-			if (ACPI_FAILURE(status))
-				break;
-			if (bus != root->id.bus) {
-				printk(KERN_INFO PREFIX
-				       "PCI _CRS %d overrides _BBN 0\n", bus);
-				root->id.bus = bus;
-			}
-			break;
+	status = try_get_root_bridge_busnr(device->handle, &bus);
+	if (ACPI_SUCCESS(status))
+		root->id.bus = bus;
+	else {
+		status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN,					       NULL, &value);
+		if (ACPI_SUCCESS(status))
+			root->id.bus = (u16) value;
+		else if (status == AE_NOT_FOUND)
+			root->id.bus = 0;
+		else {
+			ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BBN"));
+			result = -ENODEV;
+			goto end;
 		}
 	}
+
 	/*
 	 * Device & Function
 	 * -----------------
author
#3 lore raw

PATCH 2/5 ACPI: pci_root: simplify acpi_pci_root_add() control flow

In reply to Bjorn Helgaas

By looking up the segment & bus number earlier, we don't have to worry about cleaning up if it fails.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c |  100 ++++++++++++++++++-----------------------------
 1 files changed, 38 insertions(+), 62 deletions(-)
drivers/acpi/pci_root.c +38 -62
index 7847732..c780008 100644
@@ -185,19 +185,22 @@ get_root_bridge_busnr_callback(struct acpi_resource *resource, void *data)
 	return AE_OK;
 }
 
-static acpi_status try_get_root_bridge_busnr(acpi_handle handle, int *busnum)
+static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
+					     unsigned long long *bus)
 {
 	acpi_status status;
+	int busnum;
 
-	*busnum = -1;
+	busnum = -1;
 	status =
 	    acpi_walk_resources(handle, METHOD_NAME__CRS,
-				get_root_bridge_busnr_callback, busnum);
+				get_root_bridge_busnr_callback, &busnum);
 	if (ACPI_FAILURE(status))
 		return status;
 	/* Check if we really get a bus number from _CRS */
-	if (*busnum == -1)
+	if (busnum == -1)
 		return AE_ERROR;
+	*bus = busnum;
 	return AE_OK;
 }
 

@@ -468,24 +471,39 @@ EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
-	int result = 0;
-	struct acpi_pci_root *root = NULL;
-	acpi_status status = AE_OK;
-	unsigned long long value = 0;
-	acpi_handle handle = NULL;
+	unsigned long long segment, bus;
+	acpi_status status;
+	int result;
+	struct acpi_pci_root *root;
+	acpi_handle handle;
 	struct acpi_device *child;
 	u32 flags, base_flags;
-	int bus;
 
+	segment = 0;
+	status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
+				       &segment);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		printk(KERN_ERR PREFIX "can't evaluate _SEG\n");
+		return -ENODEV;
+	}
 
-	if (!device)
-		return -EINVAL;
+	/* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
+	bus = 0;
+	status = try_get_root_bridge_busnr(device->handle, &bus);
+	if (ACPI_FAILURE(status)) {
+		status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN,					       NULL, &bus);
+		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+			printk(KERN_ERR PREFIX
+			     "no bus number in _CRS and can't evaluate _BBN\n");
+			return -ENODEV;
+		}
+	}
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
 	if (!root)
 		return -ENOMEM;
-	INIT_LIST_HEAD(&root->node);
 
+	INIT_LIST_HEAD(&root->node);
 	root->device = device;
 	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);

@@ -498,54 +516,13 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
 	acpi_pci_osc_support(root, flags);
 
-	/* 
-	 * Segment
-	 * -------
-	 * Obtained via _SEG, if exists, otherwise assumed to be zero (0).
-	 */
-	status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
-				       &value);
-	switch (status) {
-	case AE_OK:
-		root->id.segment = (u16) value;
-		break;
-	case AE_NOT_FOUND:
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-				  "Assuming segment 0 (no _SEG)\n"));
-		root->id.segment = 0;
-		break;
-	default:
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _SEG"));
-		result = -ENODEV;
-		goto end;
-	}
-
-	/* 
-	 * Bus
-	 * ---
-	 * Check _CRS first, then _BBN.  If no _BBN, default to zero.
-	 */
-	status = try_get_root_bridge_busnr(device->handle, &bus);
-	if (ACPI_SUCCESS(status))
-		root->id.bus = bus;
-	else {
-		status = acpi_evaluate_integer(device->handle, METHOD_NAME__BBN,					       NULL, &value);
-		if (ACPI_SUCCESS(status))
-			root->id.bus = (u16) value;
-		else if (status == AE_NOT_FOUND)
-			root->id.bus = 0;
-		else {
-			ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BBN"));
-			result = -ENODEV;
-			goto end;
-		}
-	}
-
 	/*
 	 * Device & Function
 	 * -----------------
 	 * Obtained from _ADR (which has already been evaluated for us).
 	 */
+	root->id.segment = segment & 0xFFFF;
+	root->id.bus = bus & 0xFF;
 	root->id.device = device->pnp.bus_address >> 16;
 	root->id.function = device->pnp.bus_address & 0xFFFF;
 

@@ -611,13 +588,12 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	if (flags != base_flags)
 		acpi_pci_osc_support(root, flags);
 
-      end:
-	if (result) {
-		if (!list_empty(&root->node))
-			list_del(&root->node);
-		kfree(root);
-	}
+	return 0;
 
+end:
+	if (!list_empty(&root->node))
+		list_del(&root->node);
+	kfree(root);
 	return result;
 }
author
#4 lore raw

PATCH 3/5 ACPI: pci_root: use driver data rather than list lookup

In reply to Bjorn Helgaas

There's no need to search the list to find the acpi_pci_root structure. We saved it as device->driver_data when we added the device.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)
drivers/acpi/pci_root.c +4 -17
index c780008..e490a2e 100644
@@ -599,30 +599,17 @@ end:
 
 static int acpi_pci_root_start(struct acpi_device *device)
 {
-	struct acpi_pci_root *root;
-
+	struct acpi_pci_root *root = acpi_driver_data(device);
 
-	list_for_each_entry(root, &acpi_pci_roots, node) {
-		if (root->device == device) {
-			pci_bus_add_devices(root->bus);
-			return 0;
-		}
-	}
-	return -ENODEV;
+	pci_bus_add_devices(root->bus);
+	return 0;
 }
 
 static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
-	struct acpi_pci_root *root = NULL;
-
-
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
-
-	root = acpi_driver_data(device);
+	struct acpi_pci_root *root = acpi_driver_data(device);
 
 	kfree(root);
-
 	return 0;
 }
author
#5 lore raw

PATCH 4/5 ACPI: pci_root: simplify list traversals

In reply to Bjorn Helgaas

Using list_for_each_entry() makes traversing the root list easier.

Signed-off-by Bjorn Helgaas <[email protected]>
Reviewed-by Alex Chiang <[email protected]>
---
 drivers/acpi/pci_root.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)
drivers/acpi/pci_root.c +9 -14
index e490a2e..d9d34e2 100644
@@ -82,7 +82,7 @@ static DEFINE_MUTEX(osc_lock);
 int acpi_pci_register_driver(struct acpi_pci_driver *driver)
 {
 	int n = 0;
-	struct list_head *entry;
+	struct acpi_pci_root *root;
 
 	struct acpi_pci_driver **pptr = &sub_driver;
 	while (*pptr)

@@ -92,9 +92,7 @@ int acpi_pci_register_driver(struct acpi_pci_driver *driver)
 	if (!driver->add)
 		return 0;
 
-	list_for_each(entry, &acpi_pci_roots) {
-		struct acpi_pci_root *root;
-		root = list_entry(entry, struct acpi_pci_root, node);
+	list_for_each_entry(root, &acpi_pci_roots, node) {
 		driver->add(root->device->handle);
 		n++;
 	}

@@ -106,7 +104,7 @@ EXPORT_SYMBOL(acpi_pci_register_driver);
 
 void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
 {
-	struct list_head *entry;
+	struct acpi_pci_root *root;
 
 	struct acpi_pci_driver **pptr = &sub_driver;
 	while (*pptr) {

@@ -120,23 +118,19 @@ void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
 	if (!driver->remove)
 		return;
 
-	list_for_each(entry, &acpi_pci_roots) {
-		struct acpi_pci_root *root;
-		root = list_entry(entry, struct acpi_pci_root, node);
+	list_for_each_entry(root, &acpi_pci_roots, node)
 		driver->remove(root->device->handle);
-	}
 }
 
 EXPORT_SYMBOL(acpi_pci_unregister_driver);
 
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
 {
-	struct acpi_pci_root *tmp;
+	struct acpi_pci_root *root;
 	
-	list_for_each_entry(tmp, &acpi_pci_roots, node) {
-		if ((tmp->id.segment == (u16) seg) && (tmp->id.bus == (u16) bus))
-			return tmp->device->handle;
-	}
+	list_for_each_entry(root, &acpi_pci_roots, node)
+		if ((root->id.segment == (u16) seg) && (root->id.bus == (u16) bus))
+			return root->device->handle;
 	return NULL;		
 }
 

@@ -325,6 +319,7 @@ static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
 static struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 {
 	struct acpi_pci_root *root;
+
 	list_for_each_entry(root, &acpi_pci_roots, node) {
 		if (root->device->handle == handle)
 			return root;
author
#6 lore raw

PATCH 5/5 ACPI: pci_root: remove unused dev/fn information

In reply to Bjorn Helgaas

We never use the PCI device & function number, so remove it to make it clear that it's not needed. Many PCI host bridges don't even appear in config space, so it's meaningless to look at stuff from _ADR, which doesn't exist in that case.

Signed-off-by Bjorn Helgaas <[email protected]>
Reviewed-by Alex Chiang <[email protected]>
---
 drivers/acpi/pci_root.c |   25 +++++++++----------------
 1 files changed, 9 insertions(+), 16 deletions(-)
drivers/acpi/pci_root.c +9 -16
index d9d34e2..8a5bf3b 100644
@@ -63,9 +63,10 @@ static struct acpi_driver acpi_pci_root_driver = {
 
 struct acpi_pci_root {
 	struct list_head node;
-	struct acpi_device * device;
-	struct acpi_pci_id id;
+	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 */

@@ -129,7 +130,7 @@ acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
 	struct acpi_pci_root *root;
 	
 	list_for_each_entry(root, &acpi_pci_roots, node)
-		if ((root->id.segment == (u16) seg) && (root->id.bus == (u16) bus))
+		if ((root->segment == (u16) seg) && (root->bus_nr == (u16) bus))
 			return root->device->handle;
 	return NULL;		
 }

@@ -500,6 +501,8 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 
 	INIT_LIST_HEAD(&root->node);
 	root->device = device;
+	root->segment = segment & 0xFFFF;
+	root->bus_nr = bus & 0xFF;
 	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
 	device->driver_data = root;

@@ -512,16 +515,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	acpi_pci_osc_support(root, flags);
 
 	/*
-	 * Device & Function
-	 * -----------------
-	 * Obtained from _ADR (which has already been evaluated for us).
-	 */
-	root->id.segment = segment & 0xFFFF;
-	root->id.bus = bus & 0xFF;
-	root->id.device = device->pnp.bus_address >> 16;
-	root->id.function = device->pnp.bus_address & 0xFFFF;
-
-	/*
 	 * TBD: Need PCI interface for enumeration/configuration of roots.
 	 */
 

@@ -530,7 +523,7 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 
 	printk(KERN_INFO PREFIX "%s [%s] (%04x:%02x)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
-	       root->id.segment, root->id.bus);
+	       root->segment, root->bus_nr);
 
 	/*
 	 * Scan the Root Bridge

@@ -539,11 +532,11 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	 * PCI namespace does not get created until this call is made (and 
 	 * thus the root bridge's pci_dev does not exist).
 	 */
-	root->bus = pci_acpi_scan_root(device, root->id.segment, root->id.bus);
+	root->bus = pci_acpi_scan_root(device, segment, bus);
 	if (!root->bus) {
 		printk(KERN_ERR PREFIX
 			    "Bus %04x:%02x not present in PCI namespace\n",
-			    root->id.segment, root->id.bus);
+			    root->segment, root->bus_nr);
 		result = -ENODEV;
 		goto end;
 	}