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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html