[PATCH v3 1/7] PCI: make PCI device create/destroy logic symmetric

[PATCH v3 1/7] PCI: make PCI device create/destroy logic symmetric

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

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 <jiang.liu@huawei.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c  |    1 -
 drivers/pci/remove.c |    4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2396111..5dbad03 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1293,7 +1293,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);
 
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 513972f..10693f5 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -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)
-- 
1.7.9.5

[PATCH v3 2/7] PCI: split registration of PCI bus devices into two stages

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

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 <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c    |    2 +-
 drivers/pci/probe.c  |    4 +++-
 drivers/pci/remove.c |   10 +++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 4b0970b..11a5c28 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -189,7 +189,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;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5dbad03..7ef8c1b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -639,6 +639,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
@@ -1672,7 +1673,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;
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 10693f5..0d03026 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -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);
 
-- 
1.7.9.5

[PATCH v3 3/7] ACPI/pci_bind: correctly update binding relationship for PCI hotplug

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Currently pci_bind.c is used to maintain binding relationship between
ACPI and PCI devices. But it's broken when handling PCI hotplug events.

For the acpiphp driver, it's designed to update the binding relationship
when PCI hotplug event happens, but the implementation is broken.
For PCI device hot-adding:
enable_device()
    pci_scan_slot()
    acpiphp_bus_add()
        acpi_bus_add()
	    acpi_pci_bind()
		acpi_get_pci_dev()
		    return NULL because dev->archdata.acpi_handle is
		    still unset
		return without updating actual binding relationship
    pci_bus_add_devices()
	pci_bus_add_device()
	    device_add()
		platform_notify()
		    acpi_bind_one()
			set dev->archdata.acpi_handle

For PCI device hot-removal,
disable_device()
    pci_stop_bus_device()
    __pci_remove_bus_device()
    acpiphp_bus_trim()
	acpi_bus_remove()
	    acpi_pci_unbind()
		return without really unbinding because PCI device has
		already been destroyed

For other PCI hotplug drivers, they even don't bother to update binding
relationships. So hook into acpi_bind_one()/acpi_unbind_one() in
drivers/acpi/glue.c to update PCI<->ACPI binding relationship.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Reviewed-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/glue.c     |   14 ++++--
 drivers/acpi/internal.h |    3 ++
 drivers/acpi/pci_bind.c |  110 +++++++++++++++++++++++++++++------------------
 drivers/acpi/pci_root.c |   40 ++++-------------
 4 files changed, 91 insertions(+), 76 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 243ee85..bb232d3 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -121,7 +121,6 @@ acpi_handle acpi_get_child(acpi_handle parent, u64 address)
 			    1, do_acpi_find_child, NULL, &find, NULL);
 	return find.handle;
 }
-
 EXPORT_SYMBOL(acpi_get_child);
 
 /* Link ACPI devices with physical devices */
@@ -142,7 +141,6 @@ struct device *acpi_get_physical_device(acpi_handle handle)
 		return get_device(dev);
 	return NULL;
 }
-
 EXPORT_SYMBOL(acpi_get_physical_device);
 
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
@@ -163,7 +161,12 @@ static int acpi_bind_one(struct device *dev, acpi_handle handle)
 	dev->archdata.acpi_handle = handle;
 
 	status = acpi_bus_get_device(handle, &acpi_dev);
-	if (!ACPI_FAILURE(status)) {
+	if (ACPI_FAILURE(status))
+		acpi_dev = NULL;
+
+	acpi_pci_bind_notify(acpi_dev, dev, true);
+
+	if (acpi_dev) {
 		int ret;
 
 		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
@@ -191,7 +194,10 @@ static int acpi_unbind_one(struct device *dev)
 					&acpi_dev)) {
 			sysfs_remove_link(&dev->kobj, "firmware_node");
 			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
-		}
+		} else
+			acpi_dev = NULL;
+
+		acpi_pci_bind_notify(acpi_dev, dev, false);
 
 		acpi_detach_data(dev->archdata.acpi_handle,
 				 acpi_glue_data_handler);
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index ca75b9c..16a692b 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -93,4 +93,7 @@ static inline int suspend_nvs_save(void) { return 0; }
 static inline void suspend_nvs_restore(void) {}
 #endif
 
+void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
+			  bool bind);
+
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index 2ef0409..66c5f4a 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -35,43 +35,44 @@
 #define _COMPONENT		ACPI_PCI_COMPONENT
 ACPI_MODULE_NAME("pci_bind");
 
-static int acpi_pci_unbind(struct acpi_device *device)
-{
-	struct pci_dev *dev;
-
-	dev = acpi_get_pci_dev(device->handle);
-	if (!dev)
-		goto out;
+static int acpi_pci_bind_cb(struct acpi_device *acpi_dev);
 
+static int acpi_pci_unbind(struct acpi_device *acpi_dev, struct pci_dev *dev)
+{
 	device_set_run_wake(&dev->dev, false);
-	pci_acpi_remove_pm_notifier(device);
+	pci_acpi_remove_pm_notifier(acpi_dev);
+
+	if (dev->subordinate) {
+		acpi_pci_irq_del_prt(dev->subordinate);
+		acpi_dev->ops.bind = NULL;
+		acpi_dev->ops.unbind = NULL;
+	}
 
-	if (!dev->subordinate)
-		goto out;
+	return 0;
+}
 
-	acpi_pci_irq_del_prt(dev->subordinate);
+static int acpi_pci_unbind_cb(struct acpi_device *acpi_dev)
+{
+	int rc = 0;
+	struct pci_dev *dev;
 
-	device->ops.bind = NULL;
-	device->ops.unbind = NULL;
+	dev = acpi_get_pci_dev(acpi_dev->handle);
+	if (dev) {
+		rc = acpi_pci_unbind(acpi_dev, dev);
+		pci_dev_put(dev);
+	}
 
-out:
-	pci_dev_put(dev);
-	return 0;
+	return rc;
 }
 
-static int acpi_pci_bind(struct acpi_device *device)
+static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 {
 	acpi_status status;
-	acpi_handle handle;
+	acpi_handle tmp_hdl;
 	struct pci_bus *bus;
-	struct pci_dev *dev;
 
-	dev = acpi_get_pci_dev(device->handle);
-	if (!dev)
-		return 0;
-
-	pci_acpi_add_pm_notifier(device, dev);
-	if (device->wakeup.flags.run_wake)
+	pci_acpi_add_pm_notifier(acpi_dev, dev);
+	if (acpi_dev->wakeup.flags.run_wake)
 		device_set_run_wake(&dev->dev, true);
 
 	/*
@@ -83,8 +84,8 @@ static int acpi_pci_bind(struct acpi_device *device)
 				  "Device %04x:%02x:%02x.%d is a PCI bridge\n",
 				  pci_domain_nr(dev->bus), dev->bus->number,
 				  PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)));
-		device->ops.bind = acpi_pci_bind;
-		device->ops.unbind = acpi_pci_unbind;
+		acpi_dev->ops.bind = acpi_pci_bind_cb;
+		acpi_dev->ops.unbind = acpi_pci_unbind_cb;
 	}
 
 	/*
@@ -95,26 +96,53 @@ static int acpi_pci_bind(struct acpi_device *device)
 	 *
 	 * TBD: Can _PRTs exist within the scope of non-bridge PCI devices?
 	 */
-	status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
-	if (ACPI_FAILURE(status))
-		goto out;
+	status = acpi_get_handle(acpi_dev->handle, METHOD_NAME__PRT, &tmp_hdl);
+	if (ACPI_SUCCESS(status)) {
+		if (dev->subordinate)
+			bus = dev->subordinate;
+		else
+			bus = dev->bus;
+		acpi_pci_irq_add_prt(acpi_dev->handle, bus);
+	}
 
-	if (dev->subordinate)
-		bus = dev->subordinate;
-	else
-		bus = dev->bus;
+	return 0;
+}
 
-	acpi_pci_irq_add_prt(device->handle, bus);
+static int acpi_pci_bind_cb(struct acpi_device *acpi_dev)
+{
+	int rc = 0;
+	struct pci_dev *dev;
 
-out:
-	pci_dev_put(dev);
-	return 0;
+	dev = acpi_get_pci_dev(acpi_dev->handle);
+	if (dev) {
+		rc = acpi_pci_bind(acpi_dev, dev);
+		pci_dev_put(dev);
+	}
+
+	return rc;
 }
 
-int acpi_pci_bind_root(struct acpi_device *device)
+int acpi_pci_bind_root(struct acpi_device *acpi_dev)
 {
-	device->ops.bind = acpi_pci_bind;
-	device->ops.unbind = acpi_pci_unbind;
+	acpi_dev->ops.bind = acpi_pci_bind_cb;
+	acpi_dev->ops.unbind = acpi_pci_unbind_cb;
 
 	return 0;
 }
+
+void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
+			  bool bind)
+{
+	if (!dev_is_pci(dev))
+		return;
+
+	if (acpi_dev && acpi_dev->parent) {
+		if (bind) {
+			if (acpi_dev->parent->ops.bind)
+				acpi_pci_bind(acpi_dev, to_pci_dev(dev));
+		} else {
+			if (acpi_dev->parent->ops.unbind)
+				acpi_pci_unbind(acpi_dev, to_pci_dev(dev));
+		}
+	}
+}
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 72a2c98..7509034 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -195,21 +195,6 @@ static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
 	return AE_OK;
 }
 
-static void acpi_pci_bridge_scan(struct acpi_device *device)
-{
-	int status;
-	struct acpi_device *child = NULL;
-
-	if (device->flags.bus_address)
-		if (device->parent && device->parent->ops.bind) {
-			status = device->parent->ops.bind(device);
-			if (!status) {
-				list_for_each_entry(child, &device->children, node)
-					acpi_pci_bridge_scan(child);
-			}
-		}
-}
-
 static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
 
 static acpi_status acpi_pci_run_osc(acpi_handle handle,
@@ -456,7 +441,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	int result;
 	struct acpi_pci_root *root;
 	acpi_handle handle;
-	struct acpi_device *child;
 	u32 flags, base_flags;
 
 	root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
@@ -521,6 +505,15 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	/* TBD: Locking */
 	list_add_tail(&root->node, &acpi_pci_roots);
 
+	/*
+	 * Attach ACPI-PCI Context
+	 * -----------------------
+	 * Thus binding the ACPI and PCI devices.
+	 */
+	result = acpi_pci_bind_root(device);
+	if (result)
+		goto end;
+
 	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
 	       root->segment, &root->secondary);
@@ -542,15 +535,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	}
 
 	/*
-	 * Attach ACPI-PCI Context
-	 * -----------------------
-	 * Thus binding the ACPI and PCI devices.
-	 */
-	result = acpi_pci_bind_root(device);
-	if (result)
-		goto end;
-
-	/*
 	 * PCI Routing Table
 	 * -----------------
 	 * Evaluate and parse _PRT, if exists.
@@ -559,12 +543,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	if (ACPI_SUCCESS(status))
 		result = acpi_pci_irq_add_prt(device->handle, root->bus);
 
-	/*
-	 * Scan and bind all _ADR-Based Devices
-	 */
-	list_for_each_entry(child, &device->children, node)
-		acpi_pci_bridge_scan(child);
-
 	/* Indicate support for various _OSC capabilities. */
 	if (pci_ext_cfg_avail(root->bus->self))
 		flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
-- 
1.7.9.5

[PATCH v3 4/7] ACPI/pci-bind: remove bind/unbind callbacks from acpi_device_ops

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Now ACPI devices are created before/destroyed after corresponding PCI
devices, and acpi_platform_notify/acpi_platform_notify_remove will
update PCI<->ACPI binding relationship when creating/destroying PCI
devices, there's no need to invoke bind/unbind callbacks from ACPI
device probe/destroy routines anymore. So remove bind/unbind callbacks
from acpi_device_ops.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Reviewed-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/pci_bind.c     |   95 ++++++++++---------------------------------
 drivers/acpi/pci_root.c     |    9 ----
 drivers/acpi/scan.c         |   21 +---------
 include/acpi/acpi_bus.h     |    4 --
 include/acpi/acpi_drivers.h |    1 -
 5 files changed, 23 insertions(+), 107 deletions(-)

diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
index 66c5f4a..aac7f9a 100644
--- a/drivers/acpi/pci_bind.c
+++ b/drivers/acpi/pci_bind.c
@@ -35,58 +35,33 @@
 #define _COMPONENT		ACPI_PCI_COMPONENT
 ACPI_MODULE_NAME("pci_bind");
 
-static int acpi_pci_bind_cb(struct acpi_device *acpi_dev);
-
 static int acpi_pci_unbind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 {
-	device_set_run_wake(&dev->dev, false);
-	pci_acpi_remove_pm_notifier(acpi_dev);
+	if (acpi_dev) {
+		device_set_run_wake(&dev->dev, false);
+		pci_acpi_remove_pm_notifier(acpi_dev);
+	}
 
-	if (dev->subordinate) {
+	if (dev->subordinate)
 		acpi_pci_irq_del_prt(dev->subordinate);
-		acpi_dev->ops.bind = NULL;
-		acpi_dev->ops.unbind = NULL;
-	}
 
 	return 0;
 }
 
-static int acpi_pci_unbind_cb(struct acpi_device *acpi_dev)
-{
-	int rc = 0;
-	struct pci_dev *dev;
-
-	dev = acpi_get_pci_dev(acpi_dev->handle);
-	if (dev) {
-		rc = acpi_pci_unbind(acpi_dev, dev);
-		pci_dev_put(dev);
-	}
-
-	return rc;
-}
-
 static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 {
 	acpi_status status;
-	acpi_handle tmp_hdl;
 	struct pci_bus *bus;
+	acpi_handle tmp_hdl;
+	acpi_handle handle;
 
-	pci_acpi_add_pm_notifier(acpi_dev, dev);
-	if (acpi_dev->wakeup.flags.run_wake)
-		device_set_run_wake(&dev->dev, true);
-
-	/*
-	 * Install the 'bind' function to facilitate callbacks for
-	 * children of the P2P bridge.
-	 */
-	if (dev->subordinate) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-				  "Device %04x:%02x:%02x.%d is a PCI bridge\n",
-				  pci_domain_nr(dev->bus), dev->bus->number,
-				  PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)));
-		acpi_dev->ops.bind = acpi_pci_bind_cb;
-		acpi_dev->ops.unbind = acpi_pci_unbind_cb;
-	}
+	if (acpi_dev) {
+		pci_acpi_add_pm_notifier(acpi_dev, dev);
+		if (acpi_dev->wakeup.flags.run_wake)
+			device_set_run_wake(&dev->dev, true);
+		handle = acpi_dev->handle;
+	} else
+		handle = DEVICE_ACPI_HANDLE(&dev->dev);
 
 	/*
 	 * Evaluate and parse _PRT, if exists.  This code allows parsing of
@@ -96,53 +71,25 @@ static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
 	 *
 	 * TBD: Can _PRTs exist within the scope of non-bridge PCI devices?
 	 */
