[PATCH v2 0/6] acpi,pci: hostbridge hotplug support

[PATCH v2 0/6] acpi,pci: hostbridge hotplug support

[Taku Izumi]

Hi all,
 
  This patchset is 2nd version of the following:
    http://marc.info/?l=linux-pci&m=134457928913775&w=2

 v1 -> v2:
   - rebased against current "next" branch
   - omit a few patches
   - split global list protection patch into 2 patches (NO3,4)

  * [PATCH v2 1/6] ACPI, PCI: Use normal list for struct acpi_pci_driver
  * [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges
  * [PATCH v2 3/6] ACPI, PCI: add acpi_pci_drivers protection
  * [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection
  * [PATCH v2 5/6] ACPI, PCI: add hostbridge removal function
  * [PATCH v2 6/6] ACPI, PCI: add resoruce-assign code for devices under hot-added hostbridge

-- 
Best regards,
Taku Izumi <izumi.taku@jp.fujitsu.com>

[PATCH v2 1/6] ACPI, PCI: Use normal list for struct acpi_pci_driver

[Taku Izumi]

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

  ACPI, PCI: Use normal list for struct acpi_pci_driver
    
  Use normal list for struct acpi_pci_driver to simplify code.
    
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   16 +++-------------
 include/linux/acpi.h    |    2 +-
 2 files changed, 4 insertions(+), 14 deletions(-)

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -72,8 +72,8 @@ static struct acpi_driver acpi_pci_root_
 };
 
 static LIST_HEAD(acpi_pci_roots);
+static LIST_HEAD(acpi_pci_drivers);
 
-static struct acpi_pci_driver *sub_driver;
 static DEFINE_MUTEX(osc_lock);
 
 int acpi_pci_register_driver(struct acpi_pci_driver *driver)
@@ -81,10 +81,7 @@ int acpi_pci_register_driver(struct acpi
 	int n = 0;
 	struct acpi_pci_root *root;
 
-	struct acpi_pci_driver **pptr = &sub_driver;
-	while (*pptr)
-		pptr = &(*pptr)->next;
-	*pptr = driver;
+	list_add_tail(&driver->node, &acpi_pci_drivers);
 
 	if (!driver->add)
 		return 0;
@@ -103,14 +100,7 @@ void acpi_pci_unregister_driver(struct a
 {
 	struct acpi_pci_root *root;
 
-	struct acpi_pci_driver **pptr = &sub_driver;
-	while (*pptr) {
-		if (*pptr == driver)
-			break;
-		pptr = &(*pptr)->next;
-	}
-	BUG_ON(!*pptr);
-	*pptr = (*pptr)->next;
+	list_del(&driver->node);
 
 	if (!driver->remove)
 		return;
Index: Bjorn-next-0903/include/linux/acpi.h
===================================================================
--- Bjorn-next-0903.orig/include/linux/acpi.h
+++ Bjorn-next-0903/include/linux/acpi.h
@@ -138,7 +138,7 @@ void acpi_penalize_isa_irq(int irq, int 
 void acpi_pci_irq_disable (struct pci_dev *dev);
 
 struct acpi_pci_driver {
-	struct acpi_pci_driver *next;
+	struct list_head node;
 	int (*add)(acpi_handle handle);
 	void (*remove)(acpi_handle handle);
 };

[PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Taku Izumi]

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

  ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges
    
  When hot-plugging PCI root bridge, acpi_pci_drivers' add()/remove()
  methods should be invoked to notify registered drivers.
    
  -v2: Move add calling to acpi_pci_root_start by Yinghai
    
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: Bjorn-next-0808/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0808.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0808/drivers/acpi/pci_root.c
@@ -626,14 +626,25 @@ end:
 static int acpi_pci_root_start(struct acpi_device *device)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
+	struct acpi_pci_driver *driver;
+
+	list_for_each_entry(driver, &acpi_pci_drivers, node)
+		if (driver->add)
+			driver->add(device->handle);
 
 	pci_bus_add_devices(root->bus);
+
 	return 0;
 }
 
 static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
+	struct acpi_pci_driver *driver;
+
+	list_for_each_entry(driver, &acpi_pci_drivers, node)
+		if (driver->remove)
+			driver->remove(root->device->handle);
 
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);

RE: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Kaneshige, Kenji]

> -----Original Message-----
> From: Taku Izumi [mailto:izumi.taku@jp.fujitsu.com]
> Sent: Monday, September 03, 2012 5:05 PM
> To: Izumi, Taku/泉 拓
> Cc: linux-pci@vger.kernel.org; bhelgaas@google.com;
> linux-acpi@vger.kernel.org; Kaneshige, Kenji/金重 憲治;
> yinghai@kernel.org; jiang.liu@huawei.com
> Subject: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when
> hot-plugging PCI root bridges
> 
> From: Jiang Liu <jiang.liu@huawei.com>
> 
>   ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges
> 
>   When hot-plugging PCI root bridge, acpi_pci_drivers' add()/remove()
>   methods should be invoked to notify registered drivers.
> 
>   -v2: Move add calling to acpi_pci_root_start by Yinghai
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_root.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Index: Bjorn-next-0808/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0808.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0808/drivers/acpi/pci_root.c
> @@ -626,14 +626,25 @@ end:
>  static int acpi_pci_root_start(struct acpi_device *device)
>  {
>  	struct acpi_pci_root *root = acpi_driver_data(device);
> +	struct acpi_pci_driver *driver;
> +
> +	list_for_each_entry(driver, &acpi_pci_drivers, node)
> +		if (driver->add)
> +			driver->add(device->handle);
> 

I think this (invoke .add callback for each acpi pci driver) should be done
after pci_bus_add_devices().

It seems ACPI pci drivers run *after* pci_bus_add_devices() at the boot time
(because no acpi pci drivers is loaded at that time). On the other hand, it
seems that ACPI pci drivers run *before* pci_bus_add_devices() at the hotadd
time. Those should be the same.

Regards,
Kenji Kaneshige

>  	pci_bus_add_devices(root->bus);
> +

Re: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Yinghai Lu]

On Tue, Sep 4, 2012 at 12:58 AM, Kaneshige, Kenji
<kaneshige.kenji@jp.fujitsu.com> wrote:
>>   -v2: Move add calling to acpi_pci_root_start by Yinghai
>>  static int acpi_pci_root_start(struct acpi_device *device)
>>  {
>>       struct acpi_pci_root *root = acpi_driver_data(device);
>> +     struct acpi_pci_driver *driver;
>> +
>> +     list_for_each_entry(driver, &acpi_pci_drivers, node)
>> +             if (driver->add)
>> +                     driver->add(device->handle);
>>
>
> I think this (invoke .add callback for each acpi pci driver) should be done
> after pci_bus_add_devices().

We need to load ioapic driver and dmar driver before normal pci drivers.

>
> It seems ACPI pci drivers run *after* pci_bus_add_devices() at the boot time
> (because no acpi pci drivers is loaded at that time). On the other hand, it
> seems that ACPI pci drivers run *before* pci_bus_add_devices() at the hotadd
> time. Those should be the same.

for boot time, sequence : normal pci driver still get loaded after
ioapic and dmar drivers.
otherwise those driver will have problem to get irq and iommu work correctly.
and if ioapic and dmar are static, and they will get initialized
around pci device get scanned and pci_bus_add_devices get called
already.
but that time normal devices driver does not get registered yet. those
pci drivers will get loaded (should be bounded)
for pci devices when those driver get registered.

Thanks

Yinghai

RE: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Kaneshige, Kenji]

> -----Original Message-----
> From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
> Yinghai Lu
> Sent: Wednesday, September 05, 2012 4:12 AM
> To: Kaneshige, Kenji/金重 憲治
> Cc: Izumi, Taku/泉 拓; linux-pci@vger.kernel.org; bhelgaas@google.com;
> linux-acpi@vger.kernel.org; jiang.liu@huawei.com
> Subject: Re: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when
> hot-plugging PCI root bridges
> 
> On Tue, Sep 4, 2012 at 12:58 AM, Kaneshige, Kenji
> <kaneshige.kenji@jp.fujitsu.com> wrote:
> >>   -v2: Move add calling to acpi_pci_root_start by Yinghai
> >>  static int acpi_pci_root_start(struct acpi_device *device)
> >>  {
> >>       struct acpi_pci_root *root = acpi_driver_data(device);
> >> +     struct acpi_pci_driver *driver;
> >> +
> >> +     list_for_each_entry(driver, &acpi_pci_drivers, node)
> >> +             if (driver->add)
> >> +                     driver->add(device->handle);
> >>
> >
> > I think this (invoke .add callback for each acpi pci driver) should be
> done
> > after pci_bus_add_devices().
> 
> We need to load ioapic driver and dmar driver before normal pci drivers.

Ok, my understanding of your comment is that IOAPIC and DMAR drivers are implemented
as ACPI PCI driver (or you have a plan to implement IOAPIC and DMAR drivers as ACPI
PCI driver), and those drivers need to be loaded before PCI device drivers. Is my
understanding correct?

Regards,
Kenji Kaneshige



> 
> >
> > It seems ACPI pci drivers run *after* pci_bus_add_devices() at the boot
> time
> > (because no acpi pci drivers is loaded at that time). On the other hand,
> it
> > seems that ACPI pci drivers run *before* pci_bus_add_devices() at the
> hotadd
> > time. Those should be the same.
> 
> for boot time, sequence : normal pci driver still get loaded after
> ioapic and dmar drivers.
> otherwise those driver will have problem to get irq and iommu work correctly.
> and if ioapic and dmar are static, and they will get initialized
> around pci device get scanned and pci_bus_add_devices get called
> already.
> but that time normal devices driver does not get registered yet. those
> pci drivers will get loaded (should be bounded)
> for pci devices when those driver get registered.
> 
> Thanks
> 
> Yinghai

Re: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Yinghai Lu]

On Tue, Sep 4, 2012 at 9:32 PM, Kaneshige, Kenji
<kaneshige.kenji@jp.fujitsu.com> wrote:
>
> Ok, my understanding of your comment is that IOAPIC and DMAR drivers are implemented
> as ACPI PCI driver (or you have a plan to implement IOAPIC and DMAR drivers as ACPI
> PCI driver), and those drivers need to be loaded before PCI device drivers. Is my
> understanding correct?

yes.

for ioapic:
http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-x86-irq

for iommu:
http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-iommu

Thanks

Yinghai

RE: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when hot-plugging PCI root bridges

[Kaneshige, Kenji]

> -----Original Message-----
> From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
> Yinghai Lu
> Sent: Wednesday, September 05, 2012 2:02 PM
> To: Kaneshige, Kenji/金重 憲治
> Cc: Izumi, Taku/泉 拓; linux-pci@vger.kernel.org; bhelgaas@google.com;
> linux-acpi@vger.kernel.org; jiang.liu@huawei.com
> Subject: Re: [PATCH v2 2/6] ACPI, PCI: Notify acpi_pci_drivers when
> hot-plugging PCI root bridges
> 
> On Tue, Sep 4, 2012 at 9:32 PM, Kaneshige, Kenji
> <kaneshige.kenji@jp.fujitsu.com> wrote:
> >
> > Ok, my understanding of your comment is that IOAPIC and DMAR drivers are
> implemented
> > as ACPI PCI driver (or you have a plan to implement IOAPIC and DMAR drivers
> as ACPI
> > PCI driver), and those drivers need to be loaded before PCI device drivers.
> Is my
> > understanding correct?
> 
> yes.
> 
> for ioapic:
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=
> shortlog;h=refs/heads/for-x86-irq
> 
> for iommu:
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=
> shortlog;h=refs/heads/for-iommu
> 

Thank you for answering.
Though I've not looked at those trees yet, the idea, which utilize ACPI pci
driver to load ioapic and dmar driver, looks good to me.

Regards,
Kenji Kaneshige

[PATCH v2 3/6] ACPI, PCI: add acpi_pci_drivers protection

[Taku Izumi]

Use mutex to protect global acpi_pci_drivers list against PCI
host bridge hotplug operations.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -27,7 +27,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/types.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci.h>
@@ -71,6 +71,8 @@ static struct acpi_driver acpi_pci_root_
 		},
 };
 
