Skip to contentSkip to conversation

Android Alarm Driver Cleanups (for staging-next/3.5)

PATCH In review John Stultz 2 participants 9 messages Apr 20, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Android Alarm Driver Cleanups (for staging-next/3.5)

This patch-set moves the Android alarm driver in staging to use the upstreamed CLOCK_BOOTTIME and alarmtimer interfaces, allowing us to greatly shrink the alarm driver.

The Google Android team has been using these patches in its AOSP tree for a little while without any reported issues.

The only gotcha is that I needed to make one alarmtimer interface non-static in order to support the android alarm driver. This sort of is a grey area for the staging tree, but I'm the author of the upstreamed alarmtimer code, and Thomas (cc'ed) had no objection to this patch going in via the staging tree, instead of -tip so this should be ok (and much simpler then dealing with cross-maintainer dependencies).

Greg: Would you mind applying this set to staging-next for 3.5?

thanks
-john

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Greg KH <[email protected]>
CC Android Kernel Team <[email protected]>

John Stultz (4):
android-alarm: Convert ALARM_ELAPSED_REALTIME to use CLOCK_BOOTTIME
alarmtimer: Provide accessor to alarmtimer rtc device
android-alarm: Rework alarm-dev.c to use upstreamed alarmtimers
android-alarm: Remove unused android alarm in-kernel interfaces

 drivers/staging/android/Kconfig         |   21 +-
 drivers/staging/android/Makefile        |    1 -
 drivers/staging/android/alarm-dev.c     |  111 +++++-
 drivers/staging/android/alarm.c         |  601 -------------------------------
 drivers/staging/android/android_alarm.h |   59 ---
 include/linux/alarmtimer.h              |    3 +
 kernel/time/alarmtimer.c                |    4 +-
 7 files changed, 106 insertions(+), 694 deletions(-)
 delete mode 100644 drivers/staging/android/alarm.c
Signature · 1 line
1.7.3.2.146.gca209
#1 lore raw

PATCH 1/4 android-alarm: Convert ALARM_ELAPSED_REALTIME to use CLOCK_BOOTTIME

The ALARM_ELAPSED_REALTIME clock domain in Android pointed to the need for something similar in linux system-wide (instead of limited to just the alarm interface).

Thus CLOCK_BOOTTIME was introduced into the upstream kernel in 2.6.39.

This patch attempts to convert the android alarm timer to utilize the kernel's CLOCK_BOOTTIME clockid for ALARM_ELAPSED_REALTIME, instead of managing it itself.

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Greg KH <[email protected]>
CC Android Kernel Team <[email protected]>
---
 drivers/staging/android/alarm-dev.c     |    3 +-
 drivers/staging/android/alarm.c         |   70 +++++++------------------------
 drivers/staging/android/android_alarm.h |    7 +++-
 3 files changed, 22 insertions(+), 58 deletions(-)
drivers/staging/android/alarm-dev.c +1 -2
index 03efb34..bc57568 100644
@@ -179,8 +179,7 @@ from_old_alarm_set:
 			break;
 		case ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP:
 		case ANDROID_ALARM_ELAPSED_REALTIME:
-			tmp_time =
-				ktime_to_timespec(alarm_get_elapsed_realtime());
+			get_monotonic_boottime(&tmp_time);
 			break;
 		case ANDROID_ALARM_TYPE_COUNT:
 		case ANDROID_ALARM_SYSTEMTIME:
drivers/staging/android/alarm.c +15 -55

118 lines not on this page · raw message

drivers/staging/android/android_alarm.h +6 -1
index 6eecbde..011be19 100644
@@ -37,6 +37,7 @@ enum android_alarm_type {
 
 #include <linux/ktime.h>
 #include <linux/rbtree.h>
+#include <linux/hrtimer.h>
 
 /*
  * The alarm interface is similar to the hrtimer interface but adds support
@@ -71,7 +72,11 @@ void android_alarm_start_range(struct android_alarm *alarm, ktime_t start,
 								ktime_t end);
 int android_alarm_try_to_cancel(struct android_alarm *alarm);
 int android_alarm_cancel(struct android_alarm *alarm);
-ktime_t alarm_get_elapsed_realtime(void);
+
+static inline ktime_t alarm_get_elapsed_realtime(void)
+{
+	return ktime_get_boottime();
+}
 
 /* set rtc while preserving elapsed realtime */
 int android_alarm_set_rtc(const struct timespec ts);
Signature · 1 line
1.7.3.2.146.gca209
#2 lore raw

PATCH 2/4 alarmtimer: Provide accessor to alarmtimer rtc device

In reply to John Stultz

The Android alarm interface provides a settime call that sets both the alarmtimer RTC device and CLOCK_REALTIME to the same value.

Since there may be multiple rtc devices, provide a hook to access the one the alarmtimer infrastructure is using.

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Greg KH <[email protected]>
CC Android Kernel Team <[email protected]>
---
 include/linux/alarmtimer.h |    3 +++
 kernel/time/alarmtimer.c   |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)
