Patchwork rtc: add in ability to push out an existing wakealarm using sysfs

login
register
mail settings
Submitter Bernie Thompson
Date May 11, 2013, 12:53 a.m.
Message ID <1368233611-1690-1-git-send-email-bhthompson@chromium.org>
Download mbox | patch
Permalink /patch/243104/
State New
Headers show

Comments

Bernie Thompson - May 11, 2013, 12:53 a.m.
This adds in the ability for the rtc sysfs code to handle += characters
at the beginning of a wakealarm setting string. This will allow the user to
attempt to push out an existing wakealarm by a provided amount.

In the case that the += characters are provided but the alarm is not active
-EVINVAL is returned.

Signed-off-by: Bernie Thompson <bhthompson@chromium.org>
---
 Documentation/rtc.txt   |  7 ++++---
 drivers/rtc/rtc-sysfs.c | 20 +++++++++++++++-----
 2 files changed, 19 insertions(+), 8 deletions(-)
Andrew Morton - May 22, 2013, 9:55 p.m.
On Fri, 10 May 2013 17:53:31 -0700 Bernie Thompson <bhthompson@chromium.org> wrote:

> This adds in the ability for the rtc sysfs code to handle += characters
> at the beginning of a wakealarm setting string. This will allow the user to
> attempt to push out an existing wakealarm by a provided amount.
> 
> In the case that the += characters are provided but the alarm is not active
> -EVINVAL is returned.
> 

Well that sounds useful, but I'm not personally in a position to
confidently judge whether it's useful enough to merge the patch.  The
changelog doesn't make a compelling case and everybody else has chosen
to sit on their hands.

So I really don't know what to do with this patch.
Bernie Thompson - May 22, 2013, 10:05 p.m.
On Wed, May 22, 2013 at 2:55 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Fri, 10 May 2013 17:53:31 -0700 Bernie Thompson <bhthompson@chromium.org> wrote:
>
> > This adds in the ability for the rtc sysfs code to handle += characters
> > at the beginning of a wakealarm setting string. This will allow the user to
> > attempt to push out an existing wakealarm by a provided amount.
> >
> > In the case that the += characters are provided but the alarm is not active
> > -EVINVAL is returned.
> >
>
> Well that sounds useful, but I'm not personally in a position to
> confidently judge whether it's useful enough to merge the patch.  The
> changelog doesn't make a compelling case and everybody else has chosen
> to sit on their hands.
>
> So I really don't know what to do with this patch.
>

Thank you for looking at this, I was just talking with Doug on CC this
morning about
explaining why this is useful, at least for my purposes in
suspend/resume testing.
The basic test goes something like:

1. Set a wake alarm from userspace 5 seconds in the future

2. Start the suspend process (echo mem > /sys/power/state)

3. After ~2.5 seconds if userspace is still running (using another
thread to check
this), move the wake alarm 5 more seconds

If the "move" involves an unset of the wakealarm then there's a
period of time where the system is midway through suspending but has
no wake alarm. It will get stuck.

We'd rather not remove the "move" since the idea is to avoid a
cancelled suspend when the alarm fires _during_ suspend.  It is
difficult for the test to tell the difference between a suspend that
was cancelled because the alarm fired too early and a suspend that was
cancelled/failed for some other reason.
Andrew Morton - May 22, 2013, 10:20 p.m.
On Wed, 22 May 2013 15:05:44 -0700 Bernie Thompson <bhthompson@chromium.org> wrote:

> On Wed, May 22, 2013 at 2:55 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 10 May 2013 17:53:31 -0700 Bernie Thompson <bhthompson@chromium.org> wrote:
> >
> > > This adds in the ability for the rtc sysfs code to handle += characters
> > > at the beginning of a wakealarm setting string. This will allow the user to
> > > attempt to push out an existing wakealarm by a provided amount.
> > >
> > > In the case that the += characters are provided but the alarm is not active
> > > -EVINVAL is returned.
> > >
> >
> > Well that sounds useful, but I'm not personally in a position to
> > confidently judge whether it's useful enough to merge the patch.  The
> > changelog doesn't make a compelling case and everybody else has chosen
> > to sit on their hands.
> >
> > So I really don't know what to do with this patch.
> >
> 
> Thank you for looking at this, I was just talking with Doug on CC this
> morning about
> explaining why this is useful, at least for my purposes in
> suspend/resume testing.
> The basic test goes something like:
> 
> 1. Set a wake alarm from userspace 5 seconds in the future
> 
> 2. Start the suspend process (echo mem > /sys/power/state)
> 
> 3. After ~2.5 seconds if userspace is still running (using another
> thread to check
> this), move the wake alarm 5 more seconds
> 
> If the "move" involves an unset of the wakealarm then there's a
> period of time where the system is midway through suspending but has
> no wake alarm. It will get stuck.
> 
> We'd rather not remove the "move" since the idea is to avoid a
> cancelled suspend when the alarm fires _during_ suspend.  It is
> difficult for the test to tell the difference between a suspend that
> was cancelled because the alarm fired too early and a suspend that was
> cancelled/failed for some other reason.

OK, I'm (easily) convinced.

I guess that if the alarm fires right in the middle of
rtc_sysfs_set_wakealarm() execution, everything collapses in a heap.

Why does the current code "Avoid accidentally clobbering active
alarms"?[*] I'd have thought that being able to reset the expiry time
of a pending alarm would be rather useful.

[*] repeat after me: comments should explain "what", not "why".  grr.

Patch

diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
index 32aa400..596b60c 100644
--- a/Documentation/rtc.txt
+++ b/Documentation/rtc.txt
@@ -153,9 +153,10 @@  since_epoch:	 The number of seconds since the epoch according to the RTC
 time:		 RTC-provided time
 wakealarm:	 The time at which the clock will generate a system wakeup
 		 event. This is a one shot wakeup event, so must be reset
-		 after wake if a daily wakeup is required. Format is either
-		 seconds since the epoch or, if there's a leading +, seconds
-		 in the future.
+		 after wake if a daily wakeup is required. Format is seconds since
+		 the epoch by default, or if there's a leading +, seconds in the
+		 future, or if there is a leading +=, seconds ahead of the current
+		 alarm.
 
 IOCTL INTERFACE
 ---------------
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index b70e2bb..4b26f86 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -164,6 +164,7 @@  rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
 {
 	ssize_t retval;
 	unsigned long now, alarm;
+	unsigned long push = 0;
 	struct rtc_wkalrm alm;
 	struct rtc_device *rtc = to_rtc_device(dev);
 	char *buf_ptr;
@@ -180,13 +181,17 @@  rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
 	buf_ptr = (char *)buf;
 	if (*buf_ptr == '+') {
 		buf_ptr++;
-		adjust = 1;
+		if (*buf_ptr == '=') {
+			buf_ptr++;
+			push = 1;
+		} else
+			adjust = 1;
 	}
 	alarm = simple_strtoul(buf_ptr, NULL, 0);
 	if (adjust) {
 		alarm += now;
 	}
-	if (alarm > now) {
+	if (alarm > now || push) {
 		/* Avoid accidentally clobbering active alarms; we can't
 		 * entirely prevent that here, without even the minimal
 		 * locking from the /dev/rtcN api.
@@ -194,9 +199,14 @@  rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
 		retval = rtc_read_alarm(rtc, &alm);
 		if (retval < 0)
 			return retval;
-		if (alm.enabled)
-			return -EBUSY;
-
+		if (alm.enabled) {
+			if (push) {
+				rtc_tm_to_time(&alm.time, &push);
+				alarm += push;
+			} else
+				return -EBUSY;
+		} else if (push)
+			return -EINVAL;
 		alm.enabled = 1;
 	} else {
 		alm.enabled = 0;