+/* Lock to protect both acpi_pci_roots and acpi_pci_drivers lists */
+static DEFINE_MUTEX(acpi_pci_root_lock);
 static LIST_HEAD(acpi_pci_roots);
 static LIST_HEAD(acpi_pci_drivers);
 
@@ -81,34 +83,30 @@ int acpi_pci_register_driver(struct acpi
 	int n = 0;
 	struct acpi_pci_root *root;
 
+	mutex_lock(&acpi_pci_root_lock);
 	list_add_tail(&driver->node, &acpi_pci_drivers);
-
-	if (!driver->add)
-		return 0;
-
-	list_for_each_entry(root, &acpi_pci_roots, node) {
-		driver->add(root->device->handle);
-		n++;
-	}
+	if (driver->add)
+		list_for_each_entry(root, &acpi_pci_roots, node) {
+			driver->add(root->device->handle);
+			n++;
+		}
+	mutex_unlock(&acpi_pci_root_lock);
 
 	return n;
 }
-
 EXPORT_SYMBOL(acpi_pci_register_driver);
 
 void acpi_pci_unregister_driver(struct acpi_pci_driver *driver)
 {
 	struct acpi_pci_root *root;
 
+	mutex_lock(&acpi_pci_root_lock);
 	list_del(&driver->node);
-
-	if (!driver->remove)
-		return;
-
-	list_for_each_entry(root, &acpi_pci_roots, node)
-		driver->remove(root->device->handle);
+	if (driver->remove)
+		list_for_each_entry(root, &acpi_pci_roots, node)
+			driver->remove(root->device->handle);
+	mutex_unlock(&acpi_pci_root_lock);
 }
-
 EXPORT_SYMBOL(acpi_pci_unregister_driver);
 
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
@@ -628,9 +626,11 @@ static int acpi_pci_root_start(struct ac
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
 
+	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->add)
 			driver->add(device->handle);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	pci_bus_add_devices(root->bus);
 
@@ -642,9 +642,11 @@ static int acpi_pci_root_remove(struct a
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
 
+	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root->device->handle);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);

[PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Taku Izumi]

Use mutex and RCU to protect global acpi_pci_roots list against
PCI host bridge hotplug operations.

RCU is used to avoid possible deadlock in function acpi_pci_find_root()
and acpi_get_pci_rootbridge_handle(). A possible call graph:
acpi_pci_register_driver()
	mutex_lock(&acpi_pci_root_lock)
		driver->add(root)
			......
				acpi_pci_find_root()

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -28,6 +28,7 @@
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/rculist.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci.h>
@@ -86,7 +87,7 @@ int acpi_pci_register_driver(struct acpi
 	mutex_lock(&acpi_pci_root_lock);
 	list_add_tail(&driver->node, &acpi_pci_drivers);
 	if (driver->add)
-		list_for_each_entry(root, &acpi_pci_roots, node) {
+		list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
 			driver->add(root->device->handle);
 			n++;
 		}
@@ -103,7 +104,7 @@ void acpi_pci_unregister_driver(struct a
 	mutex_lock(&acpi_pci_root_lock);
 	list_del(&driver->node);
 	if (driver->remove)
-		list_for_each_entry(root, &acpi_pci_roots, node)
+		list_for_each_entry_rcu(root, &acpi_pci_roots, node)
 			driver->remove(root->device->handle);
 	mutex_unlock(&acpi_pci_root_lock);
 }
@@ -112,14 +113,19 @@ EXPORT_SYMBOL(acpi_pci_unregister_driver
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
 {
 	struct acpi_pci_root *root;
+	struct acpi_handle *handle = NULL;
 	
-	list_for_each_entry(root, &acpi_pci_roots, node)
+	rcu_read_lock();
+	list_for_each_entry_rcu(root, &acpi_pci_roots, node)
 		if ((root->segment == (u16) seg) &&
-		    (root->secondary.start == (u16) bus))
-			return root->device->handle;
-	return NULL;		
-}
+		    (root->secondary.start == (u16) bus)) {
+			handle = root->device->handle;
+			break;
+		}
+	rcu_read_unlock();
 
+	return handle;
+}
 EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
 
 /**
@@ -266,10 +272,14 @@ struct acpi_pci_root *acpi_pci_find_root
 {
 	struct acpi_pci_root *root;
 
-	list_for_each_entry(root, &acpi_pci_roots, node) {
-		if (root->device->handle == handle)
+	rcu_read_lock();
+	list_for_each_entry_rcu(root, &acpi_pci_roots, node)
+		if (root->device->handle == handle) {
+			rcu_read_unlock();
 			return root;
-	}
+		}
+	rcu_read_unlock();
+
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(acpi_pci_find_root);
@@ -506,8 +516,9 @@ static int __devinit acpi_pci_root_add(s
 	 * TBD: Need PCI interface for enumeration/configuration of roots.
 	 */
 
-	/* TBD: Locking */
-	list_add_tail(&root->node, &acpi_pci_roots);
+	mutex_lock(&acpi_pci_root_lock);
+	list_add_tail_rcu(&root->node, &acpi_pci_roots);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
@@ -526,7 +537,7 @@ static int __devinit acpi_pci_root_add(s
 			    "Bus %04x:%02x not present in PCI namespace\n",
 			    root->segment, (unsigned int)root->secondary.start);
 		result = -ENODEV;
-		goto end;
+		goto out_del_root;
 	}
 
 	/*
@@ -536,7 +547,7 @@ static int __devinit acpi_pci_root_add(s
 	 */
 	result = acpi_pci_bind_root(device);
 	if (result)
