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

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

In extreme case we could need to reallocate resource for device just under root bus. For pci root bus hot-add, we need to retry to assign resources to pci devices just under pci 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

Will need to use it for pci root bridge hotplug support, rename *acpiphp* to *acpi* and move to osc.c.
Also make kacpi_hotplug_wq static after that.

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

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/osl.c                 |   24 +++++++++++++++++++--
 drivers/pci/hotplug/acpiphp_glue.c |   42 ++++++------------------------------
 include/acpi/acpiosxf.h            |    9 +++++++-
 3 files changed, 36 insertions(+), 39 deletions(-)

drivers/acpi/osl.c +12 -2

index 3ff2678..afcce46 100644
@@ -84,8 +84,7 @@ static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
-struct workqueue_struct *kacpi_hotplug_wq;
-EXPORT_SYMBOL(kacpi_hotplug_wq);
+static struct workqueue_struct *kacpi_hotplug_wq;
 
 /*
  * This list of permanent mappings is for memory that may be accessed from

@@ -1778,3 +1777,24 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
 {
 	__acpi_os_prepare_sleep = func;
 }
+
+void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
+			void (*func)(struct work_struct *work))
+{
+	struct acpi_hp_work *hp_work;
+	int ret;
+
+	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+	if (!hp_work)
+		return;
+

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

index 79db296..55e03b6 100644
@@ -1203,34 +1203,6 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK ;
 }
 
-struct acpiphp_hp_work {
-	struct work_struct work;
-	acpi_handle handle;
-	u32 type;
-	void *context;
-};
-
-static void alloc_acpiphp_hp_work(acpi_handle handle, u32 type,
-				  void *context,
-				  void (*func)(struct work_struct *work))
-{
-	struct acpiphp_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;
-
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
-	if (!ret)
-		kfree(hp_work);
-}
-
 static void _handle_hotplug_event_bridge(struct work_struct *work)
 {
 	struct acpiphp_bridge *bridge;

@@ -1239,11 +1211,11 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 				      .pointer = objname };
 	struct acpi_device *device;
 	int num_sub_bridges = 0;
-	struct acpiphp_hp_work *hp_work;
+	struct acpi_hp_work *hp_work;
 	acpi_handle handle;
 	u32 type;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(work, struct acpi_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 

@@ -1346,8 +1318,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_bridge);
+	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
 }
 
 static void _handle_hotplug_event_func(struct work_struct *work)

@@ -1356,12 +1327,12 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpiphp_hp_work *hp_work;
+	struct acpi_hp_work *hp_work;
 	acpi_handle handle;
 	u32 type;
 	void *context;
 
-	hp_work = container_of(work, struct acpiphp_hp_work, work);
+	hp_work = container_of(work, struct acpi_hp_work, work);
 	handle = hp_work->handle;
 	type = hp_work->type;
 	context = hp_work->context;

@@ -1422,8 +1393,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpiphp_hp_work(handle, type, context,
-			      _handle_hotplug_event_func);
+	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
 static acpi_status

include/acpi/acpiosxf.h +0 -0

index 4315274..adab63c 100644

Quoted 46 lines · Yinghai Lu

Will need to use it for pci root bridge hotplug support, rename *acpiphp* to *acpi* and move to osc.c.
Also make kacpi_hotplug_wq static after that.

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

Cc Len Brown <[email protected]>

Cc [email protected]

---
 drivers/acpi/osl.c                 |   24 +++++++++++++++++++--
 drivers/pci/hotplug/acpiphp_glue.c |   42 ++++++------------------------------
 include/acpi/acpiosxf.h            |    9 +++++++-
 3 files changed, 36 insertions(+), 39 deletions(-)

drivers/acpi/osl.c +16 -2

index 3ff2678..afcce46 100644
@@ -84,8 +84,7 @@ static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
-struct workqueue_struct *kacpi_hotplug_wq;
-EXPORT_SYMBOL(kacpi_hotplug_wq);
+static struct workqueue_struct *kacpi_hotplug_wq;
 
 /*
  * This list of permanent mappings is for memory that may be accessed from

@@ -1778,3 +1777,24 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
 {
      __acpi_os_prepare_sleep = func;
 }
+
+void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
+                     void (*func)(struct work_struct *work))
+{
+     struct acpi_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/osl.c void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, · line 1800

Quoted 6 lines

drivers/acpi/osl.c +6 -0

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

That should be EXPORT_SYMBOL_GPL().

Quoted 5 lines

include/acpi/acpiosxf.h +0 -0

index 4315274..adab63c 100644
@@ -193,7 +193,14 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);

include/acpi/acpiosxf.h void acpi_os_fixed_event_count(u32 fixed_event_number); · line 196

Quoted 4 lines

include/acpi/acpiosxf.h +0 -1

 /*
  * Threads and Scheduling
  */
