From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752065AbeBZSBV (ORCPT ); Mon, 26 Feb 2018 13:01:21 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:45774 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751814AbeBZSBS (ORCPT ); Mon, 26 Feb 2018 13:01:18 -0500 X-Google-Smtp-Source: AH8x226MIezAKt7ilILwlkprPwBdzT0hCsFCMnr03f8OBKFeSzcvNhEeK+zZ+AOTyqMvdBqsY27hKQ== Date: Mon, 26 Feb 2018 10:01:15 -0800 From: Brian Norris To: Jeffy Chen Cc: linux-kernel@vger.kernel.org, zyw@rock-chips.com, briannorris@google.com, dianders@google.com, jwerner@chromium.org, linux-rtc@vger.kernel.org, Alexandre Belloni , Alessandro Zummo Subject: Re: [PATCH] rtc: cros-ec: return -ETIME when refused to set alarms in the past Message-ID: <20180226180112.GA225858@rodete-desktop-imager.corp.google.com> References: <20180225081802.8965-1-jeffy.chen@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180225081802.8965-1-jeffy.chen@rock-chips.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > > 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 > >