[PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[Jeffy Chen]

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.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 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

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[Brian Norris]

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@rock-chips.com>
> ---
> 
>  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
> 
> 

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[Brian Norris]

Hi Jeffy,

A few corrections here. (Sorry, I didn't completely reread the driver
here before sending.)

On Mon, Feb 26, 2018 at 10:01:15AM -0800, Brian Norris wrote:
> 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].

In the particular case of this driver...we're actually OK because the
alarm time is programmed via an offset. So as long as we give it a
postive number, we're in the clear. We might set a longer than-expected
alarm I suppose, but that's not the end of the world...

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

Given we actually don't need this approach for the CrOS EC code, it
definitely would need to be possible to disable such code ;) But that
still doesn't mean other RTC drivers are safe.

One more note below:

> 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@rock-chips.com>
> > ---
> > 
> >  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;

I don't think we want this check on the 'disabled' case. Perhaps just
keep this under the 'else' below still?

In fact, there are *no* callers of __rtc_set_alarm() with
!alrm->enabled, but still, since this driver is *trying* to account for
that, it seems wise to retain that attempt (or else remove it entirely).

Brian

> > +
> >  	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
> > 
> > 

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[Brian Norris]

One last note:

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.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
>  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)

It's probably worth noting in the commit message that you're also fixing
the case where 'alarm_time == current_time'; in the current driver
source, it *looks* like you're setting a 0-second alarm. But in fact, 0
means EC_RTC_ALARM_CLEAR, which would disable the alarm. So you are
(correctly) returning -ETIME in that case.

Brian

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

Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past

[JeffyChen]

Hi Brian,

Thanks for your reply.

On 02/27/2018 02:37 AM, Brian Norris wrote:
>> >+	/* 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)
> It's probably worth noting in the commit message that you're also fixing
> the case where 'alarm_time == current_time'; in the current driver
> source, it*looks*  like you're setting a 0-second alarm. But in fact, 0
> means EC_RTC_ALARM_CLEAR, which would disable the alarm. So you are
> (correctly) returning -ETIME in that case.
Right, i'll rewrite the commit message, and move the check back here:)

>
> Brian
>