[PATCH] Posix CLOCK_RTC interface proof of concept

[PATCH] Posix CLOCK_RTC interface proof of concept

[John Stultz]

This provides a CLOCK_RTC clockid allowing the rtc to be managed
via a posix_clocks/timers interface. This allows applications to
program timer events that will wake the system from suspend state.
If the system is not suspended, the timers fire like normal
posix timers.

Currently, if an application wants to trigger a RTC alarm to wake up
from suspend, they must use the sysfs wakealarm interface or the rtc
chardev RTC_WKALM_SET ioctl. The limitation with this interface is
that it is fairly hardware oriented, so multiple applications cannot
set an RTC alarm without overwriting any previous alarms set by other
applications.

The in-kernel posix interface allows for the kernel to manage a list
of timers that control when the alarm is fired, so applications
do not need to coordinate access to the alarm hardware.

This patch is provided as a proof of concept, and is not submitted
for inclusion. There are still a number of issues I'd like to decide
upon before submitting any similar change.

1) This implementation does not block the direct RTC alarm control via
sysfs or chardev ioctl. It just creates a parallel interface, which
means there are race issues if both methods are used at the same time.

I'd prefer to embed the posix timer list below the sysfs/ioctl
interfaces, so those interfaces are in effect virtualized. Instead
of directly accessing the hardware, alarms set via that interface
would be only one of many possible timers managed in the timer list
by the kernel. This will provide ABI compatibility, but will cause
some heavy churn in the generic RTC management code.

2) While not commonly seen on PC/server hardware, many architectures
allow for multiple RTC devices on a system. I'm not completely
aware of the utility of multiple RTCs (other then allowing
multiple apps to trigger multiple wake events without a kernel managed
timer list), but I'm willing to be enlightened.

Exposing multiple RTCs via the posix clock interface has some tradeoffs.

a) Some application programmers may really want to see the underlying
hardware and be sophisticated enough to deal with the multiple,
possibly unsynchronized, time domains.

b) Some application programmers may not care about the hardware, and
just want a interface that works like CLOCK_REALTIME, but fires
wakeup alarms if the system is suspended.

Exposing all the RTC devices in a somewhat raw manner is probably
the most straight forward. Some extra infrastructure, like
the dynamic posix-clockid allocation Richard Cochran has
started to look into will be needed. More concerning is that
this will probably cause some grief if someone creates a cron-like
tool that uses the RTC where the RTC isn't exactly synced with
system time. When the user specified a job for 6am, do they mean
6am system time, or RTC?

And note: on many PCs, the RTC is synchronized, but kept in local
time, not UTC. So the unsynched RTC case is likely to be common.

To resolve this, we could create a somewhat meta CLOCK_RTC, which
really sets timers against CLOCK_REALTIME, but upon suspend
sets a relative alarm on the RTC for time interval remaining
on the next timer. This would probably need to be in addition to
the per-RTC interface, as we still need the per-RTC kernel
managed timer list to avoid races.

3) Adding the posix time interface makes it easier to have finer
grained capability management to decide what applications can
set a alarm timer. While this is great for creating applications
that can wake servers up from suspend mode, and simplifying the
wakeup infrastructure on cell phones, some systems may not
want applications being able to set wakeup timers. I can
imagine the "laptop in well insulated carry-on luggage" case
that comes up occasional being one of them. So some additional
thought and policy may be needed to decide when non-user-triggered
wake events should be masked or not in suspend.

So there is it. Clearly there is more work to be done, but
any comments or feedback would be greatly appreciated!

NOT FOR INCLUSION!
NOT FOR INCLUSION!
Signed-off-by: John Stultz <john.stultz@linaro.org>
CC: Alessandro Zummo <a.zummo@towertech.it>
CC: David Brownell <david-b@pacbell.net>
CC: Paul McKenney paulmck@linux.vnet.ibm.com>
CC: Richard Cochran <richardcochran@gmail.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/rtc/Makefile         |    2 +-
 drivers/rtc/posix.c          |  448 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/posix-timers.h |    5 +
 include/linux/time.h         |    1 +
 4 files changed, 455 insertions(+), 1 deletions(-)
 create mode 100644 drivers/rtc/posix.c

diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 0f207b3..e4a0b05 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -9,7 +9,7 @@ endif
 obj-$(CONFIG_RTC_LIB)		+= rtc-lib.o
 obj-$(CONFIG_RTC_HCTOSYS)	+= hctosys.o
 obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
-rtc-core-y			:= class.o interface.o
+rtc-core-y			:= class.o interface.o posix.o
 
 rtc-core-$(CONFIG_RTC_INTF_DEV)	+= rtc-dev.o
 rtc-core-$(CONFIG_RTC_INTF_PROC) += rtc-proc.o
