From 75eb2d13acca49c3c4f1c96fb04d6c9fd6013e78 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Sun, 24 Mar 2013 22:54:40 +0100 Subject: [PATCH 1/2] ACPI / PM: Fix potential problem in acpi_device_get_power() Theoretically, in some situations acpi_device_get_power() may return an incorrect result, because the settings of the power resources depended on by the device may indicate a power state shallower than the actual power state of the device. Say that two devices, A and B, depend on two power resources, X and Y, in such a way that _PR0 for both A and B list both X and Y and _PR3 for both A and B list power resource Y alone. Also suppose that _PS0 and _PS3 are present for both A and B. Then, if devices A and B are initially in D0, power resources X and Y are initially "on" and their reference counters are equal to 2. To put device A into power state D3hot the kernel will decrement the reference counter of power resource X, but that power resource won't be turned off, because it is still in use by device B (its reference counter is equal to 1). Next, _PS3 will be executed for device A. Afterward the configuration of the power resources will indicate that device A is in power state D0 (both X and Y are "on"), but in fact it is in D3hot (because _PS3 has been executed for it). In that situation, if acpi_device_get_power() is called to get the power state of device A, it will first execute _PSC for it which should return 3. That will cause acpi_device_get_power() to run acpi_power_get_inferred_state() for device A and the resultant power state will be D0, which is incorrect. To fix that change acpi_device_get_power() to first execute acpi_power_get_inferred_state() for the given device (if it depends on power resources) and to evaluate _PSC for it subsequently, so that the result inferred from the power resources configuration can be amended by the _PSC return value. Signed-off-by: Rafael J. Wysocki Reviewed-by: Aaron Lu --- drivers/acpi/device_pm.c | 43 ++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index dd314ef9bff1..96de787e6104 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -145,27 +145,36 @@ int acpi_device_get_power(struct acpi_device *device, int *state) } /* - * Get the device's power state either directly (via _PSC) or - * indirectly (via power resources). + * Get the device's power state from power resources settings and _PSC, + * if available. */ - if (device->power.flags.explicit_get) { - unsigned long long psc; - acpi_status status = acpi_evaluate_integer(device->handle, - "_PSC", NULL, &psc); - if (ACPI_FAILURE(status)) - return -ENODEV; - - result = psc; - } - /* The test below covers ACPI_STATE_UNKNOWN too. */ - if (result <= ACPI_STATE_D2) { - ; /* Do nothing. */ - } else if (device->power.flags.power_resources) { + if (device->power.flags.power_resources) { int error = acpi_power_get_inferred_state(device, &result); if (error) return error; - } else if (result == ACPI_STATE_D3_HOT) { - result = ACPI_STATE_D3; + } + if (device->power.flags.explicit_get) { + acpi_handle handle = device->handle; + unsigned long long psc; + acpi_status status; + + status = acpi_evaluate_integer(handle, "_PSC", NULL, &psc); + if (ACPI_FAILURE(status)) + return -ENODEV; + + /* + * The power resources settings may indicate a power state + * shallower than the actual power state of the device. + * + * Moreover, on systems predating ACPI 4.0, if the device + * doesn't depend on any power resources and _PSC returns 3, + * that means "power off". We need to maintain compatibility + * with those systems. + */ + if (psc > result && psc < ACPI_STATE_D3_COLD) + result = psc; + else if (result == ACPI_STATE_UNKNOWN) + result = psc > ACPI_STATE_D2 ? ACPI_STATE_D3_COLD : psc; } /* From 41a2a4665d401f79912a3da83ff9ef3e502bc12e Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 11 Apr 2013 22:41:48 +0200 Subject: [PATCH 2/2] ACPI / PM: Expose lists of device wakeup power resources to user space Commit 18a3870 (ACPI / PM: Expose lists of device power resources to user space) exposed the lists of ACPI power resources associated with power states of ACPI devices, but it didn't expose the lists of ACPI wakeup power resources, which also is necessary to get the full picture of dependencies between ACPI devices and power resources. For this reason, for every ACPI device node having a list of ACPI wakeup power resources associated with it, expose that list to user space in analogy with commit 18a3870. Signed-off-by: Rafael J. Wysocki --- .../sysfs-devices-power_resources_wakeup | 13 +++++ drivers/acpi/power.c | 58 +++++++++++++------ 2 files changed, 52 insertions(+), 19 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-devices-power_resources_wakeup diff --git a/Documentation/ABI/testing/sysfs-devices-power_resources_wakeup b/Documentation/ABI/testing/sysfs-devices-power_resources_wakeup new file mode 100644 index 000000000000..e0588feeb6e1 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-devices-power_resources_wakeup @@ -0,0 +1,13 @@ +What: /sys/devices/.../power_resources_wakeup/ +Date: April 2013 +Contact: Rafael J. Wysocki +Description: + The /sys/devices/.../power_resources_wakeup/ directory is only + present for device objects representing ACPI device nodes that + require ACPI power resources for wakeup signaling. + + If present, it contains symbolic links to device directories + representing ACPI power resources that need to be turned on for + the given device node to be able to signal wakeup. The names of + the links are the same as the names of the directories they + point to. diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c index 34f5ef11d427..0e3f57674fd1 100644 --- a/drivers/acpi/power.c +++ b/drivers/acpi/power.c @@ -459,57 +459,79 @@ static struct attribute_group attr_groups[] = { }, }; -static void acpi_power_hide_list(struct acpi_device *adev, int state) +static struct attribute_group wakeup_attr_group = { + .name = "power_resources_wakeup", + .attrs = attrs, +}; + +static void acpi_power_hide_list(struct acpi_device *adev, + struct list_head *resources, + struct attribute_group *attr_group) { - struct acpi_device_power_state *ps = &adev->power.states[state]; struct acpi_power_resource_entry *entry; - if (list_empty(&ps->resources)) + if (list_empty(resources)) return; - list_for_each_entry_reverse(entry, &ps->resources, node) { + list_for_each_entry_reverse(entry, resources, node) { struct acpi_device *res_dev = &entry->resource->device; sysfs_remove_link_from_group(&adev->dev.kobj, - attr_groups[state].name, + attr_group->name, dev_name(&res_dev->dev)); } - sysfs_remove_group(&adev->dev.kobj, &attr_groups[state]); + sysfs_remove_group(&adev->dev.kobj, attr_group); } -static void acpi_power_expose_list(struct acpi_device *adev, int state) +static void acpi_power_expose_list(struct acpi_device *adev, + struct list_head *resources, + struct attribute_group *attr_group) { - struct acpi_device_power_state *ps = &adev->power.states[state]; struct acpi_power_resource_entry *entry; int ret; - if (list_empty(&ps->resources)) + if (list_empty(resources)) return; - ret = sysfs_create_group(&adev->dev.kobj, &attr_groups[state]); + ret = sysfs_create_group(&adev->dev.kobj, attr_group); if (ret) return; - list_for_each_entry(entry, &ps->resources, node) { + list_for_each_entry(entry, resources, node) { struct acpi_device *res_dev = &entry->resource->device; ret = sysfs_add_link_to_group(&adev->dev.kobj, - attr_groups[state].name, + attr_group->name, &res_dev->dev.kobj, dev_name(&res_dev->dev)); if (ret) { - acpi_power_hide_list(adev, state); + acpi_power_hide_list(adev, resources, attr_group); break; } } } +static void acpi_power_expose_hide(struct acpi_device *adev, + struct list_head *resources, + struct attribute_group *attr_group, + bool expose) +{ + if (expose) + acpi_power_expose_list(adev, resources, attr_group); + else + acpi_power_hide_list(adev, resources, attr_group); +} + void acpi_power_add_remove_device(struct acpi_device *adev, bool add) { struct acpi_device_power_state *ps; struct acpi_power_resource_entry *entry; int state; + if (adev->wakeup.flags.valid) + acpi_power_expose_hide(adev, &adev->wakeup.resources, + &wakeup_attr_group, add); + if (!adev->power.flags.power_resources) return; @@ -523,12 +545,10 @@ void acpi_power_add_remove_device(struct acpi_device *adev, bool add) acpi_power_remove_dependent(resource, adev); } - for (state = ACPI_STATE_D0; state <= ACPI_STATE_D3_HOT; state++) { - if (add) - acpi_power_expose_list(adev, state); - else - acpi_power_hide_list(adev, state); - } + for (state = ACPI_STATE_D0; state <= ACPI_STATE_D3_HOT; state++) + acpi_power_expose_hide(adev, + &adev->power.states[state].resources, + &attr_groups[state], add); } int acpi_power_wakeup_list_init(struct list_head *list, int *system_level_p)