-extern struct workqueue_struct *kacpi_hotplug_wq;

include/acpi/acpiosxf.h is related to ACPICA, so while removing kacpi_hotplug_wq from it should be OK, please put the rest into acpi_bus.h, preferably next to the definition of acpi_eject_event.

We have partial hot-add support in acpiphp driver, and it is confusing.

Move host bridge hot-add support to pci_root.c, and keep acpiphp simple, also add hot-remove support in pci_root.c.

How to test it: if sci_emu patch is applied,

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

echo "\_SB.PCIB 1" > /sys/kernel/debug/acpi/sci_notify
to add back root bus

(quoted diff) +0 -8

-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.
-v9: Fold three following patches into this one for easy review:

a: Add missing hot_remove support for root device.
b: Tang Chen 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.
c: Tang Chen found handle_root_bridge_removal is very similiar to
acpi_bus_hot_remove_device(). Change to handle_root_bridge_removal to use acpi_bus_hot_remove_device.

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

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

drivers/acpi/pci_root.c +15 -0

index bf5108a..3ce5d80 100644
@@ -655,3 +655,142 @@ int __init acpi_pci_root_init(void)
 
 	return 0;
 }
+
+/* Support root bridge hotplug */
+
+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)) {

drivers/acpi/pci_root.c +15 -0

+		return;
+
+	ej_event->device = device;
+	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+
+	acpi_bus_hot_remove_device(ej_event);
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+	struct acpi_pci_root *root;
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	struct acpi_hp_work *hp_work;

drivers/acpi/pci_root.c +61 -0

+		if (!root)
+			handle_root_bridge_insertion(handle);
+
+		break;
+
+	case ACPI_NOTIFY_DEVICE_CHECK:
+		/* device check */
+		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+				 objname);
+		if (!root)
+			handle_root_bridge_insertion(handle);
+		break;
+
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		/* request device eject */
+		printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
+				 objname);
+		if (root)
+			handle_root_bridge_removal(root->device);
+		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_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);
+
+	return AE_OK;
+}
+
+static int __init acpi_pci_root_hp_init(void)
+{
+	int num = 0;

drivers/pci/hotplug/acpiphp_glue.c +14 -38

index 55e03b6..45917a5 100644
@@ -543,10 +543,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
 	acpi_status status;
 	acpi_handle handle = bridge->handle;
 
-	status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+	if (bridge->type != BRIDGE_TYPE_HOST) {
+		status = acpi_remove_notify_handler(handle,
+					    ACPI_SYSTEM_NOTIFY,
 					    handle_hotplug_event_bridge);
-	if (ACPI_FAILURE(status))
-		err("failed to remove notify handler\n");
+		if (ACPI_FAILURE(status))
+			err("failed to remove notify handler\n");
+	}
 
 	if ((bridge->type != BRIDGE_TYPE_HOST) &&
 	    ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)) {

@@ -630,9 +633,6 @@ static void remove_bridge(struct acpi_pci_root *root)
 	bridge = acpiphp_handle_to_bridge(handle);
 	if (bridge)
 		cleanup_bridge(bridge);
-	else
-		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-					   handle_hotplug_event_bridge);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)

@@ -1123,18 +1123,12 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus)
 }
 
 /* Program resources in newly inserted bridge */
-static int acpiphp_configure_bridge (acpi_handle handle)
+static int acpiphp_configure_p2p_bridge(acpi_handle handle)
 {
-	struct pci_bus *bus;
+	struct pci_dev *pdev = acpi_get_pci_dev(handle);
+	struct pci_bus *bus = pdev->subordinate;
 
-	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_dev_put(pdev);
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);

@@ -1144,7 +1138,7 @@ static int acpiphp_configure_bridge (acpi_handle handle)
 	return 0;
 }
 