include/linux/alarmtimer.h +3 -0
index 975009e..96c5c24 100644
@@ -76,4 +76,7 @@ static inline int alarmtimer_callback_running(struct alarm *timer)
 }
 
 
+/* Provide way to access the rtc device being used by alarmtimers */
+struct rtc_device *alarmtimer_get_rtcdev(void);
+
 #endif
kernel/time/alarmtimer.c +2 -2
index 8a538c55..aa27d39 100644
@@ -59,7 +59,7 @@ static DEFINE_SPINLOCK(rtcdev_lock);
  * If one has not already been chosen, it checks to see if a
  * functional rtc device is available.
  */
-static struct rtc_device *alarmtimer_get_rtcdev(void)
+struct rtc_device *alarmtimer_get_rtcdev(void)
 {
 	unsigned long flags;
 	struct rtc_device *ret;
@@ -115,7 +115,7 @@ static void alarmtimer_rtc_interface_remove(void)
 	class_interface_unregister(&alarmtimer_rtc_interface);
 }
 #else
-static inline struct rtc_device *alarmtimer_get_rtcdev(void)
+struct rtc_device *alarmtimer_get_rtcdev(void)
 {
 	return NULL;
 }
Signature · 1 line
1.7.3.2.146.gca209
#3 lore raw

PATCH 3/4 android-alarm: Rework alarm-dev.c to use upstreamed alarmtimers

In reply to John Stultz

This reworks the alarm-dev.c to use the upstreamed alarmtimers interface.

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Greg KH <[email protected]>
CC Android Kernel Team <[email protected]>
---
 drivers/staging/android/alarm-dev.c |  108 +++++++++++++++++++++++++++++++---
 1 files changed, 98 insertions(+), 10 deletions(-)
drivers/staging/android/alarm-dev.c +98 -10
index bc57568..7739029 100644
@@ -22,6 +22,7 @@
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
+#include <linux/alarmtimer.h>
 #include "android_alarm.h"
 
 /* XXX - Hack out wakelocks, while they are out of tree */
@@ -66,7 +67,53 @@ static uint32_t alarm_pending;
 static uint32_t alarm_enabled;
 static uint32_t wait_pending;
 
-static struct android_alarm alarms[ANDROID_ALARM_TYPE_COUNT];
+struct devalarm {
+	union {
+		struct hrtimer hrt;
+		struct alarm alrm;
+	} u;
+	enum android_alarm_type type;
+};
+
+static struct devalarm alarms[ANDROID_ALARM_TYPE_COUNT];
+
+
+static int is_wakeup(enum android_alarm_type type)
+{
+	if (type == ANDROID_ALARM_RTC_WAKEUP ||
+			type == ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP)
+		return 1;
+	return 0;
+}
+
+
+static void devalarm_start(struct devalarm *alrm, ktime_t exp)
+{
+	if (is_wakeup(alrm->type))
+		alarm_start(&alrm->u.alrm, exp);
+	else
+		hrtimer_start(&alrm->u.hrt, exp, HRTIMER_MODE_ABS);
+}
+
+
+static int devalarm_try_to_cancel(struct devalarm *alrm)
+{
+	int ret;
+	if (is_wakeup(alrm->type))
+		ret = alarm_try_to_cancel(&alrm->u.alrm);
+	else
+		ret = hrtimer_try_to_cancel(&alrm->u.hrt);
+	return ret;
+}
+
+static void devalarm_cancel(struct devalarm *alrm)
+{
+	if (is_wakeup(alrm->type))
+		alarm_cancel(&alrm->u.alrm);
+	else
+		hrtimer_cancel(&alrm->u.hrt);
+}
+
 
 static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -75,6 +122,8 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct timespec new_alarm_time;
 	struct timespec new_rtc_time;
 	struct timespec tmp_time;
+	struct rtc_time new_rtc_tm;
+	struct rtc_device *rtc_dev;
 	enum android_alarm_type alarm_type = ANDROID_ALARM_IOCTL_TO_TYPE(cmd);
 	uint32_t alarm_type_mask = 1U << alarm_type;
 