-	status = acpi_get_handle(acpi_dev->handle, METHOD_NAME__PRT, &tmp_hdl);
+	status = acpi_get_handle(handle, METHOD_NAME__PRT, &tmp_hdl);
 	if (ACPI_SUCCESS(status)) {
 		if (dev->subordinate)
 			bus = dev->subordinate;
 		else
 			bus = dev->bus;
-		acpi_pci_irq_add_prt(acpi_dev->handle, bus);
-	}
-
-	return 0;
-}
-
-static int acpi_pci_bind_cb(struct acpi_device *acpi_dev)
-{
-	int rc = 0;
-	struct pci_dev *dev;
-
-	dev = acpi_get_pci_dev(acpi_dev->handle);
-	if (dev) {
-		rc = acpi_pci_bind(acpi_dev, dev);
-		pci_dev_put(dev);
+		acpi_pci_irq_add_prt(handle, bus);
 	}
 
-	return rc;
-}
-
-int acpi_pci_bind_root(struct acpi_device *acpi_dev)
-{
-	acpi_dev->ops.bind = acpi_pci_bind_cb;
-	acpi_dev->ops.unbind = acpi_pci_unbind_cb;
-
 	return 0;
 }
 
 void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
 			  bool bind)
 {
-	if (!dev_is_pci(dev))
-		return;
-
-	if (acpi_dev && acpi_dev->parent) {
-		if (bind) {
-			if (acpi_dev->parent->ops.bind)
-				acpi_pci_bind(acpi_dev, to_pci_dev(dev));
-		} else {
-			if (acpi_dev->parent->ops.unbind)
-				acpi_pci_unbind(acpi_dev, to_pci_dev(dev));
-		}
+	if (dev_is_pci(dev)) {
+		if (bind)
+			acpi_pci_bind(acpi_dev, to_pci_dev(dev));
+		else
+			acpi_pci_unbind(acpi_dev, to_pci_dev(dev));
 	}
 }
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7509034..25d8ad4 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -505,15 +505,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	/* TBD: Locking */
 	list_add_tail(&root->node, &acpi_pci_roots);
 
-	/*
-	 * Attach ACPI-PCI Context
-	 * -----------------------
-	 * Thus binding the ACPI and PCI devices.
-	 */
-	result = acpi_pci_bind_root(device);
-	if (result)
-		goto end;
-
 	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
 	       root->segment, &root->secondary);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index d1ecca2..f31cb2f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1232,17 +1232,8 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 	dev->removal_type = ACPI_BUS_REMOVAL_EJECT;
 	device_release_driver(&dev->dev);
 
-	if (!rmdevice)
-		return 0;
-
-	/*
-	 * unbind _ADR-Based Devices when hot removal
-	 */
-	if (dev->flags.bus_address) {
-		if ((dev->parent) && (dev->parent->ops.unbind))
-			dev->parent->ops.unbind(dev);
-	}
-	acpi_device_unregister(dev, ACPI_BUS_REMOVAL_EJECT);
+	if (rmdevice)
+		acpi_device_unregister(dev, ACPI_BUS_REMOVAL_EJECT);
 
 	return 0;
 }
@@ -1319,14 +1310,6 @@ static int acpi_add_single_object(struct acpi_device **child,
 
 	result = acpi_device_register(device);
 
-	/*
-	 * Bind _ADR-Based Devices when hot add
-	 */
-	if (device->flags.bus_address) {
-		if (device->parent && device->parent->ops.bind)
-			device->parent->ops.bind(device);
-	}
-
 end:
 	if (!result) {
 		acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bde976e..ef5babf 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -120,8 +120,6 @@ struct acpi_device;
 typedef int (*acpi_op_add) (struct acpi_device * device);
 typedef int (*acpi_op_remove) (struct acpi_device * device, int type);
 typedef int (*acpi_op_start) (struct acpi_device * device);
-typedef int (*acpi_op_bind) (struct acpi_device * device);
-typedef int (*acpi_op_unbind) (struct acpi_device * device);
 typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event);
 
 struct acpi_bus_ops {
@@ -133,8 +131,6 @@ struct acpi_device_ops {
 	acpi_op_add add;
 	acpi_op_remove remove;
 	acpi_op_start start;
-	acpi_op_bind bind;
-	acpi_op_unbind unbind;
 	acpi_op_notify notify;
 };
 
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index bb145e4..fb1c0d5 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -100,7 +100,6 @@ void acpi_pci_irq_del_prt(struct pci_bus *bus);
 struct pci_bus;
 
 struct pci_dev *acpi_get_pci_dev(acpi_handle);
-int acpi_pci_bind_root(struct acpi_device *device);
 
 /* Arch-defined function to add a bus to the system */
 
-- 
1.7.9.5

[PATCH v3 5/7] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Currently the pci_slot driver doesn't update PCI slot information
when PCI device hotplug event happens, which may cause memory leak
and returning stale information to user.

So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
update PCI slot information when PCI hotplug event happens.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/acpi/pci_slot.c |   86 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index e50e31a..96dcc19 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -32,6 +32,7 @@
 #include <linux/acpi.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
+#include <linux/pci-acpi.h>
 #include <linux/dmi.h>
 
 static bool debug;
@@ -123,12 +124,7 @@ struct callback_args {
 /*
  * register_slot
  *
- * Called once for each SxFy object in the namespace. Don't worry about
- * calling pci_create_slot multiple times for the same pci_bus:device,
- * since each subsequent call simply bumps the refcount on the pci_slot.
- *
- * The number of calls to pci_destroy_slot from unregister_slot is
- * symmetrical.
+ * Called once for each SxFy object in the namespace.
  */
 static acpi_status
 register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
@@ -145,6 +141,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	if (device < 0)
 		return AE_OK;
 
+	/* Avoid duplicated records for the same slot */
+	list_for_each_entry(slot, &slot_list, list) {
+		pci_slot = slot->pci_slot;
+		if (pci_slot && pci_slot->bus == pci_bus &&
+		    pci_slot->number == device) {
+			return AE_OK;
+		}
+	}
+
 	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
 	if (!slot) {
 		err("%s: cannot allocate memory\n", __func__);
@@ -162,9 +167,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	slot->root_handle = parent_context->root_handle;
 	slot->pci_slot = pci_slot;
 	INIT_LIST_HEAD(&slot->list);
-	mutex_lock(&slot_list_lock);
 	list_add(&slot->list, &slot_list);
-	mutex_unlock(&slot_list_lock);
 
 	get_device(&pci_bus->dev);
 
@@ -299,7 +302,9 @@ acpi_pci_slot_add(acpi_handle handle)
 {
 	acpi_status status;
 
+	mutex_lock(&slot_list_lock);
 	status = walk_root_bridge(handle, register_slot);
+	mutex_unlock(&slot_list_lock);
 	if (ACPI_FAILURE(status))
 		err("%s: register_slot failure - %d\n", __func__, status);
 
@@ -354,17 +359,82 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
 	{}
 };
 
+static void acpi_pci_slot_notify_add(struct pci_dev *dev)
+{
+	acpi_handle handle;
+	struct callback_args context;
+
+	if (!dev->subordinate)
+		return;
+
+	mutex_lock(&slot_list_lock);
+	handle = DEVICE_ACPI_HANDLE(&dev->dev);
+	context.root_handle = acpi_find_root_bridge_handle(dev);
+	if (handle && context.root_handle) {
+		context.pci_bus = dev->subordinate;
+		context.user_function = register_slot;
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+				    register_slot, NULL, &context, NULL);
+	}
+	mutex_unlock(&slot_list_lock);
+}
+
+static void acpi_pci_slot_notify_del(struct pci_dev *dev)
+{
+	struct acpi_pci_slot *slot, *tmp;
+	struct pci_bus *bus = dev->subordinate;
+
+	if (!bus)
+		return;
+
+	mutex_lock(&slot_list_lock);
+	list_for_each_entry_safe(slot, tmp, &slot_list, list)
+		if (slot->pci_slot && slot->pci_slot->bus == bus) {
+			list_del(&slot->list);
+			pci_destroy_slot(slot->pci_slot);
+			put_device(&bus->dev);
+			kfree(slot);
+		}
+	mutex_unlock(&slot_list_lock);
+}
+
+static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
+				   unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpi_pci_slot_notify_add(to_pci_dev(dev));
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpi_pci_slot_notify_del(to_pci_dev(dev));
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_slot_notifier = {
+	.notifier_call = &acpi_pci_slot_notify_fn,
+};
+
 static int __init
 acpi_pci_slot_init(void)
 {
 	dmi_check_system(acpi_pci_slot_dmi_table);
 	acpi_pci_register_driver(&acpi_pci_slot_driver);
+	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
+
 	return 0;
 }
 
 static void __exit
 acpi_pci_slot_exit(void)
 {
+	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
 	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
 }
 
-- 
1.7.9.5

[PATCH v3 6/7] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Currently the acpiphp driver fails to update hotplug slot information under
several conditions, such as:
1) The bridge device is removed through /sys/bus/pci/devices/.../remove
2) The bridge device is added/removed by PCI hotplug driver other than the
   acpiphp driver itself. For example, if an IO expansion box with ACPI
   hotplug slots available is hot-added by the pciehp driver, all ACPI
   hotplug slots won't be discovered by the acpiphp driver.

So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
update ACPI hotplug slot information when PCI hotplug event happens.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   64 ++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 7be4ca5..1fb0eb7 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,6 +61,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
+static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle);
 
 /* callback routine to check for the existence of a pci dock device */
 static acpi_status
@@ -382,7 +383,7 @@ static inline void config_p2p_bridge_flags(struct acpiphp_bridge *bridge)
 
 
 /* allocate and initialize host bridge data structure */
-static void add_host_bridge(acpi_handle *handle)
+static void add_host_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
 	struct acpi_pci_root *root = acpi_pci_find_root(handle);
@@ -399,12 +400,14 @@ static void add_host_bridge(acpi_handle *handle)
 	init_bridge_misc(bridge);
 }
 
-
 /* allocate and initialize PCI-to-PCI bridge data structure */
-static void add_p2p_bridge(acpi_handle *handle)
+static void add_p2p_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
 
+	if (acpiphp_handle_to_bridge(handle))
+		return;
+
 	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
 	if (bridge == NULL) {
 		err("out of memory\n");
@@ -1405,6 +1408,58 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK ;
 }
 
+static void acpiphp_hp_notify_add(struct pci_dev *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
+
+	if (!dev->subordinate || !handle)
+		return;
+
+	/* check if this bridge has ejectable slots */
+	if (detect_ejectable_slots(handle) > 0) {
+		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
+		add_p2p_bridge(handle);
+	}
+}
+
+static void acpiphp_hp_notify_del(struct pci_dev *dev)
+{
+	struct acpiphp_bridge *bridge, *tmp;
+	struct pci_bus *bus = dev->subordinate;
+
+	if (!bus)
+		return;
+
+	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
+		if (bridge->pci_bus == bus) {
+			cleanup_bridge(bridge);
+			break;
+		}
+}
+
+static int acpi_pci_hp_notify_fn(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpiphp_hp_notify_add(to_pci_dev(dev));
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpiphp_hp_notify_del(to_pci_dev(dev));
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_hp_notifier = {
+	.notifier_call = &acpi_pci_hp_notify_fn,
+};
+
 static struct acpi_pci_driver acpi_pci_hp_driver = {
 	.add =		add_bridge,
 	.remove =	remove_bridge,
@@ -1425,6 +1480,8 @@ int __init acpiphp_glue_init(void)
 	else
 		acpi_pci_register_driver(&acpi_pci_hp_driver);
 
+	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
+
 	return 0;
 }
 
@@ -1436,6 +1493,7 @@ int __init acpiphp_glue_init(void)
  */
 void  acpiphp_glue_exit(void)
 {
+	bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
 	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
 }
 
-- 
1.7.9.5

[PATCH v3 7/7] PCI/acpiphp: serialize access to the bridge_list list

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Serialize access to the bridge_list in the acpiphp driver.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 1fb0eb7..348afd7 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -53,6 +53,7 @@
 #include "../pci.h"
 #include "acpiphp.h"
 
+static DEFINE_MUTEX(bridge_mutex);
 static LIST_HEAD(bridge_list);
 
 #define MY_NAME "acpiphp_glue"
@@ -497,8 +498,10 @@ static int add_bridge(acpi_handle handle)
 	}
 
 	/* search P2P bridges under this host bridge */
+	mutex_lock(&bridge_mutex);
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
 				     find_p2p_bridge, NULL, NULL, NULL);
+	mutex_unlock(&bridge_mutex);
 
 	if (ACPI_FAILURE(status))
 		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
@@ -595,6 +598,8 @@ static void remove_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
 
+	mutex_lock(&bridge_mutex);
+
 	/* cleanup p2p bridges under this host bridge
 	   in a depth-first manner */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
@@ -613,6 +618,8 @@ static void remove_bridge(acpi_handle handle)
 	else
 		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					   handle_hotplug_event_bridge);
+
+	mutex_unlock(&bridge_mutex);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)
@@ -1415,11 +1422,13 @@ static void acpiphp_hp_notify_add(struct pci_dev *dev)
 	if (!dev->subordinate || !handle)
 		return;
 
+	mutex_lock(&bridge_mutex);
 	/* check if this bridge has ejectable slots */
 	if (detect_ejectable_slots(handle) > 0) {
 		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
 		add_p2p_bridge(handle);
 	}
+	mutex_unlock(&bridge_mutex);
 }
 
 static void acpiphp_hp_notify_del(struct pci_dev *dev)
@@ -1430,11 +1439,13 @@ static void acpiphp_hp_notify_del(struct pci_dev *dev)
 	if (!bus)
 		return;
 
+	mutex_lock(&bridge_mutex);
 	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
 		if (bridge->pci_bus == bus) {
 			cleanup_bridge(bridge);
 			break;
 		}