-static void handle_bridge_insertion(acpi_handle handle, u32 type)
+static void handle_p2p_bridge_insertion(acpi_handle handle, u32 type)
 {
 	struct acpi_device *device;
 

@@ -1162,8 +1156,8 @@ static void handle_bridge_insertion(acpi_handle handle, u32 type)
 		err("ACPI device object missing\n");
 		return;
 	}
-	if (!acpiphp_configure_bridge(handle))
-		add_bridge(handle);
+	if (!acpiphp_configure_p2p_bridge(handle))
+		add_p2p_bridge(handle);
 	else
 		err("cannot configure and start bridge\n");
 

@@ -1221,7 +1215,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 
 	if (acpi_bus_get_device(handle, &device)) {
 		/* This bridge must have just been physically inserted */
-		handle_bridge_insertion(handle, type);
+		handle_p2p_bridge_insertion(handle, type);
 		goto out;
 	}
 

@@ -1396,21 +1390,6 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
 }
 
-static acpi_status
-find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	int *count = (int *)context;
-
-	if (!acpi_is_root_bridge(handle))
-		return AE_OK;
-
-	(*count)++;
-	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-				    handle_hotplug_event_bridge, NULL);
-
-	return AE_OK ;
-}
-
 static struct acpi_pci_driver acpi_pci_hp_driver = {
 	.add =		add_bridge,
 	.remove =	remove_bridge,

@@ -1421,15 +1400,7 @@ static struct acpi_pci_driver acpi_pci_hp_driver = {
  */
 int __init acpiphp_glue_init(void)
 {
-	int num = 0;
-
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,

Quoted 66 lines · Yinghai Lu

We have partial hot-add support in acpiphp driver, and it is confusing.

Move host bridge hot-add support to pci_root.c, and keep acpiphp simple, also add hot-remove support in pci_root.c.

How to test it: if sci_emu patch is applied,

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

echo "\_SB.PCIB 1" > /sys/kernel/debug/acpi/sci_notify
to add back root bus

(quoted diff) +0 -8

-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.
-v9: Fold three following patches into this one for easy review:
      a: Add missing hot_remove support for root device.
      b: Tang Chen 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.
      c: Tang Chen found handle_root_bridge_removal is very similiar to
         acpi_bus_hot_remove_device().  Change to handle_root_bridge_removal
         to use acpi_bus_hot_remove_device.

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

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

drivers/acpi/pci_root.c +21 -0

index bf5108a..3ce5d80 100644
@@ -655,3 +655,142 @@ int __init acpi_pci_root_init(void)
 
      return 0;
 }
+
+/* Support root bridge hotplug */
+
+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))
+                     return;
+
+             printk(KERN_DEBUG "bus exists... trim\n");

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

Quoted 4 lines

drivers/acpi/pci_root.c +4 -0

+             /* this shouldn't be in here, so remove
+              * the bus then re-add it...
+              */
+             ret_val = acpi_bus_trim(device);

You said that this followed acpiphp, but the purpose of the trimming in there seems to be to handle surprise removal and re-insertion, which I'm not sure is OK with something like a host bridge.

The drawback is that if we have a spurious ACPI_NOTIFY_BUS_CHECK or ACPI_NOTIFY_DEVICE_CHECK, we'll be trying to remove the whole bus here in response. That doesn't sound quite right.

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

Quoted 12 lines

drivers/acpi/pci_root.c +12 -0

+             printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
+     }
+     if (acpi_bus_add(handle))
+             printk(KERN_ERR "cannot add bridge to acpi list\n");
+}
+
+static void handle_root_bridge_removal(struct acpi_device *device)
+{
+     struct acpi_eject_event *ej_event;
+
+     ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+     if (!ej_event)

Shouldn't we do acpi_evaluate_hotplug_ost() here?

Quoted 20 lines

drivers/acpi/pci_root.c +19 -0

+             return;
+
+     ej_event->device = device;
+     ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+
+     acpi_bus_hot_remove_device(ej_event);
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+     struct acpi_pci_root *root;
+     char objname[64];
+     struct acpi_buffer buffer = { .length = sizeof(objname),
+                                   .pointer = objname };
+     struct acpi_hp_work *hp_work;
+     acpi_handle handle;
+     u32 type;
+
+     hp_work = container_of(work, struct acpi_hp_work, work);

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

Quoted 6 lines

drivers/acpi/pci_root.c +6 -0

+     handle = hp_work->handle;
+     type = hp_work->type;
+
+     root = acpi_pci_find_root(handle);
+
+     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);

