On 8/30/23 14:16, Stephen Boyd wrote:
Quoting Guenter Roeck (2023-08-30 00:13:09)
On 8/29/23 14:50, Stephen Boyd wrote:
Quoting Guenter Roeck (2023-08-17 15:55:32)
Some userspace applications use timerfd_create() to request wakeups after
a long period of time. For example, a backup application may request a
wakeup once per week. This is perfectly fine as long as the system does
not try to suspend. However, if the system tries to suspend and the
system's RTC does not support the required alarm timeout, the suspend
operation will fail with an error such as
rtc_cmos 00:01: Alarms can be up to one day in the future
PM: dpm_run_callback(): platform_pm_suspend+0x0/0x4a returns -22
alarmtimer alarmtimer.4.auto: platform_pm_suspend+0x0/0x4a returned -22 after 117 usecs
PM: Device alarmtimer.4.auto failed to suspend: error -22
This results in a refusal to suspend the system, causing substantial
battery drain on affected systems.
To fix the problem, use the maximum alarm time offset as reported by rtc
drivers to set the maximum alarm time. While this will result in brief
spurious wakeups from suspend, it is still much better than not suspending
at all.
Cc: Brian Norris <briannorris@xxxxxxxxxxxx>
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
v2: Rename range_max_offset -> alarm_offset_max
kernel/time/alarmtimer.c | 13 +++++++++++++
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 8d9f13d847f0..895e3a6d6444 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -290,6 +290,19 @@ static int alarmtimer_suspend(struct device *dev)
rtc_timer_cancel(rtc, &rtctimer);
rtc_read_time(rtc, &tm);
now = rtc_tm_to_ktime(tm);
+
+ /*
+ * If the RTC alarm timer only supports a limited time offset, set
+ * the alarm time to the maximum supported value.
+ * The system will wake up earlier than necessary and is expected
+ * to go back to sleep if it has nothing to do.
Does this assume that the kernel is configured for autosuspend
(CONFIG_PM_AUTOSLEEP)? Maybe we should only do this when that config is
enabled.
It doesn't really assume anything. It standardizes behavior if the rtc
does not support the requested alarm time. Today that either fails
or the rtc silently adjusts the alarm time (sometimes to 1 day + 1 minute ->
one minute) depending on the implementation in the rtc driver. With this
patch in place, the the rtc driver informing the rtc core about the limit,
the alarm would fire at the maximum time supported by the rtc if the
requested alarm time is larger than its limit.
I see that as improvement, no matter if CONFIG_PM_AUTOSLEEP is enabled or not.
Agreed. It's an improvement.
The ABI of alarmtimers seem to be "Run this timer at time X, and wake up
the system from suspend if necessary to run the timer at time X".
If userspace is the one autosuspending, then I don't know what we do, or
how the kernel knows it is OK. Maybe we need another alarmtimer clock id
that will fail creation if the wakeup time is larger than what the rtc
can be programmed for? Or maybe that new clock id can have this fixed
behavior to wakeup early with the assumption that userspace will go back
to sleep, and outdated userspace can use the original alarmtimer clock
id if they don't care about suspend failing?
I don't know how to answer this. Again, I see my suggested patch as improvement
over not suspending at all or resuming at a more or less random time, which
is what you get today depending on the rtc driver.
Actually, I would argue that the above situation applies even if the rtc supports
the requested alarm time. Currently, if userspace is the one autosuspending,
the system wakes up after the alarm time expires (assuming the rtc supports it).
Then what ? Your above question applies to that situation as well and is
really independent of the alarm time limit supported by the rtc.
The comment in the code is causing me confusion. It says
The system will wake up earlier than necessary and is expected
to go back to sleep if it has nothing to do.
I'd reword this to not talk about auto-suspend because the ABI of
alarmtimers doesn't concern itself with autosuspend.
The system will wake up earlier (possibly much earlier) than when the
alarmtimer runs. This is the best the kernel can do because the
alarmtimer exceeds the time that the rtc device can be programmed for.
Makes sense, and I agree that this is much better. I changed the comment
accordingly.
I would agree that various improvements on how to handle the situation where
the requested alarm time is larger than the rtc limit may be possible,
but I see those as independent and orthogonal to this patch.
I certainly hope that userspace isn't relying on the existing behavior.
I see another problem too. What do we do if an alarmtimer is created,
the rtc device is unregistered, and then we enter suspend? It looks like
alarmtimer_suspend() bails out early with no error, so suspend
continues. That looks wrong. Presumably we should fail suspend entirely
at that point because we'll never be able to wakeup to run the
alarmtimer.
Maybe I am missing something, but I think this is equivalent of not having
an rtc in the system, or for CONFIG_RTC_CLASS=n. Currently the system just
suspends without waking up in those situations. Changing that would be a
substantial functional change since suddenly systems without rtc would
simply fail to suspend if there is a pending alarm.
We fail alarmtimer creation in the case that CONFIG_RTC_CLASS=n or when
there isn't an rtc. See alarmtimer_get_rtcdev() and how it is called. I
doubt it ever really happens in practice, but it looks possible to
simulate by unbinding the rtc device driver.
Thanks for the clarification. That really makes me wonder what happens
if an rtc device is unregistered. The .remove_dev callback of
alarmtimer_rtc_interface is not populated, and rtc_dev is never cleared.
That means unbinding an rtc device driver should result in a crash.
Am I missing something ?
Thanks,
Guenter
