Re: [PATCH] rtc: cros-ec: Limit RTC alarm range if needed

[Alexandre Belloni]

On 31/10/2022 10:56:16-0700, Brian Norris wrote:
> CC kernel/time/alarmtimer.c maintainers
> 
> On Mon, Oct 31, 2022 at 06:10:53PM +0100, Alexandre Belloni wrote:
> > On 28/10/2022 17:54:00-0700, Guenter Roeck wrote:
> > > RTC chips on some older Chromebooks can only handle alarms less than 24
> > > hours in the future. Attempts to set an alarm beyond that range fails.
> > > The most severe impact of this limitation is that suspend requests fail
> > > if alarmtimer_suspend() tries to set an alarm for more than 24 hours
> > > in the future.
> > > 
> > > Try to set the real-time alarm to just below 24 hours if setting it to
> > > a larger value fails to work around the problem. While not perfect, it
> > > is better than just failing the call. A similar workaround is already
> > > implemented in the rtc-tps6586x driver.
> > 
> > I'm not super convinced this is actually better than failing the call
> > because your are implementing policy in the driver which is bad from a
> > user point of view. It would be way better to return -ERANGE and let
> > userspace select a better alarm time.
> 
> There is no way to signal user space. alarmtimer_suspend() is doing this
> on behalf of CLOCK_BOOTTIME_ALARM or CLOCK_REALTIME_ALARM timers, which
> were set long ago. We could possibly figure out some way to change the
> clock API to signal some kind of error back to the timer handlers, but
> that seems destined to be overly complex and not really help anyone
> (stable ABI, etc.). The right answer for alarmtimer is to just wake up a
> little early, IMO. (And failing alarmtimer_suspend() is Bad.)

But it is not the right answer from the RTC subsystem point of view
because there are many uses cases were you don't want to forcefully wake
up earlier or you are going to unnecessarily deplete a battery for
example or you may be able to select another RTC device which can wake
you later on.

> I think Guenter considered some alternative change to teach
> drivers/rtc/* and alarmtimer_suspend() to agree on an error code
> (ERANGE? or EDOM?) to do some automatic backoff there. But given the
> existing example (rtc-tps6586x) and the inconsistent use of error codes

The existing example predates actual maintenance of the subsystem. You
can't complain about inconsistent use of error codes (which I believe
has been cut down) and at the same time introduce inconsistent
behaviour.

> in drivers/rtc/, this seemed just as good of an option to me.
> 
> But if we want to shave more yaks, then we'll have a more complex /
> riskier patch set and a harder time backporting the fix. That's OK too.
> 

The issue with the current patch is that it forbids going for a better
solution because you will then take for granted that this driver can't
ever fail.

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