Message-ID: <20090208060938.25184.qmail@science.horizon.com>
Date:	Sun, 08 Feb 2009 01:09:38 -0500
From:	"George Spelvin" <linux@...izon.com>
To:	udovdh@...all.nl
Cc:	linux-kernel@...r.kernel.org, LinuxPPS@...enneenne.com,
	giometti@...eenne.com, linux@...izon.com
Subject: Re: [LinuxPPS] LinuxPPS kernel interface

Udo van den Heuvel <udovdh@...all.nl> wrote:
> Interesting ideas, thanks for the input.
>
> I think I must mention  that we also must strive for kernel inclusion.
> Kernel inclusion will broaden our exposure.
> This means we have to get rid of the issues blocking inclusion.
> (see a query like http://marc.info/?l=linux-kernel&w=2&r=1&s=pps&q=b for 
> related posts)
>
> What do you think?

I agree, that's the goal.  But it's also important to get the
interface right, preferably before publishing it to world+dog.

And then there are little things like the fact that an important header
file like timepps.h probably shouldn't live in Documentation/pps...

For example, here's a patch, on top of the previous series, that
gets rid of the timeout argument to PPS_FETCH:

diff --git a/Documentation/pps/timepps.h b/Documentation/pps/timepps.h
index 5006ca5..56746db 100644
--- a/Documentation/pps/timepps.h
+++ b/Documentation/pps/timepps.h
@@ -22,6 +22,7 @@
 #define _SYS_TIMEPPS_H_
 
 #include <errno.h>
+#include <poll.h>
 #include <sys/time.h>
 #include <sys/ioctl.h>
 #include <linux/types.h>
@@ -29,28 +30,44 @@
 
 #define LINUXPPS	1		/* signal we are using LinuxPPS */
 
+/* In case we don't have it */
+int ppoll(struct pollfd *fds, nfds_t nfds,
+          const struct timespec *timeout, const sigset_t *sigmask);
+
 /*
- * New data structures
+ * New data structures.
+ *
+ * The data types are mandated by RFC 2783, and are unfortunately not
+ * 64-bit clean.  (In particular, they use the "long" data type, which
+ * is 32 bits in 32-bit code and 64 bits in 64-bit code.)  Thus, they
+ * are not suitable for passing to the kernel on a machine which runs
+ * both kinds of code.  (Like any modern x86-64 system.)
+ *
+ * So this code does a lot of copying of data structures.
  */
+typedef __u32 pps_seq_t;		/* sequence number */
 
 struct ntp_fp {
 	unsigned int integral;
 	unsigned int fractional;
 };
+typedef struct ntp_fp ntp_fp_t;		/* NTP-compatible time stamp */
 
 union pps_timeu {
 	struct timespec tspec;
 	struct ntp_fp ntpfp;
 	unsigned long longpad[3];
 };
+typedef union pps_timeu pps_timeu_t;	/* generic data type for time stamps */
 
 struct pps_info {
-	unsigned long assert_sequence;	/* seq. num. of assert event */
-	unsigned long clear_sequence;	/* seq. num. of clear event */
+	pps_seq_t assert_sequence;	/* seq. num. of assert event */
+	pps_seq_t 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 */
 };
