Re: [PATCH V3] ACPI / OSL: Make acpi_os_map_cleanup() use call_rcu() to avoid deadlocks

[Rafael J. Wysocki]

On Wednesday, August 27, 2014 03:11:29 PM Lan Tianyu wrote:
> Deadlock is possible when CPU hotplug and evaluating ACPI method happen
> at the same time.
> 
> During CPU hotplug, acpi_cpu_soft_notify() is called under the CPU hotplug
> lock.  Then, acpi_cpu_soft_notify() calls acpi_bus_get_device() to obtain
> the struct acpi_device attached to the given ACPI handle.  The ACPICA's
> namespace lock will be acquired by acpi_bus_get_device() in the process.
> Thus it is possible to hold the ACPICA's namespace lock under the CPU
> hotplug lock.
> 
> Evaluating an ACPI method may involve accessing an operation region in
> system memory and the associated address space will be unmapped under
> the ACPICA's namespace lock after completing the access. Currently, osl.c
> uses RCU to protect memory ranges used by AML.  When unmapping them it
> calls synchronize_rcu() in acpi_os_map_cleanup(), but that blocks
> CPU hotplug by acquiring the CPU hotplug lock.  Thus it is possible to
> hold the CPU hotplug lock under the ACPICA's namespace lock.
> 
> This leads to deadlocks like the following one if AML accessing operation
> regions in memory is executed in parallel with CPU hotplug.

[cut]

> To avoid such deadlocks, modify acpi_os_map_cleanup() to use call_rcu()
> to schedule acpi_os_async_umap() asynchronously to umap memory regions
> that aren't used any more. The umap operation can't be done in the
> call_rcu()'s callback directly because the callback will be called in the
> soft irq context and acpi_unmap() holds mutex lock inside.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> [rjw: Subject and changelog.]
> Cc: All applicable <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  drivers/acpi/osl.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3abe9b2..9baef71 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -40,6 +40,7 @@
>  #include <linux/nmi.h>
>  #include <linux/acpi.h>
>  #include <linux/efi.h>
> +#include <linux/async.h>
>  #include <linux/ioport.h>
>  #include <linux/list.h>
>  #include <linux/jiffies.h>
> @@ -94,6 +95,7 @@ struct acpi_ioremap {
>  	acpi_physical_address phys;
>  	acpi_size size;
>  	unsigned long refcount;
> +	struct rcu_head rcu;
>  };
>  
>  static LIST_HEAD(acpi_ioremaps);
> @@ -423,13 +425,25 @@ static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
>  		list_del_rcu(&map->list);
>  }
>  
> +static void acpi_os_async_umap(void *data, async_cookie_t cookie)
> +{
> +	struct acpi_ioremap *map = data;
> +
> +	acpi_unmap(map->phys, map->virt);
> +	kfree(map);
> +}
> +
> +static void acpi_os_map_reclaim(struct rcu_head *rcu)
> +{
> +	struct acpi_ioremap *map = container_of(rcu, struct acpi_ioremap, rcu);
> +
> +	async_schedule(acpi_os_async_umap, map);
> +}
> +
>  static void acpi_os_map_cleanup(struct acpi_ioremap *map)
>  {
> -	if (!map->refcount) {
> -		synchronize_rcu();
> -		acpi_unmap(map->phys, map->virt);
> -		kfree(map);
> -	}
> +	if (!map->refcount)
> +		call_rcu(&map->rcu, acpi_os_map_reclaim);
>  }
>  
>  void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
> 

This goes a bit too far.  First, if you need to start an async thread,
you can simply do synchronize_rcu() from there.  Second, though, perhaps
we can address the whole deadlock in a different way.

For example, if we do something like the patch below (which I haven't
tested, but it should work if I'm not missing something horribly), we
won't be taking the ACPI namespace lock under the CPU hotplug lock
in acpi_cpu_soft_notify() any more.

Rafael

---
 drivers/acpi/processor_driver.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/processor_driver.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_driver.c
+++ linux-pm/drivers/acpi/processor_driver.c
@@ -129,7 +129,11 @@ static int acpi_cpu_soft_notify(struct n
 	if (action == CPU_STARTING || action == CPU_DYING)
 		return NOTIFY_DONE;
 
-	if (!pr || acpi_bus_get_device(pr->handle, &device))
+	if (!pr || !pr->dev)
+		return NOTIFY_DONE;
+
+	device = ACPI_COMPANION(pr->dev);
+	if (!device)
 		return NOTIFY_DONE;
 
 	if (action == CPU_ONLINE) {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/