+	mutex_unlock(&bridge_mutex);
 }
 
 static int acpi_pci_hp_notify_fn(struct notifier_block *nb,
-- 
1.7.9.5

Re: [PATCH v3 3/7] ACPI/pci_bind: correctly update binding relationship for PCI hotplug

[Bjorn Helgaas]

[+cc Rafael]

On Tue, Sep 25, 2012 at 8:29 AM, Jiang Liu <liuj97@gmail.com> wrote:
>  From: Jiang Liu <jiang.liu@huawei.com>
>
> Currently pci_bind.c is used to maintain binding relationship between
> ACPI and PCI devices. But it's broken when handling PCI hotplug events.
>
> For the acpiphp driver, it's designed to update the binding relationship
> when PCI hotplug event happens, but the implementation is broken.
> For PCI device hot-adding:
> enable_device()
>     pci_scan_slot()
>     acpiphp_bus_add()
>         acpi_bus_add()
>             acpi_pci_bind()
>                 acpi_get_pci_dev()
>                     return NULL because dev->archdata.acpi_handle is
>                     still unset
>                 return without updating actual binding relationship
>     pci_bus_add_devices()
>         pci_bus_add_device()
>             device_add()
>                 platform_notify()
>                     acpi_bind_one()
>                         set dev->archdata.acpi_handle
>
> For PCI device hot-removal,
> disable_device()
>     pci_stop_bus_device()
>     __pci_remove_bus_device()
>     acpiphp_bus_trim()
>         acpi_bus_remove()
>             acpi_pci_unbind()
>                 return without really unbinding because PCI device has
>                 already been destroyed
>
> For other PCI hotplug drivers, they even don't bother to update binding
> relationships. So hook into acpi_bind_one()/acpi_unbind_one() in
> drivers/acpi/glue.c to update PCI<->ACPI binding relationship.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Reviewed-by: Yinghai Lu <yinghai@kernel.org>

Hi Gerry,

Sorry for the delay in addressing this series.  This patch (and 4/7
and 5/7) have a lot to do with PCI/ACPI binding, and I'm afraid they
will conflict with Rafael's changes in that area.  Could you
confirm/deny that and maybe repost the non-conflicting ones that are
still useful?  I'm not sure if the create/destroy and PCI bus device
registration changes are separable from the others or not.

Bjorn

> ---
>  drivers/acpi/glue.c     |   14 ++++--
>  drivers/acpi/internal.h |    3 ++
>  drivers/acpi/pci_bind.c |  110 +++++++++++++++++++++++++++++------------------
>  drivers/acpi/pci_root.c |   40 ++++-------------
>  4 files changed, 91 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 243ee85..bb232d3 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -121,7 +121,6 @@ acpi_handle acpi_get_child(acpi_handle parent, u64 address)
>                             1, do_acpi_find_child, NULL, &find, NULL);
>         return find.handle;
>  }
> -
>  EXPORT_SYMBOL(acpi_get_child);
>
>  /* Link ACPI devices with physical devices */
> @@ -142,7 +141,6 @@ struct device *acpi_get_physical_device(acpi_handle handle)
>                 return get_device(dev);
>         return NULL;
>  }
> -
>  EXPORT_SYMBOL(acpi_get_physical_device);
>
>  static int acpi_bind_one(struct device *dev, acpi_handle handle)
> @@ -163,7 +161,12 @@ static int acpi_bind_one(struct device *dev, acpi_handle handle)
>         dev->archdata.acpi_handle = handle;
>
>         status = acpi_bus_get_device(handle, &acpi_dev);
> -       if (!ACPI_FAILURE(status)) {
> +       if (ACPI_FAILURE(status))
> +               acpi_dev = NULL;
> +
> +       acpi_pci_bind_notify(acpi_dev, dev, true);
> +
> +       if (acpi_dev) {
>                 int ret;
>
>                 ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
> @@ -191,7 +194,10 @@ static int acpi_unbind_one(struct device *dev)
>                                         &acpi_dev)) {
>                         sysfs_remove_link(&dev->kobj, "firmware_node");
>                         sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
> -               }
> +               } else
> +                       acpi_dev = NULL;
> +
> +               acpi_pci_bind_notify(acpi_dev, dev, false);
>
>                 acpi_detach_data(dev->archdata.acpi_handle,
>                                  acpi_glue_data_handler);
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index ca75b9c..16a692b 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -93,4 +93,7 @@ static inline int suspend_nvs_save(void) { return 0; }
>  static inline void suspend_nvs_restore(void) {}
>  #endif
>
> +void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
> +                         bool bind);
> +
>  #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/pci_bind.c b/drivers/acpi/pci_bind.c
> index 2ef0409..66c5f4a 100644
> --- a/drivers/acpi/pci_bind.c
> +++ b/drivers/acpi/pci_bind.c
> @@ -35,43 +35,44 @@
>  #define _COMPONENT             ACPI_PCI_COMPONENT
>  ACPI_MODULE_NAME("pci_bind");
>
> -static int acpi_pci_unbind(struct acpi_device *device)
> -{
> -       struct pci_dev *dev;
> -
> -       dev = acpi_get_pci_dev(device->handle);
> -       if (!dev)
> -               goto out;
> +static int acpi_pci_bind_cb(struct acpi_device *acpi_dev);
>
> +static int acpi_pci_unbind(struct acpi_device *acpi_dev, struct pci_dev *dev)
> +{
>         device_set_run_wake(&dev->dev, false);
> -       pci_acpi_remove_pm_notifier(device);
> +       pci_acpi_remove_pm_notifier(acpi_dev);
> +
> +       if (dev->subordinate) {
> +               acpi_pci_irq_del_prt(dev->subordinate);
> +               acpi_dev->ops.bind = NULL;
> +               acpi_dev->ops.unbind = NULL;
> +       }
>
> -       if (!dev->subordinate)
> -               goto out;
> +       return 0;
> +}
>
> -       acpi_pci_irq_del_prt(dev->subordinate);
> +static int acpi_pci_unbind_cb(struct acpi_device *acpi_dev)
> +{
> +       int rc = 0;
> +       struct pci_dev *dev;
>
> -       device->ops.bind = NULL;
> -       device->ops.unbind = NULL;
> +       dev = acpi_get_pci_dev(acpi_dev->handle);
> +       if (dev) {
> +               rc = acpi_pci_unbind(acpi_dev, dev);
> +               pci_dev_put(dev);
> +       }
>
> -out:
> -       pci_dev_put(dev);
> -       return 0;
> +       return rc;
>  }
>
> -static int acpi_pci_bind(struct acpi_device *device)
> +static int acpi_pci_bind(struct acpi_device *acpi_dev, struct pci_dev *dev)
>  {
>         acpi_status status;
> -       acpi_handle handle;
> +       acpi_handle tmp_hdl;
>         struct pci_bus *bus;
> -       struct pci_dev *dev;
>
> -       dev = acpi_get_pci_dev(device->handle);
> -       if (!dev)
> -               return 0;
> -
> -       pci_acpi_add_pm_notifier(device, dev);
> -       if (device->wakeup.flags.run_wake)
> +       pci_acpi_add_pm_notifier(acpi_dev, dev);
> +       if (acpi_dev->wakeup.flags.run_wake)
>                 device_set_run_wake(&dev->dev, true);
>
>         /*
> @@ -83,8 +84,8 @@ static int acpi_pci_bind(struct acpi_device *device)
>                                   "Device %04x:%02x:%02x.%d is a PCI bridge\n",
>                                   pci_domain_nr(dev->bus), dev->bus->number,
>                                   PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)));
> -               device->ops.bind = acpi_pci_bind;
> -               device->ops.unbind = acpi_pci_unbind;
> +               acpi_dev->ops.bind = acpi_pci_bind_cb;
> +               acpi_dev->ops.unbind = acpi_pci_unbind_cb;
>         }
>
>         /*
> @@ -95,26 +96,53 @@ static int acpi_pci_bind(struct acpi_device *device)
>          *
>          * TBD: Can _PRTs exist within the scope of non-bridge PCI devices?
>          */
> -       status = acpi_get_handle(device->handle, METHOD_NAME__PRT, &handle);
> -       if (ACPI_FAILURE(status))
> -               goto out;
> +       status = acpi_get_handle(acpi_dev->handle, METHOD_NAME__PRT, &tmp_hdl);
> +       if (ACPI_SUCCESS(status)) {
> +               if (dev->subordinate)
> +                       bus = dev->subordinate;
> +               else
> +                       bus = dev->bus;
> +               acpi_pci_irq_add_prt(acpi_dev->handle, bus);
> +       }
>
> -       if (dev->subordinate)
> -               bus = dev->subordinate;
> -       else
> -               bus = dev->bus;
> +       return 0;
> +}
>
> -       acpi_pci_irq_add_prt(device->handle, bus);
> +static int acpi_pci_bind_cb(struct acpi_device *acpi_dev)
> +{
> +       int rc = 0;
> +       struct pci_dev *dev;
>
> -out:
> -       pci_dev_put(dev);
> -       return 0;
> +       dev = acpi_get_pci_dev(acpi_dev->handle);
> +       if (dev) {
> +               rc = acpi_pci_bind(acpi_dev, dev);
> +               pci_dev_put(dev);
> +       }
> +
> +       return rc;
>  }
>
> -int acpi_pci_bind_root(struct acpi_device *device)
> +int acpi_pci_bind_root(struct acpi_device *acpi_dev)
>  {
> -       device->ops.bind = acpi_pci_bind;
> -       device->ops.unbind = acpi_pci_unbind;
> +       acpi_dev->ops.bind = acpi_pci_bind_cb;
> +       acpi_dev->ops.unbind = acpi_pci_unbind_cb;
>
>         return 0;
>  }
> +
> +void acpi_pci_bind_notify(struct acpi_device *acpi_dev, struct device *dev,
> +                         bool bind)
> +{
> +       if (!dev_is_pci(dev))
> +               return;
> +
> +       if (acpi_dev && acpi_dev->parent) {
> +               if (bind) {
> +                       if (acpi_dev->parent->ops.bind)
> +                               acpi_pci_bind(acpi_dev, to_pci_dev(dev));
> +               } else {
> +                       if (acpi_dev->parent->ops.unbind)
> +                               acpi_pci_unbind(acpi_dev, to_pci_dev(dev));
> +               }
> +       }
> +}
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 72a2c98..7509034 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -195,21 +195,6 @@ static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
>         return AE_OK;
>  }
>
> -static void acpi_pci_bridge_scan(struct acpi_device *device)
> -{
> -       int status;
> -       struct acpi_device *child = NULL;
> -
> -       if (device->flags.bus_address)
> -               if (device->parent && device->parent->ops.bind) {
> -                       status = device->parent->ops.bind(device);
> -                       if (!status) {
> -                               list_for_each_entry(child, &device->children, node)
> -                                       acpi_pci_bridge_scan(child);
> -                       }
> -               }
> -}
> -
>  static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
>
>  static acpi_status acpi_pci_run_osc(acpi_handle handle,
> @@ -456,7 +441,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>         int result;
>         struct acpi_pci_root *root;
>         acpi_handle handle;
> -       struct acpi_device *child;
>         u32 flags, base_flags;
>
>         root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
> @@ -521,6 +505,15 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>         /* TBD: Locking */
>         list_add_tail(&root->node, &acpi_pci_roots);
>
> +       /*
> +        * Attach ACPI-PCI Context
> +        * -----------------------
> +        * Thus binding the ACPI and PCI devices.
> +        */
> +       result = acpi_pci_bind_root(device);
> +       if (result)
> +               goto end;
> +
>         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
>                acpi_device_name(device), acpi_device_bid(device),
>                root->segment, &root->secondary);
> @@ -542,15 +535,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>         }
>
>         /*
> -        * Attach ACPI-PCI Context
> -        * -----------------------
> -        * Thus binding the ACPI and PCI devices.
> -        */
> -       result = acpi_pci_bind_root(device);
> -       if (result)
> -               goto end;
> -
> -       /*
>          * PCI Routing Table
>          * -----------------
>          * Evaluate and parse _PRT, if exists.
> @@ -559,12 +543,6 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>         if (ACPI_SUCCESS(status))
>                 result = acpi_pci_irq_add_prt(device->handle, root->bus);
>
> -       /*
> -        * Scan and bind all _ADR-Based Devices
> -        */
> -       list_for_each_entry(child, &device->children, node)
> -               acpi_pci_bridge_scan(child);
> -
>         /* Indicate support for various _OSC capabilities. */
>         if (pci_ext_cfg_avail(root->bus->self))
>                 flags |= OSC_EXT_PCI_CONFIG_SUPPORT;
> --
> 1.7.9.5
>

[PATCH v3 0/6] Update PCI notification patchset to latest kernel version

[Jiang Liu]

Hi Bjorn,
	Rafael has developed a patchset targetting 3.9 merging window 
which outdates the fourth and fifth patches in my previous version,
so I have just drop those two patches and updated the others to the
latest code from your pci-next branch.
	Thanks!

Jiang Liu (5):
  PCI: make PCI device create/destroy logic symmetric
  PCI: split registration of PCI bus devices into two stages
  ACPI/pci_slot: update PCI slot information when PCI hotplug event
    happens
  PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug
    happens
  PCI/acpiphp: serialize access to the bridge_list list

Yijing Wang (1):
  PCI/AER: update AER configuration when PCI hotplug event happens

 drivers/acpi/pci_slot.c            |   86 ++++++++++++++++++++++++++++++++----
 drivers/pci/bus.c                  |    2 +-
 drivers/pci/hotplug/acpiphp_glue.c |   73 +++++++++++++++++++++++++++++-
 drivers/pci/pcie/aer/aerdrv.c      |   63 +++++++++++++++++++++++++-
 drivers/pci/probe.c                |    5 ++-
 drivers/pci/remove.c               |   14 +++---
 6 files changed, 222 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/6] PCI: make PCI device create/destroy logic symmetric

[Jiang Liu]

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 <jiang.liu@huawei.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/probe.c  |    1 -
 drivers/pci/remove.c |    4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6186f03..e557d6f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1299,7 +1299,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);
 
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 7c0fd92..fc38c48 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -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)
-- 
1.7.9.5

[PATCH v3 2/6] PCI: split registration of PCI bus devices into two stages

[Jiang Liu]

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 <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c    |    2 +-
 drivers/pci/probe.c  |    4 +++-
 drivers/pci/remove.c |   10 +++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index ad6a8b6..2fea481 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -198,7 +198,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;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index e557d6f..a8315c2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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;
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index fc38c48..a1fdd0f 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -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);
 
-- 
1.7.9.5

[PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Jiang Liu]

Currently the pci_slot driver doesn't update PCI slot information
when PCI device hotplug event happens, which may cause memory leak
and returning stale information to user.

So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
update PCI slot information when PCI hotplug event happens.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/acpi/pci_slot.c |   86 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index d22585f..e04ea8e 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -32,6 +32,7 @@
 #include <linux/acpi.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
+#include <linux/pci-acpi.h>
 #include <linux/dmi.h>
 
 static bool debug;
