On Fri, Jan 29, 2021 at 10:22 PM Sakari Ailus
[off-list ref] wrote:

On Fri, Jan 29, 2021 at 05:57:17PM +0100, Rafael J. Wysocki wrote:

quoted

On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
[off-list ref] wrote:

quoted

Hi Rafael,

Thanks for the comments.

On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:

quoted

On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
[off-list ref] wrote:

quoted

Store a device's desired enumeration power state in struct
acpi_device_power_flags during acpi_device object's initialisation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/scan.c     | 6 ++++++
 include/acpi/acpi_bus.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1d7a02ee45e05..b077c645c9845 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c

@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)

 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+       unsigned long long pre;
+       acpi_status status;
        u32 i;

        /* Presence of _PS0|_PR0 indicates 'power manageable' */

@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
        if (acpi_has_method(device->handle, "_DSW"))
                device->power.flags.dsw_present = 1;

+       status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+       if (ACPI_SUCCESS(status) && !pre)
+               device->power.flags.allow_low_power_probe = 1;

While this is what has been discussed and thanks for taking it into
account, I'm now thinking that it may be cleaner to introduce a new
object to return the deepest power state of the device in which it can
be enumerated, say _DSE (Device State for Enumeration) such that 4
means D3cold, 3 - D3hot and so on, so the above check can be replaced
with something like

status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);

s/_PRE/_DSE/

?

Yes, sorry.

quoted

quoted

if (ACPI_FAILURE(status))

ACPI_SUCCESS?

Yup.

quoted

quoted

        device->power.state_for_enumeratin = dse;

And then, it is a matter of comparing ->power.state_for_enumeratin
with ->power.state and putting the device into D0 if the former is
shallower than the latter.

What do you think?

Sounds good. How about calling the function e.g.
acpi_device_resume_for_probe(), so runtime PM could be used to resume the
device if the function returns true?

I'd rather try to power it up before enabling runtime PM, because in
order to do the latter properly, you need to know if the device is
active or suspended to start with.

So you need something like (pseudo-code)

if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
   acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
   pm_runtime_set_active(dev);
} else {
   pm_runtime_set_suspended(dev);

I guess the else branch isn't needed? The device remains suspended if its
state hasn't been changed.

Assuming that the initial status is always "suspended", the else
branch is not needed.

quoted

}

and then you can enable PM-runtime.

Yes, agreed, this is what drivers should do. The I涎 framework would use
the function and conditionally power the device on before enabling runtime
PM.

This is how it's implemented by the set already but I think the change in
semantics requires a little more still.

Agreed.