PCI, ACPI: pci root bus hotplug support / pci match_driver

When system support hotplug bridge with children hotplug slots, we need to make sure that parent bridge get preallocated resource so later when device is plugged into children slot, those children devices will get resource allocated.

We do not meet this problem, because for pcie hotplug card, when acpiphp is used, pci_scan_bridge will set that for us when detect hotplug bit in slot cap.

Reported-and-tested-by Jason Baron <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

drivers/pci/hotplug/acpiphp_glue.c +26 -1

index 91b5ad8..1e5c5df 100644
@@ -797,6 +797,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 	}
 }
 
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+	struct acpiphp_func *func;
+
+	if (!dev->subordinate)
+		return;
+
+	/* quirk, or pcie could set it already */
+	if (dev->is_hotplug_bridge)
+		return;
+
+	if (PCI_SLOT(dev->devfn) != slot->device)
+		return;
+
+	list_for_each_entry(func, &slot->funcs, sibling) {
+		if (PCI_FUNC(dev->devfn) == func->function) {
+			/* check if this bridge has ejectable slots */
+			if ((detect_ejectable_slots(func->handle) > 0))
+				dev->is_hotplug_bridge = 1;
+			break;
+		}
+	}
+}
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled

@@ -831,8 +854,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
 			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 				max = pci_scan_bridge(bus, dev, max, pass);
-				if (pass && dev->subordinate)
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
 					pci_bus_size_bridges(dev->subordinate);
+				}
 			}
 		}
 	}

When system support hotplug bridge with children hotplug slots, we need to make sure that parent bridge get preallocated resource so later when device is plugged into children slot, those children devices will get resource allocated.

We do not meet this problem, because for pcie hotplug card, when acpiphp is used, pci_scan_bridge will set that for us when detect hotplug bit in slot cap.

Reported-and-tested-by Jason Baron <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Acked-by Jason Baron <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

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

index 91b5ad8..1e5c5df 100644
@@ -797,6 +797,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 	}
 }
 
+static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
+{
+	struct acpiphp_func *func;
+
+	if (!dev->subordinate)
+		return;
+
+	/* quirk, or pcie could set it already */
+	if (dev->is_hotplug_bridge)
+		return;

drivers/pci/hotplug/acpiphp_glue.c +6 -1

+		}
+	}
+}
 /**
  * enable_device - enable, configure a slot
  * @slot: slot to be enabled

@@ -831,8 +854,10 @@ static int __ref enable_device(struct acpiphp_slot *slot)
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
 			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 				max = pci_scan_bridge(bus, dev, max, pass);
-				if (pass && dev->subordinate)
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
 					pci_bus_size_bridges(dev->subordinate);
+				}
 			}
 		}
 	}

drivers/pci/hotplug/acpiphp_glue.c static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) · line 819

Quoted 6 lines

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

+     list_for_each_entry(func, &slot->funcs, sibling) {
+             if (PCI_FUNC(dev->devfn) == func->function) {
+                     /* check if this bridge has ejectable slots */
+                     if ((detect_ejectable_slots(func->handle) > 0))
+                             dev->is_hotplug_bridge = 1;
+                     break;

Hi Yinghai,
Need to put the "break" in parentheses of "if"? If the first func which device number == dev->devfn has't ejectable slot,
don't check the rest funcs whether have ejectable slots?

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/setup-bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/setup-bus.c +1 -1