Is the path guaranteed not to be longer than 64 characters?

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

Quoted 7 lines

drivers/acpi/pci_root.c +6 -0

+
+     switch (type) {
+     case ACPI_NOTIFY_BUS_CHECK:
+             /* bus enumerate */
+             printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
+                              objname);

pr_debug() would be nicer (and analogously below).

Quoted 68 lines

drivers/acpi/pci_root.c +67 -0

+             if (!root)
+                     handle_root_bridge_insertion(handle);
+
+             break;
+
+     case ACPI_NOTIFY_DEVICE_CHECK:
+             /* device check */
+             printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+                              objname);
+             if (!root)
+                     handle_root_bridge_insertion(handle);
+             break;
+
+     case ACPI_NOTIFY_EJECT_REQUEST:
+             /* request device eject */
+             printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
+                              objname);
+             if (root)
+                     handle_root_bridge_removal(root->device);
+             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_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);
+
+     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);
+
+     printk(KERN_DEBUG "Found %d acpi root devices\n", num);
+

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

Quoted 4 lines

drivers/acpi/pci_root.c +4 -0

+     return 0;
+}
+
+subsys_initcall(acpi_pci_root_hp_init);

Why do we need a separate initcall here? Is it not enough to call acpi_pci_root_hp_init() from acpi_init() or acpi_scan_init(), or acpi_pci_root_init() even?

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 3ce5d80..f3ceb61 100644
@@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 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 };

Quoted 27 lines · Yinghai Lu

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 3ce5d80..f3ceb61 100644
@@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 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 };

@@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 
      acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 

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

Quoted 7 lines

drivers/acpi/pci_root.c +4 -3

-     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-                             handle_hotplug_event_root, NULL);
-     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+     status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+                                     handle_hotplug_event_root, NULL);
+     if (ACPI_FAILURE(status))
+             printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",

Can you break that line, please? And use pr_debug()?

Quoted 28 lines · Rafael J. Wysocki

Quoted 27 lines · Yinghai Lu

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 3ce5d80..f3ceb61 100644
@@ -762,6 +762,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 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 };

@@ -774,9 +775,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 
      acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 

Quoted 9 lines

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

Quoted 7 lines

drivers/acpi/pci_root.c +4 -3

-     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-                             handle_hotplug_event_root, NULL);
-     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+     status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+                                     handle_hotplug_event_root, NULL);
+     if (ACPI_FAILURE(status))
+             printk(KERN_DEBUG "acpi root: %s notify handler is not installed, exit status: %u\n",

Can you break that line, please? And use pr_debug()?

Long line should be ok, and checkpatch.pl is not complaining about that.

Also keep the complete print out in one line, could make git grep find that code exactly.

Actually I really hate pr_debug(), that will make the generated code different with DEBUG
defined or not. And need to end user to recompile kernel to get debug output if needed.

Could have root bus hot added later and there may be slots that need acpiphp.

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

---
 drivers/pci/hotplug/acpiphp.h      |    1 -
 drivers/pci/hotplug/acpiphp_core.c |   23 ++---------------------
 drivers/pci/hotplug/acpiphp_glue.c |   22 ----------------------
 3 files changed, 2 insertions(+), 44 deletions(-)

drivers/pci/hotplug/acpiphp.h +0 -1

index a1afb5b..b3ead7a 100644
@@ -193,7 +193,6 @@ extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
 /* acpiphp_glue.c */
 extern int acpiphp_glue_init (void);
 extern void acpiphp_glue_exit (void);
-extern int acpiphp_get_num_slots (void);
 typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
 
 extern int acpiphp_enable_slot (struct acpiphp_slot *slot);

drivers/pci/hotplug/acpiphp_core.c +2 -21

index 96316b7..c2fd309 100644
@@ -50,7 +50,6 @@
 bool acpiphp_debug;
 
 /* local variables */
-static int num_slots;
 static struct acpiphp_attention_info *attention_info;
 
 #define DRIVER_VERSION	"0.5"

@@ -272,25 +271,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	return 0;
 }
 
