[RFC][PATCH 0/2] staging: alarm-dev: compat_ioctl support

[RFC][PATCH 0/2] staging: alarm-dev: compat_ioctl support

[John Stultz]

This is a first-pass at implementing compat_ioctl support for
Android's alarm-dev driver in staging.

The first patch is some refactoring to cleanup and separate the
copying of user data from the logic, and the second patch adds
the compat_ioctl support.

As noted in the patch, the only really squirly bit is the handling
of ANDROID_ALARM_SET_OLD and ANDROID_ALARM_SET_AND_WAIT_OLD. These
are present to support existing older Android applications.
Unfortunately these cause ioctl number aliasing issues with
the compat ioctls, so on 64bit they are only supported in the
compat patch (since any existing android app old enough to use
them are likely 32bit).

I'd appreciate feedback from the Android devs on if this is
an ok assumption and if it might be reasonable to establish
some sort of a phase-out timeline for the _OLD ioctl support.

Any other feedback or thoughts would be welcome!

thanks
-john

Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Serban Constantinescu <serban.constantinescu@arm.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Android Kernel Team <kernel-team@android.com>

John Stultz (2):
  staging: alarm-dev: Refactor alarm-dev ioctl code in prep for
    compat_ioctl
  staging: alarm-dev: Implement compat_ioctl support

 drivers/staging/android/alarm-dev.c     |  290 +++++++++++++++++++++----------
 drivers/staging/android/android_alarm.h |   19 ++
 2 files changed, 213 insertions(+), 96 deletions(-)

-- 
1.7.10.4

[RFC][PATCH 1/2] staging: alarm-dev: Refactor alarm-dev ioctl code in prep for compat_ioctl

[John Stultz]

Cleanup the Android alarm-dev driver's ioctl code to refactor it
in preparation for compat_ioctl support.

Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Serban Constantinescu <serban.constantinescu@arm.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/alarm-dev.c |  242 +++++++++++++++++++++--------------
 1 file changed, 146 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index a9b293f..12c570d 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -96,18 +96,116 @@ static void devalarm_cancel(struct devalarm *alrm)
 		hrtimer_cancel(&alrm->u.hrt);
 }
 
