[PATCH] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[PATCH] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[Daniel Drake]

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@endlessm.com>
Link: http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=Bv1XhXV_gY3bziOo_NPKA@mail.gmail.com
---
 drivers/acpi/device_pm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index c4d0a1c912f0..b945e37bcac0 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -543,6 +543,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;
+	acpi_status sxd_status;
 	acpi_status status;
 
 	/*
@@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		 * 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)
+		sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
+		if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
 		    || ret > ACPI_STATE_D3_COLD)
 			return -ENODATA;
 
@@ -599,7 +600,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 (sxd_status == AE_OK && 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. */
-- 
2.14.1

Re: [PATCH] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5886 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Drake/ACPI-PM-allow-deeper-wakeup-power-states-with-no-_SxD-nor-_SxW/20180319-185209
config: i386-randconfig-x074-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/acpi/device_pm.c: In function 'acpi_dev_pm_get_state':
>> drivers/acpi/device_pm.c:607:7: warning: 'sxd_status' may be used uninitialized in this function [-Wmaybe-uninitialized]
       if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
          ^

vim +/sxd_status +607 drivers/acpi/device_pm.c

   516	
   517	/**
   518	 * acpi_dev_pm_get_state - Get preferred power state of ACPI device.
   519	 * @dev: Device whose preferred target power state to return.
   520	 * @adev: ACPI device node corresponding to @dev.
   521	 * @target_state: System state to match the resultant device state.
   522	 * @d_min_p: Location to store the highest power state available to the device.
   523	 * @d_max_p: Location to store the lowest power state available to the device.
   524	 *
   525	 * Find the lowest power (highest number) and highest power (lowest number) ACPI
   526	 * device power states that the device can be in while the system is in the
   527	 * state represented by @target_state.  Store the integer numbers representing
   528	 * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
   529	 * respectively.
   530	 *
   531	 * Callers must ensure that @dev and @adev are valid pointers and that @adev
   532	 * actually corresponds to @dev before using this function.
   533	 *
   534	 * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
   535	 * returns a value that doesn't make sense.  The memory locations pointed to by
   536	 * @d_max_p and @d_min_p are only modified on success.
   537	 */
   538	static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
   539					 u32 target_state, int *d_min_p, int *d_max_p)
   540	{
   541		char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
   542		acpi_handle handle = adev->handle;
   543		unsigned long long ret;
   544		int d_min, d_max;
   545		bool wakeup = false;
   546		acpi_status sxd_status;
   547		acpi_status status;
   548	
   549		/*
   550		 * If the system state is S0, the lowest power state the device can be
   551		 * in is D3cold, unless the device has _S0W and is supposed to signal
   552		 * wakeup, in which case the return value of _S0W has to be used as the
   553		 * lowest power state available to the device.
   554		 */
   555		d_min = ACPI_STATE_D0;
   556		d_max = ACPI_STATE_D3_COLD;
   557	
   558		/*
   559		 * If present, _SxD methods return the minimum D-state (highest power
   560		 * state) we can use for the corresponding S-states.  Otherwise, the
   561		 * minimum D-state is D0 (ACPI 3.x).
   562		 */
   563		if (target_state > ACPI_STATE_S0) {
   564			/*
   565			 * We rely on acpi_evaluate_integer() not clobbering the integer
   566			 * provided if AE_NOT_FOUND is returned.
   567			 */
   568			ret = d_min;
   569			sxd_status = acpi_evaluate_integer(handle, method, NULL, &ret);
   570			if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
   571			    || ret > ACPI_STATE_D3_COLD)
   572				return -ENODATA;
   573	
   574			/*
   575			 * We need to handle legacy systems where D3hot and D3cold are
   576			 * the same and 3 is returned in both cases, so fall back to
   577			 * D3cold if D3hot is not a valid state.
   578			 */
   579			if (!adev->power.states[ret].flags.valid) {
   580				if (ret == ACPI_STATE_D3_HOT)
   581					ret = ACPI_STATE_D3_COLD;
   582				else
   583					return -ENODATA;
   584			}
   585			d_min = ret;
   586			wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
   587				&& adev->wakeup.sleep_state >= target_state;
   588		} else {
   589			wakeup = adev->wakeup.flags.valid;
   590		}
   591	
   592		/*
   593		 * If _PRW says we can wake up the system from the target sleep state,
   594		 * the D-state returned by _SxD is sufficient for that (we assume a
   595		 * wakeup-aware driver if wake is set).  Still, if _SxW exists
   596		 * (ACPI 3.x), it should return the maximum (lowest power) D-state that
   597		 * can wake the system.  _S0W may be valid, too.
   598		 */
   599		if (wakeup) {
   600			method[3] = 'W';
   601			status = acpi_evaluate_integer(handle, method, NULL, &ret);
   602			if (status == AE_NOT_FOUND) {
   603				/* No _SxW. In this case, the ACPI spec says that we
   604				 * must not go into any power state deeper than the
   605				 * value returned from _SxD.
   606				 */
 > 607				if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
   608					d_max = d_min;
   609			} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
   610				/* Fall back to D3cold if ret is not a valid state. */
   611				if (!adev->power.states[ret].flags.valid)
   612					ret = ACPI_STATE_D3_COLD;
   613	
   614				d_max = ret > d_min ? ret : d_min;
   615			} else {
   616				return -ENODATA;
   617			}
   618		}
   619	
   620		if (d_min_p)
   621			*d_min_p = d_min;
   622	
   623		if (d_max_p)
   624			*d_max_p = d_max;
   625	
   626		return 0;
   627	}
   628	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27536 bytes --]