[PATCH 0/7] rtc: Add support for limited alarm timer offsets

[PATCH 0/7] rtc: Add support for limited alarm timer offsets

[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.

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.

Some RTC 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. It would be even more undesirable to change the behavior
of existing widely used drivers such as the cmos RTC driver.

The existing range_max variable in struct rtc_device can not be used
to determine the maximum time offset supported by an rtc chip since
it describes the maximum absolute time supported by the chip, not the
maximum time offset that can be set for alarms.

To reduce the impact of this problem, introduce a new variable
rtc_time_offset in struct rtc_device to let RTC drivers report the maximum
supported alarm time 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. Use the new
variable to limit the alarm timer range.

The series is intended to solve the problem with minimal changes in the
rtc core and in affected drivers.

An alternative I had considered was to have the alarmtimer code guess the
maximum timeout supported by the rtc hardware. I discarded it as less
desirable since it had to retry repeatedly depending on rtc limitations.
This often resulted in error messages by the rtc driver.  On top of that,
it was all but impossible to support rtc chips such as tps6586x which
can only support wake alarms up to 16,383 seconds in the future.

The first patch of the series adds support for providing the maximum
supported time offset to the rtc core. The second patch uses that value
in the alarmtimer code to set the maximum wake-up time from system suspend.
Subsequent patches add support for reporting the maximum alarm timer offset
to a subset of affected drivers.

Previous discussion:
    https://lore.kernel.org/lkml/Y19AdIntJZGnBh%2Fy@google.com/T/#mc06d206d5bdb77c613712148818934b4f5640de5

----------------------------------------------------------------
Guenter Roeck (7):
      rtc: Add support for limited alarm timer offsets
      rtc: alarmtimer: Use maximum alarm time offset
      rtc: cros-ec: Detect and report supported alarm window size
      rtc: cmos: Report supported alarm limit to rtc infrastructure
      rtc: tps6586x: Report maximum alarm limit to rtc core
      rtc: ds1305: Report maximum alarm limit to rtc core
      rtc: rzn1: Report maximum alarm limit to rtc core

 drivers/rtc/rtc-cmos.c     | 11 +++++++++++
 drivers/rtc/rtc-cros-ec.c  | 38 +++++++++++++++++++++++---------------
 drivers/rtc/rtc-ds1305.c   |  3 ++-
 drivers/rtc/rtc-rzn1.c     |  1 +
 drivers/rtc/rtc-tps6586x.c |  1 +
 include/linux/rtc.h        |  1 +
 kernel/time/alarmtimer.c   | 13 +++++++++++++
 7 files changed, 52 insertions(+), 16 deletions(-)

[PATCH 1/7] rtc: Add support for limited alarm timer offsets

[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>
---
 include/linux/rtc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 1fd9c6a21ebe..b6d000ab1e5e 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 range_max_offset;
 	time64_t start_secs;
 	time64_t offset_secs;
 	bool set_start_time;
-- 
2.39.2

[PATCH 2/7] rtc: alarmtimer: Use maximum alarm time offset

[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>
---
 kernel/time/alarmtimer.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 8d9f13d847f0..cc31998a34b9 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->range_max_offset &&
+	    rtc->range_max_offset * MSEC_PER_SEC < ktime_to_ms(min))
+		min = ms_to_ktime(rtc->range_max_offset * MSEC_PER_SEC);
+
 	now = ktime_add(now, min);
 
 	/* Set alarm, if in the past reject suspend briefly to handle */
-- 
2.39.2

[PATCH 3/7] rtc: cros-ec: Detect and report supported alarm window size

[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>
---
 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..a257a24e8223 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->range_max_offset = 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

[PATCH 4/7] rtc: cmos: Report supported alarm limit to rtc infrastructure

[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>
---
 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..c565dc4d5bf5 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->range_max_offset = SECS_PER_YEAR - 1;
+	else if (cmos_rtc.day_alrm)
+		cmos_rtc.rtc->range_max_offset = SECS_PER_MONTH - 1;
+	else
+		cmos_rtc.rtc->range_max_offset = SECS_PER_DAY - 1;
+
 	rename_region(ports, dev_name(&cmos_rtc.rtc->dev));
 
 	if (!mc146818_does_rtc_work()) {
-- 
2.39.2

[PATCH 5/7] rtc: tps6586x: Report maximum alarm limit to rtc core

[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>
---
 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..11f822d1b337 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->range_max_offset = 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

[PATCH 6/7] rtc: ds1305: Report maximum alarm limit to rtc core

[Guenter Roeck]

DS1305 only supports alarms up to 24 hours in the future.
Report the limit to the RTC core.

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>
---
 drivers/rtc/rtc-ds1305.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1305.c b/drivers/rtc/rtc-ds1305.c
index ed9360486953..1d9bbf4e06cb 100644
--- a/drivers/rtc/rtc-ds1305.c
+++ b/drivers/rtc/rtc-ds1305.c
@@ -337,7 +337,7 @@ static int ds1305_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	if (later <= now)
 		return -EINVAL;
 	if ((later - now) > 24 * 60 * 60)
-		return -EDOM;
+		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->range_max_offset = 24 * 60 * 60;
 
 	ds1305_nvmem_cfg.priv = ds1305;
 	status = devm_rtc_register_device(ds1305->rtc);
-- 
2.39.2

[PATCH 7/7] rtc: rzn1: Report maximum alarm limit to rtc core

[Guenter Roeck]

RZN1 only supports alarms up to one week 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>
---
 drivers/rtc/rtc-rzn1.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index dca736caba85..c2cc3774ebb8 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -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->range_max_offset = 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

Re: [PATCH 1/7] rtc: Add support for limited alarm timer offsets

[Alexandre Belloni]

On 16/08/2023 06:39: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.
> 
> 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>
> ---
>  include/linux/rtc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 1fd9c6a21ebe..b6d000ab1e5e 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 range_max_offset;

While range_min and range_max are for the wall clock time, I would
prefer using a name that would clearly mark this as an alarm related
variable.

>  	time64_t start_secs;
>  	time64_t offset_secs;
>  	bool set_start_time;
> -- 
> 2.39.2
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Alexandre Belloni]

Hello,

On 16/08/2023 06:39:29-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.
> 
> Some RTC 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. It would be even more undesirable to change the behavior
> of existing widely used drivers such as the cmos RTC driver.
> 
> The existing range_max variable in struct rtc_device can not be used
> to determine the maximum time offset supported by an rtc chip since
> it describes the maximum absolute time supported by the chip, not the
> maximum time offset that can be set for alarms.
> 
> To reduce the impact of this problem, introduce a new variable
> rtc_time_offset in struct rtc_device to let RTC drivers report the maximum
> supported alarm time 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. Use the new
> variable to limit the alarm timer range.
> 
> The series is intended to solve the problem with minimal changes in the
> rtc core and in affected drivers.
> 
> An alternative I had considered was to have the alarmtimer code guess the
> maximum timeout supported by the rtc hardware. I discarded it as less
> desirable since it had to retry repeatedly depending on rtc limitations.
> This often resulted in error messages by the rtc driver.  On top of that,
> it was all but impossible to support rtc chips such as tps6586x which
> can only support wake alarms up to 16,383 seconds in the future.
> 
> The first patch of the series adds support for providing the maximum
> supported time offset to the rtc core. The second patch uses that value
> in the alarmtimer code to set the maximum wake-up time from system suspend.
> Subsequent patches add support for reporting the maximum alarm timer offset
> to a subset of affected drivers.
> 
> Previous discussion:
>     https://lore.kernel.org/lkml/Y19AdIntJZGnBh%2Fy@google.com/T/#mc06d206d5bdb77c613712148818934b4f5640de5
> 

I'm fine with the series, however, this doesn't solve the issue for RTCs
that have an absolute limit on the alarm (as opposed to an offset to the
current time/date).



> ----------------------------------------------------------------
> Guenter Roeck (7):
>       rtc: Add support for limited alarm timer offsets
>       rtc: alarmtimer: Use maximum alarm time offset
>       rtc: cros-ec: Detect and report supported alarm window size
>       rtc: cmos: Report supported alarm limit to rtc infrastructure
>       rtc: tps6586x: Report maximum alarm limit to rtc core
>       rtc: ds1305: Report maximum alarm limit to rtc core
>       rtc: rzn1: Report maximum alarm limit to rtc core
> 
>  drivers/rtc/rtc-cmos.c     | 11 +++++++++++
>  drivers/rtc/rtc-cros-ec.c  | 38 +++++++++++++++++++++++---------------
>  drivers/rtc/rtc-ds1305.c   |  3 ++-
>  drivers/rtc/rtc-rzn1.c     |  1 +
>  drivers/rtc/rtc-tps6586x.c |  1 +
>  include/linux/rtc.h        |  1 +
>  kernel/time/alarmtimer.c   | 13 +++++++++++++
>  7 files changed, 52 insertions(+), 16 deletions(-)

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/7] rtc: Add support for limited alarm timer offsets

[Guenter Roeck]

On Wed, Aug 16, 2023 at 04:57:30PM +0200, Alexandre Belloni wrote:
> On 16/08/2023 06:39: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.
> > 
> > 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>
> > ---
> >  include/linux/rtc.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > index 1fd9c6a21ebe..b6d000ab1e5e 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 range_max_offset;
> 
> While range_min and range_max are for the wall clock time, I would
> prefer using a name that would clearly mark this as an alarm related
> variable.

Sure, no problem. Do you have a suggestion ? alarm_range_max or
alarm_range_max_offset, maybe ? I'd also be happy to use some other
term for 'offset' if you have a suggestion.

Thanks,
Guenter

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Guenter Roeck]

Hi Alexandre,

On Wed, Aug 16, 2023 at 05:03:53PM +0200, Alexandre Belloni wrote:
> Hello,
> 
> On 16/08/2023 06:39:29-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.
> > 
> > Some RTC 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. It would be even more undesirable to change the behavior
> > of existing widely used drivers such as the cmos RTC driver.
> > 
> > The existing range_max variable in struct rtc_device can not be used
> > to determine the maximum time offset supported by an rtc chip since
> > it describes the maximum absolute time supported by the chip, not the
> > maximum time offset that can be set for alarms.
> > 
> > To reduce the impact of this problem, introduce a new variable
> > rtc_time_offset in struct rtc_device to let RTC drivers report the maximum
> > supported alarm time 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. Use the new
> > variable to limit the alarm timer range.
> > 
> > The series is intended to solve the problem with minimal changes in the
> > rtc core and in affected drivers.
> > 
> > An alternative I had considered was to have the alarmtimer code guess the
> > maximum timeout supported by the rtc hardware. I discarded it as less
> > desirable since it had to retry repeatedly depending on rtc limitations.
> > This often resulted in error messages by the rtc driver.  On top of that,
> > it was all but impossible to support rtc chips such as tps6586x which
> > can only support wake alarms up to 16,383 seconds in the future.
> > 
> > The first patch of the series adds support for providing the maximum
> > supported time offset to the rtc core. The second patch uses that value
> > in the alarmtimer code to set the maximum wake-up time from system suspend.
> > Subsequent patches add support for reporting the maximum alarm timer offset
> > to a subset of affected drivers.
> > 
> > Previous discussion:
> >     https://lore.kernel.org/lkml/Y19AdIntJZGnBh%2Fy@google.com/T/#mc06d206d5bdb77c613712148818934b4f5640de5
> > 
> 
> I'm fine with the series, however, this doesn't solve the issue for RTCs
> that have an absolute limit on the alarm (as opposed to an offset to the
> current time/date).
> 

I thought that is checked by rtc_valid_range() in rtc_set_alarm().
Am I missing something ? Of course that assumes that the absolute
maximum alarm timeout matches range_max, but I didn't find any
drivers where that would not be the case.

Thanks,
Guenter

> 
> 
> > ----------------------------------------------------------------
> > Guenter Roeck (7):
> >       rtc: Add support for limited alarm timer offsets
> >       rtc: alarmtimer: Use maximum alarm time offset
> >       rtc: cros-ec: Detect and report supported alarm window size
> >       rtc: cmos: Report supported alarm limit to rtc infrastructure
> >       rtc: tps6586x: Report maximum alarm limit to rtc core
> >       rtc: ds1305: Report maximum alarm limit to rtc core
> >       rtc: rzn1: Report maximum alarm limit to rtc core
> > 
> >  drivers/rtc/rtc-cmos.c     | 11 +++++++++++
> >  drivers/rtc/rtc-cros-ec.c  | 38 +++++++++++++++++++++++---------------
> >  drivers/rtc/rtc-ds1305.c   |  3 ++-
> >  drivers/rtc/rtc-rzn1.c     |  1 +
> >  drivers/rtc/rtc-tps6586x.c |  1 +
> >  include/linux/rtc.h        |  1 +
> >  kernel/time/alarmtimer.c   | 13 +++++++++++++
> >  7 files changed, 52 insertions(+), 16 deletions(-)
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Alexandre Belloni]

On 16/08/2023 08:50:12-0700, Guenter Roeck wrote:
> > I'm fine with the series, however, this doesn't solve the issue for RTCs
> > that have an absolute limit on the alarm (as opposed to an offset to the
> > current time/date).
> > 
> 
> I thought that is checked by rtc_valid_range() in rtc_set_alarm().
> Am I missing something ? Of course that assumes that the absolute
> maximum alarm timeout matches range_max, but I didn't find any
> drivers where that would not be the case.
> 

There are RTCs where this is not the case. When this is far away in the
future enough, the usual solution is to clip range_max which works but
is not really great intellectually.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/7] rtc: Add support for limited alarm timer offsets

[Alexandre Belloni]

On 16/08/2023 08:24:39-0700, Guenter Roeck wrote:
> On Wed, Aug 16, 2023 at 04:57:30PM +0200, Alexandre Belloni wrote:
> > On 16/08/2023 06:39: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.
> > > 
> > > 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>
> > > ---
> > >  include/linux/rtc.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > > index 1fd9c6a21ebe..b6d000ab1e5e 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 range_max_offset;
> > 
> > While range_min and range_max are for the wall clock time, I would
> > prefer using a name that would clearly mark this as an alarm related
> > variable.
> 
> Sure, no problem. Do you have a suggestion ? alarm_range_max or
> alarm_range_max_offset, maybe ? I'd also be happy to use some other
> term for 'offset' if you have a suggestion.

I don't really know, what about alarm_offset_max? This is the maximum
value of the offset for the alarm to the current time.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Guenter Roeck]