index 6d3591d..7e8739e 100644
@@ -283,7 +283,7 @@ static void assign_requested_resources_sorted(struct list_head *head,
 		idx = res - &dev_res->dev->resource[0];
 		if (resource_size(res) &&
 		    pci_assign_resource(dev_res->dev, idx)) {
-			if (fail_head && !pci_is_root_bus(dev_res->dev->bus)) {
+			if (fail_head) {
 				/*
 				 * if the failed res is for ROM BAR, and it will
 				 * be enabled later, don't add it to the list

Quoted 5 lines · Yinghai Lu

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Well, the above says that we _can_ do the change, but it doesn't explain why it is needed. So what's the reason why we need to do that?

Quoted 7 lines · Rafael J. Wysocki

Quoted 5 lines · Yinghai Lu

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Quoted 2 lines

Well, the above says that we _can_ do the change, but it doesn't explain why it is needed. So what's the reason why we need to do that?

In extreme case we could need to reallocate resource for device just under root bus.

otherwise, those devices just under root bus will not be assigned resources again.

Quoted 11 lines · Yinghai Lu

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Yinghai Lu

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Well, the above says that we _can_ do the change, but it doesn't explain why it is needed. So what's the reason why we need to do that?

Quoted 5 lines

In extreme case we could need to reallocate resource for device just under root bus.

otherwise, those devices just under root bus will not be assigned resources again.

IOW, our current code will not cover the case when we hot plug a host bridge and need to reassign resources for devices integrated into it?

Quoted 15 lines · Rafael J. Wysocki

Quoted 14 lines · Yinghai Lu

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Yinghai Lu

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Well, the above says that we _can_ do the change, but it doesn't explain why it is needed. So what's the reason why we need to do that?

In extreme case we could need to reallocate resource for device just under root bus.

Quoted 5 lines

Quoted 2 lines

otherwise, those devices just under root bus will not be assigned resources again.

IOW, our current code will not cover the case when we hot plug a host bridge and need to reassign resources for devices integrated into it?

yes. in extreme case if _CRS range is not big enough.

Quoted 19 lines · Yinghai Lu

Quoted 18 lines · Rafael J. Wysocki

Quoted 16 lines · Yinghai Lu

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Yinghai Lu

We can stop trying according to try number now and do not need to use root_bus checking as stop sign anymore.

In extreme case we could need to reallocate resource for device just under root bus.

Well, the above says that we _can_ do the change, but it doesn't explain why it is needed. So what's the reason why we need to do that?

In extreme case we could need to reallocate resource for device just under root bus.

otherwise, those devices just under root bus will not be assigned resources again.

Quoted 4 lines

Quoted 2 lines

IOW, our current code will not cover the case when we hot plug a host bridge and need to reassign resources for devices integrated into it?

yes. in extreme case if _CRS range is not big enough.

It would be good to say that in the changelog too. It's a clear justification for the change if we're going to support root bridge hotplug (which we are).

Otherwise irq_desc for pci bridge with hot-added ioapic can not be on local node.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/probe.c |    1 +
 1 file changed, 1 insertion(+)

drivers/pci/probe.c +1 -0

index 2dcd22d..b97dea5 100644
@@ -1300,6 +1300,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	dev->dev.release = pci_release_dev;
 	pci_dev_get(dev);
 
+	set_dev_node(&dev->dev, pcibus_to_node(bus));
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;

From: Jiang Liu <[email protected]>

Function pci_get_dev_by_id() will hold a reference count on the pci device returned, so pci_dev_present() should release the corresponding reference count to avoid memory leakage.

Signed-off-by Jiang Liu <[email protected]>

---
 drivers/pci/search.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/pci/search.c +5 -5

index bf969ba..d0627fa 100644
@@ -319,13 +319,13 @@ int pci_dev_present(const struct pci_device_id *ids)
 	WARN_ON(in_interrupt());
 	while (ids->vendor || ids->subvendor || ids->class_mask) {
 		found = pci_get_dev_by_id(ids, NULL);
-		if (found)
-			goto exit;
+		if (found) {
+			pci_dev_put(found);
+			return 1;
+		}
 		ids++;
 	}
-exit:
-	if (found)
-		return 1;
+
 	return 0;
 }
 EXPORT_SYMBOL(pci_dev_present);

From: Jiang Liu <[email protected]>

According to device model documentation, the way to create/destroy PCI devices should be symmetric.

/**
* device_del - delete device from system.
* @dev: device.
*
* This is the first part of the device unregistration
* sequence. This removes the device from the lists we control
* from here, has it removed from the other driver model
* subsystems it was added to in device_add(), and removes it
* from the kobject hierarchy.
*
* NOTE: this should be called manually _iff_ device_add() was
* also called manually.
*/

The rule is to either use
1) device_register()/device_unregister()
or
2) device_initialize()/device_add()/device_del()/put_device().

So change PCI core logic to follow the rule and get rid of the redundant pci_dev_get()/pci_dev_put() pair.

Signed-off-by Jiang Liu <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

Signed-by Yinghai Lu <[email protected]>

---
 drivers/pci/probe.c  |    1 -
 drivers/pci/remove.c |    4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

drivers/pci/probe.c +0 -1

index b97dea5..48b35e1 100644
@@ -1298,7 +1298,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
-	pci_dev_get(dev);
 
 	set_dev_node(&dev->dev, pcibus_to_node(bus));
 	dev->dev.dma_mask = &dev->dma_mask;

drivers/pci/remove.c +2 -2

index 7c0fd92..fc38c48 100644
@@ -22,7 +22,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 	if (dev->is_added) {
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
-		device_unregister(&dev->dev);
+		device_del(&dev->dev);
 		dev->is_added = 0;
 	}
 

@@ -37,7 +37,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	up_write(&pci_bus_sem);
 
 	pci_free_resources(dev);
-	pci_dev_put(dev);
+	put_device(&dev->dev);
 }
 
 void pci_remove_bus(struct pci_bus *bus)

From: Jiang Liu <[email protected]>

When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device, the notification handler can't hold reference count to the new PCI bus because the device object for the new bus (pci_dev->subordinate->dev) hasn't been initialized yet.

Split the registration of PCI bus device into two stages as below, so that the event handler could hold reference count to the new PCI bus when handling BUS_NOTIFY_ADD_DEVICE event.

1) device_initialize(&pci_dev->dev)
2) device_initialize(&pci_dev->subordinate->dev)
3) notify BUS_NOTIFY_ADD_DEVICE event for pci_dev
4) device_add(&pci_dev->dev)
5) device_add(&pci_dev->subordinate->dev)

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/bus.c    |    2 +-
 drivers/pci/probe.c  |    4 +++-
 drivers/pci/remove.c |   10 +++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

drivers/pci/bus.c +1 -1

index 847f3ca..5f9c728 100644
@@ -200,7 +200,7 @@ int pci_bus_add_child(struct pci_bus *bus)
 	if (bus->bridge)
 		bus->dev.parent = bus->bridge;
 
-	retval = device_register(&bus->dev);
+	retval = device_add(&bus->dev);
 	if (retval)
 		return retval;
 

drivers/pci/probe.c +3 -1

