From 45f0a85c8258741d11bda25c0a5669c06267204a Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 3 Jun 2013 21:49:52 +0200 Subject: PM / Runtime: Rework the "runtime idle" helper routine The "runtime idle" helper routine, rpm_idle(), currently ignores return values from .runtime_idle() callbacks executed by it. However, it turns out that many subsystems use pm_generic_runtime_idle() which checks the return value of the driver's callback and executes pm_runtime_suspend() for the device unless that value is not 0. If that logic is moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped and its users will not need any .runtime_idle() callbacks any more. Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle() routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and ata_port_runtime_idle(), respectively, as well as a few drivers' ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has been returned by the .runtime_idle() callback executed by it. To reduce overall code bloat, make the changes described above. Tested-by: Mika Westerberg Tested-by: Kevin Hilman Signed-off-by: Rafael J. Wysocki Acked-by: Kevin Hilman Reviewed-by: Ulf Hansson Acked-by: Alan Stern --- drivers/acpi/device_pm.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index bc493aa3af19..1eb8b6258786 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general_pm_domain = { #ifdef CONFIG_PM_RUNTIME .runtime_suspend = acpi_subsys_runtime_suspend, .runtime_resume = acpi_subsys_runtime_resume, - .runtime_idle = pm_generic_runtime_idle, #endif #ifdef CONFIG_PM_SLEEP .prepare = acpi_subsys_prepare, -- cgit v1.2.3 From b25c77efa71178f8281401e492e5c63cf7c34900 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Sun, 16 Jun 2013 00:37:42 +0200 Subject: ACPI / PM: Rename function acpi_device_power_state() and make it static There is a name clash between function acpi_device_power_state() defined in drivers/acpi/device_pm.c and structure type acpi_device_power_state defined in include/acpi/acpi_bus.h, which may be resolved by renaming the function. Additionally, that funtion may be made static, because it is not used anywhere outside of the file it is defined in. Rename acpi_device_power_state() to acpi_dev_pm_get_state(), which better reflects its purpose, and make it static. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/device_pm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 318fa32a141e..26d3fd718a04 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -399,7 +399,7 @@ bool acpi_bus_can_wakeup(acpi_handle handle) EXPORT_SYMBOL(acpi_bus_can_wakeup); /** - * acpi_device_power_state - Get preferred power state of ACPI device. + * acpi_dev_pm_get_state - Get preferred power state of ACPI device. * @dev: Device whose preferred target power state to return. * @adev: ACPI device node corresponding to @dev. * @target_state: System state to match the resultant device state. @@ -417,8 +417,8 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup); * Callers must ensure that @dev and @adev are valid pointers and that @adev * actually corresponds to @dev before using this function. */ -int acpi_device_power_state(struct device *dev, struct acpi_device *adev, - u32 target_state, int d_max_in, int *d_min_p) +static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, + u32 target_state, int d_max_in, int *d_min_p) { char acpi_method[] = "_SxD"; unsigned long long d_min, d_max; @@ -501,7 +501,6 @@ int acpi_device_power_state(struct device *dev, struct acpi_device *adev, } return d_max; } -EXPORT_SYMBOL_GPL(acpi_device_power_state); /** * acpi_pm_device_sleep_state - Get preferred power state of ACPI device. @@ -523,8 +522,8 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p, int d_max_in) return -ENODEV; } - return acpi_device_power_state(dev, adev, acpi_target_system_state(), - d_max_in, d_min_p); + return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(), + d_max_in, d_min_p); } EXPORT_SYMBOL(acpi_pm_device_sleep_state); @@ -680,8 +679,8 @@ static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev, if (!acpi_device_power_manageable(adev)) return 0; - power_state = acpi_device_power_state(dev, adev, system_state, - ACPI_STATE_D3, NULL); + power_state = acpi_dev_pm_get_state(dev, adev, system_state, + ACPI_STATE_D3, NULL); if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3) return -EIO; -- cgit v1.2.3 From 4c164ae7d8a7ee1f39b773d97794535c2c193b12 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Sun, 16 Jun 2013 00:37:50 +0200 Subject: ACPI / PM: Replace ACPI_STATE_D3 with ACPI_STATE_D3_COLD in device_pm.c The two symbols ACPI_STATE_D3 and ACPI_STATE_D3_COLD actually represent the same number (4), but ACPI_STATE_D3 is slightly ambigugous, because it may not be clear that it really means D3cold and not D3hot at first sight. Remove that ambiguity from drivers/acpi/device_pm.c by making it use ACPI_STATE_D3_COLD everywhere instead of ACPI_STATE_D3. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/device_pm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 26d3fd718a04..afc808e93855 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -424,7 +424,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, unsigned long long d_min, d_max; bool wakeup = false; - if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3) + if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD) return -EINVAL; if (d_max_in > ACPI_STATE_D3_HOT) { @@ -443,7 +443,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, * the lowest limit with the specified one. */ d_min = ACPI_STATE_D0; - d_max = ACPI_STATE_D3; + d_max = ACPI_STATE_D3_COLD; /* * If present, _SxD methods return the minimum D-state (highest power @@ -680,8 +680,8 @@ static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev, return 0; power_state = acpi_dev_pm_get_state(dev, adev, system_state, - ACPI_STATE_D3, NULL); - if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3) + ACPI_STATE_D3_COLD, NULL); + if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3_COLD) return -EIO; return acpi_device_set_power(adev, power_state); -- cgit v1.2.3 From fa1675b56537651270e79967b7f1ee4202c83bf6 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Sun, 16 Jun 2013 00:37:59 +0200 Subject: ACPI / PM: Rework and clean up acpi_dev_pm_get_state() The acpi_dev_pm_get_state() function defined in device_pm.c is quite convoluted, which isn't really necessary, and it doesn't validate the values returned by the ACPI methods executed by it appropriately. To address these shortcomings modify it in the following way. (1) Make its return value only mean whether or not it succeeded and pass the device power states determined by it through pointers. (2) Drop the d_max_in argument, used by only one of its callers, from it, and move the code related to d_max_in into that caller, acpi_pm_device_sleep_state(). (3) Make it always check the return value of acpi_evaluate_integer() and handle failures as appropriate. Moreover, make it check if the values returned by the executed ACPI methods are not out of range. (4) Make it check if the values returned by the executed ACPI methods represent valid power states of the given device and handle situations in which that's not the case gracefully. Also update the kerneldoc comments of acpi_dev_pm_get_state() and acpi_pm_device_sleep_state() to reflect the code changes. Signed-off-by: Rafael J. Wysocki --- drivers/acpi/device_pm.c | 158 +++++++++++++++++++++++++++-------------------- 1 file changed, 92 insertions(+), 66 deletions(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index afc808e93855..fd363b57a596 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -403,44 +403,37 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup); * @dev: Device whose preferred target power state to return. * @adev: ACPI device node corresponding to @dev. * @target_state: System state to match the resultant device state. - * @d_max_in: Deepest low-power state to take into consideration. - * @d_min_p: Location to store the upper limit of the allowed states range. - * Return value: Preferred power state of the device on success, -ENODEV - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure + * @d_min_p: Location to store the highest power state available to the device. + * @d_max_p: Location to store the lowest power state available to the device. * - * Find the lowest power (highest number) ACPI device power state that the - * device can be in while the system is in the state represented by - * @target_state. If @d_min_p is set, the highest power (lowest number) device - * power state that @dev can be in for the given system sleep state is stored - * at the location pointed to by it. + * Find the lowest power (highest number) and highest power (lowest number) ACPI + * device power states that the device can be in while the system is in the + * state represented by @target_state. Store the integer numbers representing + * those stats in the memory locations pointed to by @d_max_p and @d_min_p, + * respectively. * * Callers must ensure that @dev and @adev are valid pointers and that @adev * actually corresponds to @dev before using this function. + * + * Returns 0 on success or -ENODATA when one of the ACPI methods fails or + * returns a value that doesn't make sense. The memory locations pointed to by + * @d_max_p and @d_min_p are only modified on success. */ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, - u32 target_state, int d_max_in, int *d_min_p) + u32 target_state, int *d_min_p, int *d_max_p) { - char acpi_method[] = "_SxD"; - unsigned long long d_min, d_max; + char method[] = { '_', 'S', '0' + target_state, 'D', '\0' }; + acpi_handle handle = adev->handle; + unsigned long long ret; + int d_min, d_max; bool wakeup = false; + acpi_status status; - if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD) - return -EINVAL; - - if (d_max_in > ACPI_STATE_D3_HOT) { - enum pm_qos_flags_status stat; - - stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF); - if (stat == PM_QOS_FLAGS_ALL) - d_max_in = ACPI_STATE_D3_HOT; - } - - acpi_method[2] = '0' + target_state; /* - * If the sleep state is S0, the lowest limit from ACPI is D3, - * but if the device has _S0W, we will use the value from _S0W - * as the lowest limit from ACPI. Finally, we will constrain - * the lowest limit with the specified one. + * If the system state is S0, the lowest power state the device can be + * in is D3cold, unless the device has _S0W and is supposed to signal + * wakeup, in which case the return value of _S0W has to be used as the + * lowest power state available to the device. */ d_min = ACPI_STATE_D0; d_max = ACPI_STATE_D3_COLD; @@ -449,12 +442,30 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, * If present, _SxD methods return the minimum D-state (highest power * state) we can use for the corresponding S-states. Otherwise, the * minimum D-state is D0 (ACPI 3.x). - * - * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer - * provided -- that's our fault recovery, we ignore retval. */ if (target_state > ACPI_STATE_S0) { - acpi_evaluate_integer(adev->handle, acpi_method, NULL, &d_min); + /* + * We rely on acpi_evaluate_integer() not clobbering the integer + * provided if AE_NOT_FOUND is returned. + */ + ret = d_min; + status = acpi_evaluate_integer(handle, method, NULL, &ret); + if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND) + || ret > ACPI_STATE_D3_COLD) + return -ENODATA; + + /* + * We need to handle legacy systems where D3hot and D3cold are + * the same and 3 is returned in both cases, so fall back to + * D3cold if D3hot is not a valid state. + */ + if (!adev->power.states[ret].flags.valid) { + if (ret == ACPI_STATE_D3_HOT) + ret = ACPI_STATE_D3_COLD; + else + return -ENODATA; + } + d_min = ret; wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid && adev->wakeup.sleep_state >= target_state; } else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) != @@ -470,36 +481,29 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, * can wake the system. _S0W may be valid, too. */ if (wakeup) { - acpi_status status; - - acpi_method[3] = 'W'; - status = acpi_evaluate_integer(adev->handle, acpi_method, NULL, - &d_max); - if (ACPI_FAILURE(status)) { - if (target_state != ACPI_STATE_S0 || - status != AE_NOT_FOUND) + method[3] = 'W'; + status = acpi_evaluate_integer(handle, method, NULL, &ret); + if (status == AE_NOT_FOUND) { + if (target_state > ACPI_STATE_S0) d_max = d_min; - } else if (d_max < d_min) { - /* Warn the user of the broken DSDT */ - printk(KERN_WARNING "ACPI: Wrong value from %s\n", - acpi_method); - /* Sanitize it */ - d_min = d_max; + } else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) { + /* Fall back to D3cold if ret is not a valid state. */ + if (!adev->power.states[ret].flags.valid) + ret = ACPI_STATE_D3_COLD; + + d_max = ret > d_min ? ret : d_min; + } else { + return -ENODATA; } } - if (d_max_in < d_min) - return -EINVAL; if (d_min_p) *d_min_p = d_min; - /* constrain d_max with specified lowest limit (max number) */ - if (d_max > d_max_in) { - for (d_max = d_max_in; d_max > d_min; d_max--) { - if (adev->power.states[d_max].flags.valid) - break; - } - } - return d_max; + + if (d_max_p) + *d_max_p = d_max; + + return 0; } /** @@ -508,7 +512,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev, * @d_min_p: Location to store the upper limit of the allowed states range. * @d_max_in: Deepest low-power state to take into consideration. * Return value: Preferred power state of the device on success, -ENODEV - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure + * if there's no 'struct acpi_device' for @dev, -EINVAL if @d_max_in is + * incorrect, or -ENODATA on ACPI method failure. * * The caller must ensure that @dev is valid before using this function. */ @@ -516,14 +521,39 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p, int d_max_in) { acpi_handle handle = DEVICE_ACPI_HANDLE(dev); struct acpi_device *adev; + int ret, d_max; + + if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD) + return -EINVAL; + + if (d_max_in > ACPI_STATE_D3_HOT) { + enum pm_qos_flags_status stat; + + stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF); + if (stat == PM_QOS_FLAGS_ALL) + d_max_in = ACPI_STATE_D3_HOT; + } if (!handle || acpi_bus_get_device(handle, &adev)) { dev_dbg(dev, "ACPI handle without context in %s!\n", __func__); return -ENODEV; } - return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(), - d_max_in, d_min_p); + ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(), + d_min_p, &d_max); + if (ret) + return ret; + + if (d_max_in < *d_min_p) + return -EINVAL; + + if (d_max > d_max_in) { + for (d_max = d_max_in; d_max > *d_min_p; d_max--) { + if (adev->power.states[d_max].flags.valid) + break; + } + } + return d_max; } EXPORT_SYMBOL(acpi_pm_device_sleep_state); @@ -674,17 +704,13 @@ struct acpi_device *acpi_dev_pm_get_node(struct device *dev) static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev, u32 system_state) { - int power_state; + int ret, state; if (!acpi_device_power_manageable(adev)) return 0; - power_state = acpi_dev_pm_get_state(dev, adev, system_state, - ACPI_STATE_D3_COLD, NULL); - if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3_COLD) - return -EIO; - - return acpi_device_set_power(adev, power_state); + ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state); + return ret ? ret : acpi_device_set_power(adev, state); } /** -- cgit v1.2.3 From 9b5c7a5a977a330ffaf83c4d383ba247c74c800f Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 27 Jun 2013 14:01:02 +0200 Subject: ACPI / PM: Fix possible NULL pointer deref in acpi_pm_device_sleep_state() After commit fa1675b (ACPI / PM: Rework and clean up acpi_dev_pm_get_state()) a NULL pointer dereference will take place if NULL is passed to acpi_pm_device_sleep_state() as the second argument. Fix that by avoiding to use the pointer that may be NULL until it's necessary to store a return value at the location pointed to by it (if not NULL). Reported-and-tested-by: Aaron Lu Signed-off-by: Rafael J. Wysocki --- drivers/acpi/device_pm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index fd363b57a596..4c56dc830ebc 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -521,7 +521,7 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p, int d_max_in) { acpi_handle handle = DEVICE_ACPI_HANDLE(dev); struct acpi_device *adev; - int ret, d_max; + int ret, d_min, d_max; if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD) return -EINVAL; @@ -540,19 +540,23 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p, int d_max_in) } ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(), - d_min_p, &d_max); + &d_min, &d_max); if (ret) return ret; - if (d_max_in < *d_min_p) + if (d_max_in < d_min) return -EINVAL; if (d_max > d_max_in) { - for (d_max = d_max_in; d_max > *d_min_p; d_max--) { + for (d_max = d_max_in; d_max > d_min; d_max--) { if (adev->power.states[d_max].flags.valid) break; } } + + if (d_min_p) + *d_min_p = d_min; + return d_max; } EXPORT_SYMBOL(acpi_pm_device_sleep_state); -- cgit v1.2.3 From 91bdad0b6237c25a7bf8fd4604d0cc64a2005a23 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 4 Jul 2013 13:22:11 +0200 Subject: ACPI / PM: Fix corner case in acpi_bus_update_power() The role of acpi_bus_update_power() is to update the given ACPI device object's power.state field to reflect the current physical state of the device (as inferred from the configuration of power resources and _PSC, if available). For this purpose it calls acpi_device_set_power() that should update the power resources' reference counters and set power.state as appropriate. However, that doesn't work if the "new" state is D1, D2 or D3hot and the the current value of power.state means D3cold, because in that case acpi_device_set_power() will refuse to transition the device from D3cold to non-D0. To address this problem, make acpi_bus_update_power() call acpi_power_transition() directly to update the power resources' reference counters and only use acpi_device_set_power() to put the device into D0 if the current physical state of it cannot be determined. Signed-off-by: Rafael J. Wysocki Cc: 3.9+ --- drivers/acpi/device_pm.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) (limited to 'drivers/acpi/device_pm.c') diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index e9e8bb24785b..4ab807dc8518 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -324,14 +324,27 @@ int acpi_bus_update_power(acpi_handle handle, int *state_p) if (result) return result; - if (state == ACPI_STATE_UNKNOWN) + if (state == ACPI_STATE_UNKNOWN) { state = ACPI_STATE_D0; - - result = acpi_device_set_power(device, state); - if (!result && state_p) + result = acpi_device_set_power(device, state); + if (result) + return result; + } else { + if (device->power.flags.power_resources) { + /* + * We don't need to really switch the state, bu we need + * to update the power resources' reference counters. + */ + result = acpi_power_transition(device, state); + if (result) + return result; + } + device->power.state = state; + } + if (state_p) *state_p = state; - return result; + return 0; } EXPORT_SYMBOL_GPL(acpi_bus_update_power); -- cgit v1.2.3