-		goto end;
+		goto out_del_root;
 
 	/*
 	 * PCI Routing Table
@@ -614,9 +625,12 @@ static int __devinit acpi_pci_root_add(s
 
 	return 0;
 
+out_del_root:
+	mutex_lock(&acpi_pci_root_lock);
+	list_del_rcu(&root->node);
+	mutex_unlock(&acpi_pci_root_lock);
+	synchronize_rcu();
 end:
-	if (!list_empty(&root->node))
-		list_del(&root->node);
 	kfree(root);
 	return result;
 }
@@ -646,11 +660,13 @@ static int acpi_pci_root_remove(struct a
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root->device->handle);
-	mutex_unlock(&acpi_pci_root_lock);
 
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	list_del_rcu(&root->node);
+	mutex_unlock(&acpi_pci_root_lock);
+	synchronize_rcu();
 	kfree(root);
 	return 0;
 }

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Bjorn Helgaas]

On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> Use mutex and RCU to protect global acpi_pci_roots list against
> PCI host bridge hotplug operations.
>
> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
> and acpi_get_pci_rootbridge_handle(). A possible call graph:
> acpi_pci_register_driver()
>         mutex_lock(&acpi_pci_root_lock)
>                 driver->add(root)
>                         ......
>                                 acpi_pci_find_root()

Where does this path occur?  I didn't see in in the current tree
(where the only users of acpi_pci_register_driver() are for
acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
work, which adds more acpi_pci_register_driver() users.

RCU seems unnecessarily complicated for this list, but I haven't gone
through Yinghai's work yet, so I don't know what it requires.

In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
struct acpi_pci_root, which has all sorts of information that would be
useful to the .add() and .remove() methods of sub-drivers.  It seems
sort of stupid that we only pass the acpi_handle to the sub-drivers,
forcing them to use hacks like acpi_pci_find_root() to look up the
information we just threw away.  Can we just fix the .add() and
.remove() interfaces to pass something more useful so we avoid the
need for this deadlock path?

> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
>
> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> @@ -28,6 +28,7 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/rculist.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci.h>
> @@ -86,7 +87,7 @@ int acpi_pci_register_driver(struct acpi
>         mutex_lock(&acpi_pci_root_lock);
>         list_add_tail(&driver->node, &acpi_pci_drivers);
>         if (driver->add)
> -               list_for_each_entry(root, &acpi_pci_roots, node) {
> +               list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
>                         driver->add(root->device->handle);
>                         n++;
>                 }
> @@ -103,7 +104,7 @@ void acpi_pci_unregister_driver(struct a
>         mutex_lock(&acpi_pci_root_lock);
>         list_del(&driver->node);
>         if (driver->remove)
> -               list_for_each_entry(root, &acpi_pci_roots, node)
> +               list_for_each_entry_rcu(root, &acpi_pci_roots, node)
>                         driver->remove(root->device->handle);
>         mutex_unlock(&acpi_pci_root_lock);
>  }
> @@ -112,14 +113,19 @@ EXPORT_SYMBOL(acpi_pci_unregister_driver
>  acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
>  {
>         struct acpi_pci_root *root;
> +       struct acpi_handle *handle = NULL;
>
> -       list_for_each_entry(root, &acpi_pci_roots, node)
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
>                 if ((root->segment == (u16) seg) &&
> -                   (root->secondary.start == (u16) bus))
> -                       return root->device->handle;
> -       return NULL;
> -}
> +                   (root->secondary.start == (u16) bus)) {
> +                       handle = root->device->handle;
> +                       break;
> +               }
> +       rcu_read_unlock();
>
> +       return handle;
> +}
>  EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
>
>  /**
> @@ -266,10 +272,14 @@ struct acpi_pci_root *acpi_pci_find_root
>  {
>         struct acpi_pci_root *root;
>
> -       list_for_each_entry(root, &acpi_pci_roots, node) {
> -               if (root->device->handle == handle)
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> +               if (root->device->handle == handle) {
> +                       rcu_read_unlock();
>                         return root;
> -       }
> +               }
> +       rcu_read_unlock();
> +
>         return NULL;
>  }
>  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
> @@ -506,8 +516,9 @@ static int __devinit acpi_pci_root_add(s
>          * TBD: Need PCI interface for enumeration/configuration of roots.
>          */
>
> -       /* TBD: Locking */
> -       list_add_tail(&root->node, &acpi_pci_roots);
> +       mutex_lock(&acpi_pci_root_lock);
> +       list_add_tail_rcu(&root->node, &acpi_pci_roots);
> +       mutex_unlock(&acpi_pci_root_lock);
>
>         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
>                acpi_device_name(device), acpi_device_bid(device),
> @@ -526,7 +537,7 @@ static int __devinit acpi_pci_root_add(s
>                             "Bus %04x:%02x not present in PCI namespace\n",
>                             root->segment, (unsigned int)root->secondary.start);
>                 result = -ENODEV;
> -               goto end;
> +               goto out_del_root;
>         }
>
>         /*
> @@ -536,7 +547,7 @@ static int __devinit acpi_pci_root_add(s
>          */
>         result = acpi_pci_bind_root(device);
>         if (result)
> -               goto end;
> +               goto out_del_root;
>
>         /*
>          * PCI Routing Table
> @@ -614,9 +625,12 @@ static int __devinit acpi_pci_root_add(s
>
>         return 0;
>
> +out_del_root:
> +       mutex_lock(&acpi_pci_root_lock);
> +       list_del_rcu(&root->node);
> +       mutex_unlock(&acpi_pci_root_lock);
> +       synchronize_rcu();
>  end:
> -       if (!list_empty(&root->node))
> -               list_del(&root->node);
>         kfree(root);
>         return result;
>  }
> @@ -646,11 +660,13 @@ static int acpi_pci_root_remove(struct a
>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>                 if (driver->remove)
>                         driver->remove(root->device->handle);
> -       mutex_unlock(&acpi_pci_root_lock);
>
>         device_set_run_wake(root->bus->bridge, false);
>         pci_acpi_remove_bus_pm_notifier(device);
>
> +       list_del_rcu(&root->node);
> +       mutex_unlock(&acpi_pci_root_lock);
> +       synchronize_rcu();
>         kfree(root);
>         return 0;
>  }
>

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Yinghai Lu]

On Wed, Sep 12, 2012 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> Use mutex and RCU to protect global acpi_pci_roots list against
>> PCI host bridge hotplug operations.
>>
>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>> acpi_pci_register_driver()
>>         mutex_lock(&acpi_pci_root_lock)
>>                 driver->add(root)
>>                         ......
>>                                 acpi_pci_find_root()
>
> Where does this path occur?  I didn't see in in the current tree
> (where the only users of acpi_pci_register_driver() are for
> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
> work, which adds more acpi_pci_register_driver() users.
>
> RCU seems unnecessarily complicated for this list, but I haven't gone
> through Yinghai's work yet, so I don't know what it requires.
>
> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
> struct acpi_pci_root, which has all sorts of information that would be
> useful to the .add() and .remove() methods of sub-drivers.  It seems
> sort of stupid that we only pass the acpi_handle to the sub-drivers,
> forcing them to use hacks like acpi_pci_find_root() to look up the
> information we just threw away.  Can we just fix the .add() and
> .remove() interfaces to pass something more useful so we avoid the
> need for this deadlock path?

new added acpi_pci_driver for ioapic, and iommu does not call
acpi_pci_find_root().

after split out pci_root_hp.c from acpiphp_glue.c, there will be
acpi_root_configure_bridge in pci_root_hp.c. that one could be
converted to
passing device instead of handle.

Thanks

Yinghai

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Bjorn Helgaas]

On Thu, Sep 13, 2012 at 1:09 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Sep 12, 2012 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>>> Use mutex and RCU to protect global acpi_pci_roots list against
>>> PCI host bridge hotplug operations.
>>>
>>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>>> acpi_pci_register_driver()
>>>         mutex_lock(&acpi_pci_root_lock)
>>>                 driver->add(root)
>>>                         ......
>>>                                 acpi_pci_find_root()
>>
>> Where does this path occur?  I didn't see in in the current tree
>> (where the only users of acpi_pci_register_driver() are for
>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>> work, which adds more acpi_pci_register_driver() users.
>>
>> RCU seems unnecessarily complicated for this list, but I haven't gone
>> through Yinghai's work yet, so I don't know what it requires.
>>
>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>> struct acpi_pci_root, which has all sorts of information that would be
>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>> forcing them to use hacks like acpi_pci_find_root() to look up the
>> information we just threw away.  Can we just fix the .add() and
>> .remove() interfaces to pass something more useful so we avoid the
>> need for this deadlock path?
>
> new added acpi_pci_driver for ioapic, and iommu does not call
> acpi_pci_find_root().
>
> after split out pci_root_hp.c from acpiphp_glue.c, there will be
> acpi_root_configure_bridge in pci_root_hp.c. that one could be
> converted to
> passing device instead of handle.

If I understand you correctly, you're agreeing that if we change the
.add()/.remove() interfaces, there is no need for RCU here.  Correct
me if I'm wrong.

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Yinghai Lu]

On Thu, Sep 13, 2012 at 12:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> If I understand you correctly, you're agreeing that if we change the
> .add()/.remove() interfaces, there is no need for RCU here.  Correct
> me if I'm wrong.

yes.

just passing acpi_pci_root point or acpi_device pointer.

Yinghai

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Thu, Sep 13, 2012 at 2:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 13, 2012 at 12:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> If I understand you correctly, you're agreeing that if we change the
>> .add()/.remove() interfaces, there is no need for RCU here.  Correct
>> me if I'm wrong.
>
> yes.
>
> just passing acpi_pci_root point or acpi_device pointer.

related change.
1. update pci_root_hp split patch to *p2p* for some function, so will
make add_bridge to take acpi_pci_root point.
2. remove one find_acpi_pci_root calling.
3. update the interface...

now in acpiphp_glue.c::register_slot still have one calling left....
        pdev = pbus->self;
        if (pdev && pci_is_pcie(pdev)) {
                tmp = acpi_find_root_bridge_handle(pdev);
                if (tmp) {
                        struct acpi_pci_root *root = acpi_pci_find_root(tmp);

                        if (root && (root->osc_control_set &
                                        OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)) {
                                dev_printk(KERN_DEBUG, &pdev->dev, "is
already used by pciehp\n");
                                return AE_OK;
                        }
                }
        }

maybe could put acpi_pci_root *root in into every acpiphp_bridge. ?

Thanks

Yinghai

[-- Attachment #2: pci_root_acpiphp_split.patch --]
[-- Type: application/octet-stream, Size: 11201 bytes --]

Subject: [PATCH] PCI, acpiphp: Separate out hot-add support of pci host bridge

It causes confusion.

We may only need acpi hp for pci host bridge.

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

Also remove not used res_lock in the struct.

-v2: put back pci_root_hp change in one patch
-v3: add pcibios_resource_survey_bus() calling
-v4: remove not needed code with remove_bridge
-v5: put back support for acpiphp support for slots just on root bus.
-v6: change some functions to *_p2p_* to make it more clean.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/Makefile              |    1 
 drivers/acpi/pci_root_hp.c         |  238 +++++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/acpiphp_glue.c |   59 ++-------
 3 files changed, 254 insertions(+), 44 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
@@ -521,10 +521,13 @@ static void cleanup_bridge(struct acpiph
 	acpi_status status;
 	acpi_handle handle = bridge->handle;
 
-	status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+	if (bridge->type != BRIDGE_TYPE_HOST) {
+		status = acpi_remove_notify_handler(handle,
+					    ACPI_SYSTEM_NOTIFY,
 					    handle_hotplug_event_bridge);
-	if (ACPI_FAILURE(status))
-		err("failed to remove notify handler\n");
+		if (ACPI_FAILURE(status))
+			err("failed to remove notify handler\n");
+	}
 
 	if ((bridge->type != BRIDGE_TYPE_HOST) &&
 	    ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)) {
@@ -607,9 +610,6 @@ static void remove_bridge(acpi_handle ha
 	bridge = acpiphp_handle_to_bridge(handle);
 	if (bridge)
 		cleanup_bridge(bridge);
-	else
-		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-					   handle_hotplug_event_bridge);
 }
 
 static int power_on_slot(struct acpiphp_slot *slot)
@@ -1110,18 +1110,12 @@ static void acpiphp_sanitize_bus(struct
 }
 
 /* Program resources in newly inserted bridge */