index 48b35e1..dc4fde3 100644
@@ -642,6 +642,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
+	device_initialize(&child->dev);
 
 	/*
 	 * Set up the primary, secondary and subordinate

@@ -1678,7 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
 	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(&b->dev);
+	device_initialize(&b->dev);
+	error = device_add(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
 

drivers/pci/remove.c +5 -5

index fc38c48..a1fdd0f 100644
@@ -48,11 +48,11 @@ void pci_remove_bus(struct pci_bus *bus)
 	list_del(&bus->node);
 	pci_bus_release_busn_res(bus);
 	up_write(&pci_bus_sem);
-	if (!bus->is_added)
-		return;
-
-	pci_remove_legacy_files(bus);
-	device_unregister(&bus->dev);
+	if (bus->is_added) {
+		pci_remove_legacy_files(bus);
+		device_del(&bus->dev);
+	}
+	put_device(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 

Quoted 6 lines · Yinghai Lu

From: Jiang Liu <[email protected]>

When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device, the notification handler can't hold reference count to the new PCI bus because the device object for the new bus (pci_dev->subordinate->dev) hasn't been initialized yet.

It doesn't look like BUS_NOTIFY_ADD_DEVICE is used by the PCI bus type in the mainline.

Is it in linux-next?

If not, then I don't see why we need this patch.

Thanks,
Rafael

Quoted 75 lines

Split the registration of PCI bus device into two stages as below, so that the event handler could hold reference count to the new PCI bus when handling BUS_NOTIFY_ADD_DEVICE event.

1) device_initialize(&pci_dev->dev)
2) device_initialize(&pci_dev->subordinate->dev)
3) notify BUS_NOTIFY_ADD_DEVICE event for pci_dev
4) device_add(&pci_dev->dev)
5) device_add(&pci_dev->subordinate->dev)

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/bus.c    |    2 +-
 drivers/pci/probe.c  |    4 +++-
 drivers/pci/remove.c |   10 +++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

drivers/pci/bus.c +1 -1

index 847f3ca..5f9c728 100644
@@ -200,7 +200,7 @@ int pci_bus_add_child(struct pci_bus *bus)
 	if (bus->bridge)
 		bus->dev.parent = bus->bridge;
 
-	retval = device_register(&bus->dev);
+	retval = device_add(&bus->dev);
 	if (retval)
 		return retval;
 

drivers/pci/probe.c +3 -1

index 48b35e1..dc4fde3 100644
@@ -642,6 +642,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
+	device_initialize(&child->dev);
 
 	/*
 	 * Set up the primary, secondary and subordinate

@@ -1678,7 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
 	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(&b->dev);
+	device_initialize(&b->dev);
+	error = device_add(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
 

drivers/pci/remove.c +5 -5

index fc38c48..a1fdd0f 100644
@@ -48,11 +48,11 @@ void pci_remove_bus(struct pci_bus *bus)
 	list_del(&bus->node);
 	pci_bus_release_busn_res(bus);
 	up_write(&pci_bus_sem);
-	if (!bus->is_added)
-		return;
-
-	pci_remove_legacy_files(bus);
-	device_unregister(&bus->dev);
+	if (bus->is_added) {
+		pci_remove_legacy_files(bus);
+		device_del(&bus->dev);
+	}
+	put_device(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 

Quoted 11 lines · Rafael J. Wysocki

Quoted 6 lines · Yinghai Lu

From: Jiang Liu <[email protected]>

When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device, the notification handler can't hold reference count to the new PCI bus because the device object for the new bus (pci_dev->subordinate->dev) hasn't been initialized yet.

It doesn't look like BUS_NOTIFY_ADD_DEVICE is used by the PCI bus type in the mainline.

Quoted 3 lines

Is it in linux-next?

If not, then I don't see why we need this patch.

Hi Rafael,
Originally it's a preparation patch for pci_slot/acpiphp/pci_bind
related fixups. With recent work from you to remove bind/unbind from struct acpi_bus_type, we have no dependency on this patch anymore.

Current all acpi_bus_trim callers have rmdevice to 1. that means it will remove all acpi devices.

When 0, is passed, it will keep the parent.

For root bus hotremove support, we need to have pci device to be removed before acpi devices.

So try to keep all acpi devices, and only stop drivers with them.

This change should be safe because all current callers all have 1 passed.

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/scan.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/acpi/scan.c +1 -4

index e380345..db7664e 100644
@@ -1669,10 +1669,7 @@ int acpi_bus_trim(struct acpi_device *start, int rmdevice)
 			child = parent;
 			parent = parent->parent;
 
-			if (level == 0)
-				err = acpi_bus_remove(child, rmdevice);
-			else
-				err = acpi_bus_remove(child, 1);
+			err = acpi_bus_remove(child, rmdevice);
 
 			continue;
 		}

Quoted 11 lines · Yinghai Lu

Current all acpi_bus_trim callers have rmdevice to 1. that means it will remove all acpi devices.

When 0, is passed, it will keep the parent.

For root bus hotremove support, we need to have pci device to be removed before acpi devices.

So try to keep all acpi devices, and only stop drivers with them.

This change should be safe because all current callers all have 1 passed.

I'm not sure how the chanelog is related to the patch itself.

The patch modifies the behavior of acpi_bus_trim() to avoid removing all devices (not just the start point) for rmdevice==0, which doesn't really change the functionality, because all callers pass rmdevice=1 anyway.

Yes, we can make this change, but why is it necessary?

And why don't we remove the rmdevice argument from acpi_bus_trim() altogether?

Quoted 19 lines · Rafael J. Wysocki

Quoted 11 lines · Yinghai Lu

Current all acpi_bus_trim callers have rmdevice to 1. that means it will remove all acpi devices.

When 0, is passed, it will keep the parent.

For root bus hotremove support, we need to have pci device to be removed before acpi devices.

So try to keep all acpi devices, and only stop drivers with them.

This change should be safe because all current callers all have 1 passed.

I'm not sure how the chanelog is related to the patch itself.

The patch modifies the behavior of acpi_bus_trim() to avoid removing all devices (not just the start point) for rmdevice==0, which doesn't really change the functionality, because all callers pass rmdevice=1 anyway.

Quoted 3 lines

Yes, we can make this change, but why is it necessary?

And why don't we remove the rmdevice argument from acpi_bus_trim() altogether?

this patch is not needed after your changes with acpi_bus_trim.

It causes confusion.

We may only need acpi hp for pci host bridge.

Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple.

(quoted diff) +0 -7

-v2: put back pci_root_hp change in one patch
-v3: add pcibios_resource_survey_bus() calling
-v4: remove not needed code with remove_bridge
-v5: put back support for acpiphp support for slots just on root bus.
-v6: change some functions to *_p2p_* to make it more clean.
-v7: split hot_added change out.
-v8: Move to pci_root.c instead of adding another file requested by Bjorn.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/acpi/pci_root.c            |  220 ++++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/acpiphp_glue.c |   59 +++-------
 2 files changed, 235 insertions(+), 44 deletions(-)

drivers/acpi/pci_root.c +0 -0

index 471b2dc..5c1f462c 100644

drivers/acpi/pci_root.c +11 -0

+
+#define ACPI_STA_FUNCTIONING	(0x00000008)
+
+static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
+{
+	struct acpi_root_bridge *bridge;
+
+	list_for_each_entry(bridge, &acpi_root_bridge_list, list)
+		if (bridge->handle == handle)
+			return bridge;
+

drivers/acpi/pci_root.c +16 -0

+	/* if the bridge doesn't have _STA, we assume it is always there */
+	status = acpi_get_handle(handle, "_STA", &dummy_handle);
+	if (ACPI_SUCCESS(status)) {
+		unsigned long long tmp;
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_DEBUG "%s: _STA evaluation failure\n",
+						 __func__);
+			return;
+		}
+		if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+			/* don't register this object */
+			return;
+	}
+