On Wed, Aug 16, 2023 at 06:14:35PM +0200, Alexandre Belloni wrote:
> On 16/08/2023 08:50:12-0700, Guenter Roeck wrote:
> > > I'm fine with the series, however, this doesn't solve the issue for RTCs
> > > that have an absolute limit on the alarm (as opposed to an offset to the
> > > current time/date).
> > > 
> > 
> > I thought that is checked by rtc_valid_range() in rtc_set_alarm().
> > Am I missing something ? Of course that assumes that the absolute
> > maximum alarm timeout matches range_max, but I didn't find any
> > drivers where that would not be the case.
> > 
> 
> There are RTCs where this is not the case. When this is far away in the
> future enough, the usual solution is to clip range_max which works but
> is not really great intellectually.
> 
Do you have an example, by any chance ?

Thanks,
Guenter

> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 1/7] rtc: Add support for limited alarm timer offsets

[Guenter Roeck]

On Wed, Aug 16, 2023 at 06:19:05PM +0200, Alexandre Belloni wrote:
> On 16/08/2023 08:24:39-0700, Guenter Roeck wrote:
> > On Wed, Aug 16, 2023 at 04:57:30PM +0200, Alexandre Belloni wrote:
> > > On 16/08/2023 06:39: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.
> > > > 
> > > > 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>
> > > > ---
> > > >  include/linux/rtc.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > > > index 1fd9c6a21ebe..b6d000ab1e5e 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 range_max_offset;
> > > 
> > > While range_min and range_max are for the wall clock time, I would
> > > prefer using a name that would clearly mark this as an alarm related
> > > variable.
> > 
> > Sure, no problem. Do you have a suggestion ? alarm_range_max or
> > alarm_range_max_offset, maybe ? I'd also be happy to use some other
> > term for 'offset' if you have a suggestion.
> 
> I don't really know, what about alarm_offset_max? This is the maximum
> value of the offset for the alarm to the current time.
> 
Sounds good to me.