@@ -123,12 +124,7 @@ struct callback_args {
 /*
  * register_slot
  *
- * Called once for each SxFy object in the namespace. Don't worry about
- * calling pci_create_slot multiple times for the same pci_bus:device,
- * since each subsequent call simply bumps the refcount on the pci_slot.
- *
- * The number of calls to pci_destroy_slot from unregister_slot is
- * symmetrical.
+ * Called once for each SxFy object in the namespace.
  */
 static acpi_status
 register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
@@ -145,6 +141,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	if (device < 0)
 		return AE_OK;
 
+	/* Avoid duplicated records for the same slot */
+	list_for_each_entry(slot, &slot_list, list) {
+		pci_slot = slot->pci_slot;
+		if (pci_slot && pci_slot->bus == pci_bus &&
+		    pci_slot->number == device) {
+			return AE_OK;
+		}
+	}
+
 	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
 	if (!slot) {
 		err("%s: cannot allocate memory\n", __func__);
@@ -162,9 +167,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	slot->root_handle = parent_context->root_handle;
 	slot->pci_slot = pci_slot;
 	INIT_LIST_HEAD(&slot->list);
-	mutex_lock(&slot_list_lock);
 	list_add(&slot->list, &slot_list);
-	mutex_unlock(&slot_list_lock);
 
 	get_device(&pci_bus->dev);
 
@@ -274,7 +277,9 @@ acpi_pci_slot_add(struct acpi_pci_root *root)
 {
 	acpi_status status;
 
+	mutex_lock(&slot_list_lock);
 	status = walk_root_bridge(root, register_slot);
+	mutex_unlock(&slot_list_lock);
 	if (ACPI_FAILURE(status))
 		err("%s: register_slot failure - %d\n", __func__, status);
 
@@ -330,17 +335,82 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
 	{}
 };
 
+static void acpi_pci_slot_notify_add(struct pci_dev *dev)
+{
+	acpi_handle handle;
+	struct callback_args context;
+
+	if (!dev->subordinate)
+		return;
+
+	mutex_lock(&slot_list_lock);
+	handle = DEVICE_ACPI_HANDLE(&dev->dev);
+	context.root_handle = acpi_find_root_bridge_handle(dev);
+	if (handle && context.root_handle) {
+		context.pci_bus = dev->subordinate;
+		context.user_function = register_slot;
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+				    register_slot, NULL, &context, NULL);
+	}
+	mutex_unlock(&slot_list_lock);
+}
+
+static void acpi_pci_slot_notify_del(struct pci_dev *dev)
+{
+	struct acpi_pci_slot *slot, *tmp;
+	struct pci_bus *bus = dev->subordinate;
+
+	if (!bus)
+		return;
+
+	mutex_lock(&slot_list_lock);
+	list_for_each_entry_safe(slot, tmp, &slot_list, list)
+		if (slot->pci_slot && slot->pci_slot->bus == bus) {
+			list_del(&slot->list);
+			pci_destroy_slot(slot->pci_slot);
+			put_device(&bus->dev);
+			kfree(slot);
+		}
+	mutex_unlock(&slot_list_lock);
+}
+
+static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
+				   unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpi_pci_slot_notify_add(to_pci_dev(dev));
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpi_pci_slot_notify_del(to_pci_dev(dev));
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_slot_notifier = {
+	.notifier_call = &acpi_pci_slot_notify_fn,
+};
+
 static int __init
 acpi_pci_slot_init(void)
 {
 	dmi_check_system(acpi_pci_slot_dmi_table);
 	acpi_pci_register_driver(&acpi_pci_slot_driver);
+	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
+
 	return 0;
 }
 
 static void __exit
 acpi_pci_slot_exit(void)
 {
+	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
 	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
 }
 
-- 
1.7.9.5

[PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Jiang Liu]

Currently the acpiphp driver fails to update hotplug slot information under
several conditions, such as:
1) The bridge device is removed through /sys/bus/pci/devices/.../remove
2) The bridge device is added/removed by PCI hotplug driver other than the
   acpiphp driver itself. For example, if an IO expansion box with ACPI
   hotplug slots available is hot-added by the pciehp driver, all ACPI
   hotplug slots won't be discovered by the acpiphp driver.

So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
update ACPI hotplug slot information when PCI hotplug event happens.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 3d6d4fd..16fe692 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -61,6 +61,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
 static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void acpiphp_set_hpp_values(struct pci_bus *bus);
 static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
+static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle);
 
 /* callback routine to check for the existence of a pci dock device */
 static acpi_status
@@ -420,12 +421,14 @@ static void add_host_bridge(struct acpi_pci_root *root)
 	init_bridge_misc(bridge);
 }
 
-
 /* allocate and initialize PCI-to-PCI bridge data structure */
-static void add_p2p_bridge(acpi_handle *handle)
+static void add_p2p_bridge(acpi_handle handle)
 {
 	struct acpiphp_bridge *bridge;
 
+	if (acpiphp_handle_to_bridge(handle))
+		return;
+
 	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
 	if (bridge == NULL) {
 		err("out of memory\n");
@@ -1428,6 +1431,58 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK ;
 }
 
+static void acpiphp_hp_notify_add(struct pci_dev *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
+
+	if (!dev->subordinate || !handle)
+		return;
+
+	/* check if this bridge has ejectable slots */
+	if (detect_ejectable_slots(handle) > 0) {
+		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
+		add_p2p_bridge(handle);
+	}
+}
+
+static void acpiphp_hp_notify_del(struct pci_dev *dev)
+{
+	struct acpiphp_bridge *bridge, *tmp;
+	struct pci_bus *bus = dev->subordinate;
+
+	if (!bus)
+		return;
+
+	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
+		if (bridge->pci_bus == bus) {
+			cleanup_bridge(bridge);
+			break;
+		}
+}
+
+static int acpi_pci_hp_notify_fn(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpiphp_hp_notify_add(to_pci_dev(dev));
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpiphp_hp_notify_del(to_pci_dev(dev));
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pci_hp_notifier = {
+	.notifier_call = &acpi_pci_hp_notify_fn,
+};
+
 static struct acpi_pci_driver acpi_pci_hp_driver = {
 	.add =		add_bridge,
 	.remove =	remove_bridge,
@@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
 	else
 		acpi_pci_register_driver(&acpi_pci_hp_driver);
 
+	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
+
 	return 0;
 }
 
@@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void)
  */
 void  acpiphp_glue_exit(void)
 {
+	bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
 	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
 }
 
-- 
1.7.9.5

[PATCH v3 5/6] PCI/acpiphp: serialize access to the bridge_list list

[Jiang Liu]

Serialize access to the bridge_list in the acpiphp driver.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 16fe692..884cbf4 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -53,6 +53,7 @@
 #include "../pci.h"
 #include "acpiphp.h"
 
+static DEFINE_MUTEX(bridge_mutex);
 static LIST_HEAD(bridge_list);
 
 #define MY_NAME "acpiphp_glue"
@@ -519,8 +520,10 @@ static int add_bridge(struct acpi_pci_root *root)
 	}
 
 	/* search P2P bridges under this host bridge */
+	mutex_lock(&bridge_mutex);
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
 				     find_p2p_bridge, NULL, NULL, NULL);
+	mutex_unlock(&bridge_mutex);
 
 	if (ACPI_FAILURE(status))
 		warn("find_p2p_bridge failed (error code = 0x%x)\n", status);
@@ -618,6 +621,8 @@ static void remove_bridge(struct acpi_pci_root *root)
 	struct acpiphp_bridge *bridge;
 	acpi_handle handle = root->device->handle;
 
+	mutex_lock(&bridge_mutex);
+
 	/* cleanup p2p bridges under this host bridge
 	   in a depth-first manner */
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
@@ -636,6 +641,8 @@ static void remove_bridge(struct acpi_pci_root *root)
 	else
 		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					   handle_hotplug_event_bridge);
+
+	mutex_unlock(&bridge_mutex);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)
@@ -1438,11 +1445,13 @@ static void acpiphp_hp_notify_add(struct pci_dev *dev)
 	if (!dev->subordinate || !handle)
 		return;
 
+	mutex_lock(&bridge_mutex);
 	/* check if this bridge has ejectable slots */
 	if (detect_ejectable_slots(handle) > 0) {
 		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
 		add_p2p_bridge(handle);
 	}
+	mutex_unlock(&bridge_mutex);
 }
 
 static void acpiphp_hp_notify_del(struct pci_dev *dev)
@@ -1453,11 +1462,13 @@ static void acpiphp_hp_notify_del(struct pci_dev *dev)
 	if (!bus)
 		return;
 
+	mutex_lock(&bridge_mutex);
 	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
 		if (bridge->pci_bus == bus) {
 			cleanup_bridge(bridge);
 			break;
 		}
+	mutex_unlock(&bridge_mutex);
 }
 
 static int acpi_pci_hp_notify_fn(struct notifier_block *nb,
-- 
1.7.9.5

[PATCH v3 6/6] PCI/AER: update AER configuration when PCI hotplug event happens

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

The AER driver only configures downstream PCIe devices at driver
binding time and all hot-added PCIe devices won't be managed by
the AER driver.  So hook PCIe device hotplug events to setup AER
configuration for hot-added PCIe devices.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/pcie/aer/aerdrv.c |   63 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 76ef634..d825c46 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -408,6 +408,55 @@ static void aer_error_resume(struct pci_dev *dev)
 	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 }
 
+static void acpi_pcie_aer_notify_dev(struct pcie_device *dev, bool enable)
+{
+	int rc, pos;
+	u32 val = 0;
+	struct pci_dev *pdev = dev->port;
+
+	if (dev->service != PCIE_PORT_SERVICE_AER)
+		return;
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
+		return;
+	while (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) {
+		pdev = pdev->bus->self;
+		if (!pdev)
+			return;
+	}
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
+	if (!pos)
+		return;
+	rc = pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, &val);
+	if (rc || val == 0xFFFFFFFF || !(val & ROOT_PORT_INTR_ON_MESG_MASK))
+		return;
+
+	set_device_error_reporting(dev->port, &enable);
+}
+
+static int acpi_pcie_aer_notify(struct notifier_block *nb,
+		unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		acpi_pcie_aer_notify_dev(to_pcie_device(dev), true);
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		acpi_pcie_aer_notify_dev(to_pcie_device(dev), false);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block acpi_pcie_aer_notifier = {
+	.notifier_call = &acpi_pcie_aer_notify,
+};
+
 /**
  * aer_service_init - register AER root service driver
  *
@@ -415,9 +464,20 @@ static void aer_error_resume(struct pci_dev *dev)
  */
 static int __init aer_service_init(void)
 {
+	int ret;
+
 	if (!pci_aer_available() || aer_acpi_firmware_first())
 		return -ENXIO;
-	return pcie_port_service_register(&aerdriver);
+
+	ret = pcie_port_service_register(&aerdriver);
+	if (!ret) {
+		ret = bus_register_notifier(&pcie_port_bus_type,
+					    &acpi_pcie_aer_notifier);
+		if (ret)
+			pcie_port_service_unregister(&aerdriver);
+	}
+
+	return ret;
 }
 
 /**
@@ -427,6 +487,7 @@ static int __init aer_service_init(void)
  */
 static void __exit aer_service_exit(void)
 {
+	bus_unregister_notifier(&pcie_port_bus_type, &acpi_pcie_aer_notifier);
 	pcie_port_service_unregister(&aerdriver);
 }
 
-- 
1.7.9.5

Re: [PATCH v3 0/6] Update PCI notification patchset to latest kernel version

[Yinghai Lu]

On Tue, Jan 8, 2013 at 8:52 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Hi Bjorn,
>         Rafael has developed a patchset targetting 3.9 merging window
> which outdates the fourth and fifth patches in my previous version,
> so I have just drop those two patches and updated the others to the
> latest code from your pci-next branch.
>         Thanks!
>
> Jiang Liu (5):
>   PCI: make PCI device create/destroy logic symmetric
>   PCI: split registration of PCI bus devices into two stages
>   ACPI/pci_slot: update PCI slot information when PCI hotplug event
>     happens
>   PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug
>     happens
>   PCI/acpiphp: serialize access to the bridge_list list
>
> Yijing Wang (1):
>   PCI/AER: update AER configuration when PCI hotplug event happens
>

last time during my test before, looks there are some problems with last four.

so I only put first two in my pci root bus hotplug branch.

maybe my memory is wrong. too long for those patches till now.

Re: [PATCH v3 2/6] PCI: split registration of PCI bus devices into two stages

[Rafael J. Wysocki]

On Wednesday, January 09, 2013 12:52:21 AM Jiang Liu wrote:
> 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 <jiang.liu@huawei.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/bus.c    |    2 +-
>  drivers/pci/probe.c  |    4 +++-
>  drivers/pci/remove.c |   10 +++++-----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index ad6a8b6..2fea481 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -198,7 +198,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;
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e557d6f..a8315c2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);

Well, the change here isn't really necessary, as far as I can say.

>  	if (error)
>  		goto class_dev_reg_err;
>  
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index fc38c48..a1fdd0f 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -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);
>  
> 

Apart from the above

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

Hi,