drivers/acpi/pci_root.c +21 -0

+
+	list_add(&bridge->list, &acpi_root_bridge_list);
+}
+
+struct acpi_root_hp_work {
+	struct work_struct work;
+	acpi_handle handle;
+	u32 type;
+	void *context;
+};
+
+static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type,
+					void *context,
+					void (*func)(struct work_struct *work))
+{
+	struct acpi_root_hp_work *hp_work;
+	int ret;
+
+	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+	if (!hp_work)
+		return;

drivers/acpi/pci_root.c +12 -0

+
+static void handle_root_bridge_insertion(acpi_handle handle)
+{
+	struct acpi_device *device, *pdevice;
+	acpi_handle phandle;
+	int ret_val;
+
+	acpi_get_parent(handle, &phandle);
+	if (acpi_bus_get_device(phandle, &pdevice)) {
+		printk(KERN_DEBUG "no parent device, assuming NULL\n");
+		pdevice = NULL;
+	}

drivers/acpi/pci_root.c +21 -0

+		printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
+	}
+	if (acpi_bus_add(handle))
+		printk(KERN_ERR "cannot add bridge to acpi list\n");
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+	struct acpi_root_bridge *bridge;
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	struct acpi_root_hp_work *hp_work;
+	acpi_handle handle;
+	u32 type;
+
+	hp_work = container_of(work, struct acpi_root_hp_work, work);
+	handle = hp_work->handle;
+	type = hp_work->type;
+
+	bridge = acpi_root_handle_to_bridge(handle);

drivers/acpi/pci_root.c +56 -0

+			add_acpi_root_bridge(handle);
+		}
+
+		break;
+
+	case ACPI_NOTIFY_DEVICE_CHECK:
+		/* device check */
+		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+				 objname);
+		if (!bridge) {
+			handle_root_bridge_insertion(handle);
+			add_acpi_root_bridge(handle);
+		}
+		break;
+
+	default:
+		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
+				 type, objname);
+		break;
+	}
+
+	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+}
+
+static void handle_hotplug_event_root(acpi_handle handle, u32 type,
+					void *context)
+{
+	alloc_acpi_root_hp_work(handle, type, context,
+				_handle_hotplug_event_root);
+}
+
+static acpi_status __init
+find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	int *count = (int *)context;
+
+	if (!acpi_is_root_bridge(handle))
+		return AE_OK;
+
+	(*count)++;
+
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+				handle_hotplug_event_root, NULL);
+	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+
+	add_acpi_root_bridge(handle);
+
+	return AE_OK;
+}
+
+static int __init acpi_pci_root_hp_init(void)

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 680

Quoted 9 lines

drivers/acpi/pci_root.c +5 -0

@@ -673,3 +673,223 @@ int __init acpi_pci_root_init(void)
 
      return 0;
 }
+
+/*
+ * Separated from drivers/pci/hotplug/acpiphp_glue.c
+ *   only support root bridge
+ */

This comment will be useless after applying the patch.

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 687

Quoted 7 lines

drivers/acpi/pci_root.c +7 -0

+
+static LIST_HEAD(acpi_root_bridge_list);
+struct acpi_root_bridge {
+     struct list_head list;
+     acpi_handle handle;
+     u32 flags;
+};

We have struct acpi_pci_root already. Why do we need this in addition?

Also, we have acpi_pci_roots, so why do we need another list of root bridges?

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 691

Quoted 4 lines

drivers/acpi/pci_root.c +4 -0

+
+/* bridge flags */
+#define ROOT_BRIDGE_HAS_EJ0  (0x00000002)
+#define ROOT_BRIDGE_HAS_PS3  (0x00000080)

What is that needed for?

Quoted 12 lines

drivers/acpi/pci_root.c +11 -0

+
+#define ACPI_STA_FUNCTIONING (0x00000008)
+
+static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
+{
+     struct acpi_root_bridge *bridge;
+
+     list_for_each_entry(bridge, &acpi_root_bridge_list, list)
+             if (bridge->handle == handle)
+                     return bridge;
+

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 712

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+     return NULL;
+}
+
+/* allocate and initialize host bridge data structure */
+static void add_acpi_root_bridge(acpi_handle handle)
+{
+     struct acpi_root_bridge *bridge;
+     acpi_handle dummy_handle;
+     acpi_status status;
+

Why do we need to evaluate all of the methods directly here?

Don't we have a struct acpi_device for handle already?

Quoted 16 lines

drivers/acpi/pci_root.c +16 -0

+     /* if the bridge doesn't have _STA, we assume it is always there */
+     status = acpi_get_handle(handle, "_STA", &dummy_handle);
+     if (ACPI_SUCCESS(status)) {
+             unsigned long long tmp;
+
+             status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+             if (ACPI_FAILURE(status)) {
+                     printk(KERN_DEBUG "%s: _STA evaluation failure\n",
+                                              __func__);
+                     return;
+             }
+             if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+                     /* don't register this object */
+                     return;
+     }
+

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 738

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+     bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
+     if (!bridge)
+             return;
+
+     bridge->handle = handle;
+
+     if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
+             bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
+     if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
+             bridge->flags |= ROOT_BRIDGE_HAS_PS3;

All of this attempts to duplicate the scanning code from scan.c in a very incomplete and questionable way.

For example, what if the root bridge has _PR0?

Quoted 27 lines

drivers/acpi/pci_root.c +26 -0

+
+     list_add(&bridge->list, &acpi_root_bridge_list);
+}
+
+struct acpi_root_hp_work {
+     struct work_struct work;
+     acpi_handle handle;
+     u32 type;
+     void *context;
+};
+
+static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type,
+                                     void *context,
+                                     void (*func)(struct work_struct *work))
+{
+     struct acpi_root_hp_work *hp_work;
+     int ret;
+
+     hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+     if (!hp_work)
+             return;
+
+     hp_work->handle = handle;
+     hp_work->type = type;
+     hp_work->context = context;
+

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 769

Quoted 5 lines

drivers/acpi/pci_root.c +5 -0

+     INIT_WORK(&hp_work->work, func);
+     ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
+     if (!ret)
+             kfree(hp_work);
+}