-static int __init init_acpi(void)
-{
-	int retval;
-
-	/* initialize internal data structure etc. */
-	retval = acpiphp_glue_init();
-
-	/* read initial number of slots */
-	if (!retval) {
-		num_slots = acpiphp_get_num_slots();
-		if (num_slots == 0) {
-			acpiphp_glue_exit();
-			retval = -ENODEV;
-		}
-	}
-
-	return retval;
-}
-
 /**
  * release_slot - free up the memory used by a slot
  * @hotplug_slot: slot to free

@@ -379,7 +359,8 @@ static int __init acpiphp_init(void)
 		return 0;
 
 	/* read all the ACPI info from the system */
-	return init_acpi();
+	/* initialize internal data structure etc. */
+	return acpiphp_glue_init();
 }
 
 

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

index 45917a5..25d6918 100644
@@ -1416,28 +1416,6 @@ void  acpiphp_glue_exit(void)
 	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
 }
 
-
-/**
- * acpiphp_get_num_slots - count number of slots in a system
- */
-int __init acpiphp_get_num_slots(void)
-{
-	struct acpiphp_bridge *bridge;
-	int num_slots = 0;
-
-	list_for_each_entry(bridge, &bridge_list, list) {
-		dbg("Bus %04x:%02x has %d slot%s\n",
-				pci_domain_nr(bridge->pci_bus),
-				bridge->pci_bus->number, bridge->nr_slots,
-				bridge->nr_slots == 1 ? "" : "s");
-		num_slots += bridge->nr_slots;
-	}
-
-	dbg("Total %d slots\n", num_slots);
-	return num_slots;
-}
-
-
 /**
  * acpiphp_enable_slot - power on slot
  * @slot: ACPI PHP slot

If that's the case:

Acked-by Rafael J. Wysocki <[email protected]>

but please say something like this in the changelog:

"The result returned by acpiphp_get_num_slots() is meaningless, because
the bridge the slots are under may be added after this function has been called, so drop acpiphp_get_num_slots() and the code using it."

with that we could move out attaching driver for pci device, out of device_add for pci hot add path.

pci_bus_attach_device() will attach driver to pci device.

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

---
 drivers/pci/bus.c        |   10 ++++++++++
 drivers/pci/pci-driver.c |    6 +++++-
 include/linux/pci.h      |    1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

drivers/pci/bus.c +10 -0

index 847f3ca..18c1c6d 100644
@@ -160,6 +160,15 @@ pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 
 void __weak pcibios_resource_survey_bus(struct pci_bus *bus) { }
 
+static void pci_bus_attach_device(struct pci_dev *dev)
+{
+	int ret;
+
+	dev->match_driver = true;
+	ret = device_attach(&dev->dev);
+	WARN_ON(ret < 0);
+}
+
 /**
  * pci_bus_add_device - add a single device
  * @dev: device to add

@@ -181,6 +190,7 @@ int pci_bus_add_device(struct pci_dev *dev)
 	if (retval)
 		return retval;
 
+	pci_bus_attach_device(dev);
 	dev->is_added = 1;
 	pci_proc_attach_device(dev);
 	pci_create_sysfs_dev_files(dev);

drivers/pci/pci-driver.c +5 -1

index f79cbcd..acdcc3c 100644
@@ -1186,9 +1186,13 @@ pci_dev_driver(const struct pci_dev *dev)
 static int pci_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct pci_driver *pci_drv = to_pci_driver(drv);
+	struct pci_driver *pci_drv;
 	const struct pci_device_id *found_id;
 
+	if (!pci_dev->match_driver)
+		return 0;
+
+	pci_drv = to_pci_driver(drv);
 	found_id = pci_match_device(pci_drv, pci_dev);
 	if (found_id)
 		return 1;

include/linux/pci.h +1 -0

index 6860f4d..e2aed11 100644
@@ -286,6 +286,7 @@ struct pci_dev {
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 
+	bool match_driver;
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */
 	unsigned int	multifunction:1;/* Part of multi-function device */

Quoted 4 lines · Yinghai Lu

with that we could move out attaching driver for pci device, out of device_add for pci hot add path.

pci_bus_attach_device() will attach driver to pci device.

Acked-by Rafael J. Wysocki <[email protected]>

for the code, but you still aren't saying in the changelog why the change is needed.

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines · Yinghai Lu

with that we could move out attaching driver for pci device, out of device_add for pci hot add path.

pci_bus_attach_device() will attach driver to pci device.

Quoted 4 lines

Acked-by Rafael J. Wysocki <[email protected]>

for the code, but you still aren't saying in the changelog why the change is needed.

Thanks.