On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> Currently the pci_slot driver doesn't update PCI slot information
> when PCI device hotplug event happens, which may cause memory leak
> and returning stale information to user.
> 
> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> update PCI slot information when PCI hotplug event happens.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/acpi/pci_slot.c |   86 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 78 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d22585f..e04ea8e 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -32,6 +32,7 @@
>  #include <linux/acpi.h>
>  #include <acpi/acpi_bus.h>
>  #include <acpi/acpi_drivers.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/dmi.h>
>  
>  static bool debug;
> @@ -123,12 +124,7 @@ struct callback_args {
>  /*
>   * register_slot
>   *
> - * Called once for each SxFy object in the namespace. Don't worry about
> - * calling pci_create_slot multiple times for the same pci_bus:device,
> - * since each subsequent call simply bumps the refcount on the pci_slot.
> - *
> - * The number of calls to pci_destroy_slot from unregister_slot is
> - * symmetrical.
> + * Called once for each SxFy object in the namespace.
>   */
>  static acpi_status
>  register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> @@ -145,6 +141,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	if (device < 0)
>  		return AE_OK;
>  
> +	/* Avoid duplicated records for the same slot */
> +	list_for_each_entry(slot, &slot_list, list) {
> +		pci_slot = slot->pci_slot;
> +		if (pci_slot && pci_slot->bus == pci_bus &&
> +		    pci_slot->number == device) {
> +			return AE_OK;
> +		}
> +	}
> +
>  	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
>  	if (!slot) {
>  		err("%s: cannot allocate memory\n", __func__);
> @@ -162,9 +167,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	slot->root_handle = parent_context->root_handle;
>  	slot->pci_slot = pci_slot;
>  	INIT_LIST_HEAD(&slot->list);
> -	mutex_lock(&slot_list_lock);
>  	list_add(&slot->list, &slot_list);
> -	mutex_unlock(&slot_list_lock);
>  
>  	get_device(&pci_bus->dev);
>  
> @@ -274,7 +277,9 @@ acpi_pci_slot_add(struct acpi_pci_root *root)
>  {
>  	acpi_status status;
>  
> +	mutex_lock(&slot_list_lock);
>  	status = walk_root_bridge(root, register_slot);
> +	mutex_unlock(&slot_list_lock);
>  	if (ACPI_FAILURE(status))
>  		err("%s: register_slot failure - %d\n", __func__, status);
>  
> @@ -330,17 +335,82 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>  	{}
>  };
>  
> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> +{
> +	acpi_handle handle;
> +	struct callback_args context;
> +
> +	if (!dev->subordinate)
> +		return;
> +
> +	mutex_lock(&slot_list_lock);
> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
> +	context.root_handle = acpi_find_root_bridge_handle(dev);

There's a patch under discussion that removes this function.

Isn't there any other way to do this?

> +	if (handle && context.root_handle) {
> +		context.pci_bus = dev->subordinate;
> +		context.user_function = register_slot;
> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,

You can just pass 1 here I think.  Does the compiler complain?

> +				    register_slot, NULL, &context, NULL);
> +	}
> +	mutex_unlock(&slot_list_lock);
> +}
> +
> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> +{
> +	struct acpi_pci_slot *slot, *tmp;
> +	struct pci_bus *bus = dev->subordinate;
> +
> +	if (!bus)
> +		return;
> +
> +	mutex_lock(&slot_list_lock);
> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
> +			list_del(&slot->list);
> +			pci_destroy_slot(slot->pci_slot);
> +			put_device(&bus->dev);
> +			kfree(slot);
> +		}
> +	mutex_unlock(&slot_list_lock);
> +}
> +
> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> +				   unsigned long event, void *data)
> +{
> +	struct device *dev = data;
> +
> +	switch (event) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
> +		break;

Do I think correctly that this is going to be called for every PCI device
added to the system, even if it's not a bridge?

> +	case BUS_NOTIFY_DEL_DEVICE:
> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_pci_slot_notifier = {
> +	.notifier_call = &acpi_pci_slot_notify_fn,
> +};
> +
>  static int __init
>  acpi_pci_slot_init(void)
>  {
>  	dmi_check_system(acpi_pci_slot_dmi_table);
>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);

I wonder if/why this has to be so convoluted?

We have found a PCI bridge in the ACPI namespace, so we've created a struct
acpi_device for it and we've walked the namespace below it already.

Now we're creating a struct pci_dev for it and while registering it we're
going to walk the namespace below the bridge again to find and register its
slots and that is done indirectly from a bus type notifier.

Why can't we enumerate the slots directly upfront?

> +
>  	return 0;
>  }
>  
>  static void __exit
>  acpi_pci_slot_exit(void)
>  {
> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>  	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>  }

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Rafael J. Wysocki]

On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote:
> Currently the acpiphp driver fails to update hotplug slot information under
> several conditions, such as:
> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove
> 2) The bridge device is added/removed by PCI hotplug driver other than the
>    acpiphp driver itself. For example, if an IO expansion box with ACPI
>    hotplug slots available is hot-added by the pciehp driver, all ACPI
>    hotplug slots won't be discovered by the acpiphp driver.
> 
> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> update ACPI hotplug slot information when PCI hotplug event happens.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 3d6d4fd..16fe692 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -61,6 +61,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *);
>  static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void acpiphp_set_hpp_values(struct pci_bus *bus);
>  static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context);
> +static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle);
>  
>  /* callback routine to check for the existence of a pci dock device */
>  static acpi_status
> @@ -420,12 +421,14 @@ static void add_host_bridge(struct acpi_pci_root *root)
>  	init_bridge_misc(bridge);
>  }
>  
> -
>  /* allocate and initialize PCI-to-PCI bridge data structure */
> -static void add_p2p_bridge(acpi_handle *handle)
> +static void add_p2p_bridge(acpi_handle handle)
>  {
>  	struct acpiphp_bridge *bridge;
>  
> +	if (acpiphp_handle_to_bridge(handle))
> +		return;
> +
>  	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
>  	if (bridge == NULL) {
>  		err("out of memory\n");
> @@ -1428,6 +1431,58 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	return AE_OK ;
>  }
>  
> +static void acpiphp_hp_notify_add(struct pci_dev *dev)
> +{
> +	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> +
> +	if (!dev->subordinate || !handle)
> +		return;
> +
> +	/* check if this bridge has ejectable slots */
> +	if (detect_ejectable_slots(handle) > 0) {
> +		dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev));
> +		add_p2p_bridge(handle);
> +	}
> +}
> +
> +static void acpiphp_hp_notify_del(struct pci_dev *dev)
> +{
> +	struct acpiphp_bridge *bridge, *tmp;
> +	struct pci_bus *bus = dev->subordinate;
> +
> +	if (!bus)
> +		return;
> +
> +	list_for_each_entry_safe(bridge, tmp, &bridge_list, list)
> +		if (bridge->pci_bus == bus) {
> +			cleanup_bridge(bridge);
> +			break;
> +		}
> +}
> +
> +static int acpi_pci_hp_notify_fn(struct notifier_block *nb,
> +				 unsigned long event, void *data)
> +{
> +	struct device *dev = data;
> +
> +	switch (event) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		acpiphp_hp_notify_add(to_pci_dev(dev));
> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		acpiphp_hp_notify_del(to_pci_dev(dev));
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_pci_hp_notifier = {
> +	.notifier_call = &acpi_pci_hp_notify_fn,
> +};
> +
>  static struct acpi_pci_driver acpi_pci_hp_driver = {
>  	.add =		add_bridge,
>  	.remove =	remove_bridge,
> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
>  	else
>  		acpi_pci_register_driver(&acpi_pci_hp_driver);
>  
> +	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);

Your previous patch adds a PCI bus notifier for a similar thing.  Why are
you adding another one here?


> +
>  	return 0;
>  }
>  
> @@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void)
>   */
>  void  acpiphp_glue_exit(void)
>  {
> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
>  	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
>  }

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 0/6] Update PCI notification patchset to latest kernel version

[Yijing Wang]

On 2013/1/9 2:30, Yinghai Lu wrote:
> On Tue, Jan 8, 2013 at 8:52 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> Hi Bjorn,
>>         Rafael has developed a patchset targetting 3.9 merging window
>> which outdates the fourth and fifth patches in my previous version,
>> so I have just drop those two patches and updated the others to the
>> latest code from your pci-next branch.
>>         Thanks!
>>
>> Jiang Liu (5):
>>   PCI: make PCI device create/destroy logic symmetric
>>   PCI: split registration of PCI bus devices into two stages
>>   ACPI/pci_slot: update PCI slot information when PCI hotplug event
>>     happens
>>   PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug
>>     happens
>>   PCI/acpiphp: serialize access to the bridge_list list
>>
>> Yijing Wang (1):
>>   PCI/AER: update AER configuration when PCI hotplug event happens
>>
> 
> last time during my test before, looks there are some problems with last four.

Hi Yinghai,
   I will test the patchset again. Do you remember what problems occur during test the last four patches?
So I can try to find out the flaw.

Thanks!
Yijing.
> 
> so I only put first two in my pci root bus hotplug branch.
> 
> maybe my memory is wrong. too long for those patches till now.
> 
> 


-- 
Thanks!
Yijing

Re: [PATCH v3 2/6] PCI: split registration of PCI bus devices into two stages

[Jiang Liu]

On 01/09/2013 07:29 AM, Rafael J. Wysocki wrote:
> On Wednesday, January 09, 2013 12:52:21 AM Jiang Liu wrote:
snip
>> @@ -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);
> 
> Well, the change here isn't really necessary, as far as I can say.
Hi Rafael,
	The kernel documentation recommends to pair device_add() with device_del(),
so the above change is to follow that recommendation because pci_remove_bus() now
invokes device_del().

> 
>>  	if (error)
>>  		goto class_dev_reg_err;
>>  
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index fc38c48..a1fdd0f 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -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);
>>  
>>
> 
> Apart from the above
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Thanks for your review and Ack.

Regards!
Gerry

Re: [PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Jiang Liu]

On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote:
> On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote:
>> Currently the acpiphp driver fails to update hotplug slot information under
>> several conditions, such as:
>> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove
>> 2) The bridge device is added/removed by PCI hotplug driver other than the
>>    acpiphp driver itself. For example, if an IO expansion box with ACPI
>>    hotplug slots available is hot-added by the pciehp driver, all ACPI
>>    hotplug slots won't be discovered by the acpiphp driver.
>>
>> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
>> update ACPI hotplug slot information when PCI hotplug event happens.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 60 insertions(+), 2 deletions(-)
>>
snip
>> +static struct notifier_block acpi_pci_hp_notifier = {
>> +	.notifier_call = &acpi_pci_hp_notify_fn,
>> +};
>> +
>>  static struct acpi_pci_driver acpi_pci_hp_driver = {
>>  	.add =		add_bridge,
>>  	.remove =	remove_bridge,
>> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
>>  	else
>>  		acpi_pci_register_driver(&acpi_pci_hp_driver);
>>  
>> +	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
> 
> Your previous patch adds a PCI bus notifier for a similar thing.  Why are
> you adding another one here?
Hi Rafael,
	The acpiphp driver could be built as a module, and this bus notifier
is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge.
The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug
controller are related but different objects, so we use two notifiers to support
them all.
	Thanks!
	Gerry

> 
> 
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void)
>>   */
>>  void  acpiphp_glue_exit(void)
>>  {
>> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
>>  	acpi_pci_unregister_driver(&acpi_pci_hp_driver);
>>  }
> 
> Thanks,
> Rafael
> 
> 

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Jiang Liu]

Hi Rafael,
	Thanks for your great efforts to review the patch.	

On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
snip
>>  
>> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
>> +{
>> +	acpi_handle handle;
>> +	struct callback_args context;
>> +
>> +	if (!dev->subordinate)
>> +		return;
>> +
>> +	mutex_lock(&slot_list_lock);
>> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
>> +	context.root_handle = acpi_find_root_bridge_handle(dev);
> 
> There's a patch under discussion that removes this function.
> 
> Isn't there any other way to do this?
	I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
and it seems doable.

> 
>> +	if (handle && context.root_handle) {
>> +		context.pci_bus = dev->subordinate;
>> +		context.user_function = register_slot;
>> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> 
> You can just pass 1 here I think.  Does the compiler complain?
Thanks for reminder, the (u32) is unnecessary.

> 
>> +				    register_slot, NULL, &context, NULL);
>> +	}
>> +	mutex_unlock(&slot_list_lock);
>> +}
>> +
>> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
>> +{
>> +	struct acpi_pci_slot *slot, *tmp;
>> +	struct pci_bus *bus = dev->subordinate;
>> +
>> +	if (!bus)
>> +		return;
>> +
>> +	mutex_lock(&slot_list_lock);
>> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
>> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
>> +			list_del(&slot->list);
>> +			pci_destroy_slot(slot->pci_slot);
>> +			put_device(&bus->dev);
>> +			kfree(slot);
>> +		}
>> +	mutex_unlock(&slot_list_lock);
>> +}
>> +
>> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
>> +				   unsigned long event, void *data)
>> +{
>> +	struct device *dev = data;
>> +
>> +	switch (event) {
>> +	case BUS_NOTIFY_ADD_DEVICE:
>> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
>> +		break;
> 
> Do I think correctly that this is going to be called for every PCI device
> added to the system, even if it's not a bridge?
You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
will check whether it's a bridge. If preferred, I will move the check up into
acpi_pci_slot_notify_fn().

> 
>> +	case BUS_NOTIFY_DEL_DEVICE:
>> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
>> +		break;
>> +	default:
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block acpi_pci_slot_notifier = {
>> +	.notifier_call = &acpi_pci_slot_notify_fn,
>> +};
>> +
>>  static int __init
>>  acpi_pci_slot_init(void)
>>  {
>>  	dmi_check_system(acpi_pci_slot_dmi_table);
>>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
>> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> 
> I wonder if/why this has to be so convoluted?
> 
> We have found a PCI bridge in the ACPI namespace, so we've created a struct
> acpi_device for it and we've walked the namespace below it already.
> 
> Now we're creating a struct pci_dev for it and while registering it we're
> going to walk the namespace below the bridge again to find and register its
> slots and that is done indirectly from a bus type notifier.
> 
> Why can't we enumerate the slots directly upfront?
Do you mean to create the PCI slot devices when creating the ACPI devices?
I think there are two factors prevent us from doing that.
The first is that the ACPI pci_slot driver could be built as a module, so
we can't call into it from the ACPI core.
The second reason is that the PCI slot is associated with a PCI bus, and the
bus is only available until the PCI device has been created.

Thanks!
Gerry
> 
>> +
>>  	return 0;
>>  }
>>  
>>  static void __exit
>>  acpi_pci_slot_exit(void)
>>  {
>> +	bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>>  	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>>  }
> 
> Thanks,
> Rafael
> 
> 

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> Hi Rafael,
> 	Thanks for your great efforts to review the patch.	
> 
> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> snip
> >>  
> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> +{
> >> +	acpi_handle handle;
> >> +	struct callback_args context;
> >> +
> >> +	if (!dev->subordinate)
> >> +		return;
> >> +
> >> +	mutex_lock(&slot_list_lock);
> >> +	handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> +	context.root_handle = acpi_find_root_bridge_handle(dev);
> > 
> > There's a patch under discussion that removes this function.
> > 
> > Isn't there any other way to do this?
> 	I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> and it seems doable.
> 
> > 
> >> +	if (handle && context.root_handle) {
> >> +		context.pci_bus = dev->subordinate;
> >> +		context.user_function = register_slot;
> >> +		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> > 
> > You can just pass 1 here I think.  Does the compiler complain?
> Thanks for reminder, the (u32) is unnecessary.
> 
> > 
> >> +				    register_slot, NULL, &context, NULL);
> >> +	}
> >> +	mutex_unlock(&slot_list_lock);
> >> +}
> >> +
> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> +{
> >> +	struct acpi_pci_slot *slot, *tmp;
> >> +	struct pci_bus *bus = dev->subordinate;
> >> +
> >> +	if (!bus)
> >> +		return;
> >> +
> >> +	mutex_lock(&slot_list_lock);
> >> +	list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> +		if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> +			list_del(&slot->list);
> >> +			pci_destroy_slot(slot->pci_slot);
> >> +			put_device(&bus->dev);
> >> +			kfree(slot);
> >> +		}
> >> +	mutex_unlock(&slot_list_lock);
> >> +}
> >> +
> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> +				   unsigned long event, void *data)
> >> +{
> >> +	struct device *dev = data;
> >> +
> >> +	switch (event) {
> >> +	case BUS_NOTIFY_ADD_DEVICE:
> >> +		acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> +		break;
> > 
> > Do I think correctly that this is going to be called for every PCI device
> > added to the system, even if it's not a bridge?
> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> will check whether it's a bridge. If preferred, I will move the check up into
> acpi_pci_slot_notify_fn().
> 
> > 
> >> +	case BUS_NOTIFY_DEL_DEVICE:
> >> +		acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> +		break;
> >> +	default:
> >> +		return NOTIFY_DONE;
> >> +	}
> >> +
> >> +	return NOTIFY_OK;
> >> +}
> >> +
> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> +	.notifier_call = &acpi_pci_slot_notify_fn,
> >> +};
> >> +
> >>  static int __init
> >>  acpi_pci_slot_init(void)
> >>  {
> >>  	dmi_check_system(acpi_pci_slot_dmi_table);
> >>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> +	bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> > 
> > I wonder if/why this has to be so convoluted?
> > 
> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> > acpi_device for it and we've walked the namespace below it already.
> > 
> > Now we're creating a struct pci_dev for it and while registering it we're
> > going to walk the namespace below the bridge again to find and register its
> > slots and that is done indirectly from a bus type notifier.
> > 
> > Why can't we enumerate the slots directly upfront?
> Do you mean to create the PCI slot devices when creating the ACPI devices?
> I think there are two factors prevent us from doing that.
> The first is that the ACPI pci_slot driver could be built as a module, so
> we can't call into it from the ACPI core.

I didn't say about calling the pci_slot driver from the ACPI core, but about
enumerating slots in a way suitable for consumption by the pci_slot driver
when it's ready.

That said I really don't see a value in having a modular pci_slot driver.  It
is part of the hotplug infrastructure and should always be presend for this
reason, so we don't need to worry about the "pci_slot driver not present" case.

> The second reason is that the PCI slot is associated with a PCI bus, and the
> bus is only available until the PCI device has been created.

I suppose you mean "after"?  [I'm not sure if I agree with that, but whatever.]

We know which devices have slots before that happens, though.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Rafael J. Wysocki]

On Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote:
> On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote:
> > On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote:
> >> Currently the acpiphp driver fails to update hotplug slot information under
> >> several conditions, such as:
> >> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove
> >> 2) The bridge device is added/removed by PCI hotplug driver other than the
> >>    acpiphp driver itself. For example, if an IO expansion box with ACPI
> >>    hotplug slots available is hot-added by the pciehp driver, all ACPI
> >>    hotplug slots won't be discovered by the acpiphp driver.
> >>
> >> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> >> update ACPI hotplug slot information when PCI hotplug event happens.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> >> ---
> >>  drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 60 insertions(+), 2 deletions(-)
> >>
> snip
> >> +static struct notifier_block acpi_pci_hp_notifier = {
> >> +	.notifier_call = &acpi_pci_hp_notify_fn,
> >> +};
> >> +
> >>  static struct acpi_pci_driver acpi_pci_hp_driver = {
> >>  	.add =		add_bridge,
> >>  	.remove =	remove_bridge,
> >> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
> >>  	else
> >>  		acpi_pci_register_driver(&acpi_pci_hp_driver);
> >>  
> >> +	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
> > 
> > Your previous patch adds a PCI bus notifier for a similar thing.  Why are
> > you adding another one here?
> Hi Rafael,
> 	The acpiphp driver could be built as a module, and this bus notifier
> is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge.
> The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug
> controller are related but different objects, so we use two notifiers to support
> them all.

I would be much happier if those notifiers were not needed.  At least, I'm not
sure why they need to be invoked by the driver core during device registration.

In my opinion it would be much better if they were called only for the devices
that might need them instead of all PCI devices.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Bjorn Helgaas]