-static int acpiphp_configure_bridge (acpi_handle handle)
+static int acpiphp_configure_p2p_bridge(acpi_handle handle)
 {
-	struct pci_bus *bus;
+	struct pci_dev *pdev = acpi_get_pci_dev(handle);
+	struct pci_bus *bus = pdev->subordinate;
 
-	if (acpi_is_root_bridge(handle)) {
-		struct acpi_pci_root *root = acpi_pci_find_root(handle);
-		bus = root->bus;
-	} else {
-		struct pci_dev *pdev = acpi_get_pci_dev(handle);
-		bus = pdev->subordinate;
-		pci_dev_put(pdev);
-	}
+	pci_dev_put(pdev);
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);
@@ -1131,7 +1125,7 @@ static int acpiphp_configure_bridge (acp
 	return 0;
 }
 
-static void handle_bridge_insertion(acpi_handle handle, u32 type)
+static void handle_p2p_bridge_insertion(acpi_handle handle, u32 type)
 {
 	struct acpi_device *device, *pdevice;
 	acpi_handle phandle;
@@ -1151,9 +1145,9 @@ static void handle_bridge_insertion(acpi
 		err("cannot add bridge to acpi list\n");
 		return;
 	}
-	if (!acpiphp_configure_bridge(handle) &&
+	if (!acpiphp_configure_p2p_bridge(handle) &&
 		!acpi_bus_start(device))
-		add_bridge(handle);
+		add_p2p_bridge(handle);
 	else
 		err("cannot configure and start bridge\n");
 
@@ -1239,7 +1233,7 @@ static void _handle_hotplug_event_bridge
 
 	if (acpi_bus_get_device(handle, &device)) {
 		/* This bridge must have just been physically inserted */
-		handle_bridge_insertion(handle, type);
+		handle_p2p_bridge_insertion(handle, type);
 		goto out;
 	}
 
@@ -1416,21 +1410,6 @@ static void handle_hotplug_event_func(ac
 			      _handle_hotplug_event_func);
 }
 
-static acpi_status
-find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
-{
-	int *count = (int *)context;
-
-	if (!acpi_is_root_bridge(handle))
-		return AE_OK;
-
-	(*count)++;
-	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-				    handle_hotplug_event_bridge, NULL);
-
-	return AE_OK ;
-}
-
 static struct acpi_pci_driver acpi_pci_hp_driver = {
 	.add =		add_bridge,
 	.remove =	remove_bridge,
@@ -1441,15 +1420,7 @@ static struct acpi_pci_driver acpi_pci_h
  */
 int __init acpiphp_glue_init(void)
 {
-	int num = 0;
-
-	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
-			ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
-
-	if (num <= 0)
-		return -1;
-	else
-		acpi_pci_register_driver(&acpi_pci_hp_driver);
+	acpi_pci_register_driver(&acpi_pci_hp_driver);
 
 	return 0;
 }
Index: linux-2.6/drivers/acpi/pci_root_hp.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/acpi/pci_root_hp.c
@@ -0,0 +1,238 @@
+/*
+ * Separated from drivers/pci/hotplug/acpiphp_glue.c
+ *	only support root bridge
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/acpi.h>
+
+static LIST_HEAD(acpi_root_bridge_list);
+struct acpi_root_bridge {
+	struct list_head list;
+	acpi_handle handle;
+	u32 flags;
+};
+
+/* bridge flags */
+#define ROOT_BRIDGE_HAS_EJ0	(0x00000002)
+#define ROOT_BRIDGE_HAS_PS3	(0x00000080)
+
+#define ACPI_STA_FUNCTIONING	(0x00000008)
+
+static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
+{
+	struct acpi_root_bridge *bridge;
+
+	list_for_each_entry(bridge, &acpi_root_bridge_list, list)
+		if (bridge->handle == handle)
+			return bridge;
+
+	return NULL;
+}
+
+/* allocate and initialize host bridge data structure */
+static void add_acpi_root_bridge(acpi_handle handle)
+{
+	struct acpi_root_bridge *bridge;
+	acpi_handle dummy_handle;
+	acpi_status status;
+
+	/* if the bridge doesn't have _STA, we assume it is always there */
+	status = acpi_get_handle(handle, "_STA", &dummy_handle);
+	if (ACPI_SUCCESS(status)) {
+		unsigned long long tmp;
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_DEBUG "%s: _STA evaluation failure\n",
+						 __func__);
+			return;
+		}
+		if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+			/* don't register this object */
+			return;
+	}
+
+	bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
+	if (!bridge)
+		return;
+
+	bridge->handle = handle;
+
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
+		bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
+		bridge->flags |= ROOT_BRIDGE_HAS_PS3;
+
+	list_add(&bridge->list, &acpi_root_bridge_list);
+}
+
+struct acpi_root_hp_work {
+	struct work_struct work;
+	acpi_handle handle;
+	u32 type;
+	void *context;
+};
+
+static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type,
+					void *context,
+					void (*func)(struct work_struct *work))
+{
+	struct acpi_root_hp_work *hp_work;
+	int ret;
+
+	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
+	if (!hp_work)
+		return;
+
+	hp_work->handle = handle;
+	hp_work->type = type;
+	hp_work->context = context;
+
+	INIT_WORK(&hp_work->work, func);
+	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
+	if (!ret)
+		kfree(hp_work);
+}
+
+/* Program resources in newly inserted bridge */
+static void acpi_root_configure_bridge(acpi_handle handle)
+{
+	struct acpi_pci_root *root = acpi_pci_find_root(handle);
+
+	pcibios_resource_survey_bus(root->bus);
+	pci_assign_unassigned_bus_resources(root->bus);
+}
+
+static void handle_root_bridge_insertion(acpi_handle handle)
+{
+	struct acpi_device *device, *pdevice;
+	acpi_handle phandle;
+	int ret_val;
+
+	acpi_get_parent(handle, &phandle);
+	if (acpi_bus_get_device(phandle, &pdevice)) {
+		printk(KERN_DEBUG "no parent device, assuming NULL\n");
+		pdevice = NULL;
+	}
+	if (!acpi_bus_get_device(handle, &device)) {
+		/* check if  pci root_bus is removed */
+		struct acpi_pci_root *root = acpi_driver_data(device);
+		if (pci_find_bus(root->segment, root->secondary.start))
+			return;
+
+		printk(KERN_DEBUG "bus exists... trim\n");
+		/* this shouldn't be in here, so remove
+		 * the bus then re-add it...
+		 */
+		ret_val = acpi_bus_trim(device, 1);
+		printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
+	}
+	if (acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE)) {
+		printk(KERN_ERR "cannot add bridge to acpi list\n");
+		return;
+	}
+	acpi_root_configure_bridge(handle);
+	if (acpi_bus_start(device))
+		printk(KERN_ERR "cannot start bridge\n");
+}
+
+static void _handle_hotplug_event_root(struct work_struct *work)
+{
+	struct acpi_root_bridge *bridge;
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	struct acpi_root_hp_work *hp_work;
+	acpi_handle handle;
+	u32 type;
+
+	hp_work = container_of(work, struct acpi_root_hp_work, work);
+	handle = hp_work->handle;
+	type = hp_work->type;
+
+	bridge = acpi_root_handle_to_bridge(handle);
+
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+	switch (type) {
+	case ACPI_NOTIFY_BUS_CHECK:
+		/* bus enumerate */
+		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
+				 objname);
+		if (!bridge) {
+			handle_root_bridge_insertion(handle);
+			add_acpi_root_bridge(handle);
+		}
+
+		break;
+
+	case ACPI_NOTIFY_DEVICE_CHECK:
+		/* device check */
+		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
+				 objname);
+		if (!bridge) {
+			handle_root_bridge_insertion(handle);
+			add_acpi_root_bridge(handle);
+		}
+		break;
+
+	default:
+		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
+				 type, objname);
+		break;
+	}
+
+	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+}
+
+static void handle_hotplug_event_root(acpi_handle handle, u32 type,
+					void *context)
+{
+	alloc_acpi_root_hp_work(handle, type, context,
+				_handle_hotplug_event_root);
+}
+
+static acpi_status __init
+find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	char objname[64];
+	struct acpi_buffer buffer = { .length = sizeof(objname),
+				      .pointer = objname };
+	int *count = (int *)context;
+
+	if (!acpi_is_root_bridge(handle))
+		return AE_OK;
+
+	(*count)++;
+
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+
+	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+				handle_hotplug_event_root, NULL);
+	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+
+	add_acpi_root_bridge(handle);
+
+	return AE_OK;
+}
+
+static int __init acpi_pci_root_hp_init(void)
+{
+	int num = 0;
+
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+		ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
+
+	printk(KERN_DEBUG "Found %d acpi root devices\n", num);
+
+	return 0;
+}
+
+subsys_initcall(acpi_pci_root_hp_init);
Index: linux-2.6/drivers/acpi/Makefile
===================================================================
--- linux-2.6.orig/drivers/acpi/Makefile
+++ linux-2.6/drivers/acpi/Makefile
@@ -36,6 +36,7 @@ acpi-y				+= processor_core.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o pci_bind.o
+acpi-$(CONFIG_HOTPLUG)		+= pci_root_hp.o
 acpi-y				+= power.o
 acpi-y				+= event.o
 acpi-y				+= sysfs.o