The function above is called only once and used by __init stuff only. Why don't we move it to the caller and mark that caller as __init too?

Quoted 17 lines

drivers/acpi/pci_root.c +16 -0

+
+static void handle_root_bridge_insertion(acpi_handle handle)
+{
+     struct acpi_device *device, *pdevice;
+     acpi_handle phandle;
+     int ret_val;
+
+     acpi_get_parent(handle, &phandle);
+     if (acpi_bus_get_device(phandle, &pdevice)) {
+             printk(KERN_DEBUG "no parent device, assuming NULL\n");
+             pdevice = NULL;
+     }
+     if (!acpi_bus_get_device(handle, &device)) {
+             /* check if  pci root_bus is removed */
+             struct acpi_pci_root *root = acpi_driver_data(device);
+             if (pci_find_bus(root->segment, root->secondary.start))

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 791

Quoted 6 lines

drivers/acpi/pci_root.c +6 -0

+                     return;
+
+             printk(KERN_DEBUG "bus exists... trim\n");
+             /* this shouldn't be in here, so remove
+              * the bus then re-add it...
+              */

Why? Shouldn't we just bail out here?

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 796

Quoted 6 lines

drivers/acpi/pci_root.c +5 -0

+             /* remove pci devices at first */
+             ret_val = acpi_bus_trim(device, 0);
+             printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
+             /* remove acpi devices */
+             ret_val = acpi_bus_trim(device, 1);

Oh, I see why you need the second argument of acpi_bus_trim() now.

Do I think correctly that you want acpi_pci_root_remove() to be executed before all of the struct acpi_device objects are removed? In which case why don't we call acpi_pci_root_remove() directly before doing the acpi_bus_trim(device, 1) instead of making the code next to impossible to understand?

Quoted 27 lines

drivers/acpi/pci_root.c +26 -0

+             printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
+     }
+     if (acpi_bus_add(handle))
+             printk(KERN_ERR "cannot add bridge to acpi list\n");
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+     struct acpi_root_bridge *bridge;
+     char objname[64];
+     struct acpi_buffer buffer = { .length = sizeof(objname),
+                                   .pointer = objname };
+     struct acpi_root_hp_work *hp_work;
+     acpi_handle handle;
+     u32 type;
+
+     hp_work = container_of(work, struct acpi_root_hp_work, work);
+     handle = hp_work->handle;
+     type = hp_work->type;
+
+     bridge = acpi_root_handle_to_bridge(handle);
+
+     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+     switch (type) {
+     case ACPI_NOTIFY_BUS_CHECK:

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 827

Quoted 5 lines

drivers/acpi/pci_root.c +5 -0

+             /* bus enumerate */
+             printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
+                              objname);
+             if (!bridge) {
+                     handle_root_bridge_insertion(handle);

I don't think we should call add_acpi_root_bridge() for handle if the above fails. So probably handle_root_bridge_insertion() should return error codes?

Review int __init acpi_pci_root_init(void) · line 828

Quoted 2 lines

(quoted diff) +1 -0

+                     add_acpi_root_bridge(handle);

Quoted 60 lines

drivers/acpi/pci_root.c +60 -0

+             }
+
+             break;
+
+     case ACPI_NOTIFY_DEVICE_CHECK:
+             /* device check */
+             printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+                              objname);
+             if (!bridge) {
+                     handle_root_bridge_insertion(handle);
+                     add_acpi_root_bridge(handle);
+             }
+             break;
+
+     default:
+             printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
+                              type, objname);
+             break;
+     }
+
+     kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+}
+
+static void handle_hotplug_event_root(acpi_handle handle, u32 type,
+                                     void *context)
+{
+     alloc_acpi_root_hp_work(handle, type, context,
+                             _handle_hotplug_event_root);
+}
+
+static acpi_status __init
+find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+     char objname[64];
+     struct acpi_buffer buffer = { .length = sizeof(objname),
+                                   .pointer = objname };
+     int *count = (int *)context;
+
+     if (!acpi_is_root_bridge(handle))
+             return AE_OK;
+
+     (*count)++;
+
+     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+                             handle_hotplug_event_root, NULL);
+     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+
+     add_acpi_root_bridge(handle);
+
+     return AE_OK;
+}
+
+static int __init acpi_pci_root_hp_init(void)
+{
+     int num = 0;
+
+     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+             ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 893

Quoted 5 lines

drivers/acpi/pci_root.c +5 -0

+
+     printk(KERN_DEBUG "Found %d acpi root devices\n", num);
+
+     return 0;
+}

Why do we need to do that from an initcall? Couldn't we simply hook up that code to acpi_pci_root_add() somewhere?

And even if not, why don't we call acpi_pci_root_hp_init() from acpi_pci_root_init()?

All of the changes in acpiphp_glue.c look reasonable to me.

Quoted 12 lines · Rafael J. Wysocki

Review int __init acpi_pci_root_init(void) · line 680

Quoted 9 lines

(quoted diff) +5 -0

@@ -673,3 +673,223 @@ int __init acpi_pci_root_init(void)
 
      return 0;
 }
+
+/*
+ * Separated from drivers/pci/hotplug/acpiphp_glue.c
+ *   only support root bridge
+ */

This comment will be useless after applying the patch.

Quoted 9 lines

Review int __init acpi_pci_root_init(void) · line 687

Quoted 7 lines

(quoted diff) +7 -0