[+cc Myron]

On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
>> Hi Rafael,
>>       Thanks for your great efforts to review the patch.
>>
>> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
>> > Hi,
>> >
>> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
>> snip
>> >>
>> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
>> >> +{
>> >> +  acpi_handle handle;
>> >> +  struct callback_args context;
>> >> +
>> >> +  if (!dev->subordinate)
>> >> +          return;
>> >> +
>> >> +  mutex_lock(&slot_list_lock);
>> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
>> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
>> >
>> > There's a patch under discussion that removes this function.
>> >
>> > Isn't there any other way to do this?
>>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
>> and it seems doable.
>>
>> >
>> >> +  if (handle && context.root_handle) {
>> >> +          context.pci_bus = dev->subordinate;
>> >> +          context.user_function = register_slot;
>> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> >
>> > You can just pass 1 here I think.  Does the compiler complain?
>> Thanks for reminder, the (u32) is unnecessary.
>>
>> >
>> >> +                              register_slot, NULL, &context, NULL);
>> >> +  }
>> >> +  mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
>> >> +{
>> >> +  struct acpi_pci_slot *slot, *tmp;
>> >> +  struct pci_bus *bus = dev->subordinate;
>> >> +
>> >> +  if (!bus)
>> >> +          return;
>> >> +
>> >> +  mutex_lock(&slot_list_lock);
>> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
>> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
>> >> +                  list_del(&slot->list);
>> >> +                  pci_destroy_slot(slot->pci_slot);
>> >> +                  put_device(&bus->dev);
>> >> +                  kfree(slot);
>> >> +          }
>> >> +  mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
>> >> +                             unsigned long event, void *data)
>> >> +{
>> >> +  struct device *dev = data;
>> >> +
>> >> +  switch (event) {
>> >> +  case BUS_NOTIFY_ADD_DEVICE:
>> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
>> >> +          break;
>> >
>> > Do I think correctly that this is going to be called for every PCI device
>> > added to the system, even if it's not a bridge?
>> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
>> will check whether it's a bridge. If preferred, I will move the check up into
>> acpi_pci_slot_notify_fn().
>>
>> >
>> >> +  case BUS_NOTIFY_DEL_DEVICE:
>> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
>> >> +          break;
>> >> +  default:
>> >> +          return NOTIFY_DONE;
>> >> +  }
>> >> +
>> >> +  return NOTIFY_OK;
>> >> +}
>> >> +
>> >> +static struct notifier_block acpi_pci_slot_notifier = {
>> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
>> >> +};
>> >> +
>> >>  static int __init
>> >>  acpi_pci_slot_init(void)
>> >>  {
>> >>    dmi_check_system(acpi_pci_slot_dmi_table);
>> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
>> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>> >
>> > I wonder if/why this has to be so convoluted?
>> >
>> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
>> > acpi_device for it and we've walked the namespace below it already.
>> >
>> > Now we're creating a struct pci_dev for it and while registering it we're
>> > going to walk the namespace below the bridge again to find and register its
>> > slots and that is done indirectly from a bus type notifier.
>> >
>> > Why can't we enumerate the slots directly upfront?
>> Do you mean to create the PCI slot devices when creating the ACPI devices?
>> I think there are two factors prevent us from doing that.
>> The first is that the ACPI pci_slot driver could be built as a module, so
>> we can't call into it from the ACPI core.
>
> I didn't say about calling the pci_slot driver from the ACPI core, but about
> enumerating slots in a way suitable for consumption by the pci_slot driver
> when it's ready.
>
> That said I really don't see a value in having a modular pci_slot driver.  It
> is part of the hotplug infrastructure and should always be presend for this
> reason, so we don't need to worry about the "pci_slot driver not present" case.

I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
think Myron has some patches that remove that case.

I'm not sure what the best way to merge them is.  We have a bunch of
stuff this cycle that touches both ACPI and PCI.

Bjorn

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Wednesday, January 09, 2013 01:44:23 PM Bjorn Helgaas wrote:
> [+cc Myron]
> 
> On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> >> Hi Rafael,
> >>       Thanks for your great efforts to review the patch.
> >>
> >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> >> > Hi,
> >> >
> >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> >> snip
> >> >>
> >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> >> +{
> >> >> +  acpi_handle handle;
> >> >> +  struct callback_args context;
> >> >> +
> >> >> +  if (!dev->subordinate)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> >> >
> >> > There's a patch under discussion that removes this function.
> >> >
> >> > Isn't there any other way to do this?
> >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> >> and it seems doable.
> >>
> >> >
> >> >> +  if (handle && context.root_handle) {
> >> >> +          context.pci_bus = dev->subordinate;
> >> >> +          context.user_function = register_slot;
> >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> >> >
> >> > You can just pass 1 here I think.  Does the compiler complain?
> >> Thanks for reminder, the (u32) is unnecessary.
> >>
> >> >
> >> >> +                              register_slot, NULL, &context, NULL);
> >> >> +  }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> >> +{
> >> >> +  struct acpi_pci_slot *slot, *tmp;
> >> >> +  struct pci_bus *bus = dev->subordinate;
> >> >> +
> >> >> +  if (!bus)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> >> +                  list_del(&slot->list);
> >> >> +                  pci_destroy_slot(slot->pci_slot);
> >> >> +                  put_device(&bus->dev);
> >> >> +                  kfree(slot);
> >> >> +          }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> >> +                             unsigned long event, void *data)
> >> >> +{
> >> >> +  struct device *dev = data;
> >> >> +
> >> >> +  switch (event) {
> >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> >> +          break;
> >> >
> >> > Do I think correctly that this is going to be called for every PCI device
> >> > added to the system, even if it's not a bridge?
> >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> >> will check whether it's a bridge. If preferred, I will move the check up into
> >> acpi_pci_slot_notify_fn().
> >>
> >> >
> >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> >> +          break;
> >> >> +  default:
> >> >> +          return NOTIFY_DONE;
> >> >> +  }
> >> >> +
> >> >> +  return NOTIFY_OK;
> >> >> +}
> >> >> +
> >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> >> >> +};
> >> >> +
> >> >>  static int __init
> >> >>  acpi_pci_slot_init(void)
> >> >>  {
> >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> >> >
> >> > I wonder if/why this has to be so convoluted?
> >> >
> >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> >> > acpi_device for it and we've walked the namespace below it already.
> >> >
> >> > Now we're creating a struct pci_dev for it and while registering it we're
> >> > going to walk the namespace below the bridge again to find and register its
> >> > slots and that is done indirectly from a bus type notifier.
> >> >
> >> > Why can't we enumerate the slots directly upfront?
> >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> >> I think there are two factors prevent us from doing that.
> >> The first is that the ACPI pci_slot driver could be built as a module, so
> >> we can't call into it from the ACPI core.
> >
> > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > enumerating slots in a way suitable for consumption by the pci_slot driver
> > when it's ready.
> >
> > That said I really don't see a value in having a modular pci_slot driver.  It
> > is part of the hotplug infrastructure and should always be presend for this
> > reason, so we don't need to worry about the "pci_slot driver not present" case.
> 
> I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> think Myron has some patches that remove that case.
> 
> I'm not sure what the best way to merge them is.  We have a bunch of
> stuff this cycle that touches both ACPI and PCI.

Well, is there a possibility to pull my acpi-scan branch into your tree?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Myron Stowe]

[+cc Yinghai]

