On 2014年08月28日 07:37, Rafael J. Wysocki wrote:
> 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.
Yes, that will be simple.
> 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.
I considered this before. But the notify callback still will evaluate
ACPI method and may hold ACPICA's namespace lock. E.G "_CST".
Calltrace:
acpi_cpu_soft_notify()
acpi_processor_hotplug()
acpi_processor_get_power_info()
acpi_processor_get_power_info_cst()
>
> 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) {
>
--
Best regards
Tianyu Lan
--
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