+static void alarm_clear(enum android_alarm_type alarm_type)
+{
+	uint32_t alarm_type_mask = 1U << alarm_type;
+	unsigned long flags;
 
-static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+	spin_lock_irqsave(&alarm_slock, flags);
+	alarm_dbg(IO, "alarm %d clear\n", alarm_type);
+	devalarm_try_to_cancel(&alarms[alarm_type]);
+	if (alarm_pending) {
+		alarm_pending &= ~alarm_type_mask;
+		if (!alarm_pending && !wait_pending)
+			__pm_relax(&alarm_wake_lock);
+	}
+	alarm_enabled &= ~alarm_type_mask;
+	spin_unlock_irqrestore(&alarm_slock, flags);
+
+}
+
+static void alarm_set(enum android_alarm_type alarm_type,
+							struct timespec *ts)
 {
-	int rv = 0;
+	uint32_t alarm_type_mask = 1U << alarm_type;
 	unsigned long flags;
-	struct timespec new_alarm_time;
-	struct timespec new_rtc_time;
-	struct timespec tmp_time;
+
+	spin_lock_irqsave(&alarm_slock, flags);
+	alarm_dbg(IO, "alarm %d set %ld.%09ld\n",
+			alarm_type, ts->tv_sec, ts->tv_nsec);
+	alarm_enabled |= alarm_type_mask;
+	devalarm_start(&alarms[alarm_type], timespec_to_ktime(*ts));
+	spin_unlock_irqrestore(&alarm_slock, flags);
+}
+
+static int alarm_wait(void)
+{
+	unsigned long flags;
+	int rv = 0;
+
+	spin_lock_irqsave(&alarm_slock, flags);
+	alarm_dbg(IO, "alarm wait\n");
+	if (!alarm_pending && wait_pending) {
+		__pm_relax(&alarm_wake_lock);
+		wait_pending = 0;
+	}
+	spin_unlock_irqrestore(&alarm_slock, flags);
+
+	rv = wait_event_interruptible(alarm_wait_queue, alarm_pending);
+	if (rv)
+		return rv;
+
+	spin_lock_irqsave(&alarm_slock, flags);
+	rv = alarm_pending;
+	wait_pending = 1;
+	alarm_pending = 0;
+	spin_unlock_irqrestore(&alarm_slock, flags);
+
+	return rv;
+}
+
+static int alarm_set_rtc(struct timespec *ts)
+{
 	struct rtc_time new_rtc_tm;
 	struct rtc_device *rtc_dev;
+	unsigned long flags;
+	int rv = 0;
+
+	rtc_time_to_tm(ts->tv_sec, &new_rtc_tm);
+	rtc_dev = alarmtimer_get_rtcdev();
+	rv = do_settimeofday(ts);
+	if (rv < 0)
+		return rv;
+	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);
+	spin_unlock_irqrestore(&alarm_slock, flags);
+
+	return rv;
+}
+
+static int alarm_get_time(enum android_alarm_type alarm_type,
+							struct timespec *ts)
+{
+	int rv = 0;
+
+	switch (alarm_type) {
+	case ANDROID_ALARM_RTC_WAKEUP:
+	case ANDROID_ALARM_RTC:
+		getnstimeofday(ts);
+		break;
+	case ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP:
+	case ANDROID_ALARM_ELAPSED_REALTIME:
+		get_monotonic_boottime(ts);
+		break;
+	case ANDROID_ALARM_SYSTEMTIME:
+		ktime_get_ts(ts);
+		break;
+	default:
+		rv = -EINVAL;
+	}
+	return rv;
+}
+
+static long alarm_do_ioctl(struct file *file, unsigned int cmd,
+							struct timespec *ts)
+{
+	int rv = 0;
+	unsigned long flags;
 	enum android_alarm_type alarm_type = ANDROID_ALARM_IOCTL_TO_TYPE(cmd);
-	uint32_t alarm_type_mask = 1U << alarm_type;
 
 	if (alarm_type >= ANDROID_ALARM_TYPE_COUNT)
 		return -EINVAL;
@@ -130,112 +228,64 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	switch (ANDROID_ALARM_BASE_CMD(cmd)) {
 	case ANDROID_ALARM_CLEAR(0):
-		spin_lock_irqsave(&alarm_slock, flags);
-		alarm_dbg(IO, "alarm %d clear\n", alarm_type);
-		devalarm_try_to_cancel(&alarms[alarm_type]);
-		if (alarm_pending) {
-			alarm_pending &= ~alarm_type_mask;
-			if (!alarm_pending && !wait_pending)
-				__pm_relax(&alarm_wake_lock);
-		}
-		alarm_enabled &= ~alarm_type_mask;
-		spin_unlock_irqrestore(&alarm_slock, flags);
+		alarm_clear(alarm_type);
 		break;
 
 	case ANDROID_ALARM_SET_OLD:
+	case ANDROID_ALARM_SET(0):
+		alarm_set(alarm_type, ts);
+		break;
 	case ANDROID_ALARM_SET_AND_WAIT_OLD:
-		if (get_user(new_alarm_time.tv_sec, (int __user *)arg)) {
-			rv = -EFAULT;
-			goto err1;
-		}
-		new_alarm_time.tv_nsec = 0;
-		goto from_old_alarm_set;
-
 	case ANDROID_ALARM_SET_AND_WAIT(0):
-	case ANDROID_ALARM_SET(0):
-		if (copy_from_user(&new_alarm_time, (void __user *)arg,
-		    sizeof(new_alarm_time))) {
-			rv = -EFAULT;
-			goto err1;
-		}
-from_old_alarm_set:
-		spin_lock_irqsave(&alarm_slock, flags);
-		alarm_dbg(IO, "alarm %d set %ld.%09ld\n",
-			  alarm_type,
-			  new_alarm_time.tv_sec, new_alarm_time.tv_nsec);
-		alarm_enabled |= alarm_type_mask;
-		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)
-		    && cmd != ANDROID_ALARM_SET_AND_WAIT_OLD)
-			break;
+		alarm_set(alarm_type, ts);
 		/* fall though */
 	case ANDROID_ALARM_WAIT:
