On Thursday, June 4, 2020 2:15:41 PM CEST Hans de Goede wrote:
> 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)
> }
This is wrong, because the _STA return value is required to return the
current state of the power resource, which evidently is not the case
here.
>
> 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.
Now that is an important piece of information, because it basically means that
on the affected platform(s) the _PR3 is present only to indicate D3cold support.
It also means that the device doesn't really support D3hot and _PS3 puts it
straight into D3cold.
In the face of this, I don't think it is safe to use power resources if the
D0 list is empty (either _PR0 is not present or it returns an empty package).
> 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.
Well, that's sad. :-/
> > 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.
No, it wouldn't, because the _PR3 power resources above are not functional.
> > Let me cut a patch for that.
>
> Ok, let me know when you have a patch ready then I will test it.
Sure, please find appended.
That just avoids using power resources at all if the D0 list is empty.
---
drivers/acpi/scan.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -919,12 +919,9 @@ static void acpi_bus_init_power_state(st
if (buffer.length && package
&& package->type == ACPI_TYPE_PACKAGE
- && package->package.count) {
- int err = acpi_extract_power_resources(package, 0,
- &ps->resources);
- if (!err)
- device->power.flags.power_resources = 1;
- }
+ && package->package.count)
+ acpi_extract_power_resources(package, 0, &ps->resources);
+
ACPI_FREE(buffer.pointer);
}
@@ -970,6 +967,14 @@ static void acpi_bus_get_power_flags(str
for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++)
acpi_bus_init_power_state(device, i);
+ /*
+ * Use power resources only if the D0 list of them is populated, because
+ * some platforms only provide _PR3 to indicate D3cold support and in
+ * those cases the power resources list returned by _PR3 may be bogus.
+ */
+ if (!list_empty(&device->power.states[ACPI_STATE_D0].resources))
+ device->power.flags.power_resources = 1;
+
INIT_LIST_HEAD(&device->power.states[ACPI_STATE_D3_COLD].resources);
if (!list_empty(&device->power.states[ACPI_STATE_D3_HOT].resources))
device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
