Re: [RFC v1 3/8] acpi/x86: s2idle: add runtime standby transition function

[Antheas Kapenekakis]

On Fri, 13 Mar 2026 at 21:29, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Fri, Dec 26, 2025 at 11:27 AM Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote:
> >
> > Add pm_standby_transition() to allow transitioning between standby
> > states during runtime and implement it as part of the s2idle suspend
> > sequence. Update the platform_s2idle_ops structure to include a function
> > to perform firmware notifications and a function to get supported states
> >
> > Standby states are a way for userspace to indicate the interactivity of
> > the system at the current moment. Active means that the user is
> > interacting with the device, inactive that a user is not actively
> > interacting with the device, and sleep that the system should appear as
> > if it is suspended to the user, but may still perform small
> > background tasks.
> >
> > For modern ACPI s0ix laptops, the inactive state turns off the
> > backlight and the sleep state may limit the thermal envelope of the
> > device. Either may pulse the power light. This patch introduces an ACPI
> > agnostic structure to handle these transitions, so they may implemented
> > by other platforms, and does not implement them for ACPI.
>
> As already stated multiple times, the state-based terminology is not
> particularly useful in my view.

I can reword the commit message. However, here, can you be a bit more
specific? Are you also referring to the underlying implementation? If
yes, how should that change.

If the user goes from active -> snooze, there needs to be a transition
in the underlying implementation (ie we cannot skip the inactive
callbacks).