-		spin_lock_irqsave(&alarm_slock, flags);
-		alarm_dbg(IO, "alarm wait\n");
-		if (!alarm_pending && wait_pending) {
-			__pm_relax(&alarm_wake_lock);
-			wait_pending = 0;
-		}
-		spin_unlock_irqrestore(&alarm_slock, flags);
-		rv = wait_event_interruptible(alarm_wait_queue, alarm_pending);
-		if (rv)
-			goto err1;
-		spin_lock_irqsave(&alarm_slock, flags);
-		rv = alarm_pending;
-		wait_pending = 1;
-		alarm_pending = 0;
-		spin_unlock_irqrestore(&alarm_slock, flags);
+		rv = alarm_wait();
 		break;
 	case ANDROID_ALARM_SET_RTC:
-		if (copy_from_user(&new_rtc_time, (void __user *)arg,
-		    sizeof(new_rtc_time))) {
-			rv = -EFAULT;
-			goto err1;
-		}
-		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);
-		spin_unlock_irqrestore(&alarm_slock, flags);
-		if (rv < 0)
-			goto err1;
+		rv = alarm_set_rtc(ts);
 		break;
 	case ANDROID_ALARM_GET_TIME(0):
-		switch (alarm_type) {
-		case ANDROID_ALARM_RTC_WAKEUP:
-		case ANDROID_ALARM_RTC:
-			getnstimeofday(&tmp_time);
-			break;
-		case ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP:
-		case ANDROID_ALARM_ELAPSED_REALTIME:
-			get_monotonic_boottime(&tmp_time);
-			break;
-		case ANDROID_ALARM_SYSTEMTIME:
-			ktime_get_ts(&tmp_time);
-			break;
-		default:
-			rv = -EINVAL;
-			goto err1;
-		}
-		if (copy_to_user((void __user *)arg, &tmp_time,
-		    sizeof(tmp_time))) {
-			rv = -EFAULT;
-			goto err1;
-		}
+		rv = alarm_get_time(alarm_type, ts);
 		break;
 
 	default:
 		rv = -EINVAL;
 	}
-err1:
+	return rv;
+}
+
+static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+
+	struct timespec ts;
+	int rv;
+
+	switch (ANDROID_ALARM_BASE_CMD(cmd)) {
+	case ANDROID_ALARM_SET_OLD:
+	case ANDROID_ALARM_SET_AND_WAIT_OLD:
+		if (get_user(ts.tv_sec, (int __user *)arg))
+			return -EFAULT;
+
+		ts.tv_nsec = 0;
+		break;
+	case ANDROID_ALARM_SET_AND_WAIT(0):
+	case ANDROID_ALARM_SET(0):
+	case ANDROID_ALARM_SET_RTC:
+		if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+			return -EFAULT;
+		break;
+	}
+
+	rv = alarm_do_ioctl(file, cmd, &ts);
+
+	switch (ANDROID_ALARM_BASE_CMD(cmd)) {
+	case ANDROID_ALARM_GET_TIME(0):
+		if (copy_to_user((void __user *)arg, &ts, sizeof(ts)))
+			return -EFAULT;
+		break;
+	}
+
 	return rv;
 }
 
-- 
1.7.10.4

[RFC][PATCH 2/2] staging: alarm-dev: Implement compat_ioctl support

[John Stultz]

Implement compat_ioctl support for the alarm-dev ioctl.

