Hi,
On 6/4/20 1:22 PM, Rafael J. Wysocki wrote:
On Wed, Jun 3, 2020 at 9:47 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Some devices do not have a power-resource-list for D0, but do have a
power-resource-lists for e.g. D3 (_PR3).
This looks like a bug in the firmware.
The DSDT for these devices definitely has somw warts, see
below.
TBH I'm not sure of the whole inferring state from
power-resource-lists is the best idea. I think just going
for UNKNOWN as initial state would be better. The issue is
that getting the initial state wrong will lead to either
skipping _PS3 (as is happening here) or _PS0 on the first
transition to the matching state which can have undesirable
side-effects, where as just running _PS0/PS3 for a second
time _usually_ should be fine.
Anyways I know we have the inferring code for a long time,
and I guess it mostly does its job fine...
It is hard to imagine a design in which some power resources only need
to be "on" in the D3hot power state of a device and not in D0 (which
is implied by the lack of _PR0 and the presence of _PR3).
Right, except that the listed resource is a dummy power
resource, here is the AML for the USB power-resource which
is the only resource in the _PR3 list:
PowerResource (USBC, 0x00, 0x0000)
{
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}
Method (_ON, 0, NotSerialized) // _ON_: Power On
{
}
Method (_OFF, 0, NotSerialized) // _OFF: Power Off
{
}
}
So not having this in _PR0 does not matter because the
whole thing is a dummy implementation.
If you want the whole DSDT, the DSDT of one of the affected devices
is attached here: https://bugzilla.kernel.org/show_bug.cgi?id=69011
Note it seems that pretty much any Bay Trail tablet version based
device suffers from this.
So when the device goes from D0 to D3hot, we are expected to turn some
power resources "on"? What sense does this make?
I'm guessing that this only works at all, because we only use D0 and
D3cold with the affected devices and _PS0 simply turns the power
resource(s) in question on.
On these devices the "if (device->power.flags.power_resources)" check
in acpi_device_get_power() succeeds because of the presence of the _PR3
resources, so the code used to try and infer the power-state.
In this case since there is no power-resource-list for D0, we can never
infer that the device is in D0 even though it very well might be in D0.
This results in the code inferring that the device is in D3HOT and on
the first suspend acpi_device_set_power() skips calling _PS3 for the
device because it thinks the device is already in D3.
An example of a family of devices which are affected by this are
Bay Trail based devices. The ACPI device for the XHCI controller on
these devices does not have a _PR0 method, but it does have a _PR3
method. The problem described above causes the XHCI controller's _PS3
method not getting called on the first suspend of the device, which
causes these devices to not reach the S0i3 power-state during suspend.
Since we cannot infer if the device is in D0 or not when there is no
power-resource-list for D0, the best thing to do is to change
acpi_power_get_inferred_state() to return ACPI_STATE_UNKNOWN in this
case.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=205057
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/acpi/power.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index fe1e7bc91a5e..db54393a077b 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -807,6 +807,17 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
if (!device || !state)
return -EINVAL;
+ /*
+ * Some devices do not have a power-resource-list for D0, but do
+ * have a power-resource-lists for e.g. D3 so we do end up here.
+ * In this case we can never infer that the device is in D0 even
+ * though it might very well be in D0, so return ACPI_STATE_UNKNOWN.
+ */
+ if (list_empty(&device->power.states[ACPI_STATE_D0].resources)) {
+ *state = ACPI_STATE_UNKNOWN;
+ return 0;
+ }
Well, this makes things work on the particular affected platform, but
that seems to be just by accident, because _PS0 does something special
on it.
IMO this needs to be addressed elsewhere and in a different way.
Namely, it looks like if _PR0 is not present (or its return package is
empty), but _PR3 is present, we should use the _PR3 list of power
resources for D0 as well as for D3hot.
That should work too.
Let me cut a patch for that.
Ok, let me know when you have a patch ready then I will test it.
Regards,
Hans