Please check if the changelog is good to you.

---
Subject: [PATCH] PCI: Skip attaching driver in device_add()

We want to add pci device to device tree as early as possible but delay attach driver in next following path.

To make that patch smaller, in this patch:

We add match_driver field in pci_dev and default vaule is false, it will make pci_bus_match fail, so device_add will skip attaching driver, then pci_bus_attach_device() will set match_driver to true so pci_bus_match will return true and device_attach will attach driver to pci device.

---

We want to put created pci device in the device tree as soon as possible.
- just after we find it and create pci_dev struct for it.
so for_pci_dev iteration will not miss them.

But at that time, we can not load driver for them yet. Need to be after pci_assign_unsigned_resources() etc to make sure all pci devices get resource allocated at first.

Move out device registering out of pci_bus_add_devices, and
new pci_bus_add_devices() will do the device_attach work to load pci drivers

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

---
 drivers/pci/bus.c   |   47 +++--------------------------------------------
 drivers/pci/iov.c   |    7 -------
 drivers/pci/probe.c |   34 +++++++++++++++++++++++++++-------
 3 files changed, 30 insertions(+), 58 deletions(-)

drivers/pci/bus.c +1 -21

index 18c1c6d..0a55845 100644
@@ -178,22 +178,9 @@ static void pci_bus_attach_device(struct pci_dev *dev)
  */
 int pci_bus_add_device(struct pci_dev *dev)
 {
-	int retval;
-
-	pci_fixup_device(pci_fixup_final, dev);
-
-	retval = pcibios_add_device(dev);
-	if (retval)
-		return retval;
-
-	retval = device_add(&dev->dev);
-	if (retval)
-		return retval;
-
 	pci_bus_attach_device(dev);
 	dev->is_added = 1;
-	pci_proc_attach_device(dev);
-	pci_create_sysfs_dev_files(dev);
+
 	return 0;
 }
 

@@ -205,21 +192,9 @@ int pci_bus_add_device(struct pci_dev *dev)
  */
 int pci_bus_add_child(struct pci_bus *bus)
 {
-	int retval;
-
-	if (bus->bridge)
-		bus->dev.parent = bus->bridge;
-
-	retval = device_register(&bus->dev);
-	if (retval)

drivers/pci/bus.c +0 -4

 /**

@@ -245,36 +220,20 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 		if (dev->is_added)
 			continue;
 		retval = pci_bus_add_device(dev);
-		if (retval)
-			dev_err(&dev->dev, "Error adding device, continuing\n");
 	}
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		BUG_ON(!dev->is_added);
 
 		child = dev->subordinate;
-		/*
-		 * If there is an unattached subordinate bus, attach

drivers/pci/bus.c +0 -6

 		pci_bus_add_devices(child);
 
-		/*
-		 * register the bus with sysfs as the parent is now
-		 * properly registered.
-		 */
 		if (child->is_added)
 			continue;
 		retval = pci_bus_add_child(child);
-		if (retval)
-			dev_err(&dev->dev, "Error adding bus, continuing\n");
 	}
 }
 

drivers/pci/iov.c +0 -7

index bafd2bb..dbad72e 100644
@@ -48,12 +48,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
 		return NULL;
 
 	pci_bus_insert_busn_res(child, busnr, busnr);
-	child->dev.parent = bus->bridge;
 	rc = pci_bus_add_child(child);
-	if (rc) {
-		pci_remove_bus(child);
-		return NULL;
-	}
 
 	return child;
 }

@@ -123,8 +118,6 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
 	virtfn->is_virtfn = 1;
 
 	rc = pci_bus_add_device(virtfn);
-	if (rc)
-		goto failed1;
 	sprintf(buf, "virtfn%u", id);
 	rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf);
 	if (rc)

drivers/pci/probe.c +27 -5