+
+static LIST_HEAD(acpi_root_bridge_list);
+struct acpi_root_bridge {
+     struct list_head list;
+     acpi_handle handle;
+     u32 flags;
+};

We have struct acpi_pci_root already. Why do we need this in addition?

will address that in following patch.

Quoted 23 lines · Yinghai Lu

Quoted 21 lines · Rafael J. Wysocki

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 680

Quoted 9 lines

drivers/acpi/pci_root.c +5 -0

@@ -673,3 +673,223 @@ int __init acpi_pci_root_init(void)
 
      return 0;
 }
+
+/*
+ * Separated from drivers/pci/hotplug/acpiphp_glue.c
+ *   only support root bridge
+ */

This comment will be useless after applying the patch.

drivers/acpi/pci_root.c int __init acpi_pci_root_init(void) · line 687

Quoted 7 lines

drivers/acpi/pci_root.c +7 -0

+
+static LIST_HEAD(acpi_root_bridge_list);
+struct acpi_root_bridge {
+     struct list_head list;
+     acpi_handle handle;
+     u32 flags;
+};

We have struct acpi_pci_root already. Why do we need this in addition?

Quoted 1 line

will address that in following patch.

please check if you are ok with merging attached 5 patches in to one patches instead.

Do I understand correctly that you're asking whether to merge the attached patches together, so as to obtain one combined patch?

If so, yes, I'd like that to be done.

Add missing hot_remove support for root device.

How to test it?
Find out root bus number to acpi root name mapping from dmesg or /sys

echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
to remove root bus

-v2: separate stop and remove, so it will be safe for comingi
acpi_pci_bind_notify() changes.

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/pci_root.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

drivers/acpi/pci_root.c +12 -0

index 5c1f462c..5ae36d8 100644
@@ -740,6 +740,12 @@ static void add_acpi_root_bridge(acpi_handle handle)
 	list_add(&bridge->list, &acpi_root_bridge_list);
 }
 
+static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
+{
+	list_del(&bridge->list);
+	kfree(bridge);
+}
+
 struct acpi_root_hp_work {
 	struct work_struct work;
 	acpi_handle handle;

@@ -800,6 +806,61 @@ static void handle_root_bridge_insertion(acpi_handle handle)
 		printk(KERN_ERR "cannot add bridge to acpi list\n");
 }
 
+static int acpi_root_evaluate_object(acpi_handle handle, char *cmd, int val)
+{
+	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+

drivers/acpi/pci_root.c +16 -0

+	}
+
+	return 0;
+}
+
+static void handle_root_bridge_removal(acpi_handle handle,
+		 struct acpi_root_bridge *bridge)
+{
+	u32 flags = 0;
+	struct acpi_device *device;
+
+	if (bridge) {
+		flags = bridge->flags;
+		remove_acpi_root_bridge(bridge);
+	}
+

drivers/acpi/pci_root.c +2 -0

+}
+
 static void _handle_hotplug_event_root(struct work_struct *work)
 {
 	struct acpi_root_bridge *bridge;

@@ -840,6 +901,12 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		}
 		break;

Quoted 45 lines · Yinghai Lu

Add missing hot_remove support for root device.

How to test it?
Find out root bus number to acpi root name mapping from dmesg or /sys

echo "\_SB.PCIB 3" > /sys/kernel/debug/acpi/sci_notify
to remove root bus

-v2: separate stop and remove, so it will be safe for comingi
acpi_pci_bind_notify() changes.

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/pci_root.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

drivers/acpi/pci_root.c +12 -0

index 5c1f462c..5ae36d8 100644
@@ -740,6 +740,12 @@ static void add_acpi_root_bridge(acpi_handle handle)
      list_add(&bridge->list, &acpi_root_bridge_list);
 }
 
+static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
+{
+     list_del(&bridge->list);
+     kfree(bridge);
+}
+
 struct acpi_root_hp_work {
      struct work_struct work;
      acpi_handle handle;

@@ -800,6 +806,61 @@ static void handle_root_bridge_insertion(acpi_handle handle)
              printk(KERN_ERR "cannot add bridge to acpi list\n");
 }
 
+static int acpi_root_evaluate_object(acpi_handle handle, char *cmd, int val)
+{
+     acpi_status status;
+     struct acpi_object_list arg_list;
+     union acpi_object arg;
+

drivers/acpi/pci_root.c static void handle_root_bridge_insertion(acpi_handle handle) · line 824

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+     arg_list.count = 1;
+     arg_list.pointer = &arg;
+     arg.type = ACPI_TYPE_INTEGER;
+     arg.integer.value = val;
+
+     status = acpi_evaluate_object(handle, cmd, &arg_list, NULL);
+     if (ACPI_FAILURE(status)) {
+             pr_warn("%s: %s to %d failed\n",
+                              __func__, cmd, val);
+             return -1;

Please use a meaningful error code.

Quoted 16 lines

drivers/acpi/pci_root.c +16 -0

+     }
+
+     return 0;
+}
+
+static void handle_root_bridge_removal(acpi_handle handle,
+              struct acpi_root_bridge *bridge)
+{
+     u32 flags = 0;
+     struct acpi_device *device;
+
+     if (bridge) {
+             flags = bridge->flags;
+             remove_acpi_root_bridge(bridge);
+     }
+

drivers/acpi/pci_root.c static void handle_root_bridge_insertion(acpi_handle handle) · line 850

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+     if (!acpi_bus_get_device(handle, &device)) {
+             int ret_val;
+
+             /* remove pci devices at first */
+             ret_val = acpi_bus_trim(device, 0);
+             printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
+
+             /* remove acpi devices */
+             ret_val = acpi_bus_trim(device, 1);
+             printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);

First of all, I don't agree with the way acpi_bus_trim() is used here, as I said in the previous message.

Second, this code duplicates the code you're adding on [08/22] almost exactly. Please put it into a one separate function instead of duplicating it like this.

drivers/acpi/pci_root.c static void handle_root_bridge_insertion(acpi_handle handle) · line 858

Quoted 8 lines

drivers/acpi/pci_root.c +8 -0

+     }
+
+     if (flags & ROOT_BRIDGE_HAS_PS3) {
+             acpi_status status;
+
+             status = acpi_evaluate_object(handle, "_PS3", NULL, NULL);
+             if (ACPI_FAILURE(status))
+                     pr_warn("%s: _PS3 failed\n", __func__);

No, please. acpi_device_set_power() is for that.

drivers/acpi/pci_root.c static void handle_root_bridge_insertion(acpi_handle handle) · line 861

Quoted 3 lines

drivers/acpi/pci_root.c +3 -0

+     }
+     if (flags & ROOT_BRIDGE_HAS_EJ0)
+             acpi_root_evaluate_object(handle, "_EJ0", 1);

