[PATCH 0/2] ACPI: PM: Fix two issues in acpi_device_set_power()

[PATCH 0/2] ACPI: PM: Fix two issues in acpi_device_set_power()

[Rafael J. Wysocki]

Hi All,

There are two, arguably minor, issues in acpi_device_set_power() that have been
introduced while making the code follow the ACPI 6 recommendations.

One of them is that, if the device is in D3hot, transitioning it into D3cold only
involves dropping references to some power resources and it should not
involve evaluating _PS3 which may happen currently.

The second one is that it sometimes is necessary to update the power state
of a device to D0 even if it was put into D0 previously, so that needs to be
possible.

Please refer to the patch changelogs for details.

Thanks,
Rafael

[PATCH 1/2] ACPI: PM: Avoid evaluating _PS3 on transitions from D3hot to D3cold

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the power state of a device with ACPI PM is changed from D3hot to
D3cold, it merely is a matter of dropping references to additional
power resources (specifically, those in the list returned by _PR3),
and the _PS3 method should not be invoked for the device then (as
it has already been evaluated during the previous transition to
D3hot).

Fixes: 20dacb71ad28 (ACPI / PM: Rework device power management to follow ACPI 6)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -215,9 +215,15 @@ int acpi_device_set_power(struct acpi_de
 			return -ENODEV;
 		}
 
-		result = acpi_dev_pm_explicit_set(device, state);
-		if (result)
-			goto end;
+		/*
+		 * If the device goes from D3hot to D3cold, _PS3 has been
+		 * evaluated for it already, so skip it in that case.
+		 */
+		if (device->power.state < ACPI_STATE_D3_HOT) {
+			result = acpi_dev_pm_explicit_set(device, state);
+			if (result)
+				goto end;
+		}
 
 		if (device->power.flags.power_resources)
 			result = acpi_power_transition(device, target_state);

[PATCH 2/2] ACPI: PM: Allow transitions to D0 to occur in special cases

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If a device with ACPI PM is left in D0 during a system-wide
transition to the S3 (suspend-to-RAM) or S4 (hibernation) sleep
state, the actual state of the device need not be D0 during resume
from it, although its power.state value will still reflect D0 (that
is, the power state from before the system-wide transition).

In that case, the acpi_device_set_power() call made to ensure that
the power state of the device will be D0 going forward has no effect,
because the new state (D0) is equal to the one reflected by the
device's power.state value.  That does not affect power resources,
which are taken care of by acpi_resume_power_resources() called from
acpi_pm_finish() during resume from system-wide sleep states, but it
still may be necessary to invoke _PS0 for the device on top of that
in order to finalize its transition to D0.

For this reason, modify acpi_device_set_power() to allow transitions
to D0 to occur even if D0 is the current power state of the device
according to its power.state value.

That will not affect power resources, which are assumed to be in
the right configuration already (as reflected by the current values
of their reference counters), but it may cause _PS0 to be evaluated
for the device.  However, evaluating _PS0 for a device already in D0
may lead to confusion in general, so invoke _PSC (if present) to
check the device's current power state upfront and only evaluate
_PS0 for it if _PSC has returned a power state different from D0.
[If _PSC is not present or the evaluation of it fails, the power
state of the device is assumed to be D0 at this point.]

Fixes: 20dacb71ad28 (ACPI / PM: Rework device power management to follow ACPI 6)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -45,6 +45,19 @@ const char *acpi_power_state_string(int
 	}
 }
 
+static int acpi_dev_pm_explicit_get(struct acpi_device *device, int *state)
+{
+	unsigned long long psc;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(device->handle, "_PSC", NULL, &psc);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	*state = psc;
+	return 0;
+}
+
 /**
  * acpi_device_get_power - Get power state of an ACPI device.
  * @device: Device to get the power state of.
@@ -57,6 +70,7 @@ const char *acpi_power_state_string(int
 int acpi_device_get_power(struct acpi_device *device, int *state)
 {
 	int result = ACPI_STATE_UNKNOWN;
+	int error;
 
 	if (!device || !state)
 		return -EINVAL;
@@ -73,18 +87,16 @@ int acpi_device_get_power(struct acpi_de
 	 * if available.
 	 */
 	if (device->power.flags.power_resources) {
-		int error = acpi_power_get_inferred_state(device, &result);
+		error = acpi_power_get_inferred_state(device, &result);
 		if (error)
 			return error;
 	}
 	if (device->power.flags.explicit_get) {
-		acpi_handle handle = device->handle;
-		unsigned long long psc;
-		acpi_status status;
+		int psc;
 
-		status = acpi_evaluate_integer(handle, "_PSC", NULL, &psc);
-		if (ACPI_FAILURE(status))
-			return -ENODEV;
+		error = acpi_dev_pm_explicit_get(device, &psc);
+		if (error)
+			return error;
 
 		/*
 		 * The power resources settings may indicate a power state
@@ -152,7 +164,8 @@ int acpi_device_set_power(struct acpi_de
 
 	/* Make sure this is a valid target state */
 
-	if (state == device->power.state) {
+	/* There is a special case for D0 addressed below. */
+	if (state > ACPI_STATE_D0 && state == device->power.state) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
 				  device->pnp.bus_id,
 				  acpi_power_state_string(state)));