The only really iffy bit here is that the ANDROID_ALARM_SET_OLD
and ANDROID_ALARM_SET_AND_WAIT_OLD support, which is there for
older Android compatability uses a time_t. On 64bit, this size
matches the compat_timespec size, causing ioctl number aliasing.

However, since the _OLD ioctls are only there to support existing
Android applications (all of which I'm assuming are 32bit), I've
made the _OLD ioctls 32bit only (so on 64bit we only support the
compat implementation).

Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Serban Constantinescu <serban.constantinescu@arm.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/alarm-dev.c     |   48 +++++++++++++++++++++++++++++++
 drivers/staging/android/android_alarm.h |   19 ++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index 12c570d..c8589bf 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -42,9 +42,15 @@ do {									\
 	ANDROID_ALARM_RTC_WAKEUP_MASK | \
 	ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP_MASK)
 
+#ifdef CONFIG_COMPAT
+/* Compat only on 64bit! */
+#define ANDROID_ALARM_SET_OLD               _IOW('a', 2, compat_time_t)
+#define ANDROID_ALARM_SET_AND_WAIT_OLD      _IOW('a', 3, compat_time_t)
+#else
 /* support old userspace code */
 #define ANDROID_ALARM_SET_OLD               _IOW('a', 2, time_t) /* set alarm */
 #define ANDROID_ALARM_SET_AND_WAIT_OLD      _IOW('a', 3, time_t)
+#endif
 
 static int alarm_opened;
 static DEFINE_SPINLOCK(alarm_slock);
@@ -288,6 +294,45 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	return rv;
 }
+#ifdef CONFIG_COMPAT
+static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
+{
+
+	struct timespec ts;
+	int rv;
+
+	switch (ANDROID_ALARM_BASE_CMD(cmd)) {
+	case ANDROID_ALARM_SET_OLD:
+	case ANDROID_ALARM_SET_AND_WAIT_OLD:
+		if (get_user(ts.tv_sec, (int __user *)arg))
+			return -EFAULT;
+
+		ts.tv_nsec = 0;
+		break;
+	case ANDROID_ALARM_SET_AND_WAIT_COMPAT(0):
+	case ANDROID_ALARM_SET_COMPAT(0):
+	case ANDROID_ALARM_SET_RTC_COMPAT:
+		if (compat_get_timespec(&ts, (void __user *)arg))
+			return -EFAULT;
+		/* fall through */
+	case ANDROID_ALARM_GET_TIME_COMPAT(0):
+		cmd = ANDROID_ALARM_COMPAT_TO_NORM(cmd);
+		break;
+	}
+
+	rv = alarm_do_ioctl(file, cmd, &ts);
+
+	switch (ANDROID_ALARM_BASE_CMD(cmd)) {
+	case ANDROID_ALARM_GET_TIME(0): /* NOTE: we modified cmd above */
+		if (compat_put_timespec(&ts, (void __user *)arg))
+			return -EFAULT;
+		break;
+	}
+
+	return rv;
+}
+#endif
 
 static int alarm_open(struct inode *inode, struct file *file)
 {
@@ -369,6 +414,9 @@ static const struct file_operations alarm_fops = {
 	.unlocked_ioctl = alarm_ioctl,
 	.open = alarm_open,
 	.release = alarm_release,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = alarm_compat_ioctl,
+#endif
 };
 
 static struct miscdevice alarm_device = {
diff --git a/drivers/staging/android/android_alarm.h b/drivers/staging/android/android_alarm.h
index d0cafd6..4fd32f3 100644
--- a/drivers/staging/android/android_alarm.h
+++ b/drivers/staging/android/android_alarm.h
@@ -18,6 +18,7 @@
 
 #include <linux/ioctl.h>
 #include <linux/time.h>
+#include <linux/compat.h>
 
 enum android_alarm_type {
 	/* return code bit numbers or set alarm arg */
@@ -59,4 +60,22 @@ enum android_alarm_return_flags {
 #define ANDROID_ALARM_BASE_CMD(cmd)         (cmd & ~(_IOC(0, 0, 0xf0, 0)))
 #define ANDROID_ALARM_IOCTL_TO_TYPE(cmd)    (_IOC_NR(cmd) >> 4)
 
+
+#ifdef CONFIG_COMPAT
+#define ANDROID_ALARM_SET_COMPAT(type)		ALARM_IOW(2, type, \
+							struct compat_timespec)
+#define ANDROID_ALARM_SET_AND_WAIT_COMPAT(type)	ALARM_IOW(3, type, \
+							struct compat_timespec)
+#define ANDROID_ALARM_GET_TIME_COMPAT(type)	ALARM_IOW(4, type, \
+							struct compat_timespec)
+#define ANDROID_ALARM_SET_RTC_COMPAT		_IOW('a', 5, \
+							struct compat_timespec)
+#define ANDROID_ALARM_IOCTL_NR(cmd)		(_IOC_NR(cmd) & ((1<<4)-1))
+#define ANDROID_ALARM_COMPAT_TO_NORM(cmd)  \
+				ALARM_IOW(ANDROID_ALARM_IOCTL_NR(cmd), \
+					ANDROID_ALARM_IOCTL_TO_TYPE(cmd), \
+					struct timespec)
+
+#endif
+
 #endif