That seems to duplicate some code from scan.c.

From: Tang Chen <[email protected]>

We need to find out if one handle is for root bridge, and install notify handler for it to handle pci root bus hot add.
At that time, root bridge acpi device is not created yet.

So acpi_match_device_ids() will not work.

This patch add a function to check if new acpi handle's HID matches a list of IDs. The new api use acpi_device_info instead acpi_device.

-v2: updated changelog, also check length for string info...
change checking sequence by moving string comaring close to for loop.
- Yinghai
-v3: change to _GPL according to Rafael

Signed-off-by Tang Chen <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/scan.c     |   33 +++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 35 insertions(+)

drivers/acpi/scan.c +33 -0

index db7664e..8883539 100644
@@ -470,6 +470,39 @@ int acpi_match_device_ids(struct acpi_device *device,
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
+int acpi_match_object_info_ids(struct acpi_device_info *info,
+			       const struct acpi_device_id *ids)
+{
+	const struct acpi_device_id *id;
+	char *str;
+	u32 len;
+	int i;
+
+	len = info->hardware_id.length;
+	if (len) {
+		str = info->hardware_id.string;
+		if (str)
+			for (id = ids; id->id[0]; id++)
+				if (!strcmp((char *)id->id, str))
+					return 0;
+	}
+
+	for (i = 0; i < info->compatible_id_list.count; i++) {
+		len = info->compatible_id_list.ids[i].length;
+		if (!len)
+			continue;
+		str = info->compatible_id_list.ids[i].string;
+		if (!str)
+			continue;
+		for (id = ids; id->id[0]; id++)
+			if (!strcmp((char *)id->id, str))
+				return 0;
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(acpi_match_object_info_ids);
+
 static void acpi_free_ids(struct acpi_device *device)
 {
 	struct acpi_hardware_id *id, *tmp;

include/acpi/acpi_bus.h +2 -0

index a9e1421..2246ba9 100644
@@ -355,6 +355,8 @@ int acpi_bus_trim(struct acpi_device *start, int rmdevice);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids);
+int acpi_match_object_info_ids(struct acpi_device_info *info,
+			       const struct acpi_device_id *ids);
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 

From: Jiang Liu <[email protected]>

The code in pci_root_hp.c depends on function acpi_is_root_bridge() to check whether an ACPI object is a PCI host bridge or not. If an ACPI device hasn't been created for the ACPI object yet, function acpi_is_root_bridge() will return false even if the object is a PCI host bridge object. That behavior will cause two issues:
1) No ACPI notification handler installed for PCI host bridges absent at startup, so hotplug events for those bridges won't be handled.
2) rescan_root_bridge() can't reenumerate offlined PCI host bridges because the ACPI devices have been already destroyed.

So use acpi_match_object_info_ids() to correctly detect PCI host bridges.

(quoted diff) +0 -2

-v2: update to use acpi_match_object_info_ids() from Tang Chen  - Yinghai
-v3: drop the PNP0A008, according to Bjorn.

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/pci_root.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

drivers/acpi/pci_root.c +2 -0

index 5ae36d8..d30fb94 100644
@@ -923,6 +923,23 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 				_handle_hotplug_event_root);
 }
 
+static bool acpi_is_root_bridge_object(acpi_handle handle)
+{

drivers/acpi/pci_root.c +5 -0

+	kfree(info);
+
+	return ret;
+}
+
 static acpi_status __init
 find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 {

Quoted 33 lines · Yinghai Lu

From: Jiang Liu <[email protected]>

The code in pci_root_hp.c depends on function acpi_is_root_bridge() to check whether an ACPI object is a PCI host bridge or not. If an ACPI device hasn't been created for the ACPI object yet, function acpi_is_root_bridge() will return false even if the object is a PCI host bridge object. That behavior will cause two issues:
1) No ACPI notification handler installed for PCI host bridges absent at startup, so hotplug events for those bridges won't be handled.
2) rescan_root_bridge() can't reenumerate offlined PCI host bridges because the ACPI devices have been already destroyed.

So use acpi_match_object_info_ids() to correctly detect PCI host bridges.

(quoted diff) +0 -2

-v2: update to use acpi_match_object_info_ids() from Tang Chen  - Yinghai
-v3: drop the PNP0A008, according to Bjorn.

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/pci_root.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

drivers/acpi/pci_root.c +0 -0

index 5ae36d8..d30fb94 100644
@@ -923,6 +923,23 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 				_handle_hotplug_event_root);
 }
 

drivers/acpi/pci_root.c static void handle_hotplug_event_root(acpi_handle handle, u32 type, · line 937

Quoted 12 lines

drivers/acpi/pci_root.c +12 -0

+static bool acpi_is_root_bridge_object(acpi_handle handle)
+{
+	struct acpi_device_info *info = NULL;
+	acpi_status status;
+	bool ret;
+
+	status = acpi_get_object_info(handle, &info);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	ret = !acpi_match_object_info_ids(info, root_device_ids);
+

Well, I kind of don't understand why don't we check info->flags against ACPI_PCI_ROOT_BRIDGE that acpi_get_object_info() sets for us if it finds a PCI root bridge?

Quoted 11 lines

drivers/acpi/pci_root.c +5 -0

+	kfree(info);
+
+	return ret;
+}
+
 static acpi_status __init
 find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 {

drivers/acpi/pci_root.c find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) · line 954

Quoted 8 lines

drivers/acpi/pci_root.c +1 -1

@@ -931,7 +948,7 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 				      .pointer = objname };
 	int *count = (int *)context;
 
