Messages in this thread Patch in this message | | | From | "George Spelvin" <> | Date | Sun, 08 Feb 2009 01:09:38 -0500 | Subject | Re: [LinuxPPS] LinuxPPS kernel interface |
| |
Udo van den Heuvel <udovdh@xs4all.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__
| |