index 9588207..6392dac 100644
@@ -623,6 +623,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 {
 	struct pci_bus *child;
 	int i;
+	int ret;
 
 	/*
 	 * Allocate a new bus, and inherit stuff from the parent..

@@ -637,8 +638,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	child->bus_flags = parent->bus_flags;
 
 	/* initialize some portions of the bus device, but don't register it
-	 * now as the parent is not properly set up yet.  This device will get
-	 * registered later in pci_bus_add_devices()
+	 * now as the parent is not properly set up yet.
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);

@@ -651,11 +651,14 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	child->primary = parent->busn_res.start;
 	child->busn_res.end = 0xff;
 
-	if (!bridge)
-		return child;
+	if (!bridge) {
+		child->dev.parent = parent->bridge;
+		goto add_dev;
+	}
 
 	child->self = bridge;
 	child->bridge = get_device(&bridge->dev);
+	child->dev.parent = child->bridge;
 	pci_set_bus_of_node(child);
 	pci_set_bus_speed(child);
 

@@ -666,6 +669,13 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	}
 	bridge->subordinate = child;
 
+add_dev:
+	ret = device_register(&child->dev);
+	WARN_ON(ret < 0);
+
+	/* Create legacy_io and legacy_mem files for this bus */
+	pci_create_legacy_files(child);
+
 	return child;
 }
 

@@ -1296,6 +1306,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
+	int ret;
+
 	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
 

@@ -1326,6 +1338,16 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	down_write(&pci_bus_sem);
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
+
+	pci_fixup_device(pci_fixup_final, dev);
+	ret = pcibios_add_device(dev);
+	WARN_ON(ret < 0);
+	/* notifier could use pci capabilities */
+	ret = device_add(&dev->dev);
+	WARN_ON(ret < 0);
+
+	pci_proc_attach_device(dev);
+	pci_create_sysfs_dev_files(dev);
 }
 
 struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)

@@ -1656,13 +1678,13 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	char bus_addr[64];
 	char *fmt;
 
-
 	b = pci_alloc_bus();
 	if (!b)
 		return NULL;
 
 	b->sysdata = sysdata;
 	b->ops = ops;
+	b->number = b->busn_res.start = bus;
 	b2 = pci_find_bus(pci_domain_nr(b), bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */

Quoted 24 lines · Yinghai Lu

We want to put created pci device in the device tree as soon as possible.
- just after we find it and create pci_dev struct for it.
so for_pci_dev iteration will not miss them.

But at that time, we can not load driver for them yet. Need to be after pci_assign_unsigned_resources() etc to make sure all pci devices get resource allocated at first.

Move out device registering out of pci_bus_add_devices, and
new pci_bus_add_devices() will do the device_attach work to load pci drivers

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

---
 drivers/pci/bus.c   |   47 +++--------------------------------------------
 drivers/pci/iov.c   |    7 -------
 drivers/pci/probe.c |   34 +++++++++++++++++++++++++++-------
 3 files changed, 30 insertions(+), 58 deletions(-)

drivers/pci/bus.c +0 -0

index 18c1c6d..0a55845 100644
@@ -178,22 +178,9 @@ static void pci_bus_attach_device(struct pci_dev *dev)
  */

Quoted 15 lines

drivers/pci/bus.c +0 -9

@@ -205,21 +192,9 @@ int pci_bus_add_device(struct pci_dev *dev)
  */
 int pci_bus_add_child(struct pci_bus *bus)
 {
-     int retval;
-
-     if (bus->bridge)
-             bus->dev.parent = bus->bridge;
-
-     retval = device_register(&bus->dev);
-     if (retval)
-             return retval;
-
      bus->is_added = 1;
 

drivers/pci/bus.c int pci_bus_add_device(struct pci_dev *dev) · line 198

Quoted 6 lines

drivers/pci/bus.c +1 -4

-     /* Create legacy_io and legacy_mem files for this bus */
-     pci_create_legacy_files(bus);
-
-     return retval;
+     return 0;
 }

Well, what sense does this make to keep that function as is after removing almost all of the code from it?

Quoted 6 lines

drivers/pci/bus.c +0 -0

      list_for_each_entry(dev, &bus->devices, bus_list) {
              BUG_ON(!dev->is_added);

              child = dev->subordinate;

drivers/pci/bus.c +0 -2

-             /*
-              * If there is an unattached subordinate bus, attach

drivers/pci/bus.c void pci_bus_add_devices(const struct pci_bus *bus) · line 266

Quoted 10 lines

drivers/pci/bus.c +1 -7

-              * it and then scan for unattached PCI devices.
-              */
+
              if (!child)
                      continue;
-             if (list_empty(&child->node)) {
-                     down_write(&pci_bus_sem);
-                     list_add_tail(&child->node, &dev->bus->children);
-                     up_write(&pci_bus_sem);
-             }

This doesn't seem to have a replacement. Why isn't it necessary any more?