[-- Attachment #3: pci_root_hp_6.patch --]
[-- Type: application/octet-stream, Size: 1223 bytes --]

Subject: [PATCH] PCI, ACPI: Pass device instead of handle when config root bridge

So we can avoid one calling of acpi_pci_find_root().

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root_hp.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root_hp.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root_hp.c
+++ linux-2.6/drivers/acpi/pci_root_hp.c
@@ -102,9 +102,9 @@ static void alloc_acpi_root_hp_work(acpi
 }
 
 /* Program resources in newly inserted bridge */
-static void acpi_root_configure_bridge(acpi_handle handle)
+static void acpi_root_configure_bridge(struct acpi_device *device)
 {
-	struct acpi_pci_root *root = acpi_pci_find_root(handle);
+	struct acpi_pci_root *root = acpi_driver_data(device);
 
 	pcibios_resource_survey_bus(root->bus);
 	pci_assign_unassigned_bus_resources(root->bus);
@@ -138,7 +138,7 @@ static void handle_root_bridge_insertion
 		printk(KERN_ERR "cannot add bridge to acpi list\n");
 		return;
 	}
-	acpi_root_configure_bridge(handle);
+	acpi_root_configure_bridge(device);
 	if (acpi_bus_start(device))
 		printk(KERN_ERR "cannot start bridge\n");
 }

