[RFC][PATCH 0/3] staging: alarm-dev: Cleanups and compat_ioctl support (v2)

[RFC][PATCH 0/3] staging: alarm-dev: Cleanups and compat_ioctl support (v2)

[John Stultz]

Here is the second revision at implementing compat_ioctl support for
Android's alarm-dev driver in staging.

The first patch removes the pre-Android 1.0 _OLD ioctls, per Colin's
suggestion. Then the second patch does some refactoring to cleanup
and separate the copying of user data from the logic, and the final
patch adds the compat_ioctl support.

I've tested this with a trivial unit test compiled for both 32bit
and 64bit on a x86_64 kernel, and it looks ok, but closer review
would be appreciated to make sure I didn't break anything.

Additional 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: Colin Cross <ccross@google.com>
Cc: Android Kernel Team <kernel-team@android.com>

John Stultz (3):
  staging: alarm-dev: Drop pre Android 1.0 _OLD ioctls
  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     |  277 +++++++++++++++++++------------
 drivers/staging/android/android_alarm.h |   19 +++
 2 files changed, 193 insertions(+), 103 deletions(-)

-- 
1.7.10.4

[RFC][PATCH 1/3] staging: alarm-dev: Drop pre Android 1.0 _OLD ioctls

[John Stultz]

Per Colin's comment:
"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."

Thus this patch removes the ANDROID_ALARM_SET_OLD and
ANDROID_ALARM_SET_AND_WAIT_OLD ioctl compatability
logic.

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

diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index a9b293f..d423850 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -42,10 +42,6 @@ do {									\
 	ANDROID_ALARM_RTC_WAKEUP_MASK | \
 	ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP_MASK)
 
-/* 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)
-
 static int alarm_opened;
 static DEFINE_SPINLOCK(alarm_slock);
 static struct wakeup_source alarm_wake_lock;
@@ -142,15 +138,6 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		spin_unlock_irqrestore(&alarm_slock, flags);
 		break;
 
-	case ANDROID_ALARM_SET_OLD:
-	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,
@@ -158,7 +145,6 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			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,
@@ -167,8 +153,8 @@ from_old_alarm_set:
 		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)
+		if (ANDROID_ALARM_BASE_CMD(cmd) !=
+						ANDROID_ALARM_SET_AND_WAIT(0))
 			break;
 		/* fall though */
 	case ANDROID_ALARM_WAIT:
-- 
1.7.10.4

[RFC][PATCH 2/3] 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: Colin Cross <ccross@google.com>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/alarm-dev.c |  228 +++++++++++++++++++++--------------
 1 file changed, 139 insertions(+), 89 deletions(-)

diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index d423850..43af3b3 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -92,18 +92,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)
+{
+	uint32_t alarm_type_mask = 1U << alarm_type;
+	unsigned long flags;
+
+	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)
 {
-	int rv = 0;
 	unsigned long flags;
-	struct timespec new_alarm_time;
-	struct timespec new_rtc_time;
-	struct timespec tmp_time;
+	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;
@@ -126,102 +224,54 @@ 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_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;
-		}
-		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))
-			break;
+		alarm_set(alarm_type, ts);
+		break;
+	case ANDROID_ALARM_SET_AND_WAIT(0):
+		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_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 3/3] staging: alarm-dev: Implement compat_ioctl support

[John Stultz]

Implement compat_ioctl support for the alarm-dev ioctl.

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

diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index 43af3b3..ceb1c64 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -274,6 +274,38 @@ 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_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)
 {
@@ -355,6 +387,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/3] staging: alarm-dev: Cleanups and compat_ioctl support (v2)

[Greg KH]

On Fri, Jan 11, 2013 at 03:46:21PM -0800, John Stultz wrote:
> Here is the second revision at implementing compat_ioctl support for
> Android's alarm-dev driver in staging.
> 
> The first patch removes the pre-Android 1.0 _OLD ioctls, per Colin's
> suggestion. Then the second patch does some refactoring to cleanup
> and separate the copying of user data from the logic, and the final
> patch adds the compat_ioctl support.
> 
> I've tested this with a trivial unit test compiled for both 32bit
> and 64bit on a x86_64 kernel, and it looks ok, but closer review
> would be appreciated to make sure I didn't break anything.
> 
> Additional feedback or thoughts would be welcome!

These look fine to me, want me to apply them to my -next tree for 3.9?

thanks,

greg k-h

Re: [RFC][PATCH 0/3] staging: alarm-dev: Cleanups and compat_ioctl support (v2)

[John Stultz]

On 01/17/2013 04:48 PM, Greg KH wrote:
> On Fri, Jan 11, 2013 at 03:46:21PM -0800, John Stultz wrote:
>> Here is the second revision at implementing compat_ioctl support for
>> Android's alarm-dev driver in staging.
>>
>> The first patch removes the pre-Android 1.0 _OLD ioctls, per Colin's
>> suggestion. Then the second patch does some refactoring to cleanup
>> and separate the copying of user data from the logic, and the final
>> patch adds the compat_ioctl support.
>>
>> I've tested this with a trivial unit test compiled for both 32bit
>> and 64bit on a x86_64 kernel, and it looks ok, but closer review
>> would be appreciated to make sure I didn't break anything.
>>
>> Additional feedback or thoughts would be welcome!
> These look fine to me, want me to apply them to my -next tree for 3.9?

Yea, sure. I was going to resend tomorrow without the RFC tag.

Colin, Arve: No objections?

thanks
-john

Re: [RFC][PATCH 0/3] staging: alarm-dev: Cleanups and compat_ioctl support (v2)

[Arve Hjønnevåg]

On Thu, Jan 17, 2013 at 5:09 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 01/17/2013 04:48 PM, Greg KH wrote:
>>
>> On Fri, Jan 11, 2013 at 03:46:21PM -0800, John Stultz wrote:
>>>
>>> Here is the second revision at implementing compat_ioctl support for
>>> Android's alarm-dev driver in staging.
>>>
>>> The first patch removes the pre-Android 1.0 _OLD ioctls, per Colin's
>>> suggestion. Then the second patch does some refactoring to cleanup
>>> and separate the copying of user data from the logic, and the final
>>> patch adds the compat_ioctl support.
>>>
>>> I've tested this with a trivial unit test compiled for both 32bit
>>> and 64bit on a x86_64 kernel, and it looks ok, but closer review
>>> would be appreciated to make sure I didn't break anything.
>>>
>>> Additional feedback or thoughts would be welcome!
>>
>> These look fine to me, want me to apply them to my -next tree for 3.9?
>
>
> Yea, sure. I was going to resend tomorrow without the RFC tag.
>
> Colin, Arve: No objections?
>

Looks OK to me, but have not tested them.

-- 
Arve Hjønnevåg