Messages in this thread | | | Date | Mon, 29 Aug 2022 09:32:44 -0500 | Subject | Re: [PATCH v2 1/4] ACPI: s2idle: Add a new ->enter() callback for platform_s2idle_ops | From | "Limonciello, Mario" <> |
| |
On 8/29/2022 09:20, Rafael J. Wysocki wrote: > On Mon, Aug 29, 2022 at 3:29 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> On some platforms it is found that Linux more aggressively enters s2idle >> than Windows enters Modern Standby and this uncovers some synchronization >> issues for the platform. To aid in debugging this class of problems in >> the future, add support for an extra optional callback intended for >> drivers to emit extra debugging. > > I'm not liking this. > > If you want debug, why not simply add it where it is needed?
Actually this is exactly where it's needed. The synchronization issue is with what non-x86 is doing while x86 transitions into ACPI C3.
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Add a prototype for `acpi_s2idle_enter` >> >> drivers/acpi/sleep.h | 1 + >> drivers/acpi/x86/s2idle.c | 14 ++++++++++++++ >> include/linux/acpi.h | 1 + >> include/linux/suspend.h | 1 + >> kernel/power/suspend.c | 3 +++ >> 5 files changed, 20 insertions(+) >> >> diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h >> index 7fe41ee489d6..7856930a7da9 100644 >> --- a/drivers/acpi/sleep.h >> +++ b/drivers/acpi/sleep.h >> @@ -18,6 +18,7 @@ static inline acpi_status acpi_set_waking_vector(u32 wakeup_address) >> extern int acpi_s2idle_begin(void); >> extern int acpi_s2idle_prepare(void); >> extern int acpi_s2idle_prepare_late(void); >> +extern void acpi_s2idle_enter(void); > > And this name is confusing, because it suggests that the role of the > callback is to make the platform enter s2idle which isn't the case.
Can you propose another name?
> >> extern bool acpi_s2idle_wake(void); >> extern void acpi_s2idle_restore_early(void); >> extern void acpi_s2idle_restore(void); >> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c >> index f9ac12b778e6..c984093a3657 100644 >> --- a/drivers/acpi/x86/s2idle.c >> +++ b/drivers/acpi/x86/s2idle.c >> @@ -486,6 +486,19 @@ int acpi_s2idle_prepare_late(void) >> return 0; >> } >> >> +void acpi_s2idle_enter(void) >> +{ >> + struct acpi_s2idle_dev_ops *handler; >> + >> + if (!lps0_device_handle || sleep_no_lps0) >> + return; >> + >> + list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) { >> + if (handler->enter) >> + handler->enter(); >> + } >> +} >> + >> void acpi_s2idle_restore_early(void) >> { >> struct acpi_s2idle_dev_ops *handler; >> @@ -527,6 +540,7 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { >> .begin = acpi_s2idle_begin, >> .prepare = acpi_s2idle_prepare, >> .prepare_late = acpi_s2idle_prepare_late, >> + .enter = acpi_s2idle_enter, >> .wake = acpi_s2idle_wake, >> .restore_early = acpi_s2idle_restore_early, >> .restore = acpi_s2idle_restore, >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 6f64b2f3dc54..9df14b5a875c 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1076,6 +1076,7 @@ struct acpi_s2idle_dev_ops { >> struct list_head list_node; >> void (*prepare)(void); >> void (*restore)(void); >> + void (*enter)(void); >> }; >> int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg); >> void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg); >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h >> index 70f2921e2e70..5a3fdca0a628 100644 >> --- a/include/linux/suspend.h >> +++ b/include/linux/suspend.h >> @@ -191,6 +191,7 @@ struct platform_s2idle_ops { >> int (*begin)(void); >> int (*prepare)(void); >> int (*prepare_late)(void); >> + void (*enter)(void); >> bool (*wake)(void); >> void (*restore_early)(void); >> void (*restore)(void); >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c >> index 827075944d28..0c08032d6b50 100644 >> --- a/kernel/power/suspend.c >> +++ b/kernel/power/suspend.c >> @@ -136,6 +136,9 @@ static void s2idle_loop(void) >> break; >> } >> >> + if (s2idle_ops && s2idle_ops->enter) >> + s2idle_ops->enter(); >> + >> s2idle_enter(); >> } >> >> -- >> 2.34.1 >>
| |