ACK/cmnt: [SRU] [Artful/linux-oem] [PATCH 1/1] ACPI / PM: Allow deeper wakeup power states with no _SxD nor _SxW

Kai-Heng Feng kai.heng.feng at canonical.com
Wed Mar 28 15:58:16 UTC 2018


Stefan Bader <stefan.bader at canonical.com> wrote:

> On 28.03.2018 10:55, Kai-Heng Feng wrote:
>> From: Daniel Drake <drake at endlessm.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1759511
>>
>> acpi_dev_pm_get_state() is used to determine the range of allowable
>> device power states when going into S3 suspend. This is implemented
>> by executing the _S3D and _S3W ACPI methods.
>>
>> Linux follows the ACPI spec behaviour in that when _S3D is implemented
>> and _S3W is not, Linux will not go into a power state deeper than the one
>> returned by _S3D for a wakeup-enabled device.
>>
>> However, this same logic is being applied to the case when neither
>> _S3D nor _S3W are present, and the result is that this function
>> decides that the device must stay in D0 (fully on) state.
>>
>> This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and
>> _S3W are not present, so the USB controller is left in the D0 running
>> state during S3, and hence it is unable to generate a PME# wake event.
>>
>> The ACPI spec is unclear on which power states are permissable for
>> wakeup-enabled devices when both _S3D and _S3W are missing.
>> However, USB wakeups work fine on these platforms under Windows, where
>> device manager shows that they are using D3 device state for the USB
>> controller in S3.
>>
>> I assume that the "max = min" clamping done by the code here is
>> specifically written for the _S3D but no _S3W case. By making the
>> code true to those conditions, avoiding them on these platforms,
>> the controller will be put into D3 state and USB wakeups start working.
>>
>> Additionally I feel that this change makes the code more directly
>> mirror the wording of the ACPI spec and it's associated lack of clarity.
>>
>> Thanks to Mathias Nyman for pointing us in the right direction.
>>
>> Signed-off-by: Daniel Drake <drake at endlessm.com>
>> Link:  
>> http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=Bv1XhXV_gY3bziOo_NPKA@mail.gmail.com
>>
>> https://phabricator.endlessm.com/T21410
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
>> (cherry picked from commit bf8c6184e0c3d4d5e005e085e9f96f478a267b20  
>> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git  
>> linux-next)
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
>
>> ---
>
> Since this is pulled from linux-next, I would suspect this is also wanted  
> in
> Bionic, right?

This patch should be included for all linux version.
I planed to ask linux-stable to include the patch once it's in Linus' tree,  
so we will get this patch through stable update.

Kai-Heng

>
>> drivers/acpi/device_pm.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index 2ed6935d4483..5bb754b1ccb2 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -534,6 +534,7 @@ static int acpi_dev_pm_get_state(struct device *dev,  
>> struct acpi_device *adev,
>>  	unsigned long long ret;
>>  	int d_min, d_max;
>>  	bool wakeup = false;
>> +	bool has_sxd = false;
>>  	acpi_status status;
>>
>>  	/*
>> @@ -572,6 +573,10 @@ static int acpi_dev_pm_get_state(struct device  
>> *dev, struct acpi_device *adev,
>>  			else
>>  				return -ENODATA;
>>  		}
>> +
>> +		if (status == AE_OK)
>> +			has_sxd = true;
>> +
>>  		d_min = ret;
>>  		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
>>  			&& adev->wakeup.sleep_state >= target_state;
>> @@ -591,7 +596,11 @@ static int acpi_dev_pm_get_state(struct device  
>> *dev, struct acpi_device *adev,
>>  		method[3] = 'W';
>>  		status = acpi_evaluate_integer(handle, method, NULL, &ret);
>>  		if (status == AE_NOT_FOUND) {
>> -			if (target_state > ACPI_STATE_S0)
>> +			/* No _SxW. In this case, the ACPI spec says that we
>> +			 * must not go into any power state deeper than the
>> +			 * value returned from _SxD.
>> +			 */
>> +			if (has_sxd && target_state > ACPI_STATE_S0)
>>  				d_max = d_min;
>>  		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
>>  			/* Fall back to D3cold if ret is not a valid state. */






More information about the kernel-team mailing list