On Wed, 2013-01-09 at 13:44 -0700, Bjorn Helgaas wrote:
> [+cc Myron]
> 
> On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> >> Hi Rafael,
> >>       Thanks for your great efforts to review the patch.
> >>
> >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> >> > Hi,
> >> >
> >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> >> snip
> >> >>
> >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> >> >> +{
> >> >> +  acpi_handle handle;
> >> >> +  struct callback_args context;
> >> >> +
> >> >> +  if (!dev->subordinate)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> >> >
> >> > There's a patch under discussion that removes this function.
> >> >
> >> > Isn't there any other way to do this?
> >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> >> and it seems doable.
> >>
> >> >
> >> >> +  if (handle && context.root_handle) {
> >> >> +          context.pci_bus = dev->subordinate;
> >> >> +          context.user_function = register_slot;
> >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> >> >
> >> > You can just pass 1 here I think.  Does the compiler complain?
> >> Thanks for reminder, the (u32) is unnecessary.
> >>
> >> >
> >> >> +                              register_slot, NULL, &context, NULL);
> >> >> +  }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> >> >> +{
> >> >> +  struct acpi_pci_slot *slot, *tmp;
> >> >> +  struct pci_bus *bus = dev->subordinate;
> >> >> +
> >> >> +  if (!bus)
> >> >> +          return;
> >> >> +
> >> >> +  mutex_lock(&slot_list_lock);
> >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> >> >> +                  list_del(&slot->list);
> >> >> +                  pci_destroy_slot(slot->pci_slot);
> >> >> +                  put_device(&bus->dev);
> >> >> +                  kfree(slot);
> >> >> +          }
> >> >> +  mutex_unlock(&slot_list_lock);
> >> >> +}
> >> >> +
> >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> >> >> +                             unsigned long event, void *data)
> >> >> +{
> >> >> +  struct device *dev = data;
> >> >> +
> >> >> +  switch (event) {
> >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> >> >> +          break;
> >> >
> >> > Do I think correctly that this is going to be called for every PCI device
> >> > added to the system, even if it's not a bridge?
> >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> >> will check whether it's a bridge. If preferred, I will move the check up into
> >> acpi_pci_slot_notify_fn().
> >>
> >> >
> >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> >> >> +          break;
> >> >> +  default:
> >> >> +          return NOTIFY_DONE;
> >> >> +  }
> >> >> +
> >> >> +  return NOTIFY_OK;
> >> >> +}
> >> >> +
> >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> >> >> +};
> >> >> +
> >> >>  static int __init
> >> >>  acpi_pci_slot_init(void)
> >> >>  {
> >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> >> >
> >> > I wonder if/why this has to be so convoluted?
> >> >
> >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> >> > acpi_device for it and we've walked the namespace below it already.
> >> >
> >> > Now we're creating a struct pci_dev for it and while registering it we're
> >> > going to walk the namespace below the bridge again to find and register its
> >> > slots and that is done indirectly from a bus type notifier.
> >> >
> >> > Why can't we enumerate the slots directly upfront?
> >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> >> I think there are two factors prevent us from doing that.
> >> The first is that the ACPI pci_slot driver could be built as a module, so
> >> we can't call into it from the ACPI core.
> >
> > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > enumerating slots in a way suitable for consumption by the pci_slot driver
> > when it's ready.
> >
> > That said I really don't see a value in having a modular pci_slot driver.  It
> > is part of the hotplug infrastructure and should always be presend for this
> > reason, so we don't need to worry about the "pci_slot driver not present" case.
> 
> I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> think Myron has some patches that remove that case.
> 
> I'm not sure what the best way to merge them is.  We have a bunch of
> stuff this cycle that touches both ACPI and PCI.

Rafael:

The series Bjorn mentions is at https://lkml.org/lkml/2012/12/7/11  It
converts both the "ACPI Hot Plug PCI Controller Driver ("acpiphp")" and
"ACPI PCI Slot Detection Driver ("pci_slot")" sub-drivers to built-in
drivers (i.e. no longer supported as modules).

Yinghai commented back - https://lkml.org/lkml/2012/12/7/29 - indicating
a possible issue with such but his response was too terse for me to get
anything from.  Also, I've noticed that a couple of the distros, debian
being one, have made similar changes so I'm a little skeptical about
Yinghai's concerns.

Since you have similar thoughts - "I really don't see a value in having
a modular pci_slot driver" - can you foresee any issues with such?

Thanks,
 Myron
> 
> Bjorn

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Thursday, January 10, 2013 02:24:23 PM Myron Stowe wrote:
> [+cc Yinghai]
> 
> On Wed, 2013-01-09 at 13:44 -0700, Bjorn Helgaas wrote:
> > [+cc Myron]
> > 
> > On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> > >> Hi Rafael,
> > >>       Thanks for your great efforts to review the patch.
> > >>
> > >> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> > >> > Hi,
> > >> >
> > >> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> > >> snip
> > >> >>
> > >> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> > >> >> +{
> > >> >> +  acpi_handle handle;
> > >> >> +  struct callback_args context;
> > >> >> +
> > >> >> +  if (!dev->subordinate)
> > >> >> +          return;
> > >> >> +
> > >> >> +  mutex_lock(&slot_list_lock);
> > >> >> +  handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > >> >> +  context.root_handle = acpi_find_root_bridge_handle(dev);
> > >> >
> > >> > There's a patch under discussion that removes this function.
> > >> >
> > >> > Isn't there any other way to do this?
> > >>       I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> > >> and it seems doable.
> > >>
> > >> >
> > >> >> +  if (handle && context.root_handle) {
> > >> >> +          context.pci_bus = dev->subordinate;
> > >> >> +          context.user_function = register_slot;
> > >> >> +          acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> > >> >
> > >> > You can just pass 1 here I think.  Does the compiler complain?
> > >> Thanks for reminder, the (u32) is unnecessary.
> > >>
> > >> >
> > >> >> +                              register_slot, NULL, &context, NULL);
> > >> >> +  }
> > >> >> +  mutex_unlock(&slot_list_lock);
> > >> >> +}
> > >> >> +
> > >> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> > >> >> +{
> > >> >> +  struct acpi_pci_slot *slot, *tmp;
> > >> >> +  struct pci_bus *bus = dev->subordinate;
> > >> >> +
> > >> >> +  if (!bus)
> > >> >> +          return;
> > >> >> +
> > >> >> +  mutex_lock(&slot_list_lock);
> > >> >> +  list_for_each_entry_safe(slot, tmp, &slot_list, list)
> > >> >> +          if (slot->pci_slot && slot->pci_slot->bus == bus) {
> > >> >> +                  list_del(&slot->list);
> > >> >> +                  pci_destroy_slot(slot->pci_slot);
> > >> >> +                  put_device(&bus->dev);
> > >> >> +                  kfree(slot);
> > >> >> +          }
> > >> >> +  mutex_unlock(&slot_list_lock);
> > >> >> +}
> > >> >> +
> > >> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> > >> >> +                             unsigned long event, void *data)
> > >> >> +{
> > >> >> +  struct device *dev = data;
> > >> >> +
> > >> >> +  switch (event) {
> > >> >> +  case BUS_NOTIFY_ADD_DEVICE:
> > >> >> +          acpi_pci_slot_notify_add(to_pci_dev(dev));
> > >> >> +          break;
> > >> >
> > >> > Do I think correctly that this is going to be called for every PCI device
> > >> > added to the system, even if it's not a bridge?
> > >> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> > >> will check whether it's a bridge. If preferred, I will move the check up into
> > >> acpi_pci_slot_notify_fn().
> > >>
> > >> >
> > >> >> +  case BUS_NOTIFY_DEL_DEVICE:
> > >> >> +          acpi_pci_slot_notify_del(to_pci_dev(dev));
> > >> >> +          break;
> > >> >> +  default:
> > >> >> +          return NOTIFY_DONE;
> > >> >> +  }
> > >> >> +
> > >> >> +  return NOTIFY_OK;
> > >> >> +}
> > >> >> +
> > >> >> +static struct notifier_block acpi_pci_slot_notifier = {
> > >> >> +  .notifier_call = &acpi_pci_slot_notify_fn,
> > >> >> +};
> > >> >> +
> > >> >>  static int __init
> > >> >>  acpi_pci_slot_init(void)
> > >> >>  {
> > >> >>    dmi_check_system(acpi_pci_slot_dmi_table);
> > >> >>    acpi_pci_register_driver(&acpi_pci_slot_driver);
> > >> >> +  bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> > >> >
> > >> > I wonder if/why this has to be so convoluted?
> > >> >
> > >> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
> > >> > acpi_device for it and we've walked the namespace below it already.
> > >> >
> > >> > Now we're creating a struct pci_dev for it and while registering it we're
> > >> > going to walk the namespace below the bridge again to find and register its
> > >> > slots and that is done indirectly from a bus type notifier.
> > >> >
> > >> > Why can't we enumerate the slots directly upfront?
> > >> Do you mean to create the PCI slot devices when creating the ACPI devices?
> > >> I think there are two factors prevent us from doing that.
> > >> The first is that the ACPI pci_slot driver could be built as a module, so
> > >> we can't call into it from the ACPI core.
> > >
> > > I didn't say about calling the pci_slot driver from the ACPI core, but about
> > > enumerating slots in a way suitable for consumption by the pci_slot driver
> > > when it's ready.
> > >
> > > That said I really don't see a value in having a modular pci_slot driver.  It
> > > is part of the hotplug infrastructure and should always be presend for this
> > > reason, so we don't need to worry about the "pci_slot driver not present" case.
> > 
> > I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> > think Myron has some patches that remove that case.
> > 
> > I'm not sure what the best way to merge them is.  We have a bunch of
> > stuff this cycle that touches both ACPI and PCI.
> 
> Rafael:
> 
> The series Bjorn mentions is at https://lkml.org/lkml/2012/12/7/11  It
> converts both the "ACPI Hot Plug PCI Controller Driver ("acpiphp")" and
> "ACPI PCI Slot Detection Driver ("pci_slot")" sub-drivers to built-in
> drivers (i.e. no longer supported as modules).
> 
> Yinghai commented back - https://lkml.org/lkml/2012/12/7/29 - indicating
> a possible issue with such but his response was too terse for me to get
> anything from.  Also, I've noticed that a couple of the distros, debian
> being one, have made similar changes so I'm a little skeptical about
> Yinghai's concerns.
> 
> Since you have similar thoughts - "I really don't see a value in having
> a modular pci_slot driver" - can you foresee any issues with such?

Well, I don't see what functional problems that can bring.

In theory people may want to have them as modules to avoid loading them on
systems that don't use PCI hotplug, but honestly I think that the complexity
this causes us to deal with is not worth it.

Moreover, removing the modularity may actually allow us to solve some ordering
issues once and for good.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Yinghai Lu]

On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Well, I don't see what functional problems that can bring.
>
> In theory people may want to have them as modules to avoid loading them on
> systems that don't use PCI hotplug, but honestly I think that the complexity
> this causes us to deal with is not worth it.
>
> Moreover, removing the modularity may actually allow us to solve some ordering
> issues once and for good.

No, the world is not really ideal yet.

looks like laptops have problem with pci express cards.

when pciehp is used, surprise insert/removal does not work because
PresDect does not change properly, so no interrupt is generated.
--- i suspects that is silicon problem.

but when acpiphp is used, that surprise  insert/removal is working.