@@ -214,6 +227,30 @@ int acpi_device_set_power(struct acpi_de
 			if (result)
 				goto end;
 		}
+
+		if (device->power.state == ACPI_STATE_D0) {
+			int psc;
+
+			/* Nothing to do here if _PSC is not present. */
+			if (!device->power.flags.explicit_get)
+				return 0;
+
+			/*
+			 * The power state of the device was set to D0 last
+			 * time, but that might have happened before a
+			 * system-wide transition involving the platform
+			 * firmware, so it may be necessary to evaluate _PS0
+			 * for the device here.  However, use extra care here
+			 * and evaluate _PSC to check the device's current power
+			 * state, and only invoke _PS0 if the evaluation of _PSC
+			 * is successful and it returns a power state different
+			 * from D0.
+			 */
+			result = acpi_dev_pm_explicit_get(device, &psc);
+			if (result || psc == ACPI_STATE_D0)
+				return 0;
+		}
+
 		result = acpi_dev_pm_explicit_set(device, ACPI_STATE_D0);
 	}
 

Re: [PATCH 1/2] ACPI: PM: Avoid evaluating _PS3 on transitions from D3hot to D3cold

[Mika Westerberg]

On Tue, Jun 25, 2019 at 02:04:45PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the power state of a device with ACPI PM is changed from D3hot to
> D3cold, it merely is a matter of dropping references to additional
> power resources (specifically, those in the list returned by _PR3),
> and the _PS3 method should not be invoked for the device then (as
> it has already been evaluated during the previous transition to
> D3hot).
> 
> Fixes: 20dacb71ad28 (ACPI / PM: Rework device power management to follow ACPI 6)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] ACPI: PM: Allow transitions to D0 to occur in special cases

[Mika Westerberg]

On Tue, Jun 25, 2019 at 02:06:13PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a device with ACPI PM is left in D0 during a system-wide
> transition to the S3 (suspend-to-RAM) or S4 (hibernation) sleep
> state, the actual state of the device need not be D0 during resume
> from it, although its power.state value will still reflect D0 (that
> is, the power state from before the system-wide transition).
> 
> In that case, the acpi_device_set_power() call made to ensure that
> the power state of the device will be D0 going forward has no effect,
> because the new state (D0) is equal to the one reflected by the
> device's power.state value.  That does not affect power resources,
> which are taken care of by acpi_resume_power_resources() called from
> acpi_pm_finish() during resume from system-wide sleep states, but it
> still may be necessary to invoke _PS0 for the device on top of that
> in order to finalize its transition to D0.
> 
> For this reason, modify acpi_device_set_power() to allow transitions
> to D0 to occur even if D0 is the current power state of the device
> according to its power.state value.
> 
> That will not affect power resources, which are assumed to be in
> the right configuration already (as reflected by the current values
> of their reference counters), but it may cause _PS0 to be evaluated
> for the device.  However, evaluating _PS0 for a device already in D0
> may lead to confusion in general, so invoke _PSC (if present) to
> check the device's current power state upfront and only evaluate
> _PS0 for it if _PSC has returned a power state different from D0.
> [If _PSC is not present or the evaluation of it fails, the power
> state of the device is assumed to be D0 at this point.]
> 
> Fixes: 20dacb71ad28 (ACPI / PM: Rework device power management to follow ACPI 6)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/2] ACPI: PM: Avoid evaluating _PS3 on transitions from D3hot to D3cold

[Rafael J. Wysocki]

On Tuesday, June 25, 2019 4:11:16 PM CEST Mika Westerberg wrote:
> On Tue, Jun 25, 2019 at 02:04:45PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If the power state of a device with ACPI PM is changed from D3hot to
> > D3cold, it merely is a matter of dropping references to additional
> > power resources (specifically, those in the list returned by _PR3),
> > and the _PS3 method should not be invoked for the device then (as
> > it has already been evaluated during the previous transition to
> > D3hot).
> > 
> > Fixes: 20dacb71ad28 (ACPI / PM: Rework device power management to follow ACPI 6)
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 

Thanks!

I've queued this one and the [2/2] with your tag for 5.3.