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

[Rodolfo Giometti <giometti@enneenne.com>]

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

On Sun, Jul 01, 2007 at 01:03:11PM +0100, David Woodhouse wrote:
> 
> Seems reasonable enough in principle -- but whatever you do, don't use
> "long" for it. That would definitely need different behaviour for 32-bit
> vs. 64-bit. Use explicitly sized types such as uint32_t or uint64_t.

Here the patch to convert LinuxPPS data structs into fixed ones.

Please, take a look at it and report possible modifications.

Thanks for your time,

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: 14151 bytes --]

diff --git a/Documentation/pps/timepps.h b/Documentation/pps/timepps.h
index d690aa6..fe8f467 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 */
@@ -86,7 +112,6 @@ void time_pps_readlink(char *link, int linklen, char *path, int pathlen)
 
 	/* Return the file name where "link" points to */
 	path[i] = '\0';
-	return;
 }
 
 #define PPS_HAVE_FINDPATH	1
@@ -108,8 +133,10 @@ int time_pps_findpath(char *path, int pathlen, char *idstring, int idlen)
 
 int time_pps_create(int source, pps_handle_t *handle)
 {
-	if (!handle)
-		return -EINVAL;
+	if (!handle) {
+		errno = -EINVAL;
+		return -1;
+	}
 
 	/* In LinuxPPS there are no differences between a PPS source and
 	 * a PPS handle so we return the same value. */
@@ -128,12 +155,33 @@ 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)
 {
+	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);
 }
 
@@ -147,15 +195,41 @@ 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,
 				const int edge, const int tsformat)
 {
 	/* LinuxPPS doesn't implement kernel consumer feature */
-	return -EOPNOTSUPP;
+	errno = -EOPNOTSUPP;
+	return -1;
 }
 
 #endif   /* _SYS_TIMEPPS_H_ */
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..6f56931 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,15 +308,15 @@ 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 (to.sec != -1) {
+			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(
@@ -352,7 +346,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..001b8bf 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 uint32_t assert_sequence;	/* PPS' assert event seq # */
+	volatile uint32_t 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[]);