diff --git a/drivers/rtc/posix.c b/drivers/rtc/posix.c
new file mode 100644
index 0000000..65e6bfc
--- /dev/null
+++ b/drivers/rtc/posix.c
@@ -0,0 +1,448 @@
+/*
+ * RTC posix-clock/timer interface
+ *
+ * Copyright (C) 2010 IBM Corperation
+ *
+ * Author: John Stultz <john.stultz@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/spinlock.h>
+#include <linux/posix-timers.h>
+#include <linux/rbtree.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+
+struct rtc_device *rtc;
+
+/*
+ * Timer list structures.
+ */
+static DEFINE_SPINLOCK(rtctimer_lock);
+struct rb_root timer_head;
+struct rb_node *next_timer;
+
+
+/**
+ * timespec_to_time - Convert timespec to time.
+ * @ts: a timespec
+ *
+ * Helper function that converts timespec -> time.
+ * Rounds any nanoseconds up to the next second.
+ */
+static time_t timespec_to_time(struct timespec ts)
+{
+	time_t time = ts.tv_sec;
+	if (ts.tv_nsec)
+		time++;
+
+	return time;
+}
+
+/**
+ * time_to_timespec - Convert time to timespec
+ * @t: a time_t
+ *
+ * Helper function that converts time -> timespec
+ */
+static struct timespec time_to_timespec(time_t t)
+{
+	struct timespec ts;
+	ts.tv_sec = t;
+	ts.tv_nsec = 0;
+	return ts;
+}
+
+/**
+ * rtctimer_add_list - Add a timer to the timerlist
+ * @t: Pointer to timer to be added.
+ *
+ * The rtctimer_lock must be held when calling.
+ */
+static void rtctimer_add_list(struct k_itimer *t)
+{
+	struct rb_node **p = &timer_head.rb_node;
+	struct rb_node *parent = NULL;
+	struct k_itimer *ptr;
+
+	time_t expires = timespec_to_time(t->it.simple.it.it_value);
+
+	while (*p) {
+		time_t time;
+		parent = *p;
+		ptr = rb_entry(parent, struct k_itimer, it.simple.list);
+		time = timespec_to_time(ptr->it.simple.it.it_value);
+		if (expires < time)
+			p = &(*p)->rb_left;
+		else
+			p = &(*p)->rb_right;
+	}
+	rb_link_node(&t->it.simple.list, parent, p);
+	rb_insert_color(&t->it.simple.list, &timer_head);
+
+	if (!next_timer ||
+		expires < rb_entry(next_timer, struct k_itimer,
+				it.simple.list)->it.simple.it.it_value.tv_sec)
+		next_timer = &t->it.simple.list;
+}
+
+/**
+ * rtctimer_del_list - Removes a timer from the timerlist
+ * @tmr: Pointer to timer to be removed
+ *
+ * The rtctimer_lock must be held when calling.
+ */
+static void rtctimer_del_list(struct k_itimer *tmr)
+{
+	struct rb_node *node = &tmr->it.simple.list;
+
+	/* update next pointer */
+	if (next_timer == node)
+		next_timer = rb_next(node);
+	rb_erase(node, &timer_head);
+}
+
+
+/**
+ * rtctimer_get_next - Returns the next timer to be expired
+ *
+ * The rtctimer_lock must be held when calling.
+ */
+static struct k_itimer *rtctimer_get_next(void)
+{
+	if (!next_timer)
+		return NULL;
+	return rb_entry(next_timer, struct k_itimer, it.simple.list);
+}
+
+
+/**
+ * rtc_clock_getres - posix getres interface
+ * @which_clock: clockid (ignored)
+ * @tp: timespec to fill.
+ *
+ * Returns the 1sec granularity of rtc interface
+ */
+static int rtc_clock_getres(const clockid_t which_clock, struct timespec *tp)
+{
+	*tp = time_to_timespec(1);
+	return 0;
+}
+
+/**
+ * rtc_clock_get - posix clock_get interface
+ * @which_clock: clockid (ignored)
+ * @tp: timespec to fill.
+ *
+ * Provides the time from the RTC
+ */
+static int rtc_clock_get(clockid_t which_clock, struct timespec *tp)
+{
+	struct rtc_time tm;
+	time_t sec;
+	int ret;
+
+	ret = rtc_read_time(rtc, &tm);
+	if (ret)
+		return ret;
+
+	rtc_tm_to_time(&tm, &sec);
+
+	*tp = time_to_timespec(sec);
+	return 0;
+}
+
+/**
+ * rtc_clock_set - posix clock_set interface
+ * @which_clock: clockid (ignored)
+ * @tp: timespec to fill.
+ *
+ * Sets the RTC to the specified time
+ */
+static int rtc_clock_set(clockid_t clockid, struct timespec *tp)
+{
+	struct rtc_time tm;
+	int ret;
+
+	rtc_time_to_tm(tp->tv_sec, &tm);
+
+	ret = rtc_set_time(rtc, &tm);
+	return ret;
+}
+
+
+
+/**
+ * rtc_timer_create - posix timer_create interface
+ * @new_timer: k_itimer pointer to manage
+ *
+ * Initializes the k_itimer structure.
+ */
+static int rtc_timer_create(struct k_itimer *new_timer)
+{
+	new_timer->it.simple.it.it_interval = time_to_timespec(0);
+	new_timer->it.simple.it.it_value = time_to_timespec(0);
+	new_timer->it.simple.enabled = 0;
+
+	return 0;
+}
+
+
+/**
+ * rtc_timer_get - posix timer_get interface
+ * @new_timer: k_itimer pointer
+ * @cur_setting: itimerspec data to fill
+ *
+ * Copies the itimerspec data out from the k_itimer
+ */
+static void rtc_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
+{
+	*cur_setting = timr->it.simple.it;
+	return;
+}
+
+
+/**
+ * rtc_timer_del - posix timer_del interface
+ * @timr: k_itimer pointer to be deleted
+ *
+ * Cancels any programmed alarms for the given timer,
+ * then removes it from the timer list,then if needed,
+ * re-program the alarm for the next timer.
+ */
+static int rtc_timer_del(struct k_itimer *timr)
+{
+	unsigned long flags;
+	struct rtc_wkalrm alrm;
+	struct k_itimer *t;
+
+	spin_lock_irqsave(&rtctimer_lock, flags);
+
+	/* check if this timer is the current one */
+	t = rtctimer_get_next();
+	if (t && t == timr) {
+		/* Stop irq */
+		rtc_time_to_tm(0, &alrm.time);
+		alrm.enabled = 0;
+		rtc_set_alarm(rtc, &alrm);
+	}
+
+	/* Remove from list */
+	rtctimer_del_list(timr);
+	timr->it.simple.enabled = 0;
+
+	/* Set the alarm to fire on the next event */
+	t = rtctimer_get_next();
+	if (t) {
+		time_t exp = timespec_to_time(t->it.simple.it.it_value);
+		rtc_time_to_tm(exp, &alrm.time);
+		alrm.enabled = 1;
+		rtc_set_alarm(rtc, &alrm);
+	}
+	spin_unlock_irqrestore(&rtctimer_lock, flags);
+
+	return 0;
+}
+
+/**
+ * rtc_timer_set - posix timer_set interface
+ * @timr: k_itimer pointer to be deleted
+ * @flags: timer flags
+ * @new_setting: itimerspec to be used
+ * @old_setting: itimerspec being replaced
+ *
+ * Sets the timer to new_setting, adds the timer to
+ * the timer list, and if necessary sets an alarm for
+ * the new timer.
+ *
+ * If the timer was already set, it will cancel
+ * the old timer and return its value in old_settings.
+ */
+static int rtc_timer_set(struct k_itimer *timr, int flags,
+	struct itimerspec *new_setting,
+	struct itimerspec *old_setting)
+{
+	struct rtc_wkalrm alrm;
+	struct k_itimer *t;
+	unsigned long irqflag;
+	int ret = 0;
+
+	spin_lock_irqsave(&rtctimer_lock, irqflag);
+
+	if (old_setting)
+		rtc_timer_get(timr, old_setting);
+
+	/* If this timer was next up, cancel alarm */
+	t = rtctimer_get_next();
+	if (t && t == timr) {
+		rtc_time_to_tm(0, &alrm.time);
+		alrm.enabled = 0;
+		rtc_set_alarm(rtc, &alrm);
+	}
+
+	if (timr->it.simple.enabled) {
+		/* remove it from list */
+		rtctimer_del_list(timr);
+		timr->it.simple.enabled = 0;
+	}
+
+	if (!(flags & TIMER_ABSTIME)) {
+		/* Convert relative to abs */
+		struct rtc_time tm;
+		time_t now;
+		rtc_read_time(rtc, &tm);
+		rtc_tm_to_time(&tm, &now);
+
+		timr->it.simple.it.it_value = new_setting->it_value;
+		timr->it.simple.it.it_value.tv_sec += now;
+	} else
+		timr->it.simple.it.it_value = new_setting->it_value;
+
+	timr->it.simple.it.it_interval = new_setting->it_interval;
+
+	rtctimer_add_list(timr);
+	timr->it.simple.enabled = 1;
+
+	/* Set alarm */
+	t = rtctimer_get_next();
+	if (t == timr) {
+		time_t exp = timespec_to_time(t->it.simple.it.it_value);
+		rtc_time_to_tm(exp, &alrm.time);
+		alrm.enabled = 1;
+		rtc_set_alarm(rtc, &alrm);
+	}
+	spin_unlock_irqrestore(&rtctimer_lock, irqflag);
+
+	return ret;
+}
+
+
+/**
+ * no_nsleep - posix nsleep dummy interface
+ * @which_clock: ignored
+ * @flags: ignored
+ * @tsave: ignored
+ * @rmtp: ignored
+ *
+ * We don't implement nsleep, so this stub returns an error.
+ */
+static int no_nsleep(const clockid_t which_clock, int flags,
+		     struct timespec *tsave, struct timespec __user *rmtp)
+{
+	return -EOPNOTSUPP;
+}
+
+
+struct rtc_task irqhandler;
+/**
+ * rtc_handle_irq - RTC alarm interrupt callback
+ * @data: ignored
+ *
+ * This function is called when the RTC interrupt triggers.
+ * It runs through the timer list, expiring timers,
+ * then programs the alarm to fire on the next event.
+ */
+void rtc_handle_irq(void *data)
+{
+	unsigned long flags;
+	struct rtc_time tm;
+	unsigned long now;
+	struct k_itimer *ptr;
+	struct rtc_wkalrm alrm;
+
+	spin_lock_irqsave(&rtctimer_lock, flags);
+
+	/* read the clock */
+	rtc_read_time(rtc, &tm);
+	rtc_tm_to_time(&tm, &now);
+
+	/* expire timers */
+	ptr = rtctimer_get_next();
+	while (ptr) {
+		time_t exp = timespec_to_time(ptr->it.simple.it.it_value);
+		if (now < exp)
+			break;
+		if (posix_timer_event(ptr, 0) != 0)
+			ptr->it_overrun++;
+
+		rtctimer_del_list(ptr);
+		if (ptr->it.simple.it.it_interval.tv_sec ||
+				ptr->it.simple.it.it_interval.tv_nsec) {
+			/* Handle interval timers */
+			ptr->it.simple.it.it_value =
+				timespec_add(ptr->it.simple.it.it_value,
+					ptr->it.simple.it.it_interval);
+			rtctimer_add_list(ptr);
+		} else
+			ptr->it.simple.enabled = 0;
+
+		ptr = rtctimer_get_next();
+	}
+
+	/* Set the alarm to fire on the next event */
+	ptr = rtctimer_get_next();
+	if (ptr) {
+		time_t exp = timespec_to_time(ptr->it.simple.it.it_value);
+		rtc_time_to_tm(exp, &alrm.time);
+		alrm.enabled = 1;
+		rtc_set_alarm(rtc, &alrm);
+	}
+
+	spin_unlock_irqrestore(&rtctimer_lock, flags);
+}
+
+
+
+static int __init is_rtc(struct device *dev, void *name_ptr)
+{
+	*(const char **)name_ptr = dev_name(dev);
+	return 1;
+}
+
+
+/**
+ * init_rtc_posix_timers - Set up the RTC posix interface
+ *
+ * This function sets up and installs the RTC posix interface.
+ * Locates an RTC device that is usable, registers an irq handler,
+ * and installs the rtc k_clock.
+ */
+static __init int init_rtc_posix_timers(void)
+{
+	char *str = NULL;
+	int ret;
+	struct k_clock rtc_clock = {
+		.clock_getres = rtc_clock_getres,
+		.clock_get = rtc_clock_get,
+		.clock_set = rtc_clock_set,
+		.timer_create = rtc_timer_create,
+		.timer_set = rtc_timer_set,
+		.timer_del = rtc_timer_del,
+		.timer_get = rtc_timer_get,
+		.nsleep = no_nsleep,
+	};
+
+	/* Dig up an RTC device */
+	class_find_device(rtc_class, NULL, &str, is_rtc);
+	if (str)
+		rtc = rtc_class_open(str);
+	if (!rtc)
+		return -1;
+
+	irqhandler.func = rtc_handle_irq;
+	ret = rtc_irq_register(rtc, &irqhandler);
+	if (ret)
+		goto out_error;
+
+	register_posix_clock(CLOCK_RTC, &rtc_clock);
+
+	return 0;
+
+out_error:
+	rtc_class_close(rtc);
+	return -1;
+}
+late_initcall(init_rtc_posix_timers);
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 3e23844..a923a1e 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -63,6 +63,11 @@ struct k_itimer {
 			unsigned long incr;
 			unsigned long expires;
 		} mmtimer;
