lkml.org 
[lkml]   [2009]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [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__


    \
     
     \ /
      Last update: 2009-02-08 07:11    [W:0.039 / U:0.128 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site