Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> Other parts of the kernel may use constraints information to make
> decisions on what power state to put a device into.

I would say more about what the constraints are in this changelog, or
it becomes cryptic to people who are not familiar with the S0ix/LPS0
terminology.

Something like

"Some platforms may achieve additional energy conservation in deep
idle states provided that the states of all devices on the platform
meet specific requirements. In particular, they each may need to be
put into a specific minimum low-power state (or a deeper one) for this
purpose. The table of Low-Power S0 Idle (LPS0) constraints provided by
the platform firmware on some platforms using ACPI specifies that
minimum low-power state for each device.

That information may help to make decisions on what power state to put
a given device into when it is suspended, so introduce a function
allowing its caller to retrieve the minimum LPS0 low-power state for a
given device."

> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v12->v13:
>  * Drop checking for enabled, just return constraints
> v11->v12:
>  * Use for_each_lpi_constraint instead
>  * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP
> v9->v10:
>  * split from other patches
>  * kerneldoc fixes
>  * move debug statement to this function
> ---
>  drivers/acpi/x86/s2idle.c | 24 ++++++++++++++++++++++++
>  include/linux/acpi.h      |  6 ++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 1aa3cd5677bd8..dd961f3a60577 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -299,6 +299,30 @@ static void lpi_device_get_constraints(void)
>         ACPI_FREE(out_obj);
>  }
>
> +/**
> + * acpi_get_lps0_constraint - get any LPS0 constraint for a device

"get the minimum LPS0 low-power state for a device".

> + * @dev: device to get constraints for
> + *
> + * Returns:
> + *  - the value for constraint.
> + *  - Otherwise, -ENODEV.

Why not ACPI_STATE_UNKNOWN?

> + */
> +int acpi_get_lps0_constraint(struct device *dev)

I think that some overhead would be reduced below if this were taking
a struct acpi_device pointer as the argument.

> +{
> +       struct lpi_constraints *entry;
> +
> +       for_each_lpi_constraint(entry) {
> +               if (!device_match_acpi_handle(dev, entry->handle))
> +                       continue;
> +               acpi_handle_debug(entry->handle,
> +                                 "ACPI device constraint: %d\n", entry->min_dstate);
> +               return entry->min_dstate;
> +       }
> +       dev_dbg(dev, "No ACPI device constraint specified\n");
> +
> +       return -ENODEV;

ACPI_STATE_UNKNOWN?

> +}
> +
>  static void lpi_check_constraints(void)
>  {
>         struct lpi_constraints *entry;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index f1552c04a2856..6745535444c76 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1109,6 +1109,12 @@ struct acpi_s2idle_dev_ops {
>  };
>  int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
>  void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> +int acpi_get_lps0_constraint(struct device *dev);
> +#else /* CONFIG_SUSPEND && CONFIG_X86 */
> +static inline int acpi_get_lps0_constraint(struct device *dev)
> +{
> +       return -ENODEV;
> +}
>  #endif /* CONFIG_SUSPEND && CONFIG_X86 */
>  #ifndef CONFIG_IA64
>  void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
> --
> 2.34.1
>