Messages in this thread | ![/](/images/icornerl.gif) | | From | John Stultz <> | Date | Fri, 9 Feb 2024 11:30:01 -0800 | 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@google.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
| ![\](/images/icornerr.gif) |