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

Guenter Roeck posted 7 patches 2 years, 4 months ago
There is a newer version of this series
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 0/7] rtc: Add support for limited alarm timer offsets
Posted by Guenter Roeck 2 years, 4 months ago
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(-)
Re: [PATCH 0/7] rtc: Add support for limited alarm timer offsets
Posted by Alexandre Belloni 2 years, 4 months ago
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 0/7] rtc: Add support for limited alarm timer offsets
Posted by Guenter Roeck 2 years, 4 months ago
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
Posted by Alexandre Belloni 2 years, 4 months ago
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 0/7] rtc: Add support for limited alarm timer offsets
Posted by Guenter Roeck 2 years, 4 months ago
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 0/7] rtc: Add support for limited alarm timer offsets
Posted by Alexandre Belloni 2 years, 4 months ago
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
Posted by Guenter Roeck 2 years, 4 months ago
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