-	if (!acpi_is_root_bridge(handle))
+	if (!acpi_is_root_bridge_object(handle))
 		return AE_OK;
 
 	(*count)++;

Thanks,
Rafael

From: Tang Chen <[email protected]>

acpi_install_notify_handler() could fail. So check the exit status and give a better debug info.

Signed-off-by Tang Chen <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/acpi/pci_root.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/acpi/pci_root.c +1 -0

index d30fb94..862abcc 100644
@@ -943,6 +943,7 @@ static bool acpi_is_root_bridge_object(acpi_handle handle)
 static acpi_status __init
 find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
+	acpi_status status;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };

@@ -955,9 +956,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 

Tang noticed that hotplug through container will not update acpi_root_bridge list.

After closely checking, we don't need that for struct for tracking and could use acpi_pci_root directly.

Reported-by Tang Chen <[email protected]>

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/acpi/pci_root.c |  109 +++++++++--------------------------------------
 1 file changed, 20 insertions(+), 89 deletions(-)

drivers/acpi/pci_root.c +20 -89

index 862abcc..697ec65 100644
@@ -679,73 +679,12 @@ int __init acpi_pci_root_init(void)
  *	only support root bridge
  */
 
-static LIST_HEAD(acpi_root_bridge_list);
-struct acpi_root_bridge {
-	struct list_head list;
-	acpi_handle handle;
-	u32 flags;
-};
-
 /* bridge flags */
 #define ROOT_BRIDGE_HAS_EJ0	(0x00000002)
 #define ROOT_BRIDGE_HAS_PS3	(0x00000080)
 
 #define ACPI_STA_FUNCTIONING	(0x00000008)
 
-static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
-{
-	struct acpi_root_bridge *bridge;
-
-	list_for_each_entry(bridge, &acpi_root_bridge_list, list)
-		if (bridge->handle == handle)
-			return bridge;
-
-	return NULL;
-}
-
-/* allocate and initialize host bridge data structure */
-static void add_acpi_root_bridge(acpi_handle handle)
-{
-	struct acpi_root_bridge *bridge;
-	acpi_handle dummy_handle;
-	acpi_status status;
-
-	/* if the bridge doesn't have _STA, we assume it is always there */
-	status = acpi_get_handle(handle, "_STA", &dummy_handle);
-	if (ACPI_SUCCESS(status)) {
-		unsigned long long tmp;
-
-		status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
-		if (ACPI_FAILURE(status)) {
-			printk(KERN_DEBUG "%s: _STA evaluation failure\n",
-						 __func__);
-			return;
-		}
-		if ((tmp & ACPI_STA_FUNCTIONING) == 0)
-			/* don't register this object */
-			return;
-	}
-
-	bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
-	if (!bridge)
-		return;
-
-	bridge->handle = handle;
-
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
-		bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
-		bridge->flags |= ROOT_BRIDGE_HAS_PS3;
-
-	list_add(&bridge->list, &acpi_root_bridge_list);
-}
-
-static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
-{
-	list_del(&bridge->list);
-	kfree(bridge);
-}
-
 struct acpi_root_hp_work {
 	struct work_struct work;
 	acpi_handle handle;

@@ -827,28 +766,25 @@ static int acpi_root_evaluate_object(acpi_handle handle, char *cmd, int val)
 	return 0;
 }
 
-static void handle_root_bridge_removal(acpi_handle handle,
-		 struct acpi_root_bridge *bridge)
+static void handle_root_bridge_removal(struct acpi_device *device)
 {
+	int ret_val;
 	u32 flags = 0;
-	struct acpi_device *device;
+	acpi_handle dummy_handle;
+	acpi_handle handle = device->handle;
 
-	if (bridge) {
-		flags = bridge->flags;
-		remove_acpi_root_bridge(bridge);
-	}
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
+		flags |= ROOT_BRIDGE_HAS_EJ0;
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
+		flags |= ROOT_BRIDGE_HAS_PS3;
 
-	if (!acpi_bus_get_device(handle, &device)) {
-		int ret_val;
+	/* remove pci devices at first */
+	ret_val = acpi_bus_trim(device, 0);
+	printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
 
-		/* remove pci devices at first */
-		ret_val = acpi_bus_trim(device, 0);
-		printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
-
-		/* remove acpi devices */
-		ret_val = acpi_bus_trim(device, 1);
-		printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
-	}
+	/* remove acpi devices */
+	ret_val = acpi_bus_trim(device, 1);
+	printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
 
 	if (flags & ROOT_BRIDGE_HAS_PS3) {
 		acpi_status status;

@@ -863,7 +799,7 @@ static void handle_root_bridge_removal(acpi_handle handle,
 
 static void _handle_hotplug_event_root(struct work_struct *work)
 {
-	struct acpi_root_bridge *bridge;
+	struct acpi_pci_root *root;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };

@@ -875,7 +811,7 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 	handle = hp_work->handle;
 	type = hp_work->type;
 
-	bridge = acpi_root_handle_to_bridge(handle);
+	root = acpi_pci_find_root(handle);
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 

@@ -884,10 +820,8 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		/* bus enumerate */
 		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!root)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 
 		break;
 

@@ -895,17 +829,16 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		/* device check */
 		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!root)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		/* request device eject */
 		printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
 				 objname);
-		handle_root_bridge_removal(handle, bridge);
+		if (root)
+			handle_root_bridge_removal(root->device);
 		break;
 	default:
 		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",

@@ -965,8 +898,6 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 		printk(KERN_DEBUG "acpi root: %s notify handler is installed\n",
 				 objname);
 
-	add_acpi_root_bridge(handle);
-
 	return AE_OK;
 }