+		struct {
+			struct itimerspec it;
+			struct rb_node list;
+			char enabled;
+		} simple;
 	} it;
 };
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 9f15ac7..418ee44 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -295,6 +295,7 @@ struct itimerval {
  * The IDs of various hardware clocks:
  */
 #define CLOCK_SGI_CYCLE			10
+#define CLOCK_RTC			11
 #define MAX_CLOCKS			16
 #define CLOCKS_MASK			(CLOCK_REALTIME | CLOCK_MONOTONIC)
 #define CLOCKS_MONO			CLOCK_MONOTONIC
-- 
1.6.0.4

Re: [PATCH] Posix CLOCK_RTC interface proof of concept

[Richard Cochran]

On Thu, Sep 16, 2010 at 12:41:32PM -0700, John Stultz wrote:
> Exposing multiple RTCs via the posix clock interface has some tradeoffs.
> 
> a) Some application programmers may really want to see the underlying
> hardware and be sophisticated enough to deal with the multiple,
> possibly unsynchronized, time domains.
> 
> b) Some application programmers may not care about the hardware, and
> just want a interface that works like CLOCK_REALTIME, but fires
> wakeup alarms if the system is suspended.

I like the idea of offering the high-level posix clock API for various
hardware clocks. Also, keeping low-level access for specialists makes
sense, as long as there is a way to enforce correct access control. If
we make the posix clocks truly dynamic (with removal, too), then one
could offer a low level funtion to grab the clock before use.

