Re: [PATCH v9 1/7] ACPI: scan: Obtain device's desired enumeration power state

[Rafael J. Wysocki]

On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Rafael,
>
> Thanks for the comments.
>
> On Fri, Jan 29, 2021 at 03:07:57PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > >  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.

>
> > if (ACPI_FAILURE(status))
>
> ACPI_SUCCESS?

Yup.

> >         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);
}

and then you can enable PM-runtime.