some laptop like thinkpad, just don't give osc to kernel..
[    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
[    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
returned control mask: 0x0d
[    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM

and other laptop give that to kernel, in recent kernel will not give
acpiphp to have that slot, because it want to hold that for pciehp.
poor user have to pass 'pci_aspm=off" to disable _OSC for all.
--- please check the mail that i forward to you yesterday.

Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
and it should be first come and first serve policy.

Thanks

Yinghai

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Well, I don't see what functional problems that can bring.
> >
> > In theory people may want to have them as modules to avoid loading them on
> > systems that don't use PCI hotplug, but honestly I think that the complexity
> > this causes us to deal with is not worth it.
> >
> > Moreover, removing the modularity may actually allow us to solve some ordering
> > issues once and for good.
> 
> No, the world is not really ideal yet.
> 
> looks like laptops have problem with pci express cards.
> 
> when pciehp is used, surprise insert/removal does not work because
> PresDect does not change properly, so no interrupt is generated.
> --- i suspects that is silicon problem.
> 
> but when acpiphp is used, that surprise  insert/removal is working.
> 
> some laptop like thinkpad, just don't give osc to kernel..
> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> returned control mask: 0x0d
> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> 
> and other laptop give that to kernel, in recent kernel will not give
> acpiphp to have that slot, because it want to hold that for pciehp.
> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> --- please check the mail that i forward to you yesterday.

Yes, this is a bug, but I'm not sure how to fix it yet.

> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> and it should be first come and first serve policy.

And that's why you think they should be modules?  I disagree if so.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Yinghai Lu]

On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > Well, I don't see what functional problems that can bring.
>> >
>> > In theory people may want to have them as modules to avoid loading them on
>> > systems that don't use PCI hotplug, but honestly I think that the complexity
>> > this causes us to deal with is not worth it.
>> >
>> > Moreover, removing the modularity may actually allow us to solve some ordering
>> > issues once and for good.
>>
>> No, the world is not really ideal yet.
>>
>> looks like laptops have problem with pci express cards.
>>
>> when pciehp is used, surprise insert/removal does not work because
>> PresDect does not change properly, so no interrupt is generated.
>> --- i suspects that is silicon problem.
>>
>> but when acpiphp is used, that surprise  insert/removal is working.
>>
>> some laptop like thinkpad, just don't give osc to kernel..
>> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> returned control mask: 0x0d
>> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>>
>> and other laptop give that to kernel, in recent kernel will not give
>> acpiphp to have that slot, because it want to hold that for pciehp.
>> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> --- please check the mail that i forward to you yesterday.
>
> Yes, this is a bug, but I'm not sure how to fix it yet.

add one command line to control it so do not claim that in osc_control?

>
>> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> and it should be first come and first serve policy.
>
> And that's why you think they should be modules?  I disagree if so.

Yes.

maybe we can have pci=nopciehp in command line just we pci=noaer...

that should handle some corner cases.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > Well, I don't see what functional problems that can bring.
> >> >
> >> > In theory people may want to have them as modules to avoid loading them on
> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
> >> > this causes us to deal with is not worth it.
> >> >
> >> > Moreover, removing the modularity may actually allow us to solve some ordering
> >> > issues once and for good.
> >>
> >> No, the world is not really ideal yet.
> >>
> >> looks like laptops have problem with pci express cards.
> >>
> >> when pciehp is used, surprise insert/removal does not work because
> >> PresDect does not change properly, so no interrupt is generated.
> >> --- i suspects that is silicon problem.
> >>
> >> but when acpiphp is used, that surprise  insert/removal is working.
> >>
> >> some laptop like thinkpad, just don't give osc to kernel..
> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> returned control mask: 0x0d
> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >>
> >> and other laptop give that to kernel, in recent kernel will not give
> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> --- please check the mail that i forward to you yesterday.
> >
> > Yes, this is a bug, but I'm not sure how to fix it yet.
> 
> add one command line to control it so do not claim that in osc_control?

That's one option, although not very attractive so to speak.

> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> and it should be first come and first serve policy.
> >
> > And that's why you think they should be modules?  I disagree if so.
> 
> Yes.
> 
> maybe we can have pci=nopciehp in command line just we pci=noaer...
>
> that should handle some corner cases.

Yes.  In any case the user should be able to say "I know better", but having
to express that through the "right" ordering of modules is somewhat less than
straightforward in my opinion.

I need to reconsider that code again, maybe I'll figure out what can be done.
It looks like the assumptions it was written with don't really reflect the
reality entirely.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Martin Mokrejs]

Hi,
  I just hit this thread in my bloated Inbox.

Rafael J. Wysocki wrote:
> On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> Well, I don't see what functional problems that can bring.
>>>
>>> In theory people may want to have them as modules to avoid loading them on
>>> systems that don't use PCI hotplug, but honestly I think that the complexity
>>> this causes us to deal with is not worth it.
>>>
>>> Moreover, removing the modularity may actually allow us to solve some ordering
>>> issues once and for good.
>>
>> No, the world is not really ideal yet.
>>
>> looks like laptops have problem with pci express cards.
>>
>> when pciehp is used, surprise insert/removal does not work because
>> PresDect does not change properly, so no interrupt is generated.
>> --- i suspects that is silicon problem.

That's what seemed to be the conclusion half a year ago around 3.2.x/3.3.x
for my issues as well (SandyBridge C6/C200 chipset).

>>
>> but when acpiphp is used, that surprise  insert/removal is working.

That's what I discovered few days ago as well. However, there are still some
differences between individual express cards and I just need to find some time to
dig through the data I collected.

>>
>> some laptop like thinkpad, just don't give osc to kernel..
>> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> returned control mask: 0x0d
>> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>>
>> and other laptop give that to kernel, in recent kernel will not give
>> acpiphp to have that slot, because it want to hold that for pciehp.
>> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> --- please check the mail that i forward to you yesterday.
> 
> Yes, this is a bug, but I'm not sure how to fix it yet.

Looks like what I see with Dell Vostro 3550 as well.

> 
>> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> and it should be first come and first serve policy.
> 
> And that's why you think they should be modules?  I disagree if so.

For me it is easier to cold boot with a card plugged in and fiddle later with
hotplug if I want to unload the card. Until that, I can inspect wheteher PresDet
really reports the card is in, and the see if system reports same after loading
acpiphp or pciehp. I wouldn't drop the possibility to have them as modules, at
least for now when finally we have some clue what is going on and can load the
modules as we want while chasing the bugs.

But sorry for hijacking this thread, maybe I managed to delete your answers on my
thread ("Re: Dell Vostro 3550: pci_hotplug+acpiphp require 'pcie_aspm=force' on kernel command-line for hotplug to work").
Will go through web archives to make sure I did not miss something.

Martin

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Friday, January 11, 2013 12:07:43 PM Martin Mokrejs wrote:
> Hi,
>   I just hit this thread in my bloated Inbox.
> 
> Rafael J. Wysocki wrote:
> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> Well, I don't see what functional problems that can bring.
> >>>
> >>> In theory people may want to have them as modules to avoid loading them on
> >>> systems that don't use PCI hotplug, but honestly I think that the complexity
> >>> this causes us to deal with is not worth it.
> >>>
> >>> Moreover, removing the modularity may actually allow us to solve some ordering
> >>> issues once and for good.
> >>
> >> No, the world is not really ideal yet.
> >>
> >> looks like laptops have problem with pci express cards.
> >>
> >> when pciehp is used, surprise insert/removal does not work because
> >> PresDect does not change properly, so no interrupt is generated.
> >> --- i suspects that is silicon problem.
> 
> That's what seemed to be the conclusion half a year ago around 3.2.x/3.3.x
> for my issues as well (SandyBridge C6/C200 chipset).
> 
> >>
> >> but when acpiphp is used, that surprise  insert/removal is working.
> 
> That's what I discovered few days ago as well. However, there are still some
> differences between individual express cards and I just need to find some time to
> dig through the data I collected.
> 
> >>
> >> some laptop like thinkpad, just don't give osc to kernel..
> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> returned control mask: 0x0d
> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >>
> >> and other laptop give that to kernel, in recent kernel will not give
> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> --- please check the mail that i forward to you yesterday.
> > 
> > Yes, this is a bug, but I'm not sure how to fix it yet.
> 
> Looks like what I see with Dell Vostro 3550 as well.
> 
> > 
> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> and it should be first come and first serve policy.
> > 
> > And that's why you think they should be modules?  I disagree if so.
> 
> For me it is easier to cold boot with a card plugged in and fiddle later with
> hotplug if I want to unload the card. Until that, I can inspect wheteher PresDet
> really reports the card is in, and the see if system reports same after loading
> acpiphp or pciehp. I wouldn't drop the possibility to have them as modules, at
> least for now when finally we have some clue what is going on and can load the
> modules as we want while chasing the bugs.

The problem with modules is that the initialization ordering depends on the
order in which the modules are loaded.  If there are two modules, there are 2
different ways, if there are 3 modules, there are 6 ways and so on.  You get
the idea. :-)

Now, we need to take all of the possible orderings in the code and that's
quite complicated.  Also it is easy to leave one ordering untested if the
distro you use for testing happens to prefer a different one.  So the fact that
they are modules very well may be the _source_ of the problems you're seeing.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Bjorn Helgaas]

On Thu, Jan 10, 2013 at 4:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
>> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
>> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> > Well, I don't see what functional problems that can bring.
>> >> >
>> >> > In theory people may want to have them as modules to avoid loading them on
>> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
>> >> > this causes us to deal with is not worth it.
>> >> >
>> >> > Moreover, removing the modularity may actually allow us to solve some ordering
>> >> > issues once and for good.
>> >>
>> >> No, the world is not really ideal yet.
>> >>
>> >> looks like laptops have problem with pci express cards.
>> >>
>> >> when pciehp is used, surprise insert/removal does not work because
>> >> PresDect does not change properly, so no interrupt is generated.
>> >> --- i suspects that is silicon problem.
>> >>
>> >> but when acpiphp is used, that surprise  insert/removal is working.
>> >>
>> >> some laptop like thinkpad, just don't give osc to kernel..
>> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
>> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
>> >> returned control mask: 0x0d
>> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
>> >>
>> >> and other laptop give that to kernel, in recent kernel will not give
>> >> acpiphp to have that slot, because it want to hold that for pciehp.
>> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
>> >> --- please check the mail that i forward to you yesterday.
>> >
>> > Yes, this is a bug, but I'm not sure how to fix it yet.
>>
>> add one command line to control it so do not claim that in osc_control?
>
> That's one option, although not very attractive so to speak.
>
>> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
>> >> and it should be first come and first serve policy.
>> >
>> > And that's why you think they should be modules?  I disagree if so.
>>
>> Yes.
>>
>> maybe we can have pci=nopciehp in command line just we pci=noaer...
>>
>> that should handle some corner cases.
>
> Yes.  In any case the user should be able to say "I know better", but having
> to express that through the "right" ordering of modules is somewhat less than
> straightforward in my opinion.

I think it's fine if a user *can* override the default ordering.

I am completely opposed to *requiring* a user to supply a command line
option or change the order of module loading just to get hotplug to
work.  There's no sane way to document that or communicate that
information to users.

Bjorn

Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

[Rafael J. Wysocki]

On Friday, January 11, 2013 11:06:29 AM Bjorn Helgaas wrote:
> On Thu, Jan 10, 2013 at 4:59 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 10, 2013 03:40:45 PM Yinghai Lu wrote:
> >> On Thu, Jan 10, 2013 at 3:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Thursday, January 10, 2013 03:03:53 PM Yinghai Lu wrote:
> >> >> On Thu, Jan 10, 2013 at 1:50 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> > Well, I don't see what functional problems that can bring.
> >> >> >
> >> >> > In theory people may want to have them as modules to avoid loading them on
> >> >> > systems that don't use PCI hotplug, but honestly I think that the complexity
> >> >> > this causes us to deal with is not worth it.
> >> >> >
> >> >> > Moreover, removing the modularity may actually allow us to solve some ordering
> >> >> > issues once and for good.
> >> >>
> >> >> No, the world is not really ideal yet.
> >> >>
> >> >> looks like laptops have problem with pci express cards.
> >> >>
> >> >> when pciehp is used, surprise insert/removal does not work because
> >> >> PresDect does not change properly, so no interrupt is generated.
> >> >> --- i suspects that is silicon problem.
> >> >>
> >> >> but when acpiphp is used, that surprise  insert/removal is working.
> >> >>
> >> >> some laptop like thinkpad, just don't give osc to kernel..
> >> >> [    0.505117]  pci0000:00: Requesting ACPI _OSC control (0x1d)
> >> >> [    0.505413]  pci0000:00: ACPI _OSC request failed (AE_SUPPORT),
> >> >> returned control mask: 0x0d
> >> >> [    0.505517] ACPI _OSC control for PCIe not granted, disabling ASPM
> >> >>
> >> >> and other laptop give that to kernel, in recent kernel will not give
> >> >> acpiphp to have that slot, because it want to hold that for pciehp.
> >> >> poor user have to pass 'pci_aspm=off" to disable _OSC for all.
> >> >> --- please check the mail that i forward to you yesterday.
> >> >
> >> > Yes, this is a bug, but I'm not sure how to fix it yet.
> >>
> >> add one command line to control it so do not claim that in osc_control?
> >
> > That's one option, although not very attractive so to speak.
> >
> >> >> Anyway, we do need to let the user to have choice to use acpiphp and pciehp.
> >> >> and it should be first come and first serve policy.
> >> >
> >> > And that's why you think they should be modules?  I disagree if so.
> >>
> >> Yes.
> >>
> >> maybe we can have pci=nopciehp in command line just we pci=noaer...
> >>
> >> that should handle some corner cases.
> >
> > Yes.  In any case the user should be able to say "I know better", but having
> > to express that through the "right" ordering of modules is somewhat less than
> > straightforward in my opinion.
> 
> I think it's fine if a user *can* override the default ordering.
> 
> I am completely opposed to *requiring* a user to supply a command line
> option or change the order of module loading just to get hotplug to
> work.  There's no sane way to document that or communicate that
> information to users.

Well, I agree that neither of these things should be necessary, but that
means the current code is based on incorrect assumptions.

Perhaps our mistake is to believe that once we've done the _OSC handshake,
we are supposed not to use ACPI for hotplug events handling?  It looks like
the general expectation of BIOS writers is that we will use ACPI regardless
if available and we *may* use the native PCIe signaling in addition to it if
the _OSC handshake is successful.

So maybe there should be just one PCI hotplug driver that can use both
ACPI and PCIe native events?  [The same applies to PM, but the situation is
a bit simpler in there.]

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Jiang Liu]

On 01/10/2013 04:23 AM, Rafael J. Wysocki wrote:
> On Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote:
>> On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote:
>>> On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote:
>>>> Currently the acpiphp driver fails to update hotplug slot information under
>>>> several conditions, such as:
>>>> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove
>>>> 2) The bridge device is added/removed by PCI hotplug driver other than the
>>>>    acpiphp driver itself. For example, if an IO expansion box with ACPI
>>>>    hotplug slots available is hot-added by the pciehp driver, all ACPI
>>>>    hotplug slots won't be discovered by the acpiphp driver.
>>>>
>>>> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
>>>> update ACPI hotplug slot information when PCI hotplug event happens.
>>>>
>>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>>> ---
>>>>  drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 60 insertions(+), 2 deletions(-)
>>>>
>> snip
>>>> +static struct notifier_block acpi_pci_hp_notifier = {
>>>> +	.notifier_call = &acpi_pci_hp_notify_fn,
>>>> +};
>>>> +
>>>>  static struct acpi_pci_driver acpi_pci_hp_driver = {
>>>>  	.add =		add_bridge,
>>>>  	.remove =	remove_bridge,
>>>> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
>>>>  	else
>>>>  		acpi_pci_register_driver(&acpi_pci_hp_driver);
>>>>  
>>>> +	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
>>>
>>> Your previous patch adds a PCI bus notifier for a similar thing.  Why are
>>> you adding another one here?
>> Hi Rafael,
>> 	The acpiphp driver could be built as a module, and this bus notifier
>> is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge.
>> The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug
>> controller are related but different objects, so we use two notifiers to support
>> them all.
> 
> I would be much happier if those notifiers were not needed.  At least, I'm not
> sure why they need to be invoked by the driver core during device registration.
> 
> In my opinion it would be much better if they were called only for the devices
> that might need them instead of all PCI devices.
> 
Hi Rafael,
	The whole thing seems a little complex, and I will try my best to explain
about it:)
	Essentially a PCI slot is associated with a PCI bus, and a PCI bus is 
associated with a host bridge or P2P bridge. When hot-adding/hot-removing a PCI bus,
we need to create/destroy PCI slots associated with it. So the pci_slot and acpiphp
drivers need to be notified when hot-adding/hot-removing a PCI bus.
	But there's no common mechanism to get notified when PCI buses are being
hot-added/hot-removed. So my first implementation has introduced a dedicated notifier
chain for PCI bus addition/removal. Later with ongoing changes to the PCI core, 
I found we could rely on the existing bus notification mechanism too, so that becomes
the current implementation.
	I have a proposal to unify the process for boot time and hotplug as below:
1) Change pci_slot and acpiphp as built-in instead of modules.
2) Get rid of acpi_pci_driver for pci_slot and acpiphp driver.
3) Register some forms of callbacks to create/destroy hotplug controllers/slots
   when creating/destroying PCI buses.
3.a) Rely on the bus notification mechanism to invoke callbacks. This solution is
     simple, but with a little overhead
3.b) Introduce a dedicated notifier chain for PCI bus hot-adding/hot-removing.
     This solution could get rid of the overhead, but with more code changes.
3.c) Invoke callbacks from acpi_bus_type->setup/cleanup. This solution could
     only support ACPI based slots, such as pci_slot and acpiphp.
	Any recommendations?
Regards!
Gerry

> Thanks,
> Rafael
> 
> 

Re: [PATCH v3 4/6] PCI/acpiphp: update ACPI hotplug slot information when PCI hotplug happens

[Rafael J. Wysocki]

On Sunday, January 13, 2013 11:13:28 PM Jiang Liu wrote:
> On 01/10/2013 04:23 AM, Rafael J. Wysocki wrote:
> > On Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote:
> >> On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote:
> >>> On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote:
> >>>> Currently the acpiphp driver fails to update hotplug slot information under
> >>>> several conditions, such as:
> >>>> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove
> >>>> 2) The bridge device is added/removed by PCI hotplug driver other than the
> >>>>    acpiphp driver itself. For example, if an IO expansion box with ACPI
> >>>>    hotplug slots available is hot-added by the pciehp driver, all ACPI
> >>>>    hotplug slots won't be discovered by the acpiphp driver.
> >>>>
> >>>> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> >>>> update ACPI hotplug slot information when PCI hotplug event happens.
> >>>>
> >>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> >>>> ---
> >>>>  drivers/pci/hotplug/acpiphp_glue.c |   62 ++++++++++++++++++++++++++++++++++--
> >>>>  1 file changed, 60 insertions(+), 2 deletions(-)
> >>>>
> >> snip
> >>>> +static struct notifier_block acpi_pci_hp_notifier = {
> >>>> +	.notifier_call = &acpi_pci_hp_notify_fn,
> >>>> +};
> >>>> +
> >>>>  static struct acpi_pci_driver acpi_pci_hp_driver = {
> >>>>  	.add =		add_bridge,
> >>>>  	.remove =	remove_bridge,
> >>>> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void)
> >>>>  	else
> >>>>  		acpi_pci_register_driver(&acpi_pci_hp_driver);
> >>>>  
> >>>> +	bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier);
> >>>
> >>> Your previous patch adds a PCI bus notifier for a similar thing.  Why are
> >>> you adding another one here?
> >> Hi Rafael,
> >> 	The acpiphp driver could be built as a module, and this bus notifier
> >> is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge.
> >> The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug
> >> controller are related but different objects, so we use two notifiers to support
> >> them all.
> > 
> > I would be much happier if those notifiers were not needed.  At least, I'm not
> > sure why they need to be invoked by the driver core during device registration.
> > 
> > In my opinion it would be much better if they were called only for the devices
> > that might need them instead of all PCI devices.
> > 
> Hi Rafael,
> 	The whole thing seems a little complex, and I will try my best to explain
> about it:)
> 	Essentially a PCI slot is associated with a PCI bus, and a PCI bus is 
> associated with a host bridge or P2P bridge. When hot-adding/hot-removing a PCI bus,
> we need to create/destroy PCI slots associated with it. So the pci_slot and acpiphp
> drivers need to be notified when hot-adding/hot-removing a PCI bus.
> 	But there's no common mechanism to get notified when PCI buses are being
> hot-added/hot-removed. So my first implementation has introduced a dedicated notifier
> chain for PCI bus addition/removal. Later with ongoing changes to the PCI core, 
> I found we could rely on the existing bus notification mechanism too, so that becomes
> the current implementation.
> 	I have a proposal to unify the process for boot time and hotplug as below:
> 1) Change pci_slot and acpiphp as built-in instead of modules.

Sure, go ahead with that.

> 2) Get rid of acpi_pci_driver for pci_slot and acpiphp driver.

Again, I have no objection here.  Moreover, I think we might be better off
without the acpi_pci_driver things at all.

> 3) Register some forms of callbacks to create/destroy hotplug controllers/slots
>    when creating/destroying PCI buses.
> 3.a) Rely on the bus notification mechanism to invoke callbacks. This solution is
>      simple, but with a little overhead
> 3.b) Introduce a dedicated notifier chain for PCI bus hot-adding/hot-removing.
>      This solution could get rid of the overhead, but with more code changes.
> 3.c) Invoke callbacks from acpi_bus_type->setup/cleanup. This solution could
>      only support ACPI based slots, such as pci_slot and acpiphp.

As I said before, I'd prefer these notifications to be done specifically for
PCI buses or bridges, so I'd vote for 3.b), depending on the implementation.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.