Hi Jeffy,
On Sun, Feb 25, 2018 at 04:18:02PM +0800, Jeffy Chen wrote:
> We have a check in __rtc_set_alarm() to return -ETIME when the alarm
> is in the past.
>
> Since accessing a Chrome OS EC based rtc is a slow operation, we should
> do that check again inside of the EC rtc driver's .set_alarm() callback.
Thanks for the patch. I'd note that this is related to the race
documented in __rtc_set_alarm() (drivers/rtc/interface.c):
/*
* XXX - We just checked to make sure the alarm time is not
* in the past, but there is still a race window where if
* the is alarm set for the next second and the second ticks
* over right here, before we set the alarm.
*/
It feels like we should put this comment somewhere more prominent;
perhaps some kerneldoc for the .set_alarm() callback? Because I suspect
that nearly every RTC driver is susceptible to this problem.
Anyway, I think this patch is helpful, because as you note the EC
protocol is relatively slow (it's much more than just a register write),
but your patch still doesn't really cover the whole problem. Even if you
compare the current time here, time marches on between here and
EC_CMD_RTC_SET_ALARM. So you can still have the same race, where the RTC
makes another tick before we set the alarm? Just think: what if we slept
for a second right after that -ETIME check?
What happens next...depends on the implementation I suppose. It's
possible that an alarm could still immediately fire for a "past" event.
But it's also possible the alarm will get dropped [1].
I wonder if a better solution would be to re-check the clock right after
setting the alarm. If the alarm is already past, then we should return
-ETIME? Is there any harm in double-reporting an alarm? (If so, we could
try to add accounting information somehow...)
I also wonder if that check should be done in the generic code (perhaps
with a flag to opt-in or opt-out?), since this really seems like a
fundamental problem of the interface.
Brian
[1] And lest we think that dropping it is fine: this breaks, e.g.,
hwclock which relies on RTC_UIE_ON -> rtc_update_irq_enable(), which
sets a 1-second alarm and expects it to fire an interrupt.
> Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> ---
>
> drivers/rtc/rtc-cros-ec.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
> index f0ea6899c731..ee0062e2d222 100644
> --- a/drivers/rtc/rtc-cros-ec.c
> +++ b/drivers/rtc/rtc-cros-ec.c
> @@ -188,6 +188,10 @@ static int cros_ec_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> if (alarm_time < 0 || alarm_time > U32_MAX)
> return -EINVAL;
>
> + /* Don't set an alarm in the past. */
> + if ((u32)alarm_time <= current_time)
> + return -ETIME;
> +
> if (!alrm->enabled) {
> /*
> * If the alarm is being disabled, send an alarm
> @@ -196,11 +200,7 @@ static int cros_ec_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> alarm_offset = EC_RTC_ALARM_CLEAR;
> cros_ec_rtc->saved_alarm = (u32)alarm_time;
> } else {
> - /* Don't set an alarm in the past. */
> - if ((u32)alarm_time < current_time)
> - alarm_offset = EC_RTC_ALARM_CLEAR;
> - else
> - alarm_offset = (u32)alarm_time - current_time;
> + alarm_offset = (u32)alarm_time - current_time;
> }
>
> ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM, alarm_offset);
> --
> 2.11.0
>
>