> Exposing all the RTC devices in a somewhat raw manner is probably
> the most straight forward. Some extra infrastructure, like
> the dynamic posix-clockid allocation Richard Cochran has
> started to look into will be needed. More concerning is that
> this will probably cause some grief if someone creates a cron-like
> tool that uses the RTC where the RTC isn't exactly synced with
> system time. When the user specified a job for 6am, do they mean
> 6am system time, or RTC?
> 
> And note: on many PCs, the RTC is synchronized, but kept in local
> time, not UTC. So the unsynched RTC case is likely to be common.

We could also advertise the clocks properties (eg timescale) via sysfs
(or clockfs, as Greg K.H. put it).

> 3) Adding the posix time interface makes it easier to have finer
> grained capability management to decide what applications can
> set a alarm timer. While this is great for creating applications
> that can wake servers up from suspend mode, and simplifying the
> wakeup infrastructure on cell phones, some systems may not
> want applications being able to set wakeup timers. I can
> imagine the "laptop in well insulated carry-on luggage" case
> that comes up occasional being one of them. So some additional
> thought and policy may be needed to decide when non-user-triggered
> wake events should be masked or not in suspend.

Again, implementing dynamic clocks would allow the sysdadmin to just
remove the clock whenever it might cause trouble.

Overall:

