| Message ID | 20220310151705.577442-2-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] ACPI / x86: Add support for LPS0 callback handler | expand |
[Public] > -----Original Message----- > From: David E. Box <david.e.box@linux.intel.com> > Sent: Thursday, March 10, 2022 10:26 > To: Limonciello, Mario <Mario.Limonciello@amd.com>; Hans de Goede > <hdegoede@redhat.com>; Mark Gross <mgross@linux.intel.com>; Rafael J . > Wysocki <rjw@rjwysocki.net> > Cc: open list:X86 PLATFORM DRIVERS <platform-driver- > x86@vger.kernel.org>; linux-acpi@vger.kernel.org > 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@amd.com> > > --- > > 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 */
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); /* 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); 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 */
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@amd.com> --- drivers/acpi/x86/s2idle.c | 17 ++++++++++------- include/linux/acpi.h | 4 ++-- 2 files changed, 12 insertions(+), 9 deletions(-)