[-- Attachment #4: update_acpi_pci_driver_add_remove.patch --]
[-- Type: application/octet-stream, Size: 4905 bytes --]

Subject: [PATCH] PCI, acpi: Update acpi_pci_driver add/remove interface

To take acpi_pci_root pointer instead, so later caller will not need to

check the apci_pci_root list to find it.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root.c            |    4 ++--
 drivers/acpi/pci_slot.c            |   12 +++++++-----
 drivers/pci/hotplug/acpiphp_glue.c |   12 +++++++-----
 include/linux/acpi.h               |    4 ++--
 4 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6/drivers/pci/hotplug/acpiphp_glue.c
@@ -382,10 +382,10 @@ static inline void config_p2p_bridge_fla
 
 
 /* allocate and initialize host bridge data structure */
-static void add_host_bridge(acpi_handle *handle)
+static void add_host_bridge(struct acpi_pci_root *root)
 {
+        acpi_handle handle = root->device->handle;
 	struct acpiphp_bridge *bridge;
-	struct acpi_pci_root *root = acpi_pci_find_root(handle);
 
 	bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL);
 	if (bridge == NULL)
@@ -468,8 +468,9 @@ find_p2p_bridge(acpi_handle handle, u32
 
 
 /* find hot-pluggable slots, and then find P2P bridge */
-static int add_bridge(acpi_handle handle)
+static int add_bridge(struct acpi_pci_root *root)
 {
+	acpi_handle handle = root->device->handle;
 	acpi_status status;
 	unsigned long long tmp;
 	acpi_handle dummy_handle;
@@ -490,7 +491,7 @@ static int add_bridge(acpi_handle handle
 	/* check if this bridge has ejectable slots */
 	if (detect_ejectable_slots(handle) > 0) {
 		dbg("found PCI host-bus bridge with hot-pluggable slots\n");
-		add_host_bridge(handle);
+		add_host_bridge(root);
 	}
 
 	/* search P2P bridges under this host bridge */
@@ -591,8 +592,9 @@ cleanup_p2p_bridge(acpi_handle handle, u
 	return AE_OK;
 }
 
-static void remove_bridge(acpi_handle handle)
+static void remove_bridge(struct acpi_pci_root *root)
 {
+	acpi_handle handle = root->device->handle;
 	struct acpiphp_bridge *bridge;
 
 	/* cleanup p2p bridges under this host bridge
Index: linux-2.6/include/linux/acpi.h
===================================================================
--- linux-2.6.orig/include/linux/acpi.h
+++ linux-2.6/include/linux/acpi.h
@@ -139,8 +139,8 @@ void acpi_pci_irq_disable (struct pci_de
 
 struct acpi_pci_driver {
 	struct acpi_pci_driver *next;
-	int (*add)(acpi_handle handle);
-	void (*remove)(acpi_handle handle);
+	int (*add)(struct acpi_pci_root *root);
+	void (*remove)(struct acpi_pci_root *root);
 };
 
 int acpi_pci_register_driver(struct acpi_pci_driver *driver);
Index: linux-2.6/drivers/acpi/pci_slot.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_slot.c
+++ linux-2.6/drivers/acpi/pci_slot.c
@@ -67,8 +67,8 @@ struct acpi_pci_slot {
 	struct list_head list;		/* node in the list of slots */
 };
 
-static int acpi_pci_slot_add(acpi_handle handle);
-static void acpi_pci_slot_remove(acpi_handle handle);
+static int acpi_pci_slot_add(struct acpi_pci_root *root);
+static void acpi_pci_slot_remove(struct acpi_pci_root *root);
 
 static LIST_HEAD(slot_list);
 static DEFINE_MUTEX(slot_list_lock);
@@ -295,11 +295,11 @@ walk_root_bridge(acpi_handle handle, acp
  * @handle: points to an acpi_pci_root
  */
 static int
-acpi_pci_slot_add(acpi_handle handle)
+acpi_pci_slot_add(struct acpi_pci_root *root)
 {
 	acpi_status status;
 
-	status = walk_root_bridge(handle, register_slot);
+	status = walk_root_bridge(root->device->handle, register_slot);
 	if (ACPI_FAILURE(status))
 		err("%s: register_slot failure - %d\n", __func__, status);
 
@@ -311,12 +311,14 @@ acpi_pci_slot_add(acpi_handle handle)
  * @handle: points to an acpi_pci_root
  */
 static void
-acpi_pci_slot_remove(acpi_handle handle)
+acpi_pci_slot_remove(struct acpi_pci_root *root)
 {
 	struct acpi_pci_slot *slot, *tmp;
 	struct pci_bus *pbus;
+	acpi_handle handle;
 
 	mutex_lock(&slot_list_lock);
+	handle = root->device->handle;
 	list_for_each_entry_safe(slot, tmp, &slot_list, list) {
 		if (slot->root_handle == handle) {
 			list_del(&slot->list);
Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -90,7 +90,7 @@ int acpi_pci_register_driver(struct acpi
 		return 0;
 
 	list_for_each_entry(root, &acpi_pci_roots, node) {
-		driver->add(root->device->handle);
+		driver->add(root);
 		n++;
 	}
 
@@ -116,7 +116,7 @@ void acpi_pci_unregister_driver(struct a
 		return;
 
 	list_for_each_entry(root, &acpi_pci_roots, node)
-		driver->remove(root->device->handle);
+		driver->remove(root);
 }
 
 EXPORT_SYMBOL(acpi_pci_unregister_driver);

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Taku Izumi]

On Wed, 12 Sep 2012 17:40:45 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> > Use mutex and RCU to protect global acpi_pci_roots list against
> > PCI host bridge hotplug operations.
> >
> > RCU is used to avoid possible deadlock in function acpi_pci_find_root()
> > and acpi_get_pci_rootbridge_handle(). A possible call graph:
> > acpi_pci_register_driver()
> >         mutex_lock(&acpi_pci_root_lock)
> >                 driver->add(root)
> >                         ......
> >                                 acpi_pci_find_root()
> 
> Where does this path occur?  I didn't see in in the current tree
> (where the only users of acpi_pci_register_driver() are for
> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
> work, which adds more acpi_pci_register_driver() users.

  First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
  In that case I faced deadlock at the following path:
  acpiphp_glue_init
     + acpi_pci_register_driver
       ...
         + add_bridge
           + acpi_pci_find_root

  So I used RCU instead.

> RCU seems unnecessarily complicated for this list, but I haven't gone
> through Yinghai's work yet, so I don't know what it requires.
> 
> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
> struct acpi_pci_root, which has all sorts of information that would be
> useful to the .add() and .remove() methods of sub-drivers.  It seems
> sort of stupid that we only pass the acpi_handle to the sub-drivers,
> forcing them to use hacks like acpi_pci_find_root() to look up the
> information we just threw away.  Can we just fix the .add() and
> .remove() interfaces to pass something more useful so we avoid the
> need for this deadlock path?

  Maybe yes. Do you prefer imprementation without RCU ?

  Best regards,
  Taku Izumi
> 
> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > ---
> >  drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> >
> > Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> > ===================================================================
> > --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> > +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/init.h>
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/rculist.h>
> >  #include <linux/pm.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pci.h>
> > @@ -86,7 +87,7 @@ int acpi_pci_register_driver(struct acpi
> >         mutex_lock(&acpi_pci_root_lock);
> >         list_add_tail(&driver->node, &acpi_pci_drivers);
> >         if (driver->add)
> > -               list_for_each_entry(root, &acpi_pci_roots, node) {
> > +               list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
> >                         driver->add(root->device->handle);
> >                         n++;
> >                 }
> > @@ -103,7 +104,7 @@ void acpi_pci_unregister_driver(struct a
> >         mutex_lock(&acpi_pci_root_lock);
> >         list_del(&driver->node);
> >         if (driver->remove)
> > -               list_for_each_entry(root, &acpi_pci_roots, node)
> > +               list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> >                         driver->remove(root->device->handle);
> >         mutex_unlock(&acpi_pci_root_lock);
> >  }
> > @@ -112,14 +113,19 @@ EXPORT_SYMBOL(acpi_pci_unregister_driver
> >  acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
> >  {
> >         struct acpi_pci_root *root;
> > +       struct acpi_handle *handle = NULL;
> >
> > -       list_for_each_entry(root, &acpi_pci_roots, node)
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> >                 if ((root->segment == (u16) seg) &&
> > -                   (root->secondary.start == (u16) bus))
> > -                       return root->device->handle;
> > -       return NULL;
> > -}
> > +                   (root->secondary.start == (u16) bus)) {
> > +                       handle = root->device->handle;
> > +                       break;
> > +               }
> > +       rcu_read_unlock();
> >
> > +       return handle;
> > +}
> >  EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
> >
> >  /**
> > @@ -266,10 +272,14 @@ struct acpi_pci_root *acpi_pci_find_root
> >  {
> >         struct acpi_pci_root *root;
> >
> > -       list_for_each_entry(root, &acpi_pci_roots, node) {
> > -               if (root->device->handle == handle)
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> > +               if (root->device->handle == handle) {
> > +                       rcu_read_unlock();
> >                         return root;
> > -       }
> > +               }
> > +       rcu_read_unlock();
> > +
> >         return NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
> > @@ -506,8 +516,9 @@ static int __devinit acpi_pci_root_add(s
> >          * TBD: Need PCI interface for enumeration/configuration of roots.
> >          */
> >
> > -       /* TBD: Locking */
> > -       list_add_tail(&root->node, &acpi_pci_roots);
> > +       mutex_lock(&acpi_pci_root_lock);
> > +       list_add_tail_rcu(&root->node, &acpi_pci_roots);
> > +       mutex_unlock(&acpi_pci_root_lock);
> >
> >         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
> >                acpi_device_name(device), acpi_device_bid(device),
> > @@ -526,7 +537,7 @@ static int __devinit acpi_pci_root_add(s
> >                             "Bus %04x:%02x not present in PCI namespace\n",
> >                             root->segment, (unsigned int)root->secondary.start);
> >                 result = -ENODEV;
> > -               goto end;
> > +               goto out_del_root;
> >         }
> >
> >         /*
> > @@ -536,7 +547,7 @@ static int __devinit acpi_pci_root_add(s
> >          */
> >         result = acpi_pci_bind_root(device);
> >         if (result)
> > -               goto end;
> > +               goto out_del_root;
> >
> >         /*
> >          * PCI Routing Table
> > @@ -614,9 +625,12 @@ static int __devinit acpi_pci_root_add(s
> >
> >         return 0;
> >
> > +out_del_root:
> > +       mutex_lock(&acpi_pci_root_lock);
> > +       list_del_rcu(&root->node);
> > +       mutex_unlock(&acpi_pci_root_lock);
> > +       synchronize_rcu();
> >  end:
> > -       if (!list_empty(&root->node))
> > -               list_del(&root->node);
> >         kfree(root);
> >         return result;
> >  }
> > @@ -646,11 +660,13 @@ static int acpi_pci_root_remove(struct a
> >         list_for_each_entry(driver, &acpi_pci_drivers, node)
> >                 if (driver->remove)
> >                         driver->remove(root->device->handle);
> > -       mutex_unlock(&acpi_pci_root_lock);
> >
> >         device_set_run_wake(root->bus->bridge, false);
> >         pci_acpi_remove_bus_pm_notifier(device);
> >
> > +       list_del_rcu(&root->node);
> > +       mutex_unlock(&acpi_pci_root_lock);
> > +       synchronize_rcu();
> >         kfree(root);
> >         return 0;
> >  }
> >
> 


-- 
Taku Izumi <izumi.taku@jp.fujitsu.com>

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Bjorn Helgaas]

On Thu, Sep 13, 2012 at 10:35 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> On Wed, 12 Sep 2012 17:40:45 -0600
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> > Use mutex and RCU to protect global acpi_pci_roots list against
>> > PCI host bridge hotplug operations.
>> >
>> > RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>> > and acpi_get_pci_rootbridge_handle(). A possible call graph:
>> > acpi_pci_register_driver()
>> >         mutex_lock(&acpi_pci_root_lock)
>> >                 driver->add(root)
>> >                         ......
>> >                                 acpi_pci_find_root()
>>
>> Where does this path occur?  I didn't see in in the current tree
>> (where the only users of acpi_pci_register_driver() are for
>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>> work, which adds more acpi_pci_register_driver() users.
>
>   First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
>   In that case I faced deadlock at the following path:
>   acpiphp_glue_init
>      + acpi_pci_register_driver
>        ...
>          + add_bridge
>            + acpi_pci_find_root
>
>   So I used RCU instead.

Oh, right.  I missed the acpiphp_glue_init() path.  That's clearly a problem.

>> RCU seems unnecessarily complicated for this list, but I haven't gone
>> through Yinghai's work yet, so I don't know what it requires.
>>
>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>> struct acpi_pci_root, which has all sorts of information that would be
>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>> forcing them to use hacks like acpi_pci_find_root() to look up the
>> information we just threw away.  Can we just fix the .add() and
>> .remove() interfaces to pass something more useful so we avoid the
>> need for this deadlock path?
>
>   Maybe yes. Do you prefer imprementation without RCU ?

Yes, if it's possible, I prefer to avoid RCU in this case.  RCU is
appropriate for performance paths, but it's much more difficult to
analyze than mutex locking.

Host bridge hotplug is definitely not a path where performance is an
issue, and I think reworking the .add()/.remove() interfaces will
allow us to use mutex locking.

I think it will also simplify the sub-drivers because having the
struct acpi_pci_root means they can get rid of acpi_pci_find_root(),
they don't have to re-evaluate _SEG and _BBN (in acpi_pci_slot_add()
-> walk_root_bridge()), they don't have to use pci_find_bus(), etc.

Re: [PATCH v2 4/6] ACPI, PCI: add acpi_pci_roots protection

[Jiang Liu]

On 09/14/2012 10:43 PM, Bjorn Helgaas wrote:
> On Thu, Sep 13, 2012 at 10:35 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> On Wed, 12 Sep 2012 17:40:45 -0600
>> Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>>>> Use mutex and RCU to protect global acpi_pci_roots list against
>>>> PCI host bridge hotplug operations.
>>>>
>>>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>>>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>>>> acpi_pci_register_driver()
>>>>         mutex_lock(&acpi_pci_root_lock)
>>>>                 driver->add(root)
>>>>                         ......
>>>>                                 acpi_pci_find_root()
>>>
>>> Where does this path occur?  I didn't see in in the current tree
>>> (where the only users of acpi_pci_register_driver() are for
>>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>>> work, which adds more acpi_pci_register_driver() users.
>>
>>   First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
>>   In that case I faced deadlock at the following path:
>>   acpiphp_glue_init
>>      + acpi_pci_register_driver
>>        ...
>>          + add_bridge
>>            + acpi_pci_find_root
>>
>>   So I used RCU instead.
> 
> Oh, right.  I missed the acpiphp_glue_init() path.  That's clearly a problem.
It's amazing. When I was writing the code, I just realized there's a possible
deadlock scenario and then wrote defensive code. Not it's proven to be true:)

>>> RCU seems unnecessarily complicated for this list, but I haven't gone
>>> through Yinghai's work yet, so I don't know what it requires.
>>>
>>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>>> struct acpi_pci_root, which has all sorts of information that would be
>>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>>> forcing them to use hacks like acpi_pci_find_root() to look up the
>>> information we just threw away.  Can we just fix the .add() and
>>> .remove() interfaces to pass something more useful so we avoid the
>>> need for this deadlock path?
>>
>>   Maybe yes. Do you prefer imprementation without RCU ?
> 
> Yes, if it's possible, I prefer to avoid RCU in this case.  RCU is
> appropriate for performance paths, but it's much more difficult to
> analyze than mutex locking.
> 
> Host bridge hotplug is definitely not a path where performance is an
> issue, and I think reworking the .add()/.remove() interfaces will
> allow us to use mutex locking.
> 
> I think it will also simplify the sub-drivers because having the
> struct acpi_pci_root means they can get rid of acpi_pci_find_root(),
> they don't have to re-evaluate _SEG and _BBN (in acpi_pci_slot_add()
> -> walk_root_bridge()), they don't have to use pci_find_bus(), etc.
Yes, it would be better to get rid of the RCU staff.

[PATCH v2 5/6] ACPI, PCI: add hostbridge removal function

[Taku Izumi]

Currently there's no PCI-related clean-up code
in acpi_pci_root_remove() function.
This patch introduces function for hostbridge removal.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_bind.c     |    7 +++++++
 drivers/acpi/pci_root.c     |    6 ++++++
 drivers/pci/remove.c        |   14 ++++++++++++++
 include/acpi/acpi_drivers.h |    1 +
 include/linux/pci.h         |    1 +
 5 files changed, 29 insertions(+)

Index: Bjorn-next-0903/drivers/pci/remove.c
===================================================================
--- Bjorn-next-0903.orig/drivers/pci/remove.c
+++ Bjorn-next-0903/drivers/pci/remove.c
@@ -92,3 +92,17 @@ void pci_stop_and_remove_bus_device(stru
 	pci_destroy_dev(dev);
 }
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
+
+void pci_remove_host_bridge(struct pci_host_bridge *bridge)
+{
+	struct pci_bus *root = bridge->bus;
+	struct pci_dev *dev, *tmp;
+
+	list_for_each_entry_safe_reverse(dev, tmp, &root->devices, bus_list)
+		pci_stop_and_remove_bus_device(dev);
+
+	pci_remove_bus(root);
+
+	device_unregister(&bridge->dev);
+}
+EXPORT_SYMBOL(pci_remove_host_bridge);
Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -655,6 +655,7 @@ static int acpi_pci_root_remove(struct a
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
+	struct pci_host_bridge *bridge = to_pci_host_bridge(root->bus->bridge);
 
 	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
@@ -664,6 +665,11 @@ static int acpi_pci_root_remove(struct a
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	acpi_pci_irq_del_prt(root->bus);
+	acpi_pci_unbind_root(device);
+
+	pci_remove_host_bridge(bridge);
+
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
Index: Bjorn-next-0903/include/linux/pci.h
===================================================================
--- Bjorn-next-0903.orig/include/linux/pci.h
+++ Bjorn-next-0903/include/linux/pci.h
@@ -734,6 +734,7 @@ extern struct pci_dev *pci_dev_get(struc
 extern void pci_dev_put(struct pci_dev *dev);
 extern void pci_remove_bus(struct pci_bus *b);
 extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+extern void pci_remove_host_bridge(struct pci_host_bridge *bridge);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
Index: Bjorn-next-0903/drivers/acpi/pci_bind.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
+++ Bjorn-next-0903/drivers/acpi/pci_bind.c
@@ -118,3 +118,10 @@ int acpi_pci_bind_root(struct acpi_devic
 
 	return 0;
 }
+
+void  acpi_pci_unbind_root(struct acpi_device *device)
+{
+	device->ops.bind = NULL;
+	device->ops.unbind = NULL;
+}
+
Index: Bjorn-next-0903/include/acpi/acpi_drivers.h
===================================================================
--- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
+++ Bjorn-next-0903/include/acpi/acpi_drivers.h
@@ -101,6 +101,7 @@ struct pci_bus;
 
 struct pci_dev *acpi_get_pci_dev(acpi_handle);
 int acpi_pci_bind_root(struct acpi_device *device);
+void acpi_pci_unbind_root(struct acpi_device *device);
 
 /* Arch-defined function to add a bus to the system */
 

[PATCH v2 6/6] ACPI, PCI: add resoruce-assign code for devices under hot-added hostbridge

[Taku Izumi]

Devices under hot-added hostbridge have no chance to assign resources
and to configure them, so this patch adds such code for hot-added 
hostbridges at acpi_pci_root_start().

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   17 +++++++++++++++++
 include/acpi/acpi_bus.h |    1 +
 2 files changed, 18 insertions(+)

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -39,6 +39,7 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/apei.h>
+#include <linux/pci_hotplug.h>
 
 #define PREFIX "ACPI: "
 
@@ -461,6 +462,9 @@ static int __devinit acpi_pci_root_add(s
 	if (!root)
 		return -ENOMEM;
 
+	if (system_state != SYSTEM_BOOTING)
+		root->hot_added = true;
+
 	segment = 0;
 	status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
 				       &segment);
@@ -639,6 +643,7 @@ static int acpi_pci_root_start(struct ac
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
+	struct pci_dev *pdev;
 
 	mutex_lock(&acpi_pci_root_lock);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
@@ -646,6 +651,18 @@ static int acpi_pci_root_start(struct ac
 			driver->add(device->handle);
 	mutex_unlock(&acpi_pci_root_lock);
 
+	/*
+	 * Devices under hot-added hostbridge have no chance to assign
+	 * resources and to configure them, so do that here
+	 */
+	if (root->hot_added) {
+		pci_bus_size_bridges(root->bus);
+		pci_bus_assign_resources(root->bus);
+		list_for_each_entry(pdev, &root->bus->devices, bus_list)
+			pci_configure_slot(pdev);
+		pci_enable_bridges(root->bus);
+	}
+
 	pci_bus_add_devices(root->bus);
 
 	return 0;
Index: Bjorn-next-0903/include/acpi/acpi_bus.h
===================================================================
--- Bjorn-next-0903.orig/include/acpi/acpi_bus.h
+++ Bjorn-next-0903/include/acpi/acpi_bus.h
@@ -407,6 +407,7 @@ struct acpi_pci_root {
 	u32 osc_support_set;	/* _OSC state of support bits */
 	u32 osc_control_set;	/* _OSC state of control bits */
 	phys_addr_t mcfg_addr;
+	bool hot_added;
 };
 
 /* helper */

Re: [PATCH v2 6/6] ACPI, PCI: add resoruce-assign code for devices under hot-added hostbridge

[Yinghai Lu]

On Mon, Sep 3, 2012 at 1:07 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>
> Devices under hot-added hostbridge have no chance to assign resources
> and to configure them, so this patch adds such code for hot-added
> hostbridges at acpi_pci_root_start().
>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_root.c |   17 +++++++++++++++++
>  include/acpi/acpi_bus.h |    1 +
>  2 files changed, 18 insertions(+)
>
> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> @@ -39,6 +39,7 @@
>  #include <acpi/acpi_bus.h>
>  #include <acpi/acpi_drivers.h>
>  #include <acpi/apei.h>
> +#include <linux/pci_hotplug.h>
>
>  #define PREFIX "ACPI: "
>
> @@ -461,6 +462,9 @@ static int __devinit acpi_pci_root_add(s
>         if (!root)
>                 return -ENOMEM;
>
> +       if (system_state != SYSTEM_BOOTING)
> +               root->hot_added = true;
> +
>         segment = 0;
>         status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
>                                        &segment);
> @@ -639,6 +643,7 @@ static int acpi_pci_root_start(struct ac
>  {
>         struct acpi_pci_root *root = acpi_driver_data(device);
>         struct acpi_pci_driver *driver;
> +       struct pci_dev *pdev;
>
>         mutex_lock(&acpi_pci_root_lock);
>         list_for_each_entry(driver, &acpi_pci_drivers, node)
> @@ -646,6 +651,18 @@ static int acpi_pci_root_start(struct ac
>                         driver->add(device->handle);
>         mutex_unlock(&acpi_pci_root_lock);
>
> +       /*
> +        * Devices under hot-added hostbridge have no chance to assign
> +        * resources and to configure them, so do that here
> +        */
> +       if (root->hot_added) {
> +               pci_bus_size_bridges(root->bus);
> +               pci_bus_assign_resources(root->bus);
> +               list_for_each_entry(pdev, &root->bus->devices, bus_list)
> +                       pci_configure_slot(pdev);
> +               pci_enable_bridges(root->bus);
> +       }
> +
>         pci_bus_add_devices(root->bus);

after looking at your simplified version in patch5 and patch6
found that you do not understand my patchset correctly.

in my patchset there is change on:
acpi_pci_root_add
acpi_pci_root_start
acpi_pci_root_remove

and
handle_root_bridge_insertion/acpi_root_configure_bridge
handle_root_bridge_removal

for hot add path:
handle_root_bridge_insertion will call
      acpi_pci_root_add: it will scan the root bus and find all pci device
      acpi_root_configure_bridge: it will survey fw set pci resource,
and assigned unsigned resource
      acpi_pci_root_start: it will start acpi_pci driver for ioapic
controller and dmar iommu
                                    then call pci_bus_add_devices to
load other pci device drivers.

for hot remove path:
      acpi_pci_root_remove: will call pci_stop_bus_devices() to stop
drivers for all normal pci devices.
                                        then stop acpi_pci driver for
dmar and ioapic driver
                                        then call
pci_stop_and_remove_bus to remove the root bus.

the point is : keep the driver of ioapic and dmar  driver loading
before normal pci drivers, and later
stop driver for dmar/iommu, after unloading pci drivers.

Thanks

Yinghai

Re: [PATCH v2 6/6] ACPI, PCI: add resoruce-assign code for devices under hot-added hostbridge

[Taku Izumi]

On Mon, 3 Sep 2012 13:27:11 -0700
Yinghai Lu <yinghai@kernel.org> wrote:

> On Mon, Sep 3, 2012 at 1:07 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> >
> > Devices under hot-added hostbridge have no chance to assign resources
> > and to configure them, so this patch adds such code for hot-added
> > hostbridges at acpi_pci_root_start().
> >
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > ---
> >  drivers/acpi/pci_root.c |   17 +++++++++++++++++
> >  include/acpi/acpi_bus.h |    1 +
> >  2 files changed, 18 insertions(+)
> >
> > Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> > ===================================================================
> > --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> > +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> > @@ -39,6 +39,7 @@
> >  #include <acpi/acpi_bus.h>
> >  #include <acpi/acpi_drivers.h>
> >  #include <acpi/apei.h>
> > +#include <linux/pci_hotplug.h>
> >
> >  #define PREFIX "ACPI: "
> >
> > @@ -461,6 +462,9 @@ static int __devinit acpi_pci_root_add(s
> >         if (!root)
> >                 return -ENOMEM;
> >
> > +       if (system_state != SYSTEM_BOOTING)
> > +               root->hot_added = true;
> > +
> >         segment = 0;
> >         status = acpi_evaluate_integer(device->handle, METHOD_NAME__SEG, NULL,
> >                                        &segment);
> > @@ -639,6 +643,7 @@ static int acpi_pci_root_start(struct ac
> >  {
> >         struct acpi_pci_root *root = acpi_driver_data(device);
> >         struct acpi_pci_driver *driver;
> > +       struct pci_dev *pdev;
> >
> >         mutex_lock(&acpi_pci_root_lock);
> >         list_for_each_entry(driver, &acpi_pci_drivers, node)
> > @@ -646,6 +651,18 @@ static int acpi_pci_root_start(struct ac
> >                         driver->add(device->handle);
> >         mutex_unlock(&acpi_pci_root_lock);
> >
> > +       /*
> > +        * Devices under hot-added hostbridge have no chance to assign
> > +        * resources and to configure them, so do that here
> > +        */
> > +       if (root->hot_added) {
> > +               pci_bus_size_bridges(root->bus);
> > +               pci_bus_assign_resources(root->bus);
> > +               list_for_each_entry(pdev, &root->bus->devices, bus_list)
> > +                       pci_configure_slot(pdev);
> > +               pci_enable_bridges(root->bus);
> > +       }
> > +
> >         pci_bus_add_devices(root->bus);
> 
> after looking at your simplified version in patch5 and patch6
> found that you do not understand my patchset correctly.
> 
> in my patchset there is change on:
> acpi_pci_root_add
> acpi_pci_root_start
> acpi_pci_root_remove
> 
> and
> handle_root_bridge_insertion/acpi_root_configure_bridge
> handle_root_bridge_removal
> 
> for hot add path:
> handle_root_bridge_insertion will call
>       acpi_pci_root_add: it will scan the root bus and find all pci device
>       acpi_root_configure_bridge: it will survey fw set pci resource,
> and assigned unsigned resource
>       acpi_pci_root_start: it will start acpi_pci driver for ioapic
> controller and dmar iommu
>                                     then call pci_bus_add_devices to
> load other pci device drivers.
> 
> for hot remove path:
>       acpi_pci_root_remove: will call pci_stop_bus_devices() to stop
> drivers for all normal pci devices.
>                                         then stop acpi_pci driver for
> dmar and ioapic driver
>                                         then call
> pci_stop_and_remove_bus to remove the root bus.
> 
> the point is : keep the driver of ioapic and dmar  driver loading
> before normal pci drivers, and later
> stop driver for dmar/iommu, after unloading pci drivers.

 Sorry for too late response.
 I failed at rebasing. I'll resend this patch.
 Could you please review these again?

Best regards,
Taku Izumi <izumi.taku@jp.fujitsu.com>

Re: [PATCH v2 6/6] ACPI, PCI: add resoruce-assign code for devices under hot-added hostbridge

[Taku Izumi]

Currently there's no PCI-related clean-up code
in acpi_pci_root_remove() function.
This patch introduces function for hostbridge removal,
and brings back pci_stop_bus_devices() function.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_bind.c     |    7 +++++++
 drivers/acpi/pci_root.c     |    7 +++++++
 drivers/pci/remove.c        |   27 +++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |    1 +
 include/linux/pci.h         |    2 ++
 5 files changed, 44 insertions(+)

Index: Bjorn-next-0903/drivers/pci/remove.c
===================================================================
--- Bjorn-next-0903.orig/drivers/pci/remove.c
+++ Bjorn-next-0903/drivers/pci/remove.c
@@ -92,3 +92,30 @@ void pci_stop_and_remove_bus_device(stru
 	pci_destroy_dev(dev);
 }
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
+
+void pci_stop_bus_devices(struct pci_bus *bus)
+{
+	struct pci_dev *dev, *tmp;
+
+	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
+		if (dev->subordinate)
+			pci_stop_bus_devices(dev->subordinate);
+		pci_stop_dev(dev);
+	}
+
+}
+EXPORT_SYMBOL(pci_stop_bus_devices);
+
+void pci_remove_host_bridge(struct pci_host_bridge *bridge)
+{
+	struct pci_bus *root = bridge->bus;
+	struct pci_dev *dev, *tmp;
+
+	list_for_each_entry_safe_reverse(dev, tmp, &root->devices, bus_list)
+		pci_stop_and_remove_bus_device(dev);
+
+	pci_remove_bus(root);
+
+	device_unregister(&bridge->dev);
+}
+EXPORT_SYMBOL(pci_remove_host_bridge);
Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -655,8 +655,10 @@ static int acpi_pci_root_remove(struct a
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 	struct acpi_pci_driver *driver;
+	struct pci_host_bridge *bridge = to_pci_host_bridge(root->bus->bridge);
 
 	mutex_lock(&acpi_pci_root_lock);
+	pci_stop_bus_devices(root->bus);
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root->device->handle);
@@ -664,6 +666,11 @@ static int acpi_pci_root_remove(struct a
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	acpi_pci_irq_del_prt(root->bus);
+	acpi_pci_unbind_root(device);
+
+	pci_remove_host_bridge(bridge);
+
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
Index: Bjorn-next-0903/include/linux/pci.h
===================================================================
--- Bjorn-next-0903.orig/include/linux/pci.h
+++ Bjorn-next-0903/include/linux/pci.h
@@ -734,6 +734,8 @@ extern struct pci_dev *pci_dev_get(struc
 extern void pci_dev_put(struct pci_dev *dev);
 extern void pci_remove_bus(struct pci_bus *b);
 extern void pci_stop_and_remove_bus_device(struct pci_dev *dev);
+extern void pci_stop_bus_devices(struct pci_bus *bus);
+extern void pci_remove_host_bridge(struct pci_host_bridge *bridge);
 void pci_setup_cardbus(struct pci_bus *bus);
 extern void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
Index: Bjorn-next-0903/drivers/acpi/pci_bind.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_bind.c
+++ Bjorn-next-0903/drivers/acpi/pci_bind.c
@@ -118,3 +118,10 @@ int acpi_pci_bind_root(struct acpi_devic
 
 	return 0;
 }
+
+void  acpi_pci_unbind_root(struct acpi_device *device)
+{
+	device->ops.bind = NULL;
+	device->ops.unbind = NULL;
+}
+
Index: Bjorn-next-0903/include/acpi/acpi_drivers.h
===================================================================
--- Bjorn-next-0903.orig/include/acpi/acpi_drivers.h
+++ Bjorn-next-0903/include/acpi/acpi_drivers.h
@@ -101,6 +101,7 @@ struct pci_bus;
 
 struct pci_dev *acpi_get_pci_dev(acpi_handle);
 int acpi_pci_bind_root(struct acpi_device *device);
+void acpi_pci_unbind_root(struct acpi_device *device);
 
 /* Arch-defined function to add a bus to the system */