I only took a quick look at the patch, but I like the general idea.

You introduce posix timers that are *not* based on the hrtimer code,
but rather on a second implementation. I wonder whether one could
abstract the timer management code to work with different clocks.

I have been thinking about that for the PTP hardware clock stuff and
will soon post a new round of patches.

Thanks,
Richard

Re: [PATCH] Posix CLOCK_RTC interface proof of concept

[Thomas Gleixner]

On Thu, 16 Sep 2010, John Stultz wrote:
> This patch is provided as a proof of concept, and is not submitted
> for inclusion. There are still a number of issues I'd like to decide
> upon before submitting any similar change.
> 
> 1) This implementation does not block the direct RTC alarm control via
> sysfs or chardev ioctl. It just creates a parallel interface, which
> means there are race issues if both methods are used at the same time.
> 
> I'd prefer to embed the posix timer list below the sysfs/ioctl
> interfaces, so those interfaces are in effect virtualized. Instead
> of directly accessing the hardware, alarms set via that interface
> would be only one of many possible timers managed in the timer list
> by the kernel. This will provide ABI compatibility, but will cause
> some heavy churn in the generic RTC management code.

Right.
 
> 2) While not commonly seen on PC/server hardware, many architectures
> allow for multiple RTC devices on a system. I'm not completely
> aware of the utility of multiple RTCs (other then allowing
> multiple apps to trigger multiple wake events without a kernel managed
> timer list), but I'm willing to be enlightened.

AFAICT it's mostly due to lousy RTC implementations in SoC devices
where an external RTC is way better in terms of power consuption.
 
> To resolve this, we could create a somewhat meta CLOCK_RTC, which
> really sets timers against CLOCK_REALTIME, but upon suspend
> sets a relative alarm on the RTC for time interval remaining
> on the next timer. This would probably need to be in addition to
> the per-RTC interface, as we still need the per-RTC kernel
> managed timer list to avoid races.

CLOCK_RTC should really operate in the CLOCK_REALTIME domain. We can
expose the raw value via sysfs for those who are interested in it.

We do not need extra magic on suspend. Relative timers do not care
about the time domain.
 
> 3) Adding the posix time interface makes it easier to have finer
> grained capability management to decide what applications can
> set a alarm timer. While this is great for creating applications
> that can wake servers up from suspend mode, and simplifying the
> wakeup infrastructure on cell phones, some systems may not
> want applications being able to set wakeup timers. I can
> imagine the "laptop in well insulated carry-on luggage" case
> that comes up occasional being one of them. So some additional
> thought and policy may be needed to decide when non-user-triggered
> wake events should be masked or not in suspend.

Yeah, we need some central policy control for this.
 
> diff --git a/drivers/rtc/posix.c b/drivers/rtc/posix.c

I'd prefer to move this to kernel/time/

> new file mode 100644
> index 0000000..65e6bfc
> --- /dev/null
> +++ b/drivers/rtc/posix.c
> @@ -0,0 +1,448 @@
> +/*
> + * RTC posix-clock/timer interface
> + *
> + * Copyright (C) 2010 IBM Corperation
> + *
> + * Author: John Stultz <john.stultz@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/spinlock.h>
> +#include <linux/posix-timers.h>
> +#include <linux/rbtree.h>
> +#include <linux/rtc.h>
> +#include <linux/slab.h>
> +
> +struct rtc_device *rtc;
> +
> +/*
> + * Timer list structures.
> + */
> +static DEFINE_SPINLOCK(rtctimer_lock);
> +struct rb_root timer_head;

static

> +struct rb_node *next_timer;

Ditto

> +
> +
> +/**
> + * timespec_to_time - Convert timespec to time.
> + * @ts: a timespec
> + *
> + * Helper function that converts timespec -> time.
> + * Rounds any nanoseconds up to the next second.
> + */
> +static time_t timespec_to_time(struct timespec ts)
> +{
> +	time_t time = ts.tv_sec;
> +	if (ts.tv_nsec)
> +		time++;
> +
> +	return time;
> +}
> +
> +/**
> + * time_to_timespec - Convert time to timespec
> + * @t: a time_t
> + *
> + * Helper function that converts time -> timespec
> + */
> +static struct timespec time_to_timespec(time_t t)
> +{
> +	struct timespec ts;
> +	ts.tv_sec = t;
> +	ts.tv_nsec = 0;
> +	return ts;
> +}

