| Message ID | 20220314050340.1176-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/5] ACPI / x86: Add support for LPS0 callback handler | expand |
On Mon, Mar 14, 2022 at 12:03:35AM -0500, Mario Limonciello wrote: > Currenty the latest thing run during a suspend to idle attempt is > the LPS0 `prepare_late` callback and the earliest thing is the > `resume_early` callback. > > There is a desire for the `amd-pmc` driver to suspend later in the > suspend process (ideally the very last thing), so create a callback > that it or any other driver can hook into to do this. I'm wondering if this can be solved with much less code by either using device links (a device link to amd-pmc from everything that needs to be suspended before it), or with a notifier chain? Thanks, Lukas
[Public] > > +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg) > > +{ > > + struct lps0_callback_handler *handler; > > + > > + if (!lps0_device_handle || sleep_no_lps0) > > + return -ENODEV; > > + > > + handler = kmalloc(sizeof(*handler), GFP_KERNEL); > > + if (!handler) > > + return -ENOMEM; > > + handler->prepare_late_callback = arg->prepare_late_callback; > > + handler->restore_early_callback = arg->restore_early_callback; > > + handler->context = arg->context; > > + > > + mutex_lock(&lps0_callback_handler_mutex); > > + list_add(&handler->list_node, &lps0_callback_handler_head); > > + mutex_unlock(&lps0_callback_handler_mutex); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks); > > Typically with calls like these we simply let the caller own the struct > lps0_callback_handler > and only make the list_add() call here. Typically the struct > lps0_callback_handler will > be embedded in the driver_data of the driver registering the handler and it > will > call the unregister function before free-ing its driver_data. > When I put this together I was modeling it off of `struct acpi_wakeup_handler` which the handling and the use in the kernel doesn't seem to follow the design pattern you describe. Rafael - can you please confirm which direction you want to see here for this? > > + > > +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg) > > +{ > > + struct lps0_callback_handler *handler; > > + > > + mutex_lock(&lps0_callback_handler_mutex); > > + list_for_each_entry(handler, &lps0_callback_handler_head, > list_node) { > > + if (handler->prepare_late_callback == arg- > >prepare_late_callback && > > + handler->restore_early_callback == arg- > >restore_early_callback && > > + handler->context == arg->context) { > > + list_del(&handler->list_node); > > + kfree(handler); > > + break; > > + } > > + } > > + mutex_unlock(&lps0_callback_handler_mutex); > > +} > > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks); > > And this then becomes just lock, list_del, unlock. > > Regards, > > Hans
On Mon, 2022-03-14 at 13:32 +0000, Limonciello, Mario wrote: > [Public] > > > > +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg) > > > +{ > > > + struct lps0_callback_handler *handler; > > > + > > > + if (!lps0_device_handle || sleep_no_lps0) > > > + return -ENODEV; > > > + > > > + handler = kmalloc(sizeof(*handler), GFP_KERNEL); > > > + if (!handler) > > > + return -ENOMEM; > > > + handler->prepare_late_callback = arg->prepare_late_callback; > > > + handler->restore_early_callback = arg->restore_early_callback; > > > + handler->context = arg->context; > > > + > > > + mutex_lock(&lps0_callback_handler_mutex); > > > + list_add(&handler->list_node, &lps0_callback_handler_head); > > > + mutex_unlock(&lps0_callback_handler_mutex); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks); > > > > Typically with calls like these we simply let the caller own the struct > > lps0_callback_handler > > and only make the list_add() call here. Typically the struct > > lps0_callback_handler will > > be embedded in the driver_data of the driver registering the handler and it > > will > > call the unregister function before free-ing its driver_data. > > > > When I put this together I was modeling it off of `struct acpi_wakeup_handler` > which the handling and the use in the kernel doesn't seem to follow the design > pattern > you describe. > > Rafael - can you please confirm which direction you want to see here for this? > > > > + > > > +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg) > > > +{ > > > + struct lps0_callback_handler *handler; > > > + > > > + mutex_lock(&lps0_callback_handler_mutex); > > > + list_for_each_entry(handler, &lps0_callback_handler_head, > > list_node) { > > > + if (handler->prepare_late_callback == arg- > > > prepare_late_callback && > > > + handler->restore_early_callback == arg- > > > restore_early_callback && > > > + handler->context == arg->context) { > > > + list_del(&handler->list_node); > > > + kfree(handler); > > > + break; > > > + } > > > + } > > > + mutex_unlock(&lps0_callback_handler_mutex); > > > +} > > > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks); > > > > And this then becomes just lock, list_del, unlock. > > > > Regards, > > > > Hans If you keep v3, Reviewed-by: David E. Box <david.e.box@linux.intel.com>
On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 3/14/22 14:32, Limonciello, Mario wrote: > > [Public] > > > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg) > >>> +{ > >>> + struct lps0_callback_handler *handler; > >>> + > >>> + if (!lps0_device_handle || sleep_no_lps0) > >>> + return -ENODEV; > >>> + > >>> + handler = kmalloc(sizeof(*handler), GFP_KERNEL); > >>> + if (!handler) > >>> + return -ENOMEM; > >>> + handler->prepare_late_callback = arg->prepare_late_callback; > >>> + handler->restore_early_callback = arg->restore_early_callback; > >>> + handler->context = arg->context; > >>> + > >>> + mutex_lock(&lps0_callback_handler_mutex); > >>> + list_add(&handler->list_node, &lps0_callback_handler_head); > >>> + mutex_unlock(&lps0_callback_handler_mutex); > >>> + > >>> + return 0; > >>> +} > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks); > >> > >> Typically with calls like these we simply let the caller own the struct > >> lps0_callback_handler > >> and only make the list_add() call here. Typically the struct > >> lps0_callback_handler will > >> be embedded in the driver_data of the driver registering the handler and it > >> will > >> call the unregister function before free-ing its driver_data. > >> > > > > When I put this together I was modeling it off of `struct acpi_wakeup_handler` The structure added by this patch is more like struct dev_pm_ops, though. > > which the handling and the use in the kernel doesn't seem to follow the design pattern > > you describe. > > Ah, fair enough. Whatever Rafael prefers works for me. My preference at this point would be to use a notifier chain, unless that's not sufficient for some reason, because it appears to match the notifier usage model. > I pointed this out, because making this change would also make 4/5 a bit > cleaner. You are recreating the same struct lps0_callback_handler on > stack twice there, which looked weird to me. > > Note if Rafael wants to stick with the approach from this v3, then > I guess that the approach in 4/5 is fine. > > Rafael - can you please confirm which direction you want to see here for this? Done above.
[Public] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Wednesday, March 16, 2022 10:35 > To: Hans de Goede <hdegoede@redhat.com>; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: Mark Gross <mgross@linux.intel.com>; Rafael J . Wysocki > <rjw@rjwysocki.net>; open list:X86 PLATFORM DRIVERS <platform-driver- > x86@vger.kernel.org>; linux-acpi@vger.kernel.org; S-k, Shyam-sundar > <Shyam-sundar.S-k@amd.com>; Goswami, Sanket > <Sanket.Goswami@amd.com> > Subject: Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler > > On Wed, Mar 16, 2022 at 4:02 PM Rafael J. Wysocki <rafael@kernel.org> > wrote: > > > > On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede > <hdegoede@redhat.com> wrote: > > > > > > Hi, > > > > > > On 3/14/22 14:32, Limonciello, Mario wrote: > > > > [Public] > > > > > > > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg) > > > >>> +{ > > > >>> + struct lps0_callback_handler *handler; > > > >>> + > > > >>> + if (!lps0_device_handle || sleep_no_lps0) > > > >>> + return -ENODEV; > > > >>> + > > > >>> + handler = kmalloc(sizeof(*handler), GFP_KERNEL); > > > >>> + if (!handler) > > > >>> + return -ENOMEM; > > > >>> + handler->prepare_late_callback = arg->prepare_late_callback; > > > >>> + handler->restore_early_callback = arg->restore_early_callback; > > > >>> + handler->context = arg->context; > > > >>> + > > > >>> + mutex_lock(&lps0_callback_handler_mutex); > > > >>> + list_add(&handler->list_node, &lps0_callback_handler_head); > > > >>> + mutex_unlock(&lps0_callback_handler_mutex); > > > >>> + > > > >>> + return 0; > > > >>> +} > > > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks); > > > >> > > > >> Typically with calls like these we simply let the caller own the struct > > > >> lps0_callback_handler > > > >> and only make the list_add() call here. Typically the struct > > > >> lps0_callback_handler will > > > >> be embedded in the driver_data of the driver registering the handler > and it > > > >> will > > > >> call the unregister function before free-ing its driver_data. > > > >> > > > > > > > > When I put this together I was modeling it off of `struct > acpi_wakeup_handler` > > > > The structure added by this patch is more like struct dev_pm_ops, though. > > > > > > which the handling and the use in the kernel doesn't seem to follow the > design pattern > > > > you describe. > > > > > > Ah, fair enough. Whatever Rafael prefers works for me. > > > > My preference at this point would be to use a notifier chain, unless > > that's not sufficient for some reason, because it appears to match the > > notifier usage model. > > Well, I'm actually not sure about that. > > > > I pointed this out, because making this change would also make 4/5 a bit > > > cleaner. You are recreating the same struct lps0_callback_handler on > > > stack twice there, which looked weird to me. > > > > > > Note if Rafael wants to stick with the approach from this v3, then > > > I guess that the approach in 4/5 is fine. > > > > Rafael - can you please confirm which direction you want to see here > for this? > > So the idea is that the PMC driver's "suspend" method needs to be > invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with > the suspend of the other devices in the system and so it can take the > constraints into account. The reason to do nothing (besides a debug level message right now) with the constraints information is that at least on today's OEM platforms there are some instances constraints aren't met on Linux that need to be investigated and root caused. These particular constraints don't actually cause problems reaching s0ix residency though. > > What is it going to do, in the future, depending on whether or not the > constraints are met? The idea was that if constraints were met that it would send the OS_HINT as part of amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step. It would effectively block the system from getting s0ix residency unless the constraints are all met. Given we know some OEM platforms have problems in current generations with constraints it would probably need to be restricted to this behavior only on a future SOC that we are confident of all drivers and firmware are doing the right thing. By passing the information to amd_pmc we can keep that logic restricting the behavior to only those platforms that we're confident on that behavior.
>>>> My preference at this point would be to use a notifier chain, unless >>>> that's not sufficient for some reason, because it appears to match the >>>> notifier usage model. >>> >>> Well, I'm actually not sure about that. >>> >>>>> I pointed this out, because making this change would also make 4/5 a bit >>>>> cleaner. You are recreating the same struct lps0_callback_handler on >>>>> stack twice there, which looked weird to me. >>>>> >>>>> Note if Rafael wants to stick with the approach from this v3, then >>>>> I guess that the approach in 4/5 is fine. >>>>>> Rafael - can you please confirm which direction you want to see here >>> for this? >>> >>> So the idea is that the PMC driver's "suspend" method needs to be >>> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with >>> the suspend of the other devices in the system and so it can take the >>> constraints into account. >> >> The reason to do nothing (besides a debug level message right now) with the constraints >> information is that at least on today's OEM platforms there are some instances constraints >> aren't met on Linux that need to be investigated and root caused. These particular constraints >> don't actually cause problems reaching s0ix residency though. > > Why are they useful at all, then? > >>> >>> What is it going to do, in the future, depending on whether or not the >>> constraints are met? >> >> The idea was that if constraints were met that it would send the OS_HINT as part of >> amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step. >> >> It would effectively block the system from getting s0ix residency unless the constraints >> are all met. > > Why do that? I guess to both of your above questions this begs a comparison of how things work in Windows versus Linux. Windows Modern Standby has this concept of "SW DRIPS" vs "HW DRIPS". From an end user perspective you close your lid or you click Start button and hit sleep and the machine is in sleep. Whether it's in the deepest state is "invisible" to you unless you're running a sleep study. Windows will at this time requests devices to go into their deepest states and will continue to monitor them against the constraints table. When the constraints table is matched a uPEP driver is notified (this is the _DSM stuff we have in Linux too for "deepest state") and then it can do as it pleases. ON AMD's platform this sends OS_HINT. OS_HINT is meant to be indicate that the OS is done with all it's suspend actions (caches flushed, devices in D3 etc) and the system can "try" to enter s0ix. Windows will then also distinguish between different types of wakeups and have different behaviors for them. There are wakeups that can be treated as keep screen off, and then possibly go back into deepest state. "As soon as the SoC wakes and the platform exits the DRIPS state, the CPUs start running code again. However, the screen stays powered off unless the interrupt was a result of user input or connecting to a power source" https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/transitioning-between-idle-and-active-states "During the Sleep state, specific value-adding SW activity may run" https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-states So from the time that I clicked sleep in the OS, I might NOT get into the deepest state (HW DRIPS), or I might have gotten in and out several times. If a device driver failed to put a PCI device in D3 for example I would not be able to enter HW DRIPS, but the suspend wouldn't "fail". Now to contrast this to Linux when I enter suspend all drivers will run their various PM callbacks and devices will go into their deepest states. * If a driver fails, the suspend actually fails and you get an error code to go investigate what happened. * If the devices don't get into their deepest state by the time you get to the s2idle loop you don't get s0ix residency. As you can see at least for AMD's platforms OS_HINT is sent too "early". That's why this series exists in the first place. So with all that said; why look at constraints at all if stuff is working? From this design at least on Windows constraints are supposed to be a safety guard that you don't start the HW process for s0i3 process "too early". The last commit that is getting reverted in this series is an example of what could happen if the process is started prematurely. > >> Given we know some OEM platforms have problems in current generations >> with constraints it would probably need to be restricted to this behavior only on a future >> SOC that we are confident of all drivers and firmware are doing the right thing. >> >> By passing the information to amd_pmc we can keep that logic restricting the behavior to >> only those platforms that we're confident on that behavior. > > Honestly, I'm not quite sure why it is a good idea to prevent the > platform from attempting to get into S0ix via suspend-to-idle in any > case. > > You know you have to suspend. You don't know how much time you will > be suspended. The constraints can only tell you what's the > lowest-power state you can achieve at this point, but why is it > relevant? Having thought through and said all I did above, I do concede you're right with the Linux approach to sleep the constraints really don't add a lot of value. If a device fails to enter it's intended sleep states the suspend will "fail". If the suspend succeeds but the constraints table doesn't match, it's just a hint where to focus on problems. I appreciate your thoughts and I will drop the constraints passing patch in this series. With that intent of dropping that would you still like this reworked as a notifier chain or keep it as this design?
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c index abc06e7f89d8..69008c4a86ea 100644 --- a/drivers/acpi/x86/s2idle.c +++ b/drivers/acpi/x86/s2idle.c @@ -86,6 +86,9 @@ struct lpi_device_constraint_amd { int min_dstate; }; +static LIST_HEAD(lps0_callback_handler_head); +static DEFINE_MUTEX(lps0_callback_handler_mutex); + static struct lpi_constraints *lpi_constraints_table; static int lpi_constraints_table_size; static int rev_id; @@ -444,6 +447,9 @@ static struct acpi_scan_handler lps0_handler = { int acpi_s2idle_prepare_late(void) { + struct lps0_callback_handler *handler; + int rc = 0; + if (!lps0_device_handle || sleep_no_lps0) return 0; @@ -474,14 +480,34 @@ int acpi_s2idle_prepare_late(void) acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); } - return 0; + + mutex_lock(&lps0_callback_handler_mutex); + list_for_each_entry(handler, &lps0_callback_handler_head, list_node) { + if (handler->prepare_late_callback) { + rc = handler->prepare_late_callback(handler->context); + if (rc) + goto out; + } + } +out: + mutex_unlock(&lps0_callback_handler_mutex); + + return rc; } void acpi_s2idle_restore_early(void) { + struct lps0_callback_handler *handler; + if (!lps0_device_handle || sleep_no_lps0) return; + mutex_lock(&lps0_callback_handler_mutex); + list_for_each_entry(handler, &lps0_callback_handler_head, list_node) + if (handler->restore_early_callback) + handler->restore_early_callback(handler->context); + mutex_unlock(&lps0_callback_handler_mutex); + /* Modern standby exit */ if (lps0_dsm_func_mask_microsoft > 0) acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT, @@ -524,4 +550,44 @@ void acpi_s2idle_setup(void) s2idle_set_ops(&acpi_s2idle_ops_lps0); } +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg) +{ + struct lps0_callback_handler *handler; + + if (!lps0_device_handle || sleep_no_lps0) + return -ENODEV; + + handler = kmalloc(sizeof(*handler), GFP_KERNEL); + if (!handler) + return -ENOMEM; + handler->prepare_late_callback = arg->prepare_late_callback; + handler->restore_early_callback = arg->restore_early_callback; + handler->context = arg->context; + + mutex_lock(&lps0_callback_handler_mutex); + list_add(&handler->list_node, &lps0_callback_handler_head); + mutex_unlock(&lps0_callback_handler_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks); + +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg) +{ + struct lps0_callback_handler *handler; + + mutex_lock(&lps0_callback_handler_mutex); + list_for_each_entry(handler, &lps0_callback_handler_head, list_node) { + if (handler->prepare_late_callback == arg->prepare_late_callback && + handler->restore_early_callback == arg->restore_early_callback && + handler->context == arg->context) { + list_del(&handler->list_node); + kfree(handler); + break; + } + } + mutex_unlock(&lps0_callback_handler_mutex); +} +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks); + #endif /* CONFIG_SUSPEND */ diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 6274758648e3..df105f5e03e5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1023,7 +1023,16 @@ 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 +struct lps0_callback_handler { + struct list_head list_node; + int (*prepare_late_callback)(void *context); + void (*restore_early_callback)(void *context); + void *context; +}; +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg); +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg); +#endif /* CONFIG_X86 */ #ifndef CONFIG_IA64 void arch_reserve_mem_area(acpi_physical_address addr, size_t size); #else
Currenty the latest thing run during a suspend to idle attempt is the LPS0 `prepare_late` callback and the earliest thing is the `resume_early` callback. There is a desire for the `amd-pmc` driver to suspend later in the suspend process (ideally the very last thing), so create a callback that it or any other driver can hook into to do this. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- changes from v2->v3: * Check that callbacks exist before calling changes from v1->v2: * Change register/unregister arguments to be struct drivers/acpi/x86/s2idle.c | 68 ++++++++++++++++++++++++++++++++++++++- include/linux/acpi.h | 11 ++++++- 2 files changed, 77 insertions(+), 2 deletions(-)