On Thu, Aug 9, 2012 at 11:14 PM, Taku Izumi <izumi.taku@xxxxxxxxxxxxxx> wrote:
> From: Jiang Liu <jiang.liu@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> [izumi.taku@xxxxxxxxxxxxxx: a bit change at acpi_pci_root_remove()]
> Signed-off-by: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
>
> ---
> 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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
