Re: [PATCH] LinuxPPS (with new syscalls API) - new version

[Rodolfo Giometti <giometti@enneenne.com>]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Tue, Jul 03, 2007 at 09:09:50AM -0400, David Woodhouse wrote:
> 
> Looks relatively sane at first glance; busy this week so haven't looked
> very hard yet. Two thing though... you're mixing proper C types
> (uint32_t) and the Linux-specific legacy crap types (__u32). Pick one. I
> won't recommend _which_ one, because if I do I'll make Andrew unhappy.
> But pick one; don't use both at the same time.

Ok. I choose __u32. :)

> Also read Documentation/volatile-considered-harmful.txt and ponder
> deeply your use of 'volatile' on certain members of struct pps_s.

I read such document but I'm still convinced that the attribute
volatile is needed for {assert,clear}_sequence and {assert,clear}_tu
since inside pps_event() they are updated without any locks at all
thanks to the dummy_info variable which is used for unallocated PPS
sources.

Here you can find my last patch.

Thanks again,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 13980 bytes --]

diff --git a/Documentation/pps/Makefile b/Documentation/pps/Makefile
diff --git a/Documentation/pps/timepps.h b/Documentation/pps/timepps.h
index a7719eb..0427ee0 100644
--- a/Documentation/pps/timepps.h
+++ b/Documentation/pps/timepps.h
@@ -28,6 +28,32 @@
 
 /* --- 3.2 New data structures --------------------------------------------- */
 
+struct ntp_fp {
+	unsigned int integral;
+	unsigned int fractional;
+};
+
+union pps_timeu {
+	struct timespec tspec;
+	struct ntp_fp ntpfp;
+	unsigned long longpad[3];
+};
+
+struct pps_info {
+	unsigned long assert_sequence;	/* seq. num. of assert event */
+	unsigned long clear_sequence;	/* seq. num. of clear event */
+	union pps_timeu assert_tu;	/* time of assert event */
+	union pps_timeu clear_tu;	/* time of clear event */
+	int current_mode;		/* current mode bits */
+};
+
+struct pps_params {
+	int api_version;		/* API version # */
+	int mode;			/* mode bits */
+	union pps_timeu assert_off_tu;	/* offset compensation for assert */
+	union pps_timeu clear_off_tu;	/* offset compensation for clear */
+};
+
 typedef int pps_handle_t;		/* represents a PPS source */
 typedef unsigned long pps_seq_t;	/* sequence number */
 typedef struct ntp_fp ntp_fp_t;		/* NTP-compatible time stamp */
@@ -129,13 +155,34 @@ int time_pps_destroy(pps_handle_t handle)
 int time_pps_getparams(pps_handle_t handle,
 				pps_params_t *ppsparams)
 {
-	return syscall(__NR_time_pps_getparams, handle, ppsparams);
+	int ret;
+	struct pps_kparams __ppsparams;
+
+	ret = syscall(__NR_time_pps_getparams, handle, &__ppsparams);
+
+	ppsparams->api_version = __ppsparams.api_version;
+	ppsparams->mode = __ppsparams.mode;
+	ppsparams->assert_off_tu.tspec.tv_sec = __ppsparams.assert_off_tu.sec;
+	ppsparams->assert_off_tu.tspec.tv_nsec = __ppsparams.assert_off_tu.nsec;
+	ppsparams->clear_off_tu.tspec.tv_sec = __ppsparams.clear_off_tu.sec;
+	ppsparams->clear_off_tu.tspec.tv_nsec = __ppsparams.clear_off_tu.nsec;
+
+	return ret;
 }
 
 int time_pps_setparams(pps_handle_t handle,
 				const pps_params_t *ppsparams)
 {
-	return syscall(__NR_time_pps_getparams, handle, ppsparams);
+	struct pps_kparams __ppsparams;
+
+	__ppsparams.api_version = ppsparams->api_version;
+	__ppsparams.mode = ppsparams->mode;
+	__ppsparams.assert_off_tu.sec = ppsparams->assert_off_tu.tspec.tv_sec;
+	__ppsparams.assert_off_tu.nsec = ppsparams->assert_off_tu.tspec.tv_nsec;
+	__ppsparams.clear_off_tu.sec = ppsparams->clear_off_tu.tspec.tv_sec;
+	__ppsparams.clear_off_tu.nsec = ppsparams->clear_off_tu.tspec.tv_nsec;
+
+	return syscall(__NR_time_pps_getparams, handle, &__ppsparams);
 }
 
 /* Get capabilities for handle */
