Patchwork [5/7,RESEND] ACPI, PCI: Protect global lists in drivers/acpi/pci_root.c

login
register
mail settings
Submitter Taku Izumi
Date Aug. 10, 2012, 6:14 a.m.
Message ID <20120810151417.031666ce.izumi.taku@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/176366/
State Superseded
Headers show

Comments

Taku Izumi - Aug. 10, 2012, 6:14 a.m.
From: Jiang Liu <jiang.liu@huawei.com>

    ACPI, PCI: Protect global lists in drivers/acpi/pci_root.c
    
    There are two global lists inf file drivers/acpi/pci_root.c.
    One is for registered acpi_pci_drivers, the other is for
    enumerated ACPI PCI root bridge objects. These two global
    lists may change dynamically when registering/deregistering
    acpi_pci_drivers or adding/removing ACPI PCI root bridge
    objects. So protect them by mutex lock and RCU list.
    
    Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
    Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    [izumi.taku@jp.fujitsu.com: a bit change at acpi_pci_root_remove()]
    Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>

---
 drivers/acpi/pci_root.c |   86 +++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 33 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 16, 2012, 5:25 p.m.
On Thu, Aug 9, 2012 at 11:14 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
>     ACPI, PCI: Protect global lists in drivers/acpi/pci_root.c
>
>     There are two global lists inf file drivers/acpi/pci_root.c.
>     One is for registered acpi_pci_drivers, the other is for
>     enumerated ACPI PCI root bridge objects. These two global
>     lists may change dynamically when registering/deregistering
>     acpi_pci_drivers or adding/removing ACPI PCI root bridge
>     objects. So protect them by mutex lock and RCU list.

Is it possible to split this into two patches, one to add
acpi_pci_drivers protection and another to add acpi_pci_roots
protection?  That would make this easier to review.

>     Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>     Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>     [izumi.taku@jp.fujitsu.com: a bit change at acpi_pci_root_remove()]
>     Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>
> ---
>  drivers/acpi/pci_root.c |   86 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 33 deletions(-)
>
> Index: Bjorn-next/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next/drivers/acpi/pci_root.c
> @@ -27,7 +27,8 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/types.h>
> -#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci.h>
> @@ -71,6 +72,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,47 +84,48 @@ 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_rcu(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_rcu(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)
>  {
>         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);
>
>  /**
> @@ -268,10 +272,15 @@ 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);
> @@ -459,7 +468,7 @@ static int __devinit acpi_pci_root_add(s
>         if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
>                 printk(KERN_ERR PREFIX "can't evaluate _SEG\n");
>                 result = -ENODEV;
> -               goto end;
> +               goto out_free;
>         }
>
>         /* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
> @@ -484,7 +493,7 @@ static int __devinit acpi_pci_root_add(s
>                 else {
>                         printk(KERN_ERR PREFIX "can't evaluate _BBN\n");
>                         result = -ENODEV;
> -                       goto end;
> +                       goto out_free;
>                 }
>         }
>
> @@ -508,8 +517,8 @@ 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);
>
>         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
>                acpi_device_name(device), acpi_device_bid(device),
> @@ -528,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;
>         }
>
>         /*
> @@ -538,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,11 +623,15 @@ static int __devinit acpi_pci_root_add(s
>         if (device->wakeup.flags.run_wake)
>                 device_set_run_wake(root->bus->bridge, true);
>
> +       mutex_unlock(&acpi_pci_root_lock);
> +
>         return 0;
>
> -end:
> -       if (!list_empty(&root->node))
> -               list_del(&root->node);
> +out_del_root:
> +       list_del_rcu(&root->node);
> +       mutex_unlock(&acpi_pci_root_lock);
> +       synchronize_rcu();
> +out_free:
>         kfree(root);
>         return result;
>  }
> @@ -628,11 +641,13 @@ 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);
>
>         pci_bus_add_devices(root->bus);
> +       mutex_unlock(&acpi_pci_root_lock);
>
>         return 0;
>  }
> @@ -642,6 +657,8 @@ 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);
> @@ -649,6 +666,9 @@ static int acpi_pci_root_remove(struct a
>         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;
>  }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: Bjorn-next/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next.orig/drivers/acpi/pci_root.c
+++ Bjorn-next/drivers/acpi/pci_root.c
@@ -27,7 +27,8 @@ 
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/types.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/rculist.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci.h>
@@ -71,6 +72,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,47 +84,48 @@  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_rcu(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_rcu(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)
 {
 	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);
 
 /**
@@ -268,10 +272,15 @@  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);
@@ -459,7 +468,7 @@  static int __devinit acpi_pci_root_add(s
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		printk(KERN_ERR PREFIX "can't evaluate _SEG\n");
 		result = -ENODEV;
-		goto end;
+		goto out_free;
 	}
 
 	/* Check _CRS first, then _BBN.  If no _BBN, default to zero. */
@@ -484,7 +493,7 @@  static int __devinit acpi_pci_root_add(s
 		else {
 			printk(KERN_ERR PREFIX "can't evaluate _BBN\n");
 			result = -ENODEV;
-			goto end;
+			goto out_free;
 		}
 	}
 
@@ -508,8 +517,8 @@  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);
 
 	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
@@ -528,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;
 	}
 
 	/*
@@ -538,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,11 +623,15 @@  static int __devinit acpi_pci_root_add(s
 	if (device->wakeup.flags.run_wake)
 		device_set_run_wake(root->bus->bridge, true);
 
+	mutex_unlock(&acpi_pci_root_lock);
+
 	return 0;
 
-end:
-	if (!list_empty(&root->node))
-		list_del(&root->node);
+out_del_root:
+	list_del_rcu(&root->node);
+	mutex_unlock(&acpi_pci_root_lock);
+	synchronize_rcu();
+out_free:
 	kfree(root);
 	return result;
 }
@@ -628,11 +641,13 @@  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);
 
 	pci_bus_add_devices(root->bus);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	return 0;
 }
@@ -642,6 +657,8 @@  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);
@@ -649,6 +666,9 @@  static int acpi_pci_root_remove(struct a
 	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;
 }