[Public]
> -----Original Message-----
> From: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> Sent: Thursday, March 10, 2022 10:26
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Hans de Goede
> <hdegoede@xxxxxxxxxx>; Mark Gross <mgross@xxxxxxxxxxxxxxx>; Rafael J .
> Wysocki <rjw@xxxxxxxxxxxxx>
> Cc: open list:X86 PLATFORM DRIVERS <platform-driver-
> x86@xxxxxxxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] ACPI / x86: Pass the constraints checking result to
> LPS0 callback
>
> On Thu, 2022-03-10 at 09:17 -0600, Mario Limonciello wrote:
> > If constraints checking has been enabled by the LPS0 code, it may
> > also be useful for drivers using the callback to make a decision what
> > to do.
> >
> > For example this may in the future allow a failing constraints check
> > preventing another driver from notifying firmware that all required
> > devices have entered the deepest state.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > drivers/acpi/x86/s2idle.c | 17 ++++++++++-------
> > include/linux/acpi.h | 4 ++--
> > 2 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index 652dc2d75458..c737a8e5d5a5 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -88,7 +88,7 @@ struct lpi_device_constraint_amd {
> >
> > struct lps0_callback_handler {
> > struct list_head list_node;
> > - int (*prepare_late_callback)(void *context);
> > + int (*prepare_late_callback)(void *context, bool constraints);
> > void (*restore_early_callback)(void *context);
> > void *context;
> > };
> > @@ -297,7 +297,7 @@ static void lpi_device_get_constraints(void)
> > ACPI_FREE(out_obj);
> > }
> >
> > -static void lpi_check_constraints(void)
> > +static void lpi_check_constraints(bool *met)
> > {
> > int i;
> >
> > @@ -319,11 +319,13 @@ static void lpi_check_constraints(void)
> > continue;
> > }
> >
> > - if (adev->power.state < lpi_constraints_table[i].min_dstate)
> > + if (adev->power.state < lpi_constraints_table[i].min_dstate)
> {
> > acpi_handle_info(handle,
> > "LPI: Constraint not met; min power state:%s
> > current power state:%s\n",
> >
> acpi_power_state_string(lpi_constraints_table[i]
> > .min_dstate),
> > acpi_power_state_string(adev-
> >power.state));
> > + *met = false;
> > + }
> > }
> > }
> >
> > @@ -455,13 +457,14 @@ static struct acpi_scan_handler lps0_handler = {
> > int acpi_s2idle_prepare_late(void)
> > {
> > struct lps0_callback_handler *handler;
> > + bool constraints_met = true;
> > int rc = 0;
> >
> > if (!lps0_device_handle || sleep_no_lps0)
> > return 0;
> >
> > if (pm_debug_messages_on)
> > - lpi_check_constraints();
> > + lpi_check_constraints(&constraints_met);
>
> lpi_check_constraints() was only used for dumping information to dmesg. If
> you
> want to make a decision based on whether a constraint was met then you
> probably
> want to run it all the time. You could use pm_debug_messages_on inside of
> lpi_check_contraints() to decide whether to print to the log.
That's a good idea.
>
> >
> > /* Screen off */
> > if (lps0_dsm_func_mask > 0)
> > @@ -490,7 +493,7 @@ int acpi_s2idle_prepare_late(void)
> >
> > mutex_lock(&lps0_callback_handler_mutex);
> > list_for_each_entry(handler, &lps0_callback_handler_head,
> list_node) {
> > - rc = handler->prepare_late_callback(handler->context);
> > + rc = handler->prepare_late_callback(handler->context,
> > constraints_met);
>
> Otherwise, is it okay that constraints_met will always be true if
> pm_debug_messages_on isn't?
I wasn't ready to make decisions based on it, but you're right at least
running it all the time and showing the impactful messages when debugging
on is a good start.
Thanks!
>
> David
>
> > if (rc)
> > goto out;
> > }
> > @@ -554,7 +557,7 @@ void acpi_s2idle_setup(void)
> > s2idle_set_ops(&acpi_s2idle_ops_lps0);
> > }
> >
> > -int acpi_register_lps0_callbacks(int (*prepare_late)(void *context),
> > +int acpi_register_lps0_callbacks(int (*prepare_late)(void *context, bool
> > constraints),
> > void (*restore_early)(void *context),
> > void *context)
> > {
> > @@ -578,7 +581,7 @@ int acpi_register_lps0_callbacks(int
> (*prepare_late)(void
> > *context),
> > }
> > EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> >
> > -void acpi_unregister_lps0_callbacks(int (*prepare_late)(void *context),
> > +void acpi_unregister_lps0_callbacks(int (*prepare_late)(void *context,
> bool
> > constraints),
> > void (*restore_early)(void *context),
> > void *context)
> > {
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index cae0fde309f2..5aae774670dc 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1024,10 +1024,10 @@ void acpi_os_set_prepare_extended_sleep(int
> (*func)(u8
> > sleep_state,
> > acpi_status acpi_os_prepare_extended_sleep(u8 sleep_state,
> > u32 val_a, u32 val_b);
> > #ifdef CONFIG_X86
> > -int acpi_register_lps0_callbacks(int (*prepare_late)(void *context),
> > +int acpi_register_lps0_callbacks(int (*prepare_late)(void *context, bool
> > constraints),
> > void (*restore_early)(void *context),
> > void *context);
> > -void acpi_unregister_lps0_callbacks(int (*prepare_late)(void *context),
> > +void acpi_unregister_lps0_callbacks(int (*prepare_late)(void *context,
> bool
> > constraints),
> > void (*restore_early)(void *context),
> > void *context);
> > #endif /* CONFIG_X86 */
