Message-ID: <567a09b4-2184-1bd8-a3fc-8a5fd4d682ad@roeck-us.net>
Date:   Wed, 30 Aug 2023 21:23:54 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Stephen Boyd <sboyd@...nel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     Alessandro Zummo <a.zummo@...ertech.it>,
        Benson Leung <bleung@...omium.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        John Stultz <jstultz@...gle.com>, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Brian Norris <briannorris@...omium.org>
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@...omium.org>
>>>> Signed-off-by: Guenter Roeck <linux@...ck-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

Powered by blists - more mailing lists