-- 
1.7.10.4

Re: [RFC][PATCH 0/2] staging: alarm-dev: compat_ioctl support

[Colin Cross]

On Fri, Jan 11, 2013 at 1:48 PM, John Stultz <john.stultz@linaro.org> wrote:
> This is a first-pass at implementing compat_ioctl support for
> Android's alarm-dev driver in staging.
>
> The first patch is some refactoring to cleanup and separate the
> copying of user data from the logic, and the second patch adds
> the compat_ioctl support.
>
> As noted in the patch, the only really squirly bit is the handling
> of ANDROID_ALARM_SET_OLD and ANDROID_ALARM_SET_AND_WAIT_OLD. These
> are present to support existing older Android applications.
> Unfortunately these cause ioctl number aliasing issues with
> the compat ioctls, so on 64bit they are only supported in the
> compat patch (since any existing android app old enough to use
> them are likely 32bit).
>
> I'd appreciate feedback from the Android devs on if this is
> an ok assumption and if it might be reasonable to establish
> some sort of a phase-out timeline for the _OLD ioctl support.

The "support old userspace code" comment for those two ioctls has been
there since pre-Android 1.0.  Those apis are not exposed to Android
apps, I don't see any problem deleting them.

Re: [RFC][PATCH 0/2] staging: alarm-dev: compat_ioctl support

[John Stultz]

On 01/11/2013 02:03 PM, Colin Cross wrote:
> On Fri, Jan 11, 2013 at 1:48 PM, John Stultz <john.stultz@linaro.org> wrote:
>> This is a first-pass at implementing compat_ioctl support for
>> Android's alarm-dev driver in staging.
>>
>> The first patch is some refactoring to cleanup and separate the
>> copying of user data from the logic, and the second patch adds
>> the compat_ioctl support.
>>
>> As noted in the patch, the only really squirly bit is the handling
>> of ANDROID_ALARM_SET_OLD and ANDROID_ALARM_SET_AND_WAIT_OLD. These
>> are present to support existing older Android applications.
>> Unfortunately these cause ioctl number aliasing issues with
>> the compat ioctls, so on 64bit they are only supported in the
>> compat patch (since any existing android app old enough to use
>> them are likely 32bit).
>>
>> I'd appreciate feedback from the Android devs on if this is
>> an ok assumption and if it might be reasonable to establish
>> some sort of a phase-out timeline for the _OLD ioctl support.
> The "support old userspace code" comment for those two ioctls has been
> there since pre-Android 1.0.  Those apis are not exposed to Android
> apps, I don't see any problem deleting them.

That's great! I'll rework the patch set to include dropping them.

Thanks for the extra insight here!
-john