Re: [PATCH v5] ACPI: PM: Introduce CONFIG_ACPI_S2IDLE for platform-independent S2Idle support
luriwen <[email protected]>
| Newsgroups | org.kernel.vger.linux-acpi,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
在 2026/8/10 18:23, Rafael J. Wysocki (Intel) 写道: > On Mon, Aug 10, 2026 at 8:55 AM Riwen Lu <[email protected]> wrote: >> >> The ACPI S2Idle (suspend-to-idle) code is currently located in sleep.c >> and guarded by CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT. This config is >> historically tied to x86/ia64 sleep states (S3/S4), which prevents >> ACPI-based platforms like ARM64 from using the OS-centric s2idle >> framework and its associated wakeup event handling (e.g., from ACPI >> buttons or the lid). >> >> Since S2Idle is a software-driven low-power state that does not rely on >> firmware sleep states (S3/S4), it should be available to all >> ACPI-enabled platforms. >> >> To achieve this, introduce a new, independent Kconfig option, >> CONFIG_ACPI_S2IDLE. It is selected by ACPI_SYSTEM_POWER_STATES_SUPPORT >> for backward compatibility, but can now also be enabled separately by >> other platforms. >> >> Move all s2idle-specific code into a dedicated file, >> drivers/acpi/s2idle.c. This creates a clear and independent ACPI s2idle >> framework, centered around a default acpi_s2idle_ops and a weakly >> defined acpi_s2idle_setup() hook. >> >> The change allows all ACPI platforms to use this common s2idle >> infrastructure. Platforms with unique requirements can override the >> defaults via the weak function, while others (like ARM64) can now simply >> enable the config to obtain functional s2idle support with standard ACPI >> wakeup handling. >> >> Signed-off-by: Riwen Lu <[email protected]> >> --- >> v1 -> v2: >> - Fix acpi_s2idle_setup() declaration error when SUSPEND is disabled in >> x86 builds. >> >> v2 -> v3: >> - Select ACPI_S2IDLE if SUSPEND is enabled for >> ACPI_SYSTEM_POWER_STATES_SUPPORT, so the select no longer violates the >> ACPI_S2IDLE dependency on SUSPEND. >> >> v3 -> v4: >> - Add pr_fmt() in s2idle.c so log messages keep the "ACPI: PM: " prefix >> that sleep.c used; without it the moved pr_info/pr_warn would lose the >> subsystem prefix. >> - Drop the redundant #ifdef CONFIG_SUSPEND / #else wrapper in s2idle.c. >> The !SUSPEND branch was dead code, because CONFIG_ACPI_S2IDLE itself >> depends on SUSPEND, so s2idle.c is only compiled when SUSPEND is set. >> - Make acpi_s2idle_init() return void. The int return value was always 0 >> in the real implementation and -ENXIO in the stub, and the only caller >> (acpi_bus_init()) ignores it; a void interface is honest and the stub >> becomes a clean no-op. >> - Mark the weak acpi_s2idle_setup() default __init, matching the x86 >> override (which is already __init) and allowing the code to be discarded >> after init. >> - Use SPDX-License-Identifier: GPL-2.0-only in s2idle.c for consistency >> with sleep.c (the file the code was split out from). >> - Trivial cleanups: remove a double blank line in the Makefile and drop >> the redundant "Warning: " prefix from a pr_warn() message. >> >> v4 -> v5: >> - Add #include <linux/interrupt.h> to s2idle.c. enable_irq_wake(), >> disable_irq_wake() and rearm_wake_irq() are declared there. > > OK, so I'm totally unconvinced about the approach here, while I agree > that it would be good to allow s2idle to also work on platforms using > HW-reduced ACPI. > > I'll be mostly offline this week though, so let's get back to it later. > Hi Rafael, Hope you're back and had a good break. I'm reworking this based on your "unconvinced about the approach" feedback. Before I post the next revision, could you share what direction you'd prefer for enabling s2idle on HW-reduced ACPI platforms? I'd rather align with your expectation first than risk another revision that misses it. Thanks >> drivers/acpi/Kconfig | 14 ++++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/bus.c | 3 + >> drivers/acpi/internal.h | 10 ++- >> drivers/acpi/s2idle.c | 163 ++++++++++++++++++++++++++++++++++++++++ >> drivers/acpi/sleep.c | 145 ----------------------------------- >> 6 files changed, 189 insertions(+), 147 deletions(-) >> create mode 100644 drivers/acpi/s2idle.c >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index f165d14cf61a..8c0918747bbb 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -55,6 +55,7 @@ config ACPI_GENERIC_GSI >> >> config ACPI_SYSTEM_POWER_STATES_SUPPORT >> bool >> + select ACPI_S2IDLE if SUSPEND >> >> config ACPI_CCA_REQUIRED >> bool >> @@ -113,6 +114,19 @@ config ACPI_SLEEP >> depends on ACPI_SYSTEM_POWER_STATES_SUPPORT >> default y >> >> +config ACPI_S2IDLE >> + bool "ACPI suspend-to-idle support" >> + depends on SUSPEND >> + help >> + This option enables the core ACPI framework for the suspend-to-idle >> + (S2Idle) power state. This state is software-driven and does not >> + require platform firmware sleep states (S3/S4). >> + >> + It is automatically selected by platforms with >> + CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT (like x86), but can also >> + be enabled independently by other ACPI platforms (e.g., ARM64) to >> + gain support for low-power idle and ACPI wakeup events. >> + >> config ACPI_REV_OVERRIDE_POSSIBLE >> bool "Allow supported ACPI revision to be overridden" >> depends on X86 >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index d1b0affb844f..f1e89e45e876 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -35,6 +35,7 @@ acpi-$(CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT) += sleep.o >> acpi-y += device_sysfs.o device_pm.o >> acpi-$(CONFIG_ACPI_SLEEP) += proc.o >> >> +acpi-$(CONFIG_ACPI_S2IDLE) += s2idle.o >> >> # >> # ACPI Bus and Device Drivers >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >> index a30a904f6535..eedcd940859c 100644 >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -1534,6 +1534,9 @@ static int __init acpi_bus_init(void) >> /* Initialize sleep structures */ >> acpi_sleep_init(); >> >> + /* Initialize default acpi s2idle ops */ >> + acpi_s2idle_init(); >> + >> /* >> * Get the system interrupt model and evaluate \_PIC. >> */ >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h >> index 40f875b265a9..5a08315d308a 100644 >> --- a/drivers/acpi/internal.h >> +++ b/drivers/acpi/internal.h >> @@ -267,13 +267,19 @@ static inline bool acpi_ec_dispatch_gpe(void) >> Suspend/Resume >> -------------------------------------------------------------------------- */ >> #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT >> -extern bool acpi_s2idle_wakeup(void); >> extern int acpi_sleep_init(void); >> #else >> -static inline bool acpi_s2idle_wakeup(void) { return false; } >> static inline int acpi_sleep_init(void) { return -ENXIO; } >> #endif >> >> +#ifdef CONFIG_ACPI_S2IDLE >> +extern bool acpi_s2idle_wakeup(void); >> +extern void acpi_s2idle_init(void); >> +#else >> +static inline bool acpi_s2idle_wakeup(void) { return false; } >> +static inline void acpi_s2idle_init(void) {} >> +#endif >> + >> #ifdef CONFIG_ACPI_SLEEP >> void acpi_sleep_proc_init(void); >> int suspend_nvs_alloc(void); >> diff --git a/drivers/acpi/s2idle.c b/drivers/acpi/s2idle.c >> new file mode 100644 >> index 000000000000..b5cd9649acda >> --- /dev/null >> +++ b/drivers/acpi/s2idle.c >> @@ -0,0 +1,163 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * s2idle.c - ACPI suspend-to-idle support. >> + * >> + */ >> + >> +#define pr_fmt(fmt) "ACPI: PM: " fmt >> + >> +#include <linux/acpi.h> >> +#include <linux/device.h> >> +#include <linux/interrupt.h> >> +#include <linux/suspend.h> >> +#include <linux/irq.h> >> + >> +#include "internal.h" >> +#include "sleep.h" >> + >> +static bool s2idle_wakeup; >> + >> +int acpi_s2idle_begin(void) >> +{ >> + acpi_scan_lock_acquire(); >> + return 0; >> +} >> + >> +int acpi_s2idle_prepare(void) >> +{ >> + if (acpi_sci_irq_valid()) { >> + int error; >> + >> + error = enable_irq_wake(acpi_sci_irq); >> + if (error) >> + pr_warn("Failed to enable wakeup from IRQ %d: %d\n", >> + acpi_sci_irq, error); >> + >> + acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE); >> + } >> + >> + acpi_enable_wakeup_devices(ACPI_STATE_S0); >> + >> + /* Change the configuration of GPEs to avoid spurious wakeup. */ >> + acpi_enable_all_wakeup_gpes(); >> + acpi_os_wait_events_complete(); >> + >> + s2idle_wakeup = true; >> + return 0; >> +} >> + >> +bool acpi_s2idle_wake(void) >> +{ >> + if (!acpi_sci_irq_valid()) >> + return pm_wakeup_pending(); >> + >> + while (pm_wakeup_pending()) { >> + /* >> + * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the >> + * SCI has not triggered while suspended, so bail out (the >> + * wakeup is pending anyway and the SCI is not the source of >> + * it). >> + */ >> + if (irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) { >> + pm_pr_dbg("Wakeup unrelated to ACPI SCI\n"); >> + return true; >> + } >> + >> + /* >> + * If the status bit of any enabled fixed event is set, the >> + * wakeup is regarded as valid. >> + */ >> + if (acpi_any_fixed_event_status_set()) { >> + pm_pr_dbg("ACPI fixed event wakeup\n"); >> + return true; >> + } >> + >> + /* Check wakeups from drivers sharing the SCI. */ >> + if (acpi_check_wakeup_handlers()) { >> + pm_pr_dbg("ACPI custom handler wakeup\n"); >> + return true; >> + } >> + >> + /* >> + * Check non-EC GPE wakeups and if there are none, cancel the >> + * SCI-related wakeup and dispatch the EC GPE. >> + */ >> + if (acpi_ec_dispatch_gpe()) { >> + pm_pr_dbg("ACPI non-EC GPE wakeup\n"); >> + return true; >> + } >> + >> + acpi_os_wait_events_complete(); >> + >> + /* >> + * The SCI is in the "suspended" state now and it cannot produce >> + * new wakeup events till the rearming below, so if any of them >> + * are pending here, they must be resulting from the processing >> + * of EC events above or coming from somewhere else. >> + */ >> + if (pm_wakeup_pending()) { >> + pm_pr_dbg("Wakeup after ACPI Notify sync\n"); >> + return true; >> + } >> + >> + pm_pr_dbg("Rearming ACPI SCI for wakeup\n"); >> + >> + pm_wakeup_clear(acpi_sci_irq); >> + rearm_wake_irq(acpi_sci_irq); >> + } >> + >> + return false; >> +} >> + >> +void acpi_s2idle_restore(void) >> +{ >> + /* >> + * Drain pending events before restoring the working-state configuration >> + * of GPEs. >> + */ >> + acpi_os_wait_events_complete(); /* synchronize GPE processing */ >> + acpi_ec_flush_work(); /* flush the EC driver's workqueues */ >> + acpi_os_wait_events_complete(); /* synchronize Notify handling */ >> + >> + s2idle_wakeup = false; >> + >> + acpi_enable_all_runtime_gpes(); >> + >> + acpi_disable_wakeup_devices(ACPI_STATE_S0); >> + >> + if (acpi_sci_irq_valid()) { >> + acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE); >> + disable_irq_wake(acpi_sci_irq); >> + } >> +} >> + >> +void acpi_s2idle_end(void) >> +{ >> + acpi_scan_lock_release(); >> +} >> + >> +static const struct platform_s2idle_ops acpi_s2idle_ops = { >> + .begin = acpi_s2idle_begin, >> + .prepare = acpi_s2idle_prepare, >> + .wake = acpi_s2idle_wake, >> + .restore = acpi_s2idle_restore, >> + .end = acpi_s2idle_end, >> +}; >> + >> +void __init __weak acpi_s2idle_setup(void) >> +{ >> + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) >> + pr_info("Efficient low-power S0 idle declared\n"); >> + >> + s2idle_set_ops(&acpi_s2idle_ops); >> +} >> + >> +bool acpi_s2idle_wakeup(void) >> +{ >> + return s2idle_wakeup; >> +} >> + >> +void __init acpi_s2idle_init(void) >> +{ >> + acpi_s2idle_setup(); >> +} >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c >> index 132a9df98471..eeaacc354c48 100644 >> --- a/drivers/acpi/sleep.c >> +++ b/drivers/acpi/sleep.c >> @@ -724,143 +724,6 @@ static const struct platform_suspend_ops acpi_suspend_ops_old = { >> .recover = acpi_pm_finish, >> }; >> >> -static bool s2idle_wakeup; >> - >> -int acpi_s2idle_begin(void) >> -{ >> - acpi_scan_lock_acquire(); >> - return 0; >> -} >> - >> -int acpi_s2idle_prepare(void) >> -{ >> - if (acpi_sci_irq_valid()) { >> - int error; >> - >> - error = enable_irq_wake(acpi_sci_irq); >> - if (error) >> - pr_warn("Warning: Failed to enable wakeup from IRQ %d: %d\n", >> - acpi_sci_irq, error); >> - >> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE); >> - } >> - >> - acpi_enable_wakeup_devices(ACPI_STATE_S0); >> - >> - /* Change the configuration of GPEs to avoid spurious wakeup. */ >> - acpi_enable_all_wakeup_gpes(); >> - acpi_os_wait_events_complete(); >> - >> - s2idle_wakeup = true; >> - return 0; >> -} >> - >> -bool acpi_s2idle_wake(void) >> -{ >> - if (!acpi_sci_irq_valid()) >> - return pm_wakeup_pending(); >> - >> - while (pm_wakeup_pending()) { >> - /* >> - * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the >> - * SCI has not triggered while suspended, so bail out (the >> - * wakeup is pending anyway and the SCI is not the source of >> - * it). >> - */ >> - if (irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) { >> - pm_pr_dbg("Wakeup unrelated to ACPI SCI\n"); >> - return true; >> - } >> - >> - /* >> - * If the status bit of any enabled fixed event is set, the >> - * wakeup is regarded as valid. >> - */ >> - if (acpi_any_fixed_event_status_set()) { >> - pm_pr_dbg("ACPI fixed event wakeup\n"); >> - return true; >> - } >> - >> - /* Check wakeups from drivers sharing the SCI. */ >> - if (acpi_check_wakeup_handlers()) { >> - pm_pr_dbg("ACPI custom handler wakeup\n"); >> - return true; >> - } >> - >> - /* >> - * Check non-EC GPE wakeups and if there are none, cancel the >> - * SCI-related wakeup and dispatch the EC GPE. >> - */ >> - if (acpi_ec_dispatch_gpe()) { >> - pm_pr_dbg("ACPI non-EC GPE wakeup\n"); >> - return true; >> - } >> - >> - acpi_os_wait_events_complete(); >> - >> - /* >> - * The SCI is in the "suspended" state now and it cannot produce >> - * new wakeup events till the rearming below, so if any of them >> - * are pending here, they must be resulting from the processing >> - * of EC events above or coming from somewhere else. >> - */ >> - if (pm_wakeup_pending()) { >> - pm_pr_dbg("Wakeup after ACPI Notify sync\n"); >> - return true; >> - } >> - >> - pm_pr_dbg("Rearming ACPI SCI for wakeup\n"); >> - >> - pm_wakeup_clear(acpi_sci_irq); >> - rearm_wake_irq(acpi_sci_irq); >> - } >> - >> - return false; >> -} >> - >> -void acpi_s2idle_restore(void) >> -{ >> - /* >> - * Drain pending events before restoring the working-state configuration >> - * of GPEs. >> - */ >> - acpi_os_wait_events_complete(); /* synchronize GPE processing */ >> - acpi_ec_flush_work(); /* flush the EC driver's workqueues */ >> - acpi_os_wait_events_complete(); /* synchronize Notify handling */ >> - >> - s2idle_wakeup = false; >> - >> - acpi_enable_all_runtime_gpes(); >> - >> - acpi_disable_wakeup_devices(ACPI_STATE_S0); >> - >> - if (acpi_sci_irq_valid()) { >> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE); >> - disable_irq_wake(acpi_sci_irq); >> - } >> -} >> - >> -void acpi_s2idle_end(void) >> -{ >> - acpi_scan_lock_release(); >> -} >> - >> -static const struct platform_s2idle_ops acpi_s2idle_ops = { >> - .begin = acpi_s2idle_begin, >> - .prepare = acpi_s2idle_prepare, >> - .wake = acpi_s2idle_wake, >> - .restore = acpi_s2idle_restore, >> - .end = acpi_s2idle_end, >> -}; >> - >> -void __weak acpi_s2idle_setup(void) >> -{ >> - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) >> - pr_info("Efficient low-power S0 idle declared\n"); >> - >> - s2idle_set_ops(&acpi_s2idle_ops); >> -} >> - >> static void __init acpi_sleep_suspend_setup(void) >> { >> bool suspend_ops_needed = false; >> @@ -875,20 +738,12 @@ static void __init acpi_sleep_suspend_setup(void) >> if (suspend_ops_needed) >> suspend_set_ops(old_suspend_ordering ? >> &acpi_suspend_ops_old : &acpi_suspend_ops); >> - >> - acpi_s2idle_setup(); >> } >> >> #else /* !CONFIG_SUSPEND */ >> -#define s2idle_wakeup (false) >> static inline void acpi_sleep_suspend_setup(void) {} >> #endif /* !CONFIG_SUSPEND */ >> >> -bool acpi_s2idle_wakeup(void) >> -{ >> - return s2idle_wakeup; >> -} >> - >> #ifdef CONFIG_PM_SLEEP >> static u32 saved_bm_rld; >> >> -- >> 2.25.1 >>