> I would prefer switching over to condition-indication-based terminology, like
>
> inactive - indicate to the user that there's no activity related to
> running user workloads in the system
> snooze - indicate to the user that <the above> and the thermal
> envelope may have changed
> active - the system is active
>
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > ---
> >  include/linux/suspend.h |  26 +++++++
> >  kernel/power/suspend.c  | 153 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 179 insertions(+)
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index b02876f1ae38..916dee124758 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -40,6 +40,25 @@ typedef int __bitwise suspend_state_t;
> >  #define PM_SUSPEND_MIN         PM_SUSPEND_TO_IDLE
> >  #define PM_SUSPEND_MAX         ((__force suspend_state_t) 4)
> >
> > +typedef int __bitwise standby_state_t;
> > +
> > +#define PM_STANDBY_ACTIVE      ((__force standby_state_t) 0)
> > +#define PM_STANDBY_INACTIVE    ((__force standby_state_t) 1)
> > +#define PM_STANDBY_SLEEP       ((__force standby_state_t) 2)
> > +#define PM_STANDBY_RESUME      ((__force standby_state_t) 3)
> > +#define PM_STANDBY_MIN         PM_STANDBY_ACTIVE
> > +#define PM_STANDBY_MAX         ((__force standby_state_t) 4)
> > +
> > +typedef int __bitwise standby_notification_t;
> > +
> > +#define PM_SN_INACTIVE_ENTRY   ((__force standby_notification_t) 0)
> > +#define PM_SN_INACTIVE_EXIT    ((__force standby_notification_t) 1)
> > +#define PM_SN_SLEEP_ENTRY      ((__force standby_notification_t) 2)
> > +#define PM_SN_SLEEP_EXIT       ((__force standby_notification_t) 3)
> > +#define PM_SN_RESUME           ((__force standby_notification_t) 4)
> > +#define PM_SN_MIN              PM_STANDBY_DISPLAY_OFF
> > +#define PM_SN_MAX              ((__force standby_notification_t) 5)
> > +
> >  /**
> >   * struct platform_suspend_ops - Callbacks for managing platform dependent
> >   *     system sleep states.
> > @@ -132,6 +151,8 @@ struct platform_suspend_ops {
> >  };
> >
> >  struct platform_s2idle_ops {
> > +       u8 (*get_standby_states)(void);
> > +       int (*do_notification)(standby_notification_t state);
>
> No, this needs to be separate from platform_s2idle_ops and maybe less ad-hoc.

Can you suggest an alternative location? I would prefer to avoid
introducing a new struct.

> >         int (*begin)(void);
> >         int (*prepare)(void);
> >         int (*prepare_late)(void);
> > @@ -276,6 +297,11 @@ extern void arch_suspend_enable_irqs(void);
> >
> >  extern int pm_suspend(suspend_state_t state);
> >  extern bool sync_on_suspend_enabled;
> > +
> > +extern void pm_standby_refresh_states(void);
> > +extern int pm_standby_transition(standby_state_t state);
> > +extern void pm_standby_set_state(standby_state_t state);
> > +extern int pm_standby_get_state(void);
> >  #else /* !CONFIG_SUSPEND */
> >  #define suspend_valid_only_mem NULL
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 2da4482bb6eb..ede1ba483fa5 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -46,12 +46,21 @@ static const char * const mem_sleep_labels[] = {
> >         [PM_SUSPEND_MEM] = "deep",
> >  };
> >  const char *mem_sleep_states[PM_SUSPEND_MAX];
> > +static const char * const standby_labels[] = {
> > +       [PM_STANDBY_ACTIVE] = "active",
> > +       [PM_STANDBY_INACTIVE] = "inactive",
> > +       [PM_STANDBY_SLEEP] = "sleep",
> > +       [PM_STANDBY_RESUME] = "resume",
> > +};
> > +const char *standby_states[PM_STANDBY_MAX];
> >
> >  suspend_state_t mem_sleep_current = PM_SUSPEND_TO_IDLE;
> >  suspend_state_t mem_sleep_default = PM_SUSPEND_MAX;
> >  suspend_state_t pm_suspend_target_state;
> >  EXPORT_SYMBOL_GPL(pm_suspend_target_state);
> >
> > +standby_state_t standby_current = PM_STANDBY_ACTIVE;
> > +
> >  unsigned int pm_suspend_global_flags;
> >  EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
> >
> > @@ -195,6 +204,9 @@ void __init pm_states_init(void)
> >          * initialize mem_sleep_states[] accordingly here.
> >          */
> >         mem_sleep_states[PM_SUSPEND_TO_IDLE] = mem_sleep_labels[PM_SUSPEND_TO_IDLE];
> > +
> > +       /* Always support the active runtime standby state. */
> > +       standby_states[PM_STANDBY_ACTIVE] = standby_labels[PM_STANDBY_ACTIVE];
> >  }
> >
> >  static int __init mem_sleep_default_setup(char *str)
> > @@ -334,6 +346,141 @@ static bool platform_suspend_again(suspend_state_t state)
> >                 suspend_ops->suspend_again() : false;
> >  }
> >
> > +static int platform_standby_notify(standby_notification_t state)
> > +{
> > +       return s2idle_ops && s2idle_ops->do_notification ?
> > +                      s2idle_ops->do_notification(state) :
> > +                      0;
> > +}
> > +
> > +/**
> > + * pm_standby_refresh_states - Refresh the supported runtime standby states
> > + */
> > +void pm_standby_refresh_states(void)
> > +{
> > +       u8 standby_support = s2idle_ops && s2idle_ops->get_standby_states ?
> > +                                    s2idle_ops->get_standby_states() :
> > +                                    0;
> > +
> > +       standby_states[PM_STANDBY_INACTIVE] =
> > +               standby_support & BIT(PM_STANDBY_INACTIVE) ?
> > +                       standby_labels[PM_STANDBY_INACTIVE] :
> > +                       NULL;
> > +       standby_states[PM_STANDBY_SLEEP] =
> > +               standby_support & BIT(PM_STANDBY_SLEEP) ?
> > +                       standby_labels[PM_STANDBY_SLEEP] :
> > +                       NULL;
> > +       standby_states[PM_STANDBY_RESUME] =
> > +               standby_support & BIT(PM_STANDBY_RESUME) ?
> > +                       standby_labels[PM_STANDBY_RESUME] :
> > +                       NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_standby_refresh_states);
> > +
> > +/**
> > + * pm_standby_transition - Transition between standby states
> > + *
> > + * Configure the runtime standby state of the system. Entering these states
> > + * may change the appearance of the system (e.g., keyboard backlight) or limit
> > + * the thermal envelope of the system (e.g., PLx to 5W).
> > + *
> > + * Returns an error if the transition fails. The function does not rollback.
> > + */
> > +int pm_standby_transition(standby_state_t state)
> > +{
> > +       int error;
> > +
> > +       if (state == standby_current)
> > +               return 0;
> > +       if (state > PM_STANDBY_MAX)
> > +               return -EINVAL;
> > +
> > +       pm_standby_refresh_states();
> > +
> > +       pm_pr_dbg("Transitioning from standby state %s to %s\n",
> > +                 standby_states[standby_current], standby_states[state]);
> > +
> > +       /* Resume can only be entered if we are on the sleep state. */
> > +       if (state == PM_STANDBY_RESUME) {
> > +               if (standby_current != PM_STANDBY_SLEEP)
> > +                       return -EINVAL;
> > +               standby_current = PM_STANDBY_RESUME;
> > +               return platform_standby_notify(PM_SN_RESUME);
> > +       }
> > +
> > +       /*
> > +        * The system should not be able to re-enter Sleep from resume as it
> > +        * is undefined behavior. As part of setting the state to "Resume",
> > +        * userspace promised a transition to "Inactive" or "Active".
> > +        */
> > +       if (standby_current == PM_STANDBY_RESUME && state == PM_STANDBY_SLEEP)
> > +               return -EINVAL;
> > +
> > +       /* Resume is the Sleep state logic-wise. */
> > +       if (standby_current == PM_STANDBY_RESUME)
> > +               standby_current = PM_STANDBY_SLEEP;
> > +
> > +       if (standby_current < state) {
> > +               for (; standby_current < state; standby_current++) {
> > +                       switch (standby_current + 1) {
> > +                       case PM_STANDBY_INACTIVE:
> > +                               error = platform_standby_notify(PM_SN_INACTIVE_ENTRY);
> > +                               break;
> > +                       case PM_STANDBY_SLEEP:
> > +                               error = platform_standby_notify(PM_SN_SLEEP_ENTRY);
> > +                               break;
> > +                       }
> > +
> > +                       if (error) {
> > +                               /* Rollback to previous valid state */
> > +                               while (standby_current > PM_STANDBY_ACTIVE &&
> > +                                      !standby_states[standby_current])
> > +                                       standby_current--;
> > +                               return error;
> > +                       }
> > +               }
> > +       } else if (standby_current > state) {
> > +               for (; standby_current > state; standby_current--) {
> > +                       switch (standby_current) {
> > +                       case PM_STANDBY_SLEEP:
> > +                               error = platform_standby_notify(PM_SN_SLEEP_EXIT);
> > +                               break;
> > +                       case PM_STANDBY_INACTIVE:
> > +                               error = platform_standby_notify(PM_SN_INACTIVE_EXIT);
> > +                               break;
> > +                       }
> > +
> > +                       if (error) {
> > +                               /* Rollback to previous valid state */
> > +                               while (standby_current < PM_STANDBY_SLEEP &&
> > +                                      !standby_states[standby_current])
> > +                                       standby_current++;
> > +                               return error;
> > +                       }
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * pm_standby_set_state - Set the current standby state and skip the transition
> > + */
> > +void pm_standby_set_state(standby_state_t state)
> > +{
> > +       standby_current = state;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_standby_set_state);
> > +
> > +/**
> > + * pm_standby_get_state - Returns the current standby state
> > + */
> > +int pm_standby_get_state(void)
> > +{
> > +       return standby_current;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_standby_get_state);
> > +
> >  #ifdef CONFIG_PM_DEBUG
> >  static unsigned int pm_test_delay = 5;
> >  module_param(pm_test_delay, uint, 0644);
> > @@ -572,6 +719,7 @@ static void suspend_finish(void)
> >   */
> >  static int enter_state(suspend_state_t state)
> >  {
> > +       standby_state_t standby_prior;
> >         int error;
> >
> >         trace_suspend_resume(TPS("suspend_enter"), state, true);
> > @@ -588,6 +736,9 @@ static int enter_state(suspend_state_t state)
> >         if (!mutex_trylock(&system_transition_mutex))
> >                 return -EBUSY;
> >
> > +       standby_prior = standby_current;
> > +       pm_standby_transition(PM_STANDBY_SLEEP);
>
> I'm not sure if it is actually safe to evaluate PNP0D80 _DSM function
> 7 (sleep entry notification) at this point because of possible
> ordering constraints.
>
> As I said once before in another thread, I would like the platform
> notifications to be moved to the places in which you want them to take
> place before making any other changes and only once this is proven to
> work, further changes can be made.

Following the conclusion of this discussion I can spin a non-RFC
keeping the rename and doing a simple re-ordering, plus cleaning up
the asus related code for the Ally so that it works correctly. This
way, the re-ordering can be tested individually and merged first.

Antheas

> > +
> >         if (state == PM_SUSPEND_TO_IDLE)
> >                 s2idle_begin();
> >
> > @@ -619,6 +770,8 @@ static int enter_state(suspend_state_t state)
> >         pm_pr_dbg("Finishing wakeup.\n");
> >         suspend_finish();
> >   Unlock:
> > +       pm_standby_transition(standby_prior);
> > +
> >         mutex_unlock(&system_transition_mutex);
> >         return error;
> >  }
> > --
>