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

From: Rodolfo Giometti
Date: Mon Jul 09 2007 - 09:18:22 EST

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@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxxx
Embedded Systems giometti@xxxxxxxx
UNIX programming phone: +39 349 2432127
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[]);