@@ -148,8 +195,33 @@ int time_pps_fetch(pps_handle_t handle, const int tsformat,
 				pps_info_t *ppsinfobuf,
 				const struct timespec *timeout)
 {
-	return syscall(__NR_time_pps_fetch, handle,
-				tsformat, ppsinfobuf, timeout);
+	struct pps_kinfo __ppsinfobuf;
+	struct pps_ktime __timeout;
+	int ret;
+
+	/* Sanity checks */
+	if (tsformat != PPS_TSFMT_TSPEC) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (timeout) {
+		__timeout.sec = timeout->tv_sec;
+		__timeout.nsec = timeout->tv_nsec;
+	}
+
+	ret = syscall(__NR_time_pps_fetch, handle, &__ppsinfobuf,
+			timeout ? &__timeout : NULL);
+
+	ppsinfobuf->assert_sequence = __ppsinfobuf.assert_sequence;
+	ppsinfobuf->clear_sequence = __ppsinfobuf.clear_sequence;
+	ppsinfobuf->assert_tu.tspec.tv_sec = __ppsinfobuf.assert_tu.sec;
+	ppsinfobuf->assert_tu.tspec.tv_nsec = __ppsinfobuf.assert_tu.nsec;
+	ppsinfobuf->clear_tu.tspec.tv_sec = __ppsinfobuf.clear_tu.sec;
+	ppsinfobuf->clear_tu.tspec.tv_nsec = __ppsinfobuf.clear_tu.nsec;
+	ppsinfobuf->current_mode = __ppsinfobuf.current_mode;
+
+	return ret;
 }
 
 int time_pps_kcbind(pps_handle_t handle, const int kernel_consumer,
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index dd24c31..b6ad93e 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -31,17 +31,17 @@
  * Local functions
  */
 
-static void pps_add_offset(struct timespec *ts, struct timespec *offset)
+static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
 {
-	ts->tv_nsec += offset->tv_nsec;
-	if (ts->tv_nsec >= NSEC_PER_SEC) {
-		ts->tv_nsec -= NSEC_PER_SEC;
-		ts->tv_sec++;
-	} else if (ts->tv_nsec < 0) {
-		ts->tv_nsec += NSEC_PER_SEC;
-		ts->tv_sec--;
+	ts->nsec += offset->nsec;
+	if (ts->nsec >= NSEC_PER_SEC) {
+		ts->nsec -= NSEC_PER_SEC;
+		ts->sec++;
+	} else if (ts->nsec < 0) {
+		ts->nsec += NSEC_PER_SEC;
+		ts->sec--;
 	}
-	ts->tv_sec += offset->tv_sec;
+	ts->sec += offset->sec;
 }
 
 /*
@@ -87,7 +87,7 @@ static int __pps_register_source(struct pps_source_info_s *info,
 	/* Allocate the PPS source.
 	 *
 	 * Note that we should reset all fields BUT "info" one! */
-	memset(&(pps_source[i].params), 0, sizeof(struct pps_params));
+	memset(&(pps_source[i].params), 0, sizeof(struct pps_kparams));
 	pps_source[i].params.api_version = PPS_API_VERS;
 	pps_source[i].params.mode = default_params;
 	pps_source[i].assert_sequence = 0;
@@ -155,10 +155,15 @@ EXPORT_SYMBOL(pps_unregister_source);
 
 void pps_event(int source, int event, void *data)
 {
-	struct timespec ts;
+	struct timespec __ts;
+	struct pps_ktime ts;
 
 	/* First of all we get the time stamp... */
-	getnstimeofday(&ts);
+	getnstimeofday(&__ts);
+
+	/* ... and translate it to PPS time data struct */
+	ts.sec = __ts.tv_sec;
+	ts.nsec = __ts.tv_nsec;
 
 	if ((event & (PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0 ) {
 		pps_err("unknow event (%x) for source %d", event, source);
@@ -175,24 +180,24 @@ void pps_event(int source, int event, void *data)
 		/* We have to add an offset? */
 		if (pps_source[source].params.mode&PPS_OFFSETASSERT)
 			pps_add_offset(&ts,
-				&pps_source[source].params.assert_off_tu.tspec);
+				&pps_source[source].params.assert_off_tu);
 
 		/* Save the time stamp */
-		pps_source[source].assert_tu.tspec = ts;
+		pps_source[source].assert_tu = ts;
 		pps_source[source].assert_sequence++;
-		pps_dbg("capture assert seq #%lu for source %d", 
+		pps_dbg("capture assert seq #%u for source %d", 
 			pps_source[source].assert_sequence, source);
 	}
 	if (event & PPS_CAPTURECLEAR) {
 		/* We have to add an offset? */
 		if (pps_source[source].params.mode&PPS_OFFSETCLEAR)
 			pps_add_offset(&ts,
-				&pps_source[source].params.clear_off_tu.tspec);
+				&pps_source[source].params.clear_off_tu);
 
 		/* Save the time stamp */
-		pps_source[source].clear_tu.tspec = ts;
+		pps_source[source].clear_tu = ts;
 		pps_source[source].clear_sequence++;
-		pps_dbg("capture clear seq #%lu for source %d", 
+		pps_dbg("capture clear seq #%u for source %d", 
 			pps_source[source].clear_sequence, source);
 	}
 
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 428c3b7..3545c58 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -168,7 +168,7 @@ sys_time_pps_cmd_exit:
 }
 
 asmlinkage long sys_time_pps_getparams(int source,
-					struct pps_params __user *params)
+					struct pps_kparams __user *params)
 {
 	int ret = 0;
 
@@ -189,7 +189,7 @@ asmlinkage long sys_time_pps_getparams(int source,
 
 	/* Return current parameters */
 	ret = copy_to_user(params, &pps_source[source].params,
-						sizeof(struct pps_params));
+						sizeof(struct pps_kparams));
 	if (ret)
 		ret = -EFAULT;
 
@@ -200,7 +200,7 @@ sys_time_pps_getparams_exit:
 }
 
 asmlinkage long sys_time_pps_setparams(int source,
-					const struct pps_params __user *params)
+					const struct pps_kparams __user *params)
 {
 	int ret;
 
@@ -233,7 +233,7 @@ asmlinkage long sys_time_pps_setparams(int source,
 
 	/* Save the new parameters */
 	ret = copy_from_user(&pps_source[source].params, params,
-						sizeof(struct pps_params));
+						sizeof(struct pps_kparams));
 	if (ret) {
 		ret = -EFAULT;
 		goto sys_time_pps_setparams_exit;
@@ -282,22 +282,16 @@ sys_time_pps_getcap_exit:
 	return ret;
 }
 
-asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
-					struct pps_info __user *info, 
-					const struct timespec __user *timeout)
+asmlinkage long sys_time_pps_fetch(int source, struct pps_kinfo __user *info, 
+					const struct pps_ktime __user *timeout)
 {
 	unsigned long ticks;
-	struct pps_info pi;
-	struct timespec to;
+	struct pps_kinfo pi;
+	struct pps_ktime to;
 	int ret;
 
 	pps_dbg("%s: source %d", __FUNCTION__, source);
 
-	/* Sanity checks */
-	if (tsformat != PPS_TSFMT_TSPEC) {
-		pps_dbg("unsupported time format");
-		return -EINVAL;
- 	}
 	if (!info)
 		return -EINVAL;
 
@@ -314,25 +308,23 @@ asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
 
  	/* Manage the timeout */
 	if (timeout) {
-		ret = copy_from_user(&to, timeout, sizeof(struct timespec));
+		ret = copy_from_user(&to, timeout, sizeof(struct pps_ktime));
 		if (ret) {
 			goto sys_time_pps_fetch_exit;
 			ret = -EFAULT;
 		}
-		if (to.tv_sec != -1) {
-			pps_dbg("timeout %ld.%09ld", to.tv_sec, to.tv_nsec);
-			ticks = to.tv_sec * HZ;
-			ticks += to.tv_nsec / (NSEC_PER_SEC / HZ);
-
-			if (ticks != 0) {
-				ret = wait_event_interruptible_timeout(
-					pps_source[source].queue,
-					pps_source[source].go, ticks);
-  				if (ret == 0) {
-					pps_dbg("timeout expired");
-					ret = -ETIMEDOUT;
-					goto sys_time_pps_fetch_exit;
-				}
+		pps_dbg("timeout %d.%09d", to.sec, to.nsec);
+		ticks = to.sec * HZ;
+		ticks += to.nsec / (NSEC_PER_SEC / HZ);
+
+		if (ticks != 0) {
+			ret = wait_event_interruptible_timeout(
+				pps_source[source].queue,
+				pps_source[source].go, ticks);
+  			if (ret == 0) {
+				pps_dbg("timeout expired");
+				ret = -ETIMEDOUT;
+				goto sys_time_pps_fetch_exit;
 			}
 		}
  	} else
@@ -352,7 +344,7 @@ asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
 	pi.assert_tu = pps_source[source].assert_tu;
 	pi.clear_tu = pps_source[source].clear_tu;
 	pi.current_mode = pps_source[source].current_mode;
-	ret = copy_to_user(info, &pi, sizeof(struct pps_info));
+	ret = copy_to_user(info, &pi, sizeof(struct pps_kinfo));
 	if (ret)
 		ret = -EFAULT;
 
diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
index 0dae24b..46d7b7f 100644
--- a/drivers/pps/sysfs.c
+++ b/drivers/pps/sysfs.c
@@ -34,9 +34,8 @@ static ssize_t pps_show_assert(struct class_device *cdev, char *buf)
 {
 	struct pps_s *dev = class_get_devdata(cdev);
 
-	return sprintf(buf, "%ld.%09ld#%ld\n",
-			dev->assert_tu.tspec.tv_sec,
-			dev->assert_tu.tspec.tv_nsec,
+	return sprintf(buf, "%d.%09d#%d\n",
+			dev->assert_tu.sec, dev->assert_tu.nsec,
 			dev->assert_sequence);
 }
 
@@ -44,9 +43,8 @@ static ssize_t pps_show_clear(struct class_device *cdev, char *buf)
 {
 	struct pps_s *dev = class_get_devdata(cdev);
 
-	return sprintf(buf, "%ld.%09ld#%ld\n",
-			dev->clear_tu.tspec.tv_sec,
-			dev->clear_tu.tspec.tv_nsec,
+	return sprintf(buf, "%d.%09d#%d\n",
+			dev->clear_tu.sec, dev->clear_tu.nsec,
 			dev->clear_sequence);
 }
 
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 9e3af51..05397cd 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -44,30 +44,24 @@
 
 #define PPS_MAX_NAME_LEN	32
 
-struct ntp_fp {
-	unsigned int integral;
-	unsigned int fractional;
+struct pps_ktime {
+	__u32 sec;
+	__u32 nsec;
 };
 
-union pps_timeu {
-	struct timespec tspec;
-	struct ntp_fp ntpfp;
-	unsigned long longpad[3];
-};
-
-struct pps_info {
-	unsigned long assert_sequence;	/* seq. num. of assert event */
-	unsigned long clear_sequence; 	/* seq. num. of clear event */
-	union pps_timeu assert_tu;	/* time of assert event */
-	union pps_timeu clear_tu;	/* time of clear event */
+struct pps_kinfo {
+	__u32 assert_sequence;		/* seq. num. of assert event */
+	__u32 clear_sequence; 		/* seq. num. of clear event */
+	struct pps_ktime assert_tu;	/* time of assert event */
+	struct pps_ktime clear_tu;	/* time of clear event */
 	int current_mode;		/* current mode bits */
 };
 
-struct pps_params {
+struct pps_kparams {
 	int api_version;		/* API version # */
 	int mode;			/* mode bits */
-	union pps_timeu assert_off_tu;	/* offset compensation for assert */
-	union pps_timeu clear_off_tu;	/* offset compensation for clear */
+	struct pps_ktime assert_off_tu;	/* offset compensation for assert */
+	struct pps_ktime clear_off_tu;	/* offset compensation for clear */
 };
 
 /*
@@ -162,12 +156,12 @@ struct pps_source_info_s {
 struct pps_s {
 	struct pps_source_info_s *info;		/* PSS source info */
 
-	struct pps_params params;		/* PPS's current params */
+	struct pps_kparams params;		/* PPS's current params */
 
-	volatile unsigned long assert_sequence;	/* PPS' assert event seq # */
-	volatile unsigned long clear_sequence;	/* PPS' clear event seq # */
-	volatile union pps_timeu assert_tu;
-	volatile union pps_timeu clear_tu;
+	volatile __u32 assert_sequence;		/* PPS' assert event seq # */
+	volatile __u32 clear_sequence;		/* PPS' clear event seq # */
+	volatile struct pps_ktime assert_tu;
+	volatile struct pps_ktime clear_tu;
 	int current_mode;			/* PPS mode at event time */
 
 	int go;					/* PPS event is arrived? */
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 853a21e..bfc8899 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -614,13 +614,12 @@ asmlinkage long sys_eventfd(unsigned int count);
 
 asmlinkage long sys_time_pps_cmd(int cmd, void __user *arg);
 asmlinkage long sys_time_pps_getparams(int source,
-					struct pps_params __user *params);
+				       struct pps_kparams __user *params);
 asmlinkage long sys_time_pps_setparams(int source,
-					const struct pps_params __user *params);
+				       const struct pps_kparams __user *params);
 asmlinkage long sys_time_pps_getcap(int source, int __user *mode);
-asmlinkage long sys_time_pps_fetch(int source, const int tsformat,
-					struct pps_info __user *info,
-					const struct timespec __user *timeout);
+asmlinkage long sys_time_pps_fetch(int source, struct pps_kinfo __user *info,
+				       const struct pps_ktime __user *timeout);
 
 int kernel_execve(const char *filename, char *const argv[], char *const envp[]);