| Message ID | 20230817225537.4053865-2-linux@roeck-us.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: Add support for limited alarm timer offsets | expand |
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;
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;
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)
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;
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(+)