Thanks,
Guenter

> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 7/7] rtc: rzn1: Report maximum alarm limit to rtc core

[Miquel Raynal]

Hi Guenter,

linux@roeck-us.net wrote on Wed, 16 Aug 2023 06:39:36 -0700:

> RZN1 only supports alarms up to one week 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>
> ---
>  drivers/rtc/rtc-rzn1.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index dca736caba85..c2cc3774ebb8 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -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->range_max_offset = 7 * 86400;

When we set the alarm, we use:

	farest = rtc_tm_to_time64(&tm_now) + (7 * 86400);

What about using range_max_offset (whatever its final name) in this
calculation as it will be now set in probe? It would further clarify
its purpose.

>  	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);

With the above change,

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH 7/7] rtc: rzn1: Report maximum alarm limit to rtc core

[Guenter Roeck]

On Thu, Aug 17, 2023 at 10:03:41AM +0200, Miquel Raynal wrote:
> Hi Guenter,
> 
> linux@roeck-us.net wrote on Wed, 16 Aug 2023 06:39:36 -0700:
> 
> > RZN1 only supports alarms up to one week 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>
> > ---
> >  drivers/rtc/rtc-rzn1.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> > index dca736caba85..c2cc3774ebb8 100644
> > --- a/drivers/rtc/rtc-rzn1.c
> > +++ b/drivers/rtc/rtc-rzn1.c
> > @@ -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->range_max_offset = 7 * 86400;
> 
> When we set the alarm, we use:
> 
> 	farest = rtc_tm_to_time64(&tm_now) + (7 * 86400);
> 
> What about using range_max_offset (whatever its final name) in this
> calculation as it will be now set in probe? It would further clarify
> its purpose.

Excellent idea. I'll make that change.

Thanks,
Guenter

> 
> >  	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);
> 
> With the above change,
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Alexandre Belloni]

On 16/08/2023 12:12:14-0700, Guenter Roeck wrote:
> On Wed, Aug 16, 2023 at 06:14:35PM +0200, Alexandre Belloni wrote:
> > On 16/08/2023 08:50:12-0700, Guenter Roeck wrote:
> > > > I'm fine with the series, however, this doesn't solve the issue for RTCs
> > > > that have an absolute limit on the alarm (as opposed to an offset to the
> > > > current time/date).
> > > > 
> > > 
> > > I thought that is checked by rtc_valid_range() in rtc_set_alarm().
> > > Am I missing something ? Of course that assumes that the absolute
> > > maximum alarm timeout matches range_max, but I didn't find any
> > > drivers where that would not be the case.
> > > 
> > 
> > There are RTCs where this is not the case. When this is far away in the
> > future enough, the usual solution is to clip range_max which works but
> > is not really great intellectually.
> > 
> Do you have an example, by any chance ?
> 

I'm sorry, I've been looking and I couldn't find it anymore. Don't
bother with this for now.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets

[Guenter Roeck]

On Thu, Aug 17, 2023 at 09:51:17PM +0200, Alexandre Belloni wrote:
> On 16/08/2023 12:12:14-0700, Guenter Roeck wrote:
> > On Wed, Aug 16, 2023 at 06:14:35PM +0200, Alexandre Belloni wrote:
> > > On 16/08/2023 08:50:12-0700, Guenter Roeck wrote:
> > > > > I'm fine with the series, however, this doesn't solve the issue for RTCs
> > > > > that have an absolute limit on the alarm (as opposed to an offset to the
> > > > > current time/date).
> > > > > 
> > > > 
> > > > I thought that is checked by rtc_valid_range() in rtc_set_alarm().
> > > > Am I missing something ? Of course that assumes that the absolute
> > > > maximum alarm timeout matches range_max, but I didn't find any
> > > > drivers where that would not be the case.
> > > > 
> > > 
> > > There are RTCs where this is not the case. When this is far away in the
> > > future enough, the usual solution is to clip range_max which works but
> > > is not really great intellectually.
> > > 
> > Do you have an example, by any chance ?
> > 
> 
> I'm sorry, I've been looking and I couldn't find it anymore. Don't
> bother with this for now.
> 

I have been looking as well and did not find it either.
I'll go ahead and send v2 with the suggested changes.

Thanks,
Guenter