* [PATCH v2 1/7] rtc: Add support for limited alarm timer offsets
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-23 16:50 ` Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset Guenter Roeck
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
Some alarm timers are based on time offsets, not on absolute times.
In some situations, the amount of time that can be scheduled in the
future is limited. This may result in a refusal to suspend the system,
causing substantial battery drain.
Some RTC alarm drivers remedy the situation by setting the alarm time
to the maximum supported time if a request for an out-of-range timeout
is made. This is not really desirable since it may result in unexpected
early wakeups.
To reduce the impact of this problem, let RTC drivers report the maximum
supported alarm timer offset. The code setting alarm timers can then
decide if it wants to reject setting alarm timers to a larger value, if it
wants to implement recurring alarms until the actually requested alarm
time is met, or if it wants to accept the limited alarm time.
Only introduce the necessary variable into struct rtc_device.
Code to set and use the variable will follow with subsequent patches.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
include/linux/rtc.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 1fd9c6a21ebe..4c0bcbeb1f00 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -146,6 +146,7 @@ struct rtc_device {
time64_t range_min;
timeu64_t range_max;
+ timeu64_t alarm_offset_max;
time64_t start_secs;
time64_t offset_secs;
bool set_start_time;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/7] rtc: Add support for limited alarm timer offsets
2023-08-17 22:55 ` [PATCH v2 1/7] " Guenter Roeck
@ 2023-08-23 16:50 ` Guenter Roeck
2023-08-23 22:51 ` Alexandre Belloni
0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2023-08-23 16:50 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris
Hi Alexandre,
On Thu, Aug 17, 2023 at 03:55:31PM -0700, Guenter Roeck wrote:
> Some alarm timers are based on time offsets, not on absolute times.
> In some situations, the amount of time that can be scheduled in the
> future is limited. This may result in a refusal to suspend the system,
> causing substantial battery drain.
>
> Some RTC alarm drivers remedy the situation by setting the alarm time
> to the maximum supported time if a request for an out-of-range timeout
> is made. This is not really desirable since it may result in unexpected
> early wakeups.
>
> To reduce the impact of this problem, let RTC drivers report the maximum
> supported alarm timer offset. The code setting alarm timers can then
> decide if it wants to reject setting alarm timers to a larger value, if it
> wants to implement recurring alarms until the actually requested alarm
> time is met, or if it wants to accept the limited alarm time.
>
> Only introduce the necessary variable into struct rtc_device.
> Code to set and use the variable will follow with subsequent patches.
>
> Cc: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
I guess it is a bit late to get the series into v6.6, but would it be
possible to apply it to a -next branch to get some more test coverage ?
Either case, do you have any additional comments / feedback ?
Thanks,
Guenter
> ---
> v2: Rename range_max_offset -> alarm_offset_max
>
> include/linux/rtc.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 1fd9c6a21ebe..4c0bcbeb1f00 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -146,6 +146,7 @@ struct rtc_device {
>
> time64_t range_min;
> timeu64_t range_max;
> + timeu64_t alarm_offset_max;
> time64_t start_secs;
> time64_t offset_secs;
> bool set_start_time;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/7] rtc: Add support for limited alarm timer offsets
2023-08-23 16:50 ` Guenter Roeck
@ 2023-08-23 22:51 ` Alexandre Belloni
2023-08-24 3:26 ` Guenter Roeck
0 siblings, 1 reply; 21+ messages in thread
From: Alexandre Belloni @ 2023-08-23 22:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris
Hello,
On 23/08/2023 09:50:47-0700, Guenter Roeck wrote:
> Hi Alexandre,
>
> On Thu, Aug 17, 2023 at 03:55:31PM -0700, Guenter Roeck wrote:
> > Some alarm timers are based on time offsets, not on absolute times.
> > In some situations, the amount of time that can be scheduled in the
> > future is limited. This may result in a refusal to suspend the system,
> > causing substantial battery drain.
> >
> > Some RTC alarm drivers remedy the situation by setting the alarm time
> > to the maximum supported time if a request for an out-of-range timeout
> > is made. This is not really desirable since it may result in unexpected
> > early wakeups.
> >
> > To reduce the impact of this problem, let RTC drivers report the maximum
> > supported alarm timer offset. The code setting alarm timers can then
> > decide if it wants to reject setting alarm timers to a larger value, if it
> > wants to implement recurring alarms until the actually requested alarm
> > time is met, or if it wants to accept the limited alarm time.
> >
> > Only introduce the necessary variable into struct rtc_device.
> > Code to set and use the variable will follow with subsequent patches.
> >
> > Cc: Brian Norris <briannorris@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> I guess it is a bit late to get the series into v6.6, but would it be
> possible to apply it to a -next branch to get some more test coverage ?
>
I'm probably going to take 1 and 3-7 for 6.6 once I get a reliable
internet access. I can't take 2/7 without a review or ack from the time
maintainers.
> Either case, do you have any additional comments / feedback ?
>
The main issue that remains is that after 2/7, the rtc_device structure
is not opaque anymore to its user as alarmtimer_suspend now directly
accesses one of the members. But I'd have to find which RTCs have an
absolute limit so we can design a proper API. I may also decide that it
is good enough to require that the alarm range must cover the registered
RTC range.
> Thanks,
> Guenter
>
> > ---
> > v2: Rename range_max_offset -> alarm_offset_max
> >
> > include/linux/rtc.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > index 1fd9c6a21ebe..4c0bcbeb1f00 100644
> > --- a/include/linux/rtc.h
> > +++ b/include/linux/rtc.h
> > @@ -146,6 +146,7 @@ struct rtc_device {
> >
> > time64_t range_min;
> > timeu64_t range_max;
> > + timeu64_t alarm_offset_max;
> > time64_t start_secs;
> > time64_t offset_secs;
> > bool set_start_time;
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/7] rtc: Add support for limited alarm timer offsets
2023-08-23 22:51 ` Alexandre Belloni
@ 2023-08-24 3:26 ` Guenter Roeck
0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-24 3:26 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris
On Thu, Aug 24, 2023 at 12:51:29AM +0200, Alexandre Belloni wrote:
> Hello,
>
> On 23/08/2023 09:50:47-0700, Guenter Roeck wrote:
> > Hi Alexandre,
> >
> > On Thu, Aug 17, 2023 at 03:55:31PM -0700, Guenter Roeck wrote:
> > > Some alarm timers are based on time offsets, not on absolute times.
> > > In some situations, the amount of time that can be scheduled in the
> > > future is limited. This may result in a refusal to suspend the system,
> > > causing substantial battery drain.
> > >
> > > Some RTC alarm drivers remedy the situation by setting the alarm time
> > > to the maximum supported time if a request for an out-of-range timeout
> > > is made. This is not really desirable since it may result in unexpected
> > > early wakeups.
> > >
> > > To reduce the impact of this problem, let RTC drivers report the maximum
> > > supported alarm timer offset. The code setting alarm timers can then
> > > decide if it wants to reject setting alarm timers to a larger value, if it
> > > wants to implement recurring alarms until the actually requested alarm
> > > time is met, or if it wants to accept the limited alarm time.
> > >
> > > Only introduce the necessary variable into struct rtc_device.
> > > Code to set and use the variable will follow with subsequent patches.
> > >
> > > Cc: Brian Norris <briannorris@chromium.org>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >
> > I guess it is a bit late to get the series into v6.6, but would it be
> > possible to apply it to a -next branch to get some more test coverage ?
> >
>
> I'm probably going to take 1 and 3-7 for 6.6 once I get a reliable
> internet access. I can't take 2/7 without a review or ack from the time
> maintainers.
>
Ok, makes sense.
> > Either case, do you have any additional comments / feedback ?
> >
>
> The main issue that remains is that after 2/7, the rtc_device structure
> is not opaque anymore to its user as alarmtimer_suspend now directly
> accesses one of the members. But I'd have to find which RTCs have an
> absolute limit so we can design a proper API. I may also decide that it
> is good enough to require that the alarm range must cover the registered
> RTC range.
>
What exactly do you have in mind ? For our use case, we need
alarmtimer_suspend() to either sleep for the requested time, or as long
as possible. Would an API function returning the alarm limit address
your concerns ?
Regarding relative/absolute, it is a mixed bag. Below is what I found
for the alphabetically first ~40 drivers. Note that "absolute" in the
configuration column means that the alarm time is specified as absolute
value (e.g, at 1am), not that the limit is absolute or relative to the
current time. The limit column gives a hint if the limit is relative
or absolute. In many cases the alarm is configured as absolute value
but limited to day/week/month/year in the future.
On a side note, struct rtc_device isn't exactly opaque even today.
Both alarmtimer.c and ntp.c already access some of its members.
Thanks,
Guenter
---
driver configuration limit comments
rtc-88pm80x.c relative 24 hours silently adjusted
rtc-88pm860x.c relative 32 bit seconds unchecked
rtc-ab8500.c absolute ~2032 unchecked
rtc-ab-b5ze-s3.c relative 1 month -EINVAL
rtc-ab-eoz9.c absolute 1 month month & year ignored (1 month + 1 day -> fires after 1 day)
rtc-abx80x.c absolute 1 year year ignored (1 year + 1 day -> fires after 1 day)
rtc-ac100.c absolute 2069 -EINVAL
rtc-armada38x.c absolute 2106 unchecked
rtc-as3722.c absolute unlimited unchecked
rtc-asm9260.c absolute unlimited unchecked
rtc-at91rm9200.c absolute 1 year year ignored (1 year + 1 day -> fires after 1 day)
rtc-at91sam9.c relative 32 bit seconds unchecked
rtc-bd70528.c absolute 2099 unchecked
rtc-brcmstb-waketimer.c absolute 2106 unchecked
rtc-cadence.c absolute 2999 unchecked
rtc-cmos.c absolute day/month/year -EINVAL if out of range
rtc-cpcap.c absolute ~2059 in days since 1970, day masked, unchecked (2060 -> 1971)
rtc-cros-ec.c relative 24h or max -EINVAL
rtc-da9052.c absolute 2063 unchecked, year masked
rtc-da9055.c absolute 2133 ? unchecked, year masked
rtc-da9063.c absolute 2063 unchecked, year masked
rtc-digicolor.c relative 32-bit seconds unchecked
rtc-ds1286.c absolute 24 hours unchecked, month & year ignored
rtc-ds1305.c absolute 24 hours -EINVAL
rtc-ds1307.c absolute 1 year unchecked, year ignored
rtc-ds1343.c absolute 1 month unchecked, month / year ignored
rtc-ds1374.c relative ~194 days unchecked, masked (195 days -> fires after 1 day)
rtc-ds1511.c absolute 1 month unchecked, month/year ignored
rtc-ds1553.c absolute 1 month unchecked, month/year ignored
rtc-ds1685.c absolute 1 month unchecked, month/year ignored
rtc-ds3232.c absolute 1 month unchecked, month/year ignored
rtc-efi.c absolute not specified several years
rtc-fm3130.c absolute 1 year year ignored
rtc-fsl-ftm-alarm.c relative 262 seconds -ERANGE
rtc-goldfish.c absolute ~500 years unchecked
rtc-hym8563.c absolute 1 month unchecked, month/year ignored
rtc-imxdi.c abolute 2106 unchecked (32 bit seconds since 1970)
rtc-imx-sc.c absolute unknown Value passed to SCU which determines if valid
rtc-isl1208.c absolute 1 year unchecked, year ignored
rtc-jz4740.c absolute 2106 unchecked (32 bit seconds since 1970)
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 1/7] " Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-25 3:52 ` John Stultz
2023-08-29 21:50 ` Stephen Boyd
2023-08-17 22:55 ` [PATCH v2 3/7] rtc: cros-ec: Detect and report supported alarm window size Guenter Roeck
` (5 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
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 +++++++++++++
1 file changed, 13 insertions(+)
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.
+ * It would be desirable to handle such early wakeups without fully
+ * waking up the system, but it is unknown if this is even possible.
+ */
+ if (rtc->alarm_offset_max &&
+ rtc->alarm_offset_max * MSEC_PER_SEC < ktime_to_ms(min))
+ min = ms_to_ktime(rtc->alarm_offset_max * MSEC_PER_SEC);
+
now = ktime_add(now, min);
/* Set alarm, if in the past reject suspend briefly to handle */
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-17 22:55 ` [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset Guenter Roeck
@ 2023-08-25 3:52 ` John Stultz
2023-08-25 5:46 ` Guenter Roeck
2023-08-26 11:15 ` Guenter Roeck
2023-08-29 21:50 ` Stephen Boyd
1 sibling, 2 replies; 21+ messages in thread
From: John Stultz @ 2023-08-25 3:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexandre Belloni, Alessandro Zummo, Benson Leung, Miquel Raynal,
Thomas Gleixner, Stephen Boyd, linux-rtc, linux-kernel,
Brian Norris
On Thu, Aug 17, 2023 at 3:55 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> 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.
> + * It would be desirable to handle such early wakeups without fully
> + * waking up the system, but it is unknown if this is even possible.
> + */
> + if (rtc->alarm_offset_max &&
> + rtc->alarm_offset_max * MSEC_PER_SEC < ktime_to_ms(min))
> + min = ms_to_ktime(rtc->alarm_offset_max * MSEC_PER_SEC);
I don't really have an objection here, but I wonder if this would be
better abstracted by a rtc_ function?
ktime_t rtc_bound_ktime_interval(ktime interval)
{
if (!rtc->alarm_offset_max)
return interval;
return ms_to_ktime(min(rtc->alarm_offset_max, ktime_to_ms(interval)));
}
(simple enough to throw into rtc.h maybe as an inline function?)
Then the above would be tweaked to:
min = rtc_bound_interval(min);
thanks
-john
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-25 3:52 ` John Stultz
@ 2023-08-25 5:46 ` Guenter Roeck
2023-08-26 11:15 ` Guenter Roeck
1 sibling, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-25 5:46 UTC (permalink / raw)
To: John Stultz
Cc: Alexandre Belloni, Alessandro Zummo, Benson Leung, Miquel Raynal,
Thomas Gleixner, Stephen Boyd, linux-rtc, linux-kernel,
Brian Norris
On Thu, Aug 24, 2023 at 08:52:44PM -0700, John Stultz wrote:
> On Thu, Aug 17, 2023 at 3:55 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > 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 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > 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.
> > + * It would be desirable to handle such early wakeups without fully
> > + * waking up the system, but it is unknown if this is even possible.
> > + */
> > + if (rtc->alarm_offset_max &&
> > + rtc->alarm_offset_max * MSEC_PER_SEC < ktime_to_ms(min))
> > + min = ms_to_ktime(rtc->alarm_offset_max * MSEC_PER_SEC);
>
> I don't really have an objection here, but I wonder if this would be
> better abstracted by a rtc_ function?
>
> ktime_t rtc_bound_ktime_interval(ktime interval)
Probably more like like rtc_bound_alarm_interval(),
but fine with me.
> {
> if (!rtc->alarm_offset_max)
> return interval;
> return ms_to_ktime(min(rtc->alarm_offset_max, ktime_to_ms(interval)));
alarm_offset_max is in seconds, so that would need some tweaking.
Guenter
> }
>
> (simple enough to throw into rtc.h maybe as an inline function?)
>
> Then the above would be tweaked to:
> min = rtc_bound_interval(min);
>
> thanks
> -john
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-25 3:52 ` John Stultz
2023-08-25 5:46 ` Guenter Roeck
@ 2023-08-26 11:15 ` Guenter Roeck
1 sibling, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-26 11:15 UTC (permalink / raw)
To: John Stultz
Cc: Alexandre Belloni, Alessandro Zummo, Benson Leung, Miquel Raynal,
Thomas Gleixner, Stephen Boyd, linux-rtc, linux-kernel,
Brian Norris
On Thu, Aug 24, 2023 at 08:52:44PM -0700, John Stultz wrote:
> On Thu, Aug 17, 2023 at 3:55 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > 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 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > 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.
Side note, since someone asked: The behavior is exactly what we see today
if a rtc driver silently truncates the requested alarm time. The system
is _expected_ to to back to sleep, but what it does depends on its actual
configuration. For example, if suspend was requested manually and
auto-suspend is disabled, it may well be that the system stays up after
the initial timeout. Again, that is exactly the same behavior that
is observed today, but with this patch the system doesn't fail the
initial suspend request because the requested timeout is not supported
by the rtc driver.
Not sure if I should add some text along that line above for clarification.
If so please let me know.
> > + * It would be desirable to handle such early wakeups without fully
> > + * waking up the system, but it is unknown if this is even possible.
> > + */
> > + if (rtc->alarm_offset_max &&
> > + rtc->alarm_offset_max * MSEC_PER_SEC < ktime_to_ms(min))
> > + min = ms_to_ktime(rtc->alarm_offset_max * MSEC_PER_SEC);
>
> I don't really have an objection here, but I wonder if this would be
> better abstracted by a rtc_ function?
>
> ktime_t rtc_bound_ktime_interval(ktime interval)
> {
> if (!rtc->alarm_offset_max)
> return interval;
> return ms_to_ktime(min(rtc->alarm_offset_max, ktime_to_ms(interval)));
> }
>
> (simple enough to throw into rtc.h maybe as an inline function?)
>
> Then the above would be tweaked to:
> min = rtc_bound_interval(min);
>
How should I proceed ? Would sending two patches on top of patch 1
to introduce the API and use it be ok ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-17 22:55 ` [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset Guenter Roeck
2023-08-25 3:52 ` John Stultz
@ 2023-08-29 21:50 ` Stephen Boyd
2023-08-30 7:13 ` Guenter Roeck
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2023-08-29 21:50 UTC (permalink / raw)
To: Alexandre Belloni, Guenter Roeck
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
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 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> 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.
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 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-29 21:50 ` Stephen Boyd
@ 2023-08-30 7:13 ` Guenter Roeck
2023-08-30 21:16 ` Stephen Boyd
0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2023-08-30 7:13 UTC (permalink / raw)
To: Stephen Boyd, Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, linux-rtc, linux-kernel, Brian Norris
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.
> 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.
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 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.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-30 7:13 ` Guenter Roeck
@ 2023-08-30 21:16 ` Stephen Boyd
2023-08-31 4:23 ` Guenter Roeck
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2023-08-30 21:16 UTC (permalink / raw)
To: Alexandre Belloni, Guenter Roeck
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, linux-rtc, linux-kernel, Brian Norris
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.
>
> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-30 21:16 ` Stephen Boyd
@ 2023-08-31 4:23 ` Guenter Roeck
2023-09-06 21:06 ` Stephen Boyd
0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2023-08-31 4:23 UTC (permalink / raw)
To: Stephen Boyd, Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, linux-rtc, linux-kernel, Brian Norris
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset
2023-08-31 4:23 ` Guenter Roeck
@ 2023-09-06 21:06 ` Stephen Boyd
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2023-09-06 21:06 UTC (permalink / raw)
To: Alexandre Belloni, Guenter Roeck
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, linux-rtc, linux-kernel, Brian Norris
Quoting Guenter Roeck (2023-08-30 21:23:54)
> On 8/30/23 14:16, Stephen Boyd wrote:
> >
> > 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 ?
>
Yeah it looks like a potential problem, but most likely nobody actually
removes the rtc device from the system. It would be good to handle this
case in a followup patch anyway though.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/7] rtc: cros-ec: Detect and report supported alarm window size
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 1/7] " Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 2/7] rtc: alarmtimer: Use maximum alarm time offset Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 4/7] rtc: cmos: Report supported alarm limit to rtc infrastructure Guenter Roeck
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
The RTC on some older Chromebooks can only handle alarms less than
24 hours in the future. The only way to find out is to try to set
an alarm further in the future. If that fails, assume that the RTC
connected to the EC can only handle less than 24 hours of alarm
window, and report that value to the RTC core.
After that change, it is no longer necessary to limit the alarm time
when setting it. Report any excessive alarms to the caller instead.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
drivers/rtc/rtc-cros-ec.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
index 998ab8606f0b..0cd397c04ff0 100644
--- a/drivers/rtc/rtc-cros-ec.c
+++ b/drivers/rtc/rtc-cros-ec.c
@@ -182,21 +182,15 @@ static int cros_ec_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM, alarm_offset);
if (ret < 0) {
- if (ret == -EINVAL && alarm_offset >= SECS_PER_DAY) {
- /*
- * RTC chips on some older Chromebooks can only handle
- * alarms up to 24h in the future. Try to set an alarm
- * below that limit to avoid suspend failures.
- */
- ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM,
- SECS_PER_DAY - 1);
- }
-
- if (ret < 0) {
- dev_err(dev, "error setting alarm in %u seconds: %d\n",
- alarm_offset, ret);
- return ret;
- }
+ dev_err(dev, "error setting alarm in %u seconds: %d\n",
+ alarm_offset, ret);
+ /*
+ * The EC code returns -EINVAL if the alarm time is too
+ * far in the future. Convert it to the expected error code.
+ */
+ if (ret == -EINVAL)
+ ret = -ERANGE;
+ return ret;
}
return 0;
@@ -355,6 +349,20 @@ static int cros_ec_rtc_probe(struct platform_device *pdev)
cros_ec_rtc->rtc->ops = &cros_ec_rtc_ops;
cros_ec_rtc->rtc->range_max = U32_MAX;
+ /*
+ * The RTC on some older Chromebooks can only handle alarms less than
+ * 24 hours in the future. The only way to find out is to try to set an
+ * alarm further in the future. If that fails, assume that the RTC
+ * connected to the EC can only handle less than 24 hours of alarm
+ * window.
+ */
+ ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM, SECS_PER_DAY * 2);
+ if (ret == -EINVAL)
+ cros_ec_rtc->rtc->alarm_offset_max = SECS_PER_DAY - 1;
+
+ (void)cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM,
+ EC_RTC_ALARM_CLEAR);
+
ret = devm_rtc_register_device(cros_ec_rtc->rtc);
if (ret)
return ret;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/7] rtc: cmos: Report supported alarm limit to rtc infrastructure
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
` (2 preceding siblings ...)
2023-08-17 22:55 ` [PATCH v2 3/7] rtc: cros-ec: Detect and report supported alarm window size Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 5/7] rtc: tps6586x: Report maximum alarm limit to rtc core Guenter Roeck
` (3 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
The alarm window supported by the cmos RTC depends on the chip
and its configuration. Report the limit to the RTC core.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
drivers/rtc/rtc-cmos.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index c9416fe8542d..228fb2d11c70 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -913,6 +913,10 @@ static inline void cmos_check_acpi_rtc_status(struct device *dev,
#define INITSECTION __init
#endif
+#define SECS_PER_DAY (24 * 60 * 60)
+#define SECS_PER_MONTH (28 * SECS_PER_DAY)
+#define SECS_PER_YEAR (365 * SECS_PER_DAY)
+
static int INITSECTION
cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
{
@@ -1019,6 +1023,13 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
goto cleanup0;
}
+ if (cmos_rtc.mon_alrm)
+ cmos_rtc.rtc->alarm_offset_max = SECS_PER_YEAR - 1;
+ else if (cmos_rtc.day_alrm)
+ cmos_rtc.rtc->alarm_offset_max = SECS_PER_MONTH - 1;
+ else
+ cmos_rtc.rtc->alarm_offset_max = SECS_PER_DAY - 1;
+
rename_region(ports, dev_name(&cmos_rtc.rtc->dev));
if (!mc146818_does_rtc_work()) {
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/7] rtc: tps6586x: Report maximum alarm limit to rtc core
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
` (3 preceding siblings ...)
2023-08-17 22:55 ` [PATCH v2 4/7] rtc: cmos: Report supported alarm limit to rtc infrastructure Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 6/7] rtc: ds1305: " Guenter Roeck
` (2 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
tps6586x only supports alarms up to 16,383 seconds in the future.
Report the limit to the RTC core.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
drivers/rtc/rtc-tps6586x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
index 9f14e2475747..20faf08c254c 100644
--- a/drivers/rtc/rtc-tps6586x.c
+++ b/drivers/rtc/rtc-tps6586x.c
@@ -252,6 +252,7 @@ static int tps6586x_rtc_probe(struct platform_device *pdev)
rtc->rtc->ops = &tps6586x_rtc_ops;
rtc->rtc->range_max = (1ULL << 30) - 1; /* 30-bit seconds */
+ rtc->rtc->alarm_offset_max = ALM1_VALID_RANGE_IN_SEC;
rtc->rtc->start_secs = mktime64(2009, 1, 1, 0, 0, 0);
rtc->rtc->set_start_time = true;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/7] rtc: ds1305: Report maximum alarm limit to rtc core
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
` (4 preceding siblings ...)
2023-08-17 22:55 ` [PATCH v2 5/7] rtc: tps6586x: Report maximum alarm limit to rtc core Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-17 22:55 ` [PATCH v2 7/7] rtc: rzn1: " Guenter Roeck
2023-08-27 21:51 ` (subset) [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Alexandre Belloni
7 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
DS1305 only supports alarms up to 24 hours in the future.
Report the limit to the RTC core, and use the reported limit
to validate the requested alarm time when setting it.
If the alarm is too large when trying to set an alarm, return -ERANGE
instead of -EDOM to align with error codes returned by other rtc drivers.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
Use the new variable to validate the limit when setting the alarm
drivers/rtc/rtc-ds1305.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-ds1305.c b/drivers/rtc/rtc-ds1305.c
index ed9360486953..d4de401548b4 100644
--- a/drivers/rtc/rtc-ds1305.c
+++ b/drivers/rtc/rtc-ds1305.c
@@ -336,8 +336,8 @@ static int ds1305_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
/* make sure alarm fires within the next 24 hours */
if (later <= now)
return -EINVAL;
- if ((later - now) > 24 * 60 * 60)
- return -EDOM;
+ if ((later - now) > ds1305->rtc->alarm_offset_max)
+ return -ERANGE;
/* disable alarm if needed */
if (ds1305->ctrl[0] & DS1305_AEI0) {
@@ -691,6 +691,7 @@ static int ds1305_probe(struct spi_device *spi)
ds1305->rtc->ops = &ds1305_ops;
ds1305->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
ds1305->rtc->range_max = RTC_TIMESTAMP_END_2099;
+ ds1305->rtc->alarm_offset_max = 24 * 60 * 60;
ds1305_nvmem_cfg.priv = ds1305;
status = devm_rtc_register_device(ds1305->rtc);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 7/7] rtc: rzn1: Report maximum alarm limit to rtc core
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
` (5 preceding siblings ...)
2023-08-17 22:55 ` [PATCH v2 6/7] rtc: ds1305: " Guenter Roeck
@ 2023-08-17 22:55 ` Guenter Roeck
2023-08-18 7:32 ` Miquel Raynal
2023-08-27 21:51 ` (subset) [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Alexandre Belloni
7 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2023-08-17 22:55 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris,
Guenter Roeck
RZN1 only supports alarms up to one week in the future.
Report the limit to the RTC core and use the reported limit
to validate the requested alarm time when setting it.
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Rename range_max_offset -> alarm_offset_max
Use the new variable to validate the limit when setting the alarm
drivers/rtc/rtc-rzn1.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index dca736caba85..c0f448bce145 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -227,7 +227,7 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return ret;
/* We cannot set alarms more than one week ahead */
- farest = rtc_tm_to_time64(&tm_now) + (7 * 86400);
+ farest = rtc_tm_to_time64(&tm_now) + rtc->rtcdev->alarm_offset_max;
alarm = rtc_tm_to_time64(tm);
if (time_after(alarm, farest))
return -ERANGE;
@@ -351,6 +351,7 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
+ rtc->rtcdev->alarm_offset_max = 7 * 86400;
rtc->rtcdev->ops = &rzn1_rtc_ops;
set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 7/7] rtc: rzn1: Report maximum alarm limit to rtc core
2023-08-17 22:55 ` [PATCH v2 7/7] rtc: rzn1: " Guenter Roeck
@ 2023-08-18 7:32 ` Miquel Raynal
0 siblings, 0 replies; 21+ messages in thread
From: Miquel Raynal @ 2023-08-18 7:32 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alexandre Belloni, Alessandro Zummo, Benson Leung,
Thomas Gleixner, John Stultz, Stephen Boyd, linux-rtc,
linux-kernel, Brian Norris
Hi Guenter,
linux@roeck-us.net wrote on Thu, 17 Aug 2023 15:55:37 -0700:
> RZN1 only supports alarms up to one week in the future.
> Report the limit to the RTC core and use the reported limit
> to validate the requested alarm time when setting it.
>
> Cc: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: (subset) [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets
2023-08-17 22:55 [PATCH v2 0/7] rtc: Add support for limited alarm timer offsets Guenter Roeck
` (6 preceding siblings ...)
2023-08-17 22:55 ` [PATCH v2 7/7] rtc: rzn1: " Guenter Roeck
@ 2023-08-27 21:51 ` Alexandre Belloni
7 siblings, 0 replies; 21+ messages in thread
From: Alexandre Belloni @ 2023-08-27 21:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alessandro Zummo, Benson Leung, Miquel Raynal, Thomas Gleixner,
John Stultz, Stephen Boyd, linux-rtc, linux-kernel, Brian Norris
On Thu, 17 Aug 2023 15:55:30 -0700, Guenter Roeck wrote:
> Some alarm timers are based on time offsets, not on absolute times.
> In some situations, the amount of time that can be scheduled in the
> future is limited. This may result in a refusal to suspend the system,
> causing substantial battery drain.
>
> This problem was previously observed on a Chromebook using the cros_ec
> rtc driver. EC variants on some older Chromebooks only support 24 hours
> of alarm time in the future. To work around the problem on affected
> Chromebooks, code to limit the maximum alarm time was added to the cros_ec
> rtc driver with commit f27efee66370 ("rtc: cros-ec: Limit RTC alarm range
> if needed"). The problem is now seen again on a system using the cmos
> RTC driver on hardware limited to 24 hours of alarm time, so a more
> generic solution is needed.
>
> [...]
Applied, thanks!
[1/7] rtc: Add support for limited alarm timer offsets
commit: 781589e40ac5f929f58824c15448e1ba49c3ac32
[3/7] rtc: cros-ec: Detect and report supported alarm window size
commit: 00c3092d881bc9d63dc36eecd140cdb38962c7ec
[4/7] rtc: cmos: Report supported alarm limit to rtc infrastructure
commit: 2546e7083f2ea96bdd6157961dc2748d65a9e487
[5/7] rtc: tps6586x: Report maximum alarm limit to rtc core
commit: 3637bbdc8a446b8edb369383d2abc816c96ee864
[6/7] rtc: ds1305: Report maximum alarm limit to rtc core
commit: 46b79ac0b463e155b098805ff66f1f22ff249b45
[7/7] rtc: rzn1: Report maximum alarm limit to rtc core
commit: 2b0386d578836b9cd5d2e63cff38b7229c319e4a
Best regards,
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 21+ messages in thread