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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Jan 29, 2021 at 10:22 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Fri, Jan 29, 2021 at 05:57:17PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Jan 29, 2021 at 5:45 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> 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@linux.intel.com> 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@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.
> >
> > >
> > > > 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);
>
> 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.

> > }
> >
> > 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.