* [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources
@ 2020-06-03 19:46 Hans de Goede
2020-06-04 11:22 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2020-06-03 19:46 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown
Cc: Hans de Goede, Andy Shevchenko, Mika Westerberg, youling257, linux-acpi
Some devices do not have a power-resource-list for D0, but do have a
power-resource-lists for e.g. D3 (_PR3).
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@redhat.com>
---
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;
+ }
+
/*
* We know a device's inferred power state when all the resources
* required for a given D-state are 'on'.
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources
2020-06-03 19:46 [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources Hans de Goede
@ 2020-06-04 11:22 ` Rafael J. Wysocki
2020-06-04 12:15 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2020-06-04 11:22 UTC (permalink / raw)
To: Hans de Goede
Cc: Rafael J . Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg,
youling257, ACPI Devel Maling List
On Wed, Jun 3, 2020 at 9:47 PM Hans de Goede <hdegoede@redhat.com> 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.
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).
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@redhat.com>
> ---
> 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.
Let me cut a patch for that.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources
2020-06-04 11:22 ` Rafael J. Wysocki
@ 2020-06-04 12:15 ` Hans de Goede
2020-06-04 14:42 ` Rafael J. Wysocki
2020-06-04 17:22 ` [PATCH] ACPI: PM: Avoid using power resources if there are none for D0 Rafael J. Wysocki
0 siblings, 2 replies; 6+ messages in thread
From: Hans de Goede @ 2020-06-04 12:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J . Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg,
youling257, ACPI Devel Maling List
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@redhat.com> 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@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources
2020-06-04 12:15 ` Hans de Goede
@ 2020-06-04 14:42 ` Rafael J. Wysocki
2020-06-04 17:22 ` [PATCH] ACPI: PM: Avoid using power resources if there are none for D0 Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2020-06-04 14:42 UTC (permalink / raw)
To: Hans de Goede
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg,
youling257, ACPI Devel Maling List
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@redhat.com> 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@redhat.com>
> >> ---
> >> 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ACPI: PM: Avoid using power resources if there are none for D0
2020-06-04 12:15 ` Hans de Goede
2020-06-04 14:42 ` Rafael J. Wysocki
@ 2020-06-04 17:22 ` Rafael J. Wysocki
2020-06-06 15:45 ` Hans de Goede
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2020-06-04 17:22 UTC (permalink / raw)
To: Hans de Goede, ACPI Devel Maling List
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg,
youling257, LKML, Linux PM
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
As recently reported, some platforms provide a list of power
resources for device power state D3hot, through the _PR3 object,
but they do not provide a list of power resources for device power
state D0.
Among other things, this causes acpi_device_get_power() to return
D3hot as the current state of the device in question if all of the
D3hot power resources are "on", because it sees the power_resources
flag set and calls acpi_power_get_inferred_state() which finds that
D3hot is the shallowest power state with all of the associated power
resources turned "on", so that's what it returns. Moreover, that
value takes precedence over the acpi_dev_pm_explicit_get() return
value, because it means a deeper power state. The device may very
well be in D0 physically at that point, however.
Moreover, the presence of _PR3 without _PR0 for a given device
means that only one D3-level power state can be supported by it.
Namely, because there are no power resources to turn "off" when
transitioning the device from D0 into D3cold (which should be
supported since _PR3 is present), the evaluation of _PS3 should
be sufficient to put it straight into D3cold, but this means that
the effect of turning "on" the _PR3 power resources is unclear,
so it is better to avoid doing that altogether. Consequently,
there is no practical way do distinguish D3cold from D3hot for
the device in question and the power states of it can be labeled
so that D3hot is the deepest supported one (and Linux assumes
that putting a device into D3hot via ACPI may cause power to be
removed from it anyway, for legacy reasons).
To work around the problem described above modify the ACPI
enumeration of devices so that power resources are only used
for device power management if the list of D0 power resources
is not empty and make it mart D3cold as supported only if that
is the case and the D3hot list of power resources is not empty
too.
Fixes: ef85bdbec444 ("ACPI / scan: Consolidate extraction of power resources lists")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=205057
Link: https://lore.kernel.org/linux-acpi/20200603194659.185757-1-hdegoede@redhat.com/
Reported-by: Hans de Goede <hdegoede@redhat.com>
Cc: 3.10+ <stable@vger.kernel.org> # 3.10+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/device_pm.c | 2 +-
drivers/acpi/scan.c | 28 +++++++++++++++++++---------
2 files changed, 20 insertions(+), 10 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);
}
@@ -971,14 +968,27 @@ static void acpi_bus_get_power_flags(str
acpi_bus_init_power_state(device, i);
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;
- /* Set defaults for D0 and D3hot states (always valid) */
+ /* Set the defaults for D0 and D3hot (always supported). */
device->power.states[ACPI_STATE_D0].flags.valid = 1;
device->power.states[ACPI_STATE_D0].power = 100;
device->power.states[ACPI_STATE_D3_HOT].flags.valid = 1;
+ /*
+ * Use power resources only if the D0 list of them is populated, because
+ * some platforms may provide _PR3 only to indicate D3cold support and
+ * in those cases the power resources list returned by it may be bogus.
+ */
+ if (!list_empty(&device->power.states[ACPI_STATE_D0].resources)) {
+ device->power.flags.power_resources = 1;
+ /*
+ * D3cold is supported if the D3hot list of power resources is
+ * not empty.
+ */
+ if (!list_empty(&device->power.states[ACPI_STATE_D3_HOT].resources))
+ device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
+ }
+
if (acpi_bus_init_power(device))
device->flags.power_manageable = 0;
}
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -186,7 +186,7 @@ int acpi_device_set_power(struct acpi_de
* possibly drop references to the power resources in use.
*/
state = ACPI_STATE_D3_HOT;
- /* If _PR3 is not available, use D3hot as the target state. */
+ /* If D3cold is not supported, use D3hot as the target state. */
if (!device->power.states[ACPI_STATE_D3_COLD].flags.valid)
target_state = state;
} else if (!device->power.states[state].flags.valid) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI: PM: Avoid using power resources if there are none for D0
2020-06-04 17:22 ` [PATCH] ACPI: PM: Avoid using power resources if there are none for D0 Rafael J. Wysocki
@ 2020-06-06 15:45 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2020-06-06 15:45 UTC (permalink / raw)
To: Rafael J. Wysocki, ACPI Devel Maling List
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg,
youling257, LKML, Linux PM
Hi,
On 6/4/20 7:22 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> As recently reported, some platforms provide a list of power
> resources for device power state D3hot, through the _PR3 object,
> but they do not provide a list of power resources for device power
> state D0.
>
> Among other things, this causes acpi_device_get_power() to return
> D3hot as the current state of the device in question if all of the
> D3hot power resources are "on", because it sees the power_resources
> flag set and calls acpi_power_get_inferred_state() which finds that
> D3hot is the shallowest power state with all of the associated power
> resources turned "on", so that's what it returns. Moreover, that
> value takes precedence over the acpi_dev_pm_explicit_get() return
> value, because it means a deeper power state. The device may very
> well be in D0 physically at that point, however.
>
> Moreover, the presence of _PR3 without _PR0 for a given device
> means that only one D3-level power state can be supported by it.
> Namely, because there are no power resources to turn "off" when
> transitioning the device from D0 into D3cold (which should be
> supported since _PR3 is present), the evaluation of _PS3 should
> be sufficient to put it straight into D3cold, but this means that
> the effect of turning "on" the _PR3 power resources is unclear,
> so it is better to avoid doing that altogether. Consequently,
> there is no practical way do distinguish D3cold from D3hot for
> the device in question and the power states of it can be labeled
> so that D3hot is the deepest supported one (and Linux assumes
> that putting a device into D3hot via ACPI may cause power to be
> removed from it anyway, for legacy reasons).
>
> To work around the problem described above modify the ACPI
> enumeration of devices so that power resources are only used
> for device power management if the list of D0 power resources
> is not empty and make it mart D3cold as supported only if that
> is the case and the D3hot list of power resources is not empty
> too.
>
> Fixes: ef85bdbec444 ("ACPI / scan: Consolidate extraction of power resources lists")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=205057
> Link: https://lore.kernel.org/linux-acpi/20200603194659.185757-1-hdegoede@redhat.com/
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Cc: 3.10+ <stable@vger.kernel.org> # 3.10+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Thank you.
I've tested this and I can confirm that it fixes the issue:
Tested-by: Hans de Goede <hdegoede@redhat.com>
I've also looked at the code and it looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/acpi/device_pm.c | 2 +-
> drivers/acpi/scan.c | 28 +++++++++++++++++++---------
> 2 files changed, 20 insertions(+), 10 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);
> }
>
> @@ -971,14 +968,27 @@ static void acpi_bus_get_power_flags(str
> acpi_bus_init_power_state(device, i);
>
> 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;
>
> - /* Set defaults for D0 and D3hot states (always valid) */
> + /* Set the defaults for D0 and D3hot (always supported). */
> device->power.states[ACPI_STATE_D0].flags.valid = 1;
> device->power.states[ACPI_STATE_D0].power = 100;
> device->power.states[ACPI_STATE_D3_HOT].flags.valid = 1;
>
> + /*
> + * Use power resources only if the D0 list of them is populated, because
> + * some platforms may provide _PR3 only to indicate D3cold support and
> + * in those cases the power resources list returned by it may be bogus.
> + */
> + if (!list_empty(&device->power.states[ACPI_STATE_D0].resources)) {
> + device->power.flags.power_resources = 1;
> + /*
> + * D3cold is supported if the D3hot list of power resources is
> + * not empty.
> + */
> + if (!list_empty(&device->power.states[ACPI_STATE_D3_HOT].resources))
> + device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> + }
> +
> if (acpi_bus_init_power(device))
> device->flags.power_manageable = 0;
> }
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -186,7 +186,7 @@ int acpi_device_set_power(struct acpi_de
> * possibly drop references to the power resources in use.
> */
> state = ACPI_STATE_D3_HOT;
> - /* If _PR3 is not available, use D3hot as the target state. */
> + /* If D3cold is not supported, use D3hot as the target state. */
> if (!device->power.states[ACPI_STATE_D3_COLD].flags.valid)
> target_state = state;
> } else if (!device->power.states[state].flags.valid) {
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-06 15:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 19:46 [PATCH] ACPI / PM: Do not infer power-state if there are no D0 power-resources Hans de Goede
2020-06-04 11:22 ` Rafael J. Wysocki
2020-06-04 12:15 ` Hans de Goede
2020-06-04 14:42 ` Rafael J. Wysocki
2020-06-04 17:22 ` [PATCH] ACPI: PM: Avoid using power resources if there are none for D0 Rafael J. Wysocki
2020-06-06 15:45 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).