[v3,1/2] alarmtimer: Add PM notifier to check early for imminent alarm
Commit Message
The alarmtimer driver currently fails suspend attempts when there is an
alarm pending within the next 2 seconds, 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.
Signed-off-by: Pranav Prasad <pranavpp@google.com>
---
kernel/time/alarmtimer.c | 100 +++++++++++++++++++++++++++++++--------
1 file changed, 79 insertions(+), 21 deletions(-)
Comments
On Wed, Feb 14 2024 at 09:29, Pranav Prasad wrote:
> +static int alarmtimer_pm_callback(struct notifier_block *nb,
> + unsigned long mode, void *_unused)
> +{
> + struct rtc_device *rtc;
> + ktime_t min, expires;
> + int type;
> +
> + switch (mode) {
> + case PM_SUSPEND_PREPARE:
> + rtc = alarmtimer_get_rtcdev();
> + /* If we have no rtcdev, just return */
> + if (!rtc)
> + return NOTIFY_DONE;
> +
> + /* Find the soonest timer to expire */
> + if (!alarmtimer_get_soonest(&min, &expires, &type))
> + return NOTIFY_DONE;
Brilliant. Instead of a NULL pointer dereference you decided to add
undefined behaviour this time.
As it survived your "testing" it is obviously correct, right?
I'm tired of your approach to throw stuff at the wall in a hurry and see
what sticks.
Stop this frenzy. Sit down, take your time and do proper engineering
before coming back with this to me.
Thanks,
tglx
@@ -27,6 +27,7 @@
#include <linux/compat.h>
#include <linux/module.h>
#include <linux/time_namespace.h>
+#include <linux/suspend.h>
#include "posix-timers.h"
@@ -63,6 +64,78 @@ static struct rtc_timer rtctimer;
static struct rtc_device *rtcdev;
static DEFINE_SPINLOCK(rtcdev_lock);
+/**
+ * alarmtimer_get_soonest - Finds the soonest alarm to expire among the
+ * alarm bases.
+ * @min: ptr to relative time to the soonest alarm to expire
+ * @expires: ptr to absolute time of the soonest alarm to expire
+ * @type: ptr to alarm type
+ *
+ * Returns true if soonest alarm was found, returns false if don't care.
+ */
+static bool alarmtimer_get_soonest(ktime_t *min,
+ ktime_t *expires, int *type)
+{
+ unsigned long flags;
+ int i;
+
+ /* Find the soonest timer to expire */
+ for (i = 0; i < ALARM_NUMTYPE; i++) {
+ struct alarm_base *base = &alarm_bases[i];
+ struct timerqueue_node *next;
+ ktime_t delta;
+
+ spin_lock_irqsave(&base->lock, flags);
+ next = timerqueue_getnext(&base->timerqueue);
+ spin_unlock_irqrestore(&base->lock, flags);
+ if (!next)
+ continue;
+ delta = ktime_sub(next->expires, base->get_ktime());
+ if (*min == 0 || delta < *min) {
+ *expires = next->expires;
+ *min = delta;
+ *type = i;
+ }
+ }
+
+ if (*min == 0)
+ return false;
+
+ return true;
+}
+
+static int alarmtimer_pm_callback(struct notifier_block *nb,
+ unsigned long mode, void *_unused)
+{
+ struct rtc_device *rtc;
+ ktime_t min, expires;
+ int type;
+
+ switch (mode) {
+ case PM_SUSPEND_PREPARE:
+ rtc = alarmtimer_get_rtcdev();
+ /* If we have no rtcdev, just return */
+ if (!rtc)
+ return NOTIFY_DONE;
+
+ /* Find the soonest timer to expire */
+ if (!alarmtimer_get_soonest(&min, &expires, &type))
+ return NOTIFY_DONE;
+
+ if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
+ pr_debug("Suspend abort due to imminent alarm\n");
+ pm_wakeup_event(&rtc->dev, 2 * MSEC_PER_SEC);
+ return notifier_from_errno(-ETIME);
+ }
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block alarmtimer_pm_notifier = {
+ .notifier_call = alarmtimer_pm_callback,
+};
+
/**
* alarmtimer_get_rtcdev - Return selected rtcdevice
*
@@ -126,6 +199,7 @@ static int alarmtimer_rtc_add_device(struct device *dev)
static inline void alarmtimer_rtc_timer_init(void)
{
rtc_timer_init(&rtctimer, NULL, NULL);
+ register_pm_notifier(&alarmtimer_pm_notifier);
}
static struct class_interface alarmtimer_rtc_interface = {
@@ -241,10 +315,10 @@ EXPORT_SYMBOL_GPL(alarm_expires_remaining);
static int alarmtimer_suspend(struct device *dev)
{
ktime_t min, now, expires;
- int i, ret, type;
struct rtc_device *rtc;
unsigned long flags;
struct rtc_time tm;
+ int ret, type;
spin_lock_irqsave(&freezer_delta_lock, flags);
min = freezer_delta;
@@ -258,30 +332,14 @@ static int alarmtimer_suspend(struct device *dev)
if (!rtc)
return 0;
- /* Find the soonest timer to expire*/
- for (i = 0; i < ALARM_NUMTYPE; i++) {
- struct alarm_base *base = &alarm_bases[i];
- struct timerqueue_node *next;
- ktime_t delta;
-
- spin_lock_irqsave(&base->lock, flags);
- next = timerqueue_getnext(&base->timerqueue);
- spin_unlock_irqrestore(&base->lock, flags);
- if (!next)
- continue;
- delta = ktime_sub(next->expires, base->get_ktime());
- if (!min || (delta < min)) {
- expires = next->expires;
- min = delta;
- type = i;
- }
- }
- if (min == 0)
+ /* Find the soonest timer to expire */
+ if (!alarmtimer_get_soonest(&min, &expires, &type))
return 0;
if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
+ pr_debug("Suspend abort due to imminent alarm\n");
pm_wakeup_event(dev, 2 * MSEC_PER_SEC);
- return -EBUSY;
+ return -ETIME;
}
trace_alarmtimer_suspend(expires, type);