+typedef struct pps_info pps_info_t;
 
 struct pps_params {
 	int api_version;		/* API version # */
@@ -58,13 +75,9 @@ struct pps_params {
 	union pps_timeu assert_off_tu;	/* offset compensation for assert */
 	union pps_timeu clear_off_tu;	/* offset compensation for clear */
 };
+typedef struct pps_params pps_params_t;
 
 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 */
-typedef union pps_timeu pps_timeu_t;	/* generic data type for time stamps */
-typedef struct pps_info pps_info_t;
-typedef struct pps_params pps_params_t;
 
 #define assert_timestamp        assert_tu.tspec
 #define clear_timestamp         clear_tu.tspec
@@ -155,7 +168,8 @@ static __inline int time_pps_fetch(pps_handle_t handle, const int tsformat,
 					pps_info_t *ppsinfobuf,
 					const struct timespec *timeout)
 {
-	struct pps_fdata __fdata;
+	struct pps_kinfo kinfo;
+	struct pollfd pfd = { .fd = handle, .events = POLLIN };
 	int ret;
 
 	/* Sanity checks */
@@ -164,22 +178,23 @@ static __inline int time_pps_fetch(pps_handle_t handle, const int tsformat,
 		return -1;
 	}
 
-	if (timeout) {
-		__fdata.timeout.sec = timeout->tv_sec;
-		__fdata.timeout.nsec = timeout->tv_nsec;
-		__fdata.timeout.flags = ~PPS_TIME_INVALID;
-	} else
-		__fdata.timeout.flags = PPS_TIME_INVALID;
-
-	ret = ioctl(handle, PPS_FETCH, &__fdata);
-
-	ppsinfobuf->assert_sequence = __fdata.info.assert_sequence;
-	ppsinfobuf->clear_sequence = __fdata.info.clear_sequence;
-	ppsinfobuf->assert_tu.tspec.tv_sec = __fdata.info.assert_tu.sec;
-	ppsinfobuf->assert_tu.tspec.tv_nsec = __fdata.info.assert_tu.nsec;
-	ppsinfobuf->clear_tu.tspec.tv_sec = __fdata.info.clear_tu.sec;
-	ppsinfobuf->clear_tu.tspec.tv_nsec = __fdata.info.clear_tu.nsec;
-	ppsinfobuf->current_mode = __fdata.info.current_mode;
+	ret = ppoll(&pfd, 1, timeout, NULL);
+	if (ret < 0)
+		return ret;
+	if (ret == 0) {
+		errno = ETIMEDOUT;	/* Should be EAGAIN! */
+		return -1;
+	}
+		
+	ret = ioctl(handle, PPS_FETCH, &kinfo);
+
+	ppsinfobuf->assert_sequence         = kinfo.assert_sequence;
+	ppsinfobuf->clear_sequence          = kinfo.clear_sequence;
+	ppsinfobuf->assert_tu.tspec.tv_sec  = kinfo.assert_tu.sec;
+	ppsinfobuf->assert_tu.tspec.tv_nsec = kinfo.assert_tu.nsec;
+	ppsinfobuf->clear_tu.tspec.tv_sec   = kinfo.clear_tu.sec;
+	ppsinfobuf->clear_tu.tspec.tv_nsec  = kinfo.clear_tu.nsec;
+	ppsinfobuf->current_mode            = kinfo.current_mode;
 
 	return ret;
 }
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 7a32920..d3d4c49 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -151,10 +151,9 @@ static long pps_cdev_ioctl(struct file *file,
 						struct pps_device, cdev);
 	struct pps_kinfo *info = file->private_data;
 	void __user *uarg = (void __user *) arg;
-	struct pps_fdata __user *fuarg = uarg;
+	struct pps_kinfo __user *fuarg = uarg;
 	struct pps_kparams params;
-	struct pps_fdata fdata;
-	unsigned long ticks;
+	struct pps_kinfo kinfo;
 	u32 seq1, seq2;
 	int err;
 
@@ -246,59 +245,24 @@ static long pps_cdev_ioctl(struct file *file,
 
 		if (!(file->f_mode & FMODE_READ))
 			return -EBADF;	/* Not open for read */
-		err = copy_from_user(&fdata.timeout, &fuarg->timeout,
-			sizeof fdata.timeout);
-		if (err)
-			return -EFAULT;
-
-		/* Manage the timeout */
-		if (fdata.timeout.flags & PPS_TIME_INVALID)
-			err = wait_event_interruptible(pps->queue,
-						pps_is_ready(pps, info));
-		else {
-			pr_debug("timeout %lld.%09d\n",
-					(long long) fdata.timeout.sec,
-					fdata.timeout.nsec);
-			ticks = fdata.timeout.sec * HZ;
-			ticks += fdata.timeout.nsec / (NSEC_PER_SEC / HZ);
-
-			if (ticks != 0) {
-				err = wait_event_interruptible_timeout(
-						pps->queue,
-						pps_is_ready(pps, info),
-						ticks);
-				/*
-				 * RFC 2783 requires this error, although
-				 * it really should be EAGAIN.
-				 */
-				if (err == 0)
-					return -ETIMEDOUT;
-			}
-		}
-
-		/* Check for pending signals */
-		if (err == -ERESTARTSYS) {
-			pr_debug("pending signal caught\n");
-			return -EINTR;
-		}
 
-		/* Return the fetched timestamp */
-		fdata.info.assert_sequence = info->assert_sequence = seq1 =
+		/* Return the most recent timestamp */
+		kinfo.assert_sequence = info->assert_sequence = seq1 =
 			pps->assert_sequence;
-		fdata.info.clear_sequence = info->clear_sequence = seq2 =
+		kinfo.clear_sequence = info->clear_sequence = seq2 =
 			pps->clear_sequence;
 
 		read_barrier_depends();
 
-		fdata.info.assert_tu = pps->assert_tu[seq1 % 4];
+		kinfo.assert_tu = pps->assert_tu[seq1 % 4];
 		if (info->current_mode & PPS_OFFSETASSERT)
-			pps_add_offset(&fdata.info.assert_tu, &info->assert_tu);
-		fdata.info.clear_tu = pps->clear_tu[seq2 % 4];
+			pps_add_offset(&kinfo.assert_tu, &info->assert_tu);
+		kinfo.clear_tu = pps->clear_tu[seq2 % 4];
 		if (info->current_mode & PPS_OFFSETCLEAR)
-			pps_add_offset(&fdata.info.clear_tu, &info->clear_tu);
-		fdata.info.current_mode = info->current_mode;
+			pps_add_offset(&kinfo.clear_tu, &info->clear_tu);
+		kinfo.current_mode = info->current_mode;
 
-		err = copy_to_user(&fuarg->info, &fdata.info, sizeof fdata.info);
+		err = copy_to_user(fuarg, &kinfo, sizeof kinfo);
 		if (err)
 			return -EFAULT;
 
diff --git a/include/linux/pps.h b/include/linux/pps.h
index deb739c..868d66e 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -108,18 +108,13 @@ struct pps_kparams {
  * Here begins the implementation-specific part!
  */
 
-struct pps_fdata {
-	struct pps_kinfo info;
-	struct pps_ktime timeout;
-};
-
 #include <linux/ioctl.h>
 
 #define PPS_CHECK		_IO('p', 0xa0)
 #define PPS_GETPARAMS		_IOR('p', 0xa1, struct pps_kparams *)
 #define PPS_SETPARAMS		_IOW('p', 0xa2, struct pps_kparams *)
 #define PPS_GETCAP		_IOR('p', 0xa3, int *)
-#define PPS_FETCH		_IOWR('p', 0xa4, struct pps_fdata *)
+#define PPS_FETCH		_IOWR('p', 0xa4, struct pps_kinfo *)
 
 #ifdef __KERNEL__
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists