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@xxxxxxxxxx/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