Messages in this thread | | | Date | Wed, 30 Aug 2023 21:23:54 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset |
| |
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@chromium.org> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> 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
| |