Messages in this thread | | | Date | Mon, 29 Aug 2022 09:51:35 -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:40, Rafael J. Wysocki wrote: > On Mon, Aug 29, 2022 at 4:32 PM Limonciello, Mario > <mario.limonciello@amd.com> wrote: >> >> 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. > > I meant directly, without adding new driver callbacks. Adding > callbacks for debug only seems a bit excessive. > > But I guess the problem is that you need to ask the PMC about > something then, don't you?
Yeah, that's exactly it. The code changes there won't make sense generically. I suppose it's possible to make a global symbol and only call it in the AMD case, but this approach should be a lot cleaner.
I did CC you in the other 3 patches in the series (in platform-x86) so you can see exactly how it's used. Patch 4/4 is the one that uses this.
https://patchwork.kernel.org/project/platform-driver-x86/patch/20220829132908.5254-5-mario.limonciello@amd.com/
> >>> >>>> 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? > > acpi_s2idle_check() or similar.
OK, will plan to change over to this name for v3.
> >>> >>>> 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(); >>>> } >>>> >>>> --
| |