@@ -101,7 +150,7 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case ANDROID_ALARM_CLEAR(0):
 		spin_lock_irqsave(&alarm_slock, flags);
 		pr_alarm(IO, "alarm %d clear\n", alarm_type);
-		android_alarm_try_to_cancel(&alarms[alarm_type]);
+		devalarm_try_to_cancel(&alarms[alarm_type]);
 		if (alarm_pending) {
 			alarm_pending &= ~alarm_type_mask;
 			if (!alarm_pending && !wait_pending)
@@ -132,8 +181,7 @@ from_old_alarm_set:
 		pr_alarm(IO, "alarm %d set %ld.%09ld\n", alarm_type,
 			new_alarm_time.tv_sec, new_alarm_time.tv_nsec);
 		alarm_enabled |= alarm_type_mask;
-		android_alarm_start_range(&alarms[alarm_type],
-			timespec_to_ktime(new_alarm_time),
+		devalarm_start(&alarms[alarm_type],
 			timespec_to_ktime(new_alarm_time));
 		spin_unlock_irqrestore(&alarm_slock, flags);
 		if (ANDROID_ALARM_BASE_CMD(cmd) != ANDROID_ALARM_SET_AND_WAIT(0)
@@ -163,7 +211,13 @@ from_old_alarm_set:
 			rv = -EFAULT;
 			goto err1;
 		}
-		rv = android_alarm_set_rtc(new_rtc_time);
+		rtc_time_to_tm(new_rtc_time.tv_sec, &new_rtc_tm);
+		rtc_dev = alarmtimer_get_rtcdev();
+		rv = do_settimeofday(&new_rtc_time);
+		if (rv < 0)
+			goto err1;
+		if (rtc_dev)
+			rv = rtc_set_time(rtc_dev, &new_rtc_tm);
 		spin_lock_irqsave(&alarm_slock, flags);
 		alarm_pending |= ANDROID_ALARM_TIME_CHANGE_MASK;
 		wake_up(&alarm_wait_queue);
@@ -223,7 +277,7 @@ static int alarm_release(struct inode *inode, struct file *file)
 				alarm_enabled &= ~alarm_type_mask;
 			}
 			spin_unlock_irqrestore(&alarm_slock, flags);
-			android_alarm_cancel(&alarms[i]);
+			devalarm_cancel(&alarms[i]);
 			spin_lock_irqsave(&alarm_slock, flags);
 		}
 		if (alarm_pending | wait_pending) {
@@ -240,12 +294,12 @@ static int alarm_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static void alarm_triggered(struct android_alarm *alarm)
+static void devalarm_triggered(struct devalarm *alarm)
 {
 	unsigned long flags;
 	uint32_t alarm_type_mask = 1U << alarm->type;
 
-	pr_alarm(INT, "alarm_triggered type %d\n", alarm->type);
+	pr_alarm(INT, "devalarm_triggered type %d\n", alarm->type);
 	spin_lock_irqsave(&alarm_slock, flags);
 	if (alarm_enabled & alarm_type_mask) {
 		wake_lock_timeout(&alarm_wake_lock, 5 * HZ);
@@ -256,6 +310,25 @@ static void alarm_triggered(struct android_alarm *alarm)
 	spin_unlock_irqrestore(&alarm_slock, flags);
 }
 
+
+static enum hrtimer_restart devalarm_hrthandler(struct hrtimer *hrt)
+{
+	struct devalarm *devalrm = container_of(hrt, struct devalarm, u.hrt);
+
+	devalarm_triggered(devalrm);
+	return HRTIMER_NORESTART;
+}
+
+static enum alarmtimer_restart devalarm_alarmhandler(struct alarm *alrm,
+							ktime_t now)
+{
+	struct devalarm *devalrm = container_of(alrm, struct devalarm, u.alrm);
+
+	devalarm_triggered(devalrm);
+	return ALARMTIMER_NORESTART;
+}
+
+
 static const struct file_operations alarm_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = alarm_ioctl,
@@ -278,8 +351,23 @@ static int __init alarm_dev_init(void)
 	if (err)
 		return err;
 
-	for (i = 0; i < ANDROID_ALARM_TYPE_COUNT; i++)
-		android_alarm_init(&alarms[i], i, alarm_triggered);
+	alarm_init(&alarms[ANDROID_ALARM_RTC_WAKEUP].u.alrm,
+			ALARM_REALTIME, devalarm_alarmhandler);
+	hrtimer_init(&alarms[ANDROID_ALARM_RTC].u.hrt,
+			CLOCK_REALTIME, HRTIMER_MODE_ABS);
+	alarm_init(&alarms[ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP].u.alrm,
+			ALARM_BOOTTIME, devalarm_alarmhandler);
+	hrtimer_init(&alarms[ANDROID_ALARM_ELAPSED_REALTIME].u.hrt,
+			CLOCK_BOOTTIME, HRTIMER_MODE_ABS);
+	hrtimer_init(&alarms[ANDROID_ALARM_SYSTEMTIME].u.hrt,
+			CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+
+	for (i = 0; i < ANDROID_ALARM_TYPE_COUNT; i++) {
+		alarms[i].type = i;
+		if (!is_wakeup(i))
+			alarms[i].u.hrt.function = devalarm_hrthandler;
+	}
+
 	wake_lock_init(&alarm_wake_lock, WAKE_LOCK_SUSPEND, "alarm");
 
 	return 0;
Signature · 1 line
1.7.3.2.146.gca209
maintainer
#4 lore raw

Re: [PATCH 3/4] android-alarm: Rework alarm-dev.c to use upstreamed alarmtimers

Quoted 10 lines · John Stultz

This reworks the alarm-dev.c to use the upstreamed alarmtimers interface.

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Android Kernel Team <[email protected]>
---
 drivers/staging/android/alarm-dev.c |  108 +++++++++++++++++++++++++++++++---
 1 files changed, 98 insertions(+), 10 deletions(-)

After this patch, I get the following warnings:

CC drivers/staging/android/alarm-dev.o
drivers/staging/android/alarm-dev.c: In function ‘devalarm_start’:
drivers/staging/android/alarm-dev.c:93:3: warning: passing argument 1 of ‘alarm_start’ from incompatible pointer type [enabled by default] include/linux/alarmtimer.h:46:6: note: expected ‘struct alarm *’ but argument is of type ‘struct android_alarm *’
drivers/staging/android/alarm-dev.c: In function ‘alarm_dev_init’:
drivers/staging/android/alarm-dev.c:354:2: warning: passing argument 3 of ‘android_alarm_init’ from incompatible pointer type [enabled by default] drivers/staging/android/android_alarm.h:69:6: note: expected ‘void (*)(struct android_alarm *)’ but argument is of type ‘enum alarmtimer_restart (*)(struct android_alarm *, union ktime_t)’ drivers/staging/android/alarm-dev.c:358:2: warning: passing argument 3 of ‘android_alarm_init’ from incompatible pointer type [enabled by default] drivers/staging/android/android_alarm.h:69:6: note: expected ‘void (*)(struct android_alarm *)’ but argument is of type ‘enum alarmtimer_restart (*)(struct android_alarm *, union ktime_t)’

Care to send a patch that fixes this up as well, as odds are, something just went wrong...

thanks,

greg k-h

#5 lore raw

PATCH 4/4 android-alarm: Remove unused android alarm in-kernel interfaces

In reply to John Stultz

Now that alarm-dev.c uses the upstreamed alarmtimer interfaces, we can remove the otherwise unused in-kernel android alarm api.

CC Colin Cross <[email protected]>
CC Thomas Gleixner <[email protected]>
CC Greg KH <[email protected]>
CC Android Kernel Team <[email protected]>
---
 drivers/staging/android/Kconfig         |   21 +-
 drivers/staging/android/Makefile        |    1 -
 drivers/staging/android/alarm.c         |  561 -------------------------------
 drivers/staging/android/android_alarm.h |   64 ----
 4 files changed, 2 insertions(+), 645 deletions(-)
 delete mode 100644 drivers/staging/android/alarm.c
drivers/staging/android/Kconfig +2 -19
index eb1dee2..775229d 100644
@@ -55,31 +55,14 @@ config ANDROID_LOW_MEMORY_KILLER
 
 source "drivers/staging/android/switch/Kconfig"
 
-config ANDROID_INTF_ALARM
+config ANDROID_INTF_ALARM_DEV
 	bool "Android alarm driver"
 	depends on RTC_CLASS
 	default n
 	help
 	  Provides non-wakeup and rtc backed wakeup alarms based on rtc or
 	  elapsed realtime, and a non-wakeup alarm on the monotonic clock.
-	  Also provides an interface to set the wall time which must be used
-	  for elapsed realtime to work.
-
-config ANDROID_INTF_ALARM_DEV
-	bool "Android alarm device"
-	depends on ANDROID_INTF_ALARM
-	default y
-	help
-	  Exports the alarm interface to user-space.
-
-config ANDROID_ALARM_OLDDRV_COMPAT
-	bool "Android Alarm compatability with old drivers"
-	depends on ANDROID_INTF_ALARM
-	default n
-	help
-	  Provides preprocessor alias to aid compatability with
-	  older out-of-tree drivers that use the Android Alarm
-	  in-kernel API. This will be removed eventually.
+	  Also exports the alarm interface to user-space.
 
 endif # if ANDROID
 
drivers/staging/android/Makefile +0 -1
index 9b6c9ed..b4be69f 100644
@@ -7,5 +7,4 @@ obj-$(CONFIG_ANDROID_TIMED_OUTPUT)	+= timed_output.o
 obj-$(CONFIG_ANDROID_TIMED_GPIO)	+= timed_gpio.o
 obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
 obj-$(CONFIG_ANDROID_SWITCH)		+= switch/
-obj-$(CONFIG_ANDROID_INTF_ALARM)	+= alarm.o
 obj-$(CONFIG_ANDROID_INTF_ALARM_DEV)	+= alarm-dev.o
drivers/staging/android/alarm.c deleted +0 -561

561 lines not on this page · raw message

drivers/staging/android/android_alarm.h +0 -64

70 lines not on this page · raw message

Signature · 1 line
1.7.3.2.146.gca209
maintainer
#6 lore raw
In reply to John Stultz
Quoted 15 lines · John Stultz

This patch-set moves the Android alarm driver in staging to use the upstreamed CLOCK_BOOTTIME and alarmtimer interfaces, allowing us to greatly shrink the alarm driver.

The Google Android team has been using these patches in its AOSP tree for a little while without any reported issues.

The only gotcha is that I needed to make one alarmtimer interface non-static in order to support the android alarm driver. This sort of is a grey area for the staging tree, but I'm the author of the upstreamed alarmtimer code, and Thomas (cc'ed) had no objection to this patch going in via the staging tree, instead of -tip so this should be ok (and much simpler then dealing with cross-maintainer dependencies).

Quoted 1 line

Greg: Would you mind applying this set to staging-next for 3.5?

It took some messing with (due to some over-eager people fixing spelling mistakes and compiler warnings that you didn't have in your tree), but I've applied these now. Thanks for doing this work.

But, as I already said, this patch set produces some big compiler warnings, can you please fix them up before I get nasty messages from the linux-next maintainer?

thanks,

greg k-h

#7 lore raw
Quoted 9 lines · Greg KH
Quoted 1 line · John Stultz

Greg: Would you mind applying this set to staging-next for 3.5?

It took some messing with (due to some over-eager people fixing spelling mistakes and compiler warnings that you didn't have in your tree), but I've applied these now. Thanks for doing this work.

But, as I already said, this patch set produces some big compiler warnings, can you please fix them up before I get nasty messages from the linux-next maintainer?

Ok. So I manged to trigger the warning at patch 3/4 too. However, patch 4/4 resolves it.

The problem is the android alarm driver and the alarmtimer code have a naming collision with "struct alarm", and there were some macros (see CONFIG_ANDROID_ALARM_OLDDRV_COMPAT in staging/android/android_alarm.h before these patches) that allow a easier transition over to the new code for out of tree drivers.

Ideally I would have yanked those macros in 3/4 instead of 4/4. But after 4/4 is applied the issue should not longer be present (at least I've not been able to trigger it)

Do you want me to respin the last two patches?

thanks
-john

maintainer
#8 lore raw
Quoted 7 lines · John Stultz
Quoted 6 lines · Greg KH
Quoted 1 line · John Stultz

Greg: Would you mind applying this set to staging-next for 3.5?

It took some messing with (due to some over-eager people fixing spelling mistakes and compiler warnings that you didn't have in your tree), but I've applied these now. Thanks for doing this work.

Quoted 5 lines
Quoted 3 lines

But, as I already said, this patch set produces some big compiler warnings, can you please fix them up before I get nasty messages from the linux-next maintainer?

Ok. So I manged to trigger the warning at patch 3/4 too. However, patch 4/4 resolves it.

Ah, I didn't notice that, you are right, nice.

Quoted 11 lines

The problem is the android alarm driver and the alarmtimer code have a naming collision with "struct alarm", and there were some macros (see CONFIG_ANDROID_ALARM_OLDDRV_COMPAT in staging/android/android_alarm.h before these patches) that allow a easier transition over to the new code for out of tree drivers.

Ideally I would have yanked those macros in 3/4 instead of 4/4. But after 4/4 is applied the issue should not longer be present (at least I've not been able to trigger it)

Do you want me to respin the last two patches?

No, they are both applied now, so all is good, sorry for the noise.

greg k-h