Message ID | 20180225081802.8965-1-jeffy.chen@rock-chips.com |
---|---|
State | Superseded |
Headers | show |
Series | rtc: cros-ec: return -ETIME when refused to set alarms in the past | expand |
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 > >
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 > > > >
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 > >
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 >
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);
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(-)