Those should go where the other conversion routines are as
well. That's kernel/time.c, but we should move all these helper and
conversion functions into kernel/time/lib.c

> +/**
> + * rtctimer_add_list - Add a timer to the timerlist
> + * @t: Pointer to timer to be added.
> + *
> + * The rtctimer_lock must be held when calling.
> + */
> +static void rtctimer_add_list(struct k_itimer *t)
> +{
> +	struct rb_node **p = &timer_head.rb_node;
> +	struct rb_node *parent = NULL;
> +	struct k_itimer *ptr;
> +
> +	time_t expires = timespec_to_time(t->it.simple.it.it_value);
> +
> +	while (*p) {
> +		time_t time;
> +		parent = *p;
> +		ptr = rb_entry(parent, struct k_itimer, it.simple.list);
> +		time = timespec_to_time(ptr->it.simple.it.it_value);
> +		if (expires < time)
> +			p = &(*p)->rb_left;
> +		else
> +			p = &(*p)->rb_right;
> +	}
> +	rb_link_node(&t->it.simple.list, parent, p);
> +	rb_insert_color(&t->it.simple.list, &timer_head);
> +
> +	if (!next_timer ||
> +		expires < rb_entry(next_timer, struct k_itimer,
> +				it.simple.list)->it.simple.it.it_value.tv_sec)
> +		next_timer = &t->it.simple.list;

That's basically a copy of enqueue_hrtimer(). Can't we use
k_itimer.it.real for this and reuse the related functions in
hrtimer.c? Not sure if it's worth the trouble though.

> +/**
> + * no_nsleep - posix nsleep dummy interface
> + * @which_clock: ignored
> + * @flags: ignored
> + * @tsave: ignored
> + * @rmtp: ignored
> + *
> + * We don't implement nsleep, so this stub returns an error.

Why don't we implement that ?

> + */
> +static int no_nsleep(const clockid_t which_clock, int flags,
> +		     struct timespec *tsave, struct timespec __user *rmtp)
> +{
> +	return -EOPNOTSUPP;
> +}

do_posix_clock_nonanosleep() already implements the NOTSUP function.

> +
> +struct rtc_task irqhandler;

static 

Please move that to the other local variables

> +/**
> + * rtc_handle_irq - RTC alarm interrupt callback
> + * @data: ignored
> + *
> + * This function is called when the RTC interrupt triggers.
> + * It runs through the timer list, expiring timers,
> + * then programs the alarm to fire on the next event.
> + */
> +void rtc_handle_irq(void *data)

static

> +{
> +	unsigned long flags;
> +	struct rtc_time tm;
> +	unsigned long now;
> +	struct k_itimer *ptr;
> +	struct rtc_wkalrm alrm;

  alrm is only used in the if (ptr) patch below, move it there

> +
> +	spin_lock_irqsave(&rtctimer_lock, flags);
> +
> +	/* read the clock */
> +	rtc_read_time(rtc, &tm);
> +	rtc_tm_to_time(&tm, &now);
> +
> +	/* expire timers */
> +	ptr = rtctimer_get_next();
> +	while (ptr) {
> +		time_t exp = timespec_to_time(ptr->it.simple.it.it_value);
> +		if (now < exp)
> +			break;
> +		if (posix_timer_event(ptr, 0) != 0)
> +			ptr->it_overrun++;
> +
> +		rtctimer_del_list(ptr);
> +		if (ptr->it.simple.it.it_interval.tv_sec ||
> +				ptr->it.simple.it.it_interval.tv_nsec) {
> +			/* Handle interval timers */
> +			ptr->it.simple.it.it_value =
> +				timespec_add(ptr->it.simple.it.it_value,
> +					ptr->it.simple.it.it_interval);
> +			rtctimer_add_list(ptr);
> +		} else
> +			ptr->it.simple.enabled = 0;
> +
> +		ptr = rtctimer_get_next();
> +	}
> +
> +	/* Set the alarm to fire on the next event */
> +	ptr = rtctimer_get_next();
> +	if (ptr) {
> +		time_t exp = timespec_to_time(ptr->it.simple.it.it_value);

  New line before code please.

> +		rtc_time_to_tm(exp, &alrm.time);
> +		alrm.enabled = 1;
> +		rtc_set_alarm(rtc, &alrm);
> +	}
> +
> +	spin_unlock_irqrestore(&rtctimer_lock, flags);
> +}
> +
> +
> +
> +static int __init is_rtc(struct device *dev, void *name_ptr)
> +{
> +	*(const char **)name_ptr = dev_name(dev);
> +	return 1;
> +}
> +
> +
> +/**
> + * init_rtc_posix_timers - Set up the RTC posix interface
> + *
> + * This function sets up and installs the RTC posix interface.
> + * Locates an RTC device that is usable, registers an irq handler,
> + * and installs the rtc k_clock.
> + */
> +static __init int init_rtc_posix_timers(void)
> +{
> +	char *str = NULL;
> +	int ret;
> +	struct k_clock rtc_clock = {
> +		.clock_getres = rtc_clock_getres,
> +		.clock_get = rtc_clock_get,
> +		.clock_set = rtc_clock_set,
> +		.timer_create = rtc_timer_create,
> +		.timer_set = rtc_timer_set,
> +		.timer_del = rtc_timer_del,
> +		.timer_get = rtc_timer_get,
> +		.nsleep = no_nsleep,
> +	};
> +
> +	/* Dig up an RTC device */
> +	class_find_device(rtc_class, NULL, &str, is_rtc);

Hmm. How does that work when the rtc is loaded as a module after this?

Thanks,

	tglx

Re: [PATCH] Posix CLOCK_RTC interface proof of concept

[john stultz]

On Fri, 2010-09-17 at 12:59 +0200, Thomas Gleixner wrote:
> On Thu, 16 Sep 2010, John Stultz wrote:
> > To resolve this, we could create a somewhat meta CLOCK_RTC, which
> > really sets timers against CLOCK_REALTIME, but upon suspend
> > sets a relative alarm on the RTC for time interval remaining
> > on the next timer. This would probably need to be in addition to
> > the per-RTC interface, as we still need the per-RTC kernel
> > managed timer list to avoid races.
> 
> CLOCK_RTC should really operate in the CLOCK_REALTIME domain. We can
> expose the raw value via sysfs for those who are interested in it.

So you're voting for the meta-CLOCK_RTC?  I agree that its most likely
what userland will want, however there are some extra complications to
consider:

Such as, what happens on clock_settime() ? Since the CLOCK_RTC doesn't
map to specific hardware, do we create a meta-software clock as well?
Would that confuse users who think they're actually setting the RTC (as
would be likely in common single RTC case).

How does the meta-CLOCK_RTC code know which of the possibly multiple
RTCs is the best one to use?

I'm sure there's others too. Not that we can't solve them, but just be
aware it may be quite a bit of infrastructure and code to hide the
complexity from userland. 

> We do not need extra magic on suspend. Relative timers do not care
> about the time domain.

Hrm. Maybe I'm missing what you're thinking, but if we push CLOCK_RTC
timers onto the CLOCK_REALTIME base, we will still need to program the
alarm on suspend to the next CLOCK_RTC timer, right?

Or are you suggesting that we always program the alarm for each
CLOCK_RTC timer, but just use the CLOCK_REALTIME base for expiration?


> > diff --git a/drivers/rtc/posix.c b/drivers/rtc/posix.c
> 
> I'd prefer to move this to kernel/time/

Yea, since its just a proof of concept layer on top of the rtc code, I
kept it with the rest of the rtc code. I'm sure as it gets reworked,
some of the layers (timer list handling, etc) can be pulled out and into
kernel/time/.

> > +/**
> > + * time_to_timespec - Convert time to timespec
> > + * @t: a time_t
> > + *
> > + * Helper function that converts time -> timespec
> > + */
> > +static struct timespec time_to_timespec(time_t t)
> > +{
> > +	struct timespec ts;
> > +	ts.tv_sec = t;
> > +	ts.tv_nsec = 0;
> > +	return ts;
> > +}
> 
> Those should go where the other conversion routines are as
> well. That's kernel/time.c, but we should move all these helper and
> conversion functions into kernel/time/lib.c

Yea. Although I wasn't confident that the subtle rounding (takes the
ceiling when rounding) which is correct for timer expiration is right
for all cases, so it may need some clarification when it moves to common
code.


> > +/**
> > + * rtctimer_add_list - Add a timer to the timerlist
> > + * @t: Pointer to timer to be added.
> > + *
> > + * The rtctimer_lock must be held when calling.
> > + */
> > +static void rtctimer_add_list(struct k_itimer *t)
> > +{
> > +	struct rb_node **p = &timer_head.rb_node;
> > +	struct rb_node *parent = NULL;
> > +	struct k_itimer *ptr;
> > +
> > +	time_t expires = timespec_to_time(t->it.simple.it.it_value);
> > +
> > +	while (*p) {
> > +		time_t time;
> > +		parent = *p;
> > +		ptr = rb_entry(parent, struct k_itimer, it.simple.list);
> > +		time = timespec_to_time(ptr->it.simple.it.it_value);
> > +		if (expires < time)
> > +			p = &(*p)->rb_left;
> > +		else
> > +			p = &(*p)->rb_right;
> > +	}
> > +	rb_link_node(&t->it.simple.list, parent, p);
> > +	rb_insert_color(&t->it.simple.list, &timer_head);
> > +
> > +	if (!next_timer ||
> > +		expires < rb_entry(next_timer, struct k_itimer,
> > +				it.simple.list)->it.simple.it.it_value.tv_sec)
> > +		next_timer = &t->it.simple.list;
> 
> That's basically a copy of enqueue_hrtimer(). Can't we use
> k_itimer.it.real for this and reuse the related functions in
> hrtimer.c? Not sure if it's worth the trouble though.

Yes, there's also a similar duplication in drivers/char/mmtimer.c and
Richard Cochran commented on the same (although I'm not sure if he ended
up using the hrtimer for his similar PTP code).

I was hoping to find something I could re-use in the hrtimer code, but
it seemed a little tightly linked to the fixed CLOCK_MONOTONIC/REALTIME
bases. I suspect the generic rbtree timer list implementation could be
pulled out and made more reusable, but for this proof of concept, that
seemed a bit too intrusive (I don't want to bury the basic point of what
I'm trying to do in a mountain of cleanup code :).


> > +/**
> > + * no_nsleep - posix nsleep dummy interface
> > + * @which_clock: ignored
> > + * @flags: ignored
> > + * @tsave: ignored
> > + * @rmtp: ignored
> > + *
> > + * We don't implement nsleep, so this stub returns an error.
> 
> Why don't we implement that ?

Well, I guess we could. With the RTC granularity being 1 second, I think
the idea that anyone would want to use it for nsleep seems a little
ridiculous to me. But I guess longer sleepers that need to wake the
system from suspend could make sense here. So, I sort of look at this as
a nice-to-have and will see about implementing it in the later
revisions.


> > +static __init int init_rtc_posix_timers(void)
> > +{
> > +	char *str = NULL;
> > +	int ret;
> > +	struct k_clock rtc_clock = {
> > +		.clock_getres = rtc_clock_getres,
> > +		.clock_get = rtc_clock_get,
> > +		.clock_set = rtc_clock_set,
> > +		.timer_create = rtc_timer_create,
> > +		.timer_set = rtc_timer_set,
> > +		.timer_del = rtc_timer_del,
> > +		.timer_get = rtc_timer_get,
> > +		.nsleep = no_nsleep,
> > +	};
> > +
> > +	/* Dig up an RTC device */
> > +	class_find_device(rtc_class, NULL, &str, is_rtc);
> 
> Hmm. How does that work when the rtc is loaded as a module after this?

It probably wouldn't. But this is more an issue with how the interface
is layered ontop of the rtc infrastructure, instead of the rtc
infrastructure registering the posix clock when an rtc is registered.

Thanks for the very detailed review!
-john

Re: [PATCH] Posix CLOCK_RTC interface proof of concept

[Richard Cochran]

On Fri, Sep 17, 2010 at 11:42:38AM -0700, john stultz wrote:
> On Fri, 2010-09-17 at 12:59 +0200, Thomas Gleixner wrote:
> > That's basically a copy of enqueue_hrtimer(). Can't we use
> > k_itimer.it.real for this and reuse the related functions in
> > hrtimer.c? Not sure if it's worth the trouble though.
> 
> Yes, there's also a similar duplication in drivers/char/mmtimer.c and
> Richard Cochran commented on the same (although I'm not sure if he ended
> up using the hrtimer for his similar PTP code).

After taking a look at the hrtimer code, I decided to remove the timer
implementation from the patch set. I do have the userland interface to
timer_create/settime in place, but no clock drivers implement it yet.
 
> I was hoping to find something I could re-use in the hrtimer code, but
> it seemed a little tightly linked to the fixed CLOCK_MONOTONIC/REALTIME
> bases.

That was my impression, too. It looks like hrtimers are hard-coded to
the clock event device via a global per-cpu variable. Would it be
possible to decouple this for multiple, different clock event devices?

Thanks,
Richard

Re: [PATCH] Posix CLOCK_RTC interface proof of concept

[Thomas Gleixner]

On Sat, 18 Sep 2010, Richard Cochran wrote:

> On Fri, Sep 17, 2010 at 11:42:38AM -0700, john stultz wrote:
> > On Fri, 2010-09-17 at 12:59 +0200, Thomas Gleixner wrote:
> > > That's basically a copy of enqueue_hrtimer(). Can't we use
> > > k_itimer.it.real for this and reuse the related functions in
> > > hrtimer.c? Not sure if it's worth the trouble though.
> > 
> > Yes, there's also a similar duplication in drivers/char/mmtimer.c and
> > Richard Cochran commented on the same (although I'm not sure if he ended
> > up using the hrtimer for his similar PTP code).
> 
> After taking a look at the hrtimer code, I decided to remove the timer
> implementation from the patch set. I do have the userland interface to
> timer_create/settime in place, but no clock drivers implement it yet.
>  
> > I was hoping to find something I could re-use in the hrtimer code, but
> > it seemed a little tightly linked to the fixed CLOCK_MONOTONIC/REALTIME
> > bases.
> 
> That was my impression, too. It looks like hrtimers are hard-coded to
> the clock event device via a global per-cpu variable. Would it be
> possible to decouple this for multiple, different clock event devices?

It needs some thought because the REALTIME/MONOTONIC implementation
needs to stay with the per cpu ness, but for RTC and others we need a
global queue. Though it shouldn't be too hard to solve that.

Thanks,

	tglx