linux-kernel - Re: [PATCH v2 2/2] alarmtimer: Modify alarmtimer suspend callback to check for imminent alarm using PM notifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANDhNCoNuAXSn7-M1SY6q+RZoPjEa1_dHzpdG20et2Sbm5AoDw@mail.gmail.com>
Date: Fri, 9 Feb 2024 11:30:01 -0800
From: John Stultz <jstultz@...gle.com>
To: Pranav Prasad <pranavpp@...gle.com>
Cc: tglx@...utronix.de, sboyd@...nel.org, linux-kernel@...r.kernel.org, 
	krossmo@...gle.com
Subject: Re: [PATCH v2 2/2] alarmtimer: Modify alarmtimer suspend callback to
 check for imminent alarm using PM notifier

On Thu, Feb 8, 2024 at 11:56 AM Pranav Prasad <pranavpp@...gle.com> wrote:
>
> The alarmtimer driver currently fails suspend attempts when there is an
> alarm pending within the next suspend_check_duration_ns nanoseconds, since
> the system is expected to wake up soon anyway. The entire suspend process
> is initiated even though the system will immediately awaken. This process
> includes substantial work before the suspend fails and additional work
> afterwards to undo the failed suspend that was attempted. Therefore on
> battery-powered devices that initiate suspend attempts from userspace, it
> may be advantageous to be able to fail the suspend earlier in the suspend
> flow to avoid power consumption instead of unnecessarily doing extra work.
> As one data point, an analysis of a subset of Android devices showed that
> imminent alarms account for roughly 40% of all suspend failures on average
> leading to unnecessary power wastage.
>
> To facilitate this, register a PM notifier in the alarmtimer subsystem
> that checks if an alarm is imminent during the prepare stage of kernel
> suspend denoted by the event PM_SUSPEND_PREPARE. If an alarm is imminent,
> it returns the errno code ETIME instead of EBUSY to userspace in order to
> make it easily diagnosable.

Thanks for continuing to iterate on this!

One concern below...

> +static int alarmtimer_pm_callback(struct notifier_block *nb,
> +                           unsigned long mode, void *_unused)
> +{
> +       ktime_t min, expires;
> +       struct rtc_device *rtc = NULL;
> +       int type;
> +
> +       switch (mode) {
> +       case PM_SUSPEND_PREPARE:
> +               /* Find the soonest timer to expire */
> +               if (!alarmtimer_get_soonest(rtc, &min, &expires, &type))
> +                       return NOTIFY_DONE;
> +
> +               if (ktime_to_ns(min) <
> +                       suspend_check_duration_ms * NSEC_PER_MSEC) {
> +                       pr_warn("[%s] Suspend abort due to imminent alarm\n", __func__);
> +                       pm_wakeup_event(&rtc->dev, suspend_check_duration_ms);
> +                       return notifier_from_errno(-ETIME);
> +               }
> +       }
> +
> +       return NOTIFY_DONE;
> +}
> +

So the alarmtimer_pm_callback provides an earlier warning that we have
an imminent alarm, looks ok to me.


> @@ -296,49 +379,14 @@ EXPORT_SYMBOL_GPL(alarm_expires_remaining);
>  static int alarmtimer_suspend(struct device *dev)
>  {
..
> +       /* Find the soonest timer to expire */
> +       if (!alarmtimer_get_soonest(rtc, &min, &expires, &type))
>                 return 0;
>
> -       if (ktime_to_ns(min) < suspend_check_duration_ms * NSEC_PER_MSEC) {
> -               pm_wakeup_event(dev, suspend_check_duration_ms);
> -               return -EBUSY;
> -       }

It seems like we'd want to preserve the check in alarmtimer_suspend()
as well, no? As the various suspend calls might take awhile and in
that time, the next timer may have slipped into the window of being
imminent.

thanks
-john