This patchset contains several changes that improve an overall
design/performance of PPS subsystem. I'd like these patches to be
merged mainline if no one objects.
Patches 1-3 are bugfixes.
Patches 4-11 are other improvements to PPS subsystem.
Patches 12-14 add kernel consumer support.
Patch 15 adds parallel port PPS client.
Patch 16 adds parallel port PPS generator.
You can find description for my previous patchset (it describes patches
12-16 in more detailed) here: http://lkml.org/lkml/2010/2/24/189
This patchset is tested against the vanilla 2.6.35 kernel. But we are
actually using it on 2.6.33.7-rt29 rt-preempt kernel most of the time.
Those who are interested in other versions of the patchset can find
them in my git repository:
git://github.com/ago/linux-2.6.git
There is one problem however: kernel consumer works bad (if enabled)
when CONFIG_NO_HZ is enabled. The reason for this is commit
a092ff0f90cae22b2ac8028ecd2c6f6c1a9e4601. Without it hardpps() is able
to sync to 1us precision in about 10 seconds. With CONFIG_NO_HZ it is
not syncing at all. This only affects patches 12-14, others are ok.
2John Stultz: the problem is not with logarithmic accumulation because
it works good if NTP_INTERVAL_FREQ is set to 2.
Changelog
v2 -> v3:
* add patches 1-11
* add clear_wait parameter to pps_parport
* add delay parameter to pps_gen_parport
* fix seqlock unlocking
* fix issues pointed out by Rodolfo Giometti:
* move CONFIG_NTP_PPS to drivers/pps/Kconfig
* swap parport client and generator patches
* style and typo fixes
* move patch that adds unified timestamp gathering to be the beginning
v1 -> v2:
* fix issues pointed out by John Stultz:
* style fixes
* add a about the authorship of the original code
* replace timespec with pps_normtime struct where timespec is used
in a wrong way
* fix seqlock usage in hardpps()
* unbind kernel consumer on device removal
* send raw timestamp instead of the last difference to hardpps() which
simplifies the code and is less error-prone
* update comments in the kernel consumer code to match the reality
* split the patch that adds MONOTONIC_RAW timestmaps into two
* other small fixes
Alexander Gordeev (16):
pps: trivial fixes
pps: declare variables where they are used in switch
pps: fix race in PPS_FETCH handler
pps: unify timestamp gathering
pps: access pps device by direct pointer
pps: convert printk/pr_* to dev_*
pps: move idr stuff to pps.c
pps: add async PPS event handler
pps: don't disable interrupts when using spin locks
pps: use BUG_ON for kernel API safety checks
pps: simplify conditions a bit
ntp: add hardpps implementation
pps: capture MONOTONIC_RAW timestamps as well
pps: add kernel consumer support
pps: add parallel port PPS client
pps: add parallel port PPS signal generator
Documentation/ioctl/ioctl-number.txt | 2 +-
drivers/pps/Kconfig | 10 +
drivers/pps/Makefile | 2 +-
drivers/pps/clients/Kconfig | 7 +
drivers/pps/clients/Makefile | 1 +
drivers/pps/clients/pps-ktimer.c | 43 ++--
drivers/pps/clients/pps-ldisc.c | 49 ++--
drivers/pps/clients/pps_parport.c | 244 +++++++++++++++++
drivers/pps/generators/Kconfig | 17 ++
drivers/pps/generators/Makefile | 9 +
drivers/pps/generators/pps_gen_parport.c | 274 +++++++++++++++++++
drivers/pps/kapi.c | 330 +++++++++++-------------
drivers/pps/pps.c | 201 +++++++++++---
include/linux/pps.h | 7 +
include/linux/pps_kernel.h | 64 ++++-
include/linux/serial_core.h | 5 +-
include/linux/time.h | 2 +
include/linux/timex.h | 1 +
include/linux/tty_ldisc.h | 5 +-
kernel/time/ntp.c | 421 ++++++++++++++++++++++++++++-
kernel/time/timekeeping.c | 34 +++
21 files changed, 1419 insertions(+), 309 deletions(-)
create mode 100644 drivers/pps/clients/pps_parport.c
create mode 100644 drivers/pps/generators/Kconfig
create mode 100644 drivers/pps/generators/Makefile
create mode 100644 drivers/pps/generators/pps_gen_parport.c
Move variable declarations where they are used in pps_cdev_ioctl.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/pps.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index ca5183b..c76afb9 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -61,8 +61,6 @@ static long pps_cdev_ioctl(struct file *file,
{
struct pps_device *pps = file->private_data;
struct pps_kparams params;
- struct pps_fdata fdata;
- unsigned long ticks;
void __user *uarg = (void __user *) arg;
int __user *iuarg = (int __user *) arg;
int err;
@@ -136,7 +134,9 @@ static long pps_cdev_ioctl(struct file *file,
break;
- case PPS_FETCH:
+ case PPS_FETCH: {
+ struct pps_fdata fdata;
+
pr_debug("PPS_FETCH: source %d\n", pps->id);
err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
@@ -149,6 +149,8 @@ static long pps_cdev_ioctl(struct file *file,
if (fdata.timeout.flags & PPS_TIME_INVALID)
err = wait_event_interruptible(pps->queue, pps->go);
else {
+ unsigned long ticks;
+
pr_debug("timeout %lld.%09d\n",
(long long) fdata.timeout.sec,
fdata.timeout.nsec);
@@ -185,7 +187,7 @@ static long pps_cdev_ioctl(struct file *file,
return -EFAULT;
break;
-
+ }
default:
return -ENOTTY;
break;
--
1.7.1
This commit adds hardpps() implementation based upon the original one
from the NTPv4 reference kernel code from David Mills. However, it is
highly optimized towards very fast syncronization and maximum stickness
to PPS signal. The typical error is less then a microsecond.
To make it sync faster I had to throw away exponential phase filter so
that the full phase offset is corrected immediately. Then I also had to
throw away median phase filter because it gives a bigger error itself
if used without exponential filter.
Maybe we will find an appropriate filtering scheme in the future but
it's not necessary if the signal quality is ok.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/Kconfig | 7 +
include/linux/timex.h | 1 +
kernel/time/ntp.c | 421 +++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 414 insertions(+), 15 deletions(-)
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 1afe4e0..33c9126 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -30,6 +30,13 @@ config PPS_DEBUG
messages to the system log. Select this if you are having a
problem with PPS support and want to see more of what is going on.
+config NTP_PPS
+ bool "PPS kernel consumer support"
+ depends on PPS
+ help
+ This option adds support for direct in-kernel time
+ syncronization using an external PPS signal.
+
source drivers/pps/clients/Kconfig
endmenu
diff --git a/include/linux/timex.h b/include/linux/timex.h
index 32d852f..d23999f 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -268,6 +268,7 @@ extern u64 tick_length;
extern void second_overflow(void);
extern void update_ntp_one_tick(void);
extern int do_adjtimex(struct timex *);
+extern void hardpps(const struct timespec *, const struct timespec *);
int read_current_timer(unsigned long *timer_val);
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index c631168..1774bad 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -14,6 +14,7 @@
#include <linux/timex.h>
#include <linux/time.h>
#include <linux/mm.h>
+#include <linux/module.h>
/*
* NTP timekeeping variables:
@@ -74,6 +75,162 @@ static long time_adjust;
/* constant (boot-param configurable) NTP tick adjustment (upscaled) */
static s64 ntp_tick_adj;
+#ifdef CONFIG_NTP_PPS
+
+/*
+ * The following variables are used when a pulse-per-second (PPS) signal
+ * is available. They establish the engineering parameters of the clock
+ * discipline loop when controlled by the PPS signal.
+ */
+#define PPS_VALID 10 /* PPS signal watchdog max (s) */
+#define PPS_POPCORN 4 /* popcorn spike threshold (shift) */
+#define PPS_INTMIN 2 /* min freq interval (s) (shift) */
+#define PPS_INTMAX 8 /* max freq interval (s) (shift) */
+#define PPS_INTCOUNT 4 /* number of consecutive good intervals to
+ increase pps_shift or consecutive bad
+ intervals to decrease it */
+#define PPS_MAXWANDER 100000 /* max PPS freq wander (ns/s) */
+
+static int pps_valid; /* signal watchdog counter */
+static long pps_tf[3]; /* phase median filter */
+static long pps_jitter; /* current jitter (ns) */
+static struct timespec pps_fbase; /* beginning of the last freq interval */
+static int pps_shift; /* current interval duration (s) (shift) */
+static int pps_intcnt; /* interval counter */
+static s64 pps_freq; /* frequency offset (scaled ns/s) */
+static long pps_stabil; /* current stability (scaled ns/s) */
+
+/*
+ * PPS signal quality monitors
+ */
+static long pps_calcnt; /* calibration intervals */
+static long pps_jitcnt; /* jitter limit exceeded */
+static long pps_stbcnt; /* stability limit exceeded */
+static long pps_errcnt; /* calibration errors */
+
+
+/* PPS kernel consumer compensates the whole phase error immediately.
+ * Otherwise, reduce the offset by a fixed factor times the time constant.
+ */
+static inline s64 ntp_offset_chunk(s64 offset)
+{
+ if (time_status & STA_PPSTIME && time_status & STA_PPSSIGNAL)
+ return offset;
+ else
+ return shift_right(offset, SHIFT_PLL + time_constant);
+}
+
+static inline void pps_reset_freq_interval(void)
+{
+ /* the PPS calibration interval may end
+ surprisingly early */
+ pps_shift = PPS_INTMIN;
+ pps_intcnt = 0;
+}
+
+/**
+ * pps_clear - Clears the PPS state variables
+ *
+ * Must be called while holding a write on the xtime_lock
+ */
+static inline void pps_clear(void)
+{
+ pps_reset_freq_interval();
+ pps_tf[0] = 0;
+ pps_tf[1] = 0;
+ pps_tf[2] = 0;
+ pps_fbase.tv_sec = pps_fbase.tv_nsec = 0;
+ pps_freq = 0;
+}
+
+/* Decrease pps_valid to indicate that another second has passed since
+ * the last PPS signal. When it reaches 0, indicate that PPS signal is
+ * missing.
+ *
+ * Must be called while holding a write on the xtime_lock
+ */
+static inline void pps_dec_valid(void)
+{
+ if (pps_valid > 0)
+ pps_valid--;
+ else {
+ time_status &= ~(STA_PPSSIGNAL | STA_PPSJITTER |
+ STA_PPSWANDER | STA_PPSERROR);
+ pps_clear();
+ }
+}
+
+static inline void pps_set_freq(s64 freq)
+{
+ pps_freq = freq;
+}
+
+static inline int is_error_status(int status)
+{
+ return (time_status & (STA_UNSYNC|STA_CLOCKERR))
+ /* PPS signal lost when either PPS time or
+ * PPS frequency synchronization requested
+ */
+ || ((time_status & (STA_PPSFREQ|STA_PPSTIME))
+ && !(time_status & STA_PPSSIGNAL))
+ /* PPS jitter exceeded when
+ * PPS time synchronization requested */
+ || ((time_status & (STA_PPSTIME|STA_PPSJITTER))
+ == (STA_PPSTIME|STA_PPSJITTER))
+ /* PPS wander exceeded or calibration error when
+ * PPS frequency synchronization requested
+ */
+ || ((time_status & STA_PPSFREQ)
+ && (time_status & (STA_PPSWANDER|STA_PPSERROR)));
+}
+
+static inline void pps_fill_timex(struct timex *txc)
+{
+ txc->ppsfreq = shift_right((pps_freq >> PPM_SCALE_INV_SHIFT) *
+ PPM_SCALE_INV, NTP_SCALE_SHIFT);
+ txc->jitter = pps_jitter;
+ if (!(time_status & STA_NANO))
+ txc->jitter /= NSEC_PER_USEC;
+ txc->shift = pps_shift;
+ txc->stabil = pps_stabil;
+ txc->jitcnt = pps_jitcnt;
+ txc->calcnt = pps_calcnt;
+ txc->errcnt = pps_errcnt;
+ txc->stbcnt = pps_stbcnt;
+}
+
+#else /* !CONFIG_NTP_PPS */
+
+static inline s64 ntp_offset_chunk(s64 offset)
+{
+ return shift_right(offset, SHIFT_PLL + time_constant);
+}
+
+static inline void pps_reset_freq_interval(void) {}
+static inline void pps_clear(void) {}
+static inline void pps_dec_valid(void) {}
+static inline void pps_set_freq(s64 freq) {}
+
+static inline int is_error_status(int status)
+{
+ return status & (STA_UNSYNC|STA_CLOCKERR);
+}
+
+static inline void pps_fill_timex(struct timex *txc)
+{
+ /* PPS is not implemented, so these are zero */
+ txc->ppsfreq = 0;
+ txc->jitter = 0;
+ txc->shift = 0;
+ txc->stabil = 0;
+ txc->jitcnt = 0;
+ txc->calcnt = 0;
+ txc->errcnt = 0;
+ txc->stbcnt = 0;
+}
+
+#endif /* CONFIG_NTP_PPS */
+
/*
* NTP methods:
*/
@@ -177,6 +334,9 @@ void ntp_clear(void)
tick_length = tick_length_base;
time_offset = 0;
+
+ /* Clear PPS state variables */
+ pps_clear();
}
/*
@@ -242,16 +402,16 @@ void second_overflow(void)
time_status |= STA_UNSYNC;
}
- /*
- * Compute the phase adjustment for the next second. The offset is
- * reduced by a fixed factor times the time constant.
- */
+ /* Compute the phase adjustment for the next second */
tick_length = tick_length_base;
- delta = shift_right(time_offset, SHIFT_PLL + time_constant);
+ delta = ntp_offset_chunk(time_offset);
time_offset -= delta;
tick_length += delta;
+ /* Check PPS signal */
+ pps_dec_valid();
+
if (!time_adjust)
return;
@@ -361,6 +521,8 @@ static inline void process_adj_status(struct timex *txc, struct timespec *ts)
if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
time_state = TIME_OK;
time_status = STA_UNSYNC;
+ /* restart PPS frequency calibration */
+ pps_reset_freq_interval();
}
/*
@@ -410,6 +572,8 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts
time_freq = txc->freq * PPM_SCALE;
time_freq = min(time_freq, MAXFREQ_SCALED);
time_freq = max(time_freq, -MAXFREQ_SCALED);
+ /* update pps_freq */
+ pps_set_freq(time_freq);
}
if (txc->modes & ADJ_MAXERROR)
@@ -500,7 +664,8 @@ int do_adjtimex(struct timex *txc)
}
result = time_state; /* mostly `TIME_OK' */
- if (time_status & (STA_UNSYNC|STA_CLOCKERR))
+ /* check for errors */
+ if (is_error_status(time_status))
result = TIME_ERROR;
txc->freq = shift_right((time_freq >> PPM_SCALE_INV_SHIFT) *
@@ -514,15 +679,8 @@ int do_adjtimex(struct timex *txc)
txc->tick = tick_usec;
txc->tai = time_tai;
- /* PPS is not implemented, so these are zero */
- txc->ppsfreq = 0;
- txc->jitter = 0;
- txc->shift = 0;
- txc->stabil = 0;
- txc->jitcnt = 0;
- txc->calcnt = 0;
- txc->errcnt = 0;
- txc->stbcnt = 0;
+ /* fill PPS status fields */
+ pps_fill_timex(txc);
write_sequnlock_irq(&xtime_lock);
@@ -536,6 +694,239 @@ int do_adjtimex(struct timex *txc)
return result;
}
+#ifdef CONFIG_NTP_PPS
+
+struct pps_normtime {
+ __kernel_time_t sec; /* seconds */
+ long nsec; /* nanoseconds */
+};
+
+/* normalize the timestamp so that nsec is in the
+ ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
+static inline struct pps_normtime pps_normalize_ts(struct timespec ts)
+{
+ struct pps_normtime norm = {
+ .sec = ts.tv_sec,
+ .nsec = ts.tv_nsec
+ };
+
+ if (norm.nsec > (NSEC_PER_SEC >> 1)) {
+ norm.nsec -= NSEC_PER_SEC;
+ norm.sec++;
+ }
+
+ return norm;
+}
+
+/* get current phase correction and jitter */
+static inline long pps_phase_filter_get(long *jitter)
+{
+ *jitter = pps_tf[0] - pps_tf[1];
+ if (*jitter < 0)
+ *jitter = -*jitter;
+
+ /* TODO: test various filters */
+ return pps_tf[0];
+}
+
+/* add the sample to the phase filter */
+static inline void pps_phase_filter_add(long err)
+{
+ pps_tf[2] = pps_tf[1];
+ pps_tf[1] = pps_tf[0];
+ pps_tf[0] = err;
+}
+
+/* decrease frequency calibration interval length.
+ * It is halved after four consecutive unstable intervals.
+ */
+static inline void pps_dec_freq_interval(void)
+{
+ if (--pps_intcnt <= -PPS_INTCOUNT) {
+ pps_intcnt = -PPS_INTCOUNT;
+ if (pps_shift > PPS_INTMIN) {
+ pps_shift--;
+ pps_intcnt = 0;
+ }
+ }
+}
+
+/* increase frequency calibration interval length.
+ * It is doubled after four consecutive stable intervals.
+ */
+static inline void pps_inc_freq_interval(void)
+{
+ if (++pps_intcnt >= PPS_INTCOUNT) {
+ pps_intcnt = PPS_INTCOUNT;
+ if (pps_shift < PPS_INTMAX) {
+ pps_shift++;
+ pps_intcnt = 0;
+ }
+ }
+}
+
+/* update clock frequency based on MONOTONIC_RAW clock PPS signal
+ * timestamps
+ *
+ * At the end of the calibration interval the difference between the
+ * first and last MONOTONIC_RAW clock timestamps divided by the length
+ * of the interval becomes the frequency update. If the interval was
+ * too long, the data are discarded.
+ * Returns the difference between old and new frequency values.
+ */
+static long hardpps_update_freq(struct pps_normtime freq_norm)
+{
+ long delta, delta_mod;
+ s64 ftemp;
+
+ /* check if the frequency interval was too long */
+ if (freq_norm.sec > (2 << pps_shift)) {
+ time_status |= STA_PPSERROR;
+ pps_errcnt++;
+ pps_dec_freq_interval();
+ pr_err("hardpps: PPSERROR: interval too long - %ld s\n",
+ freq_norm.sec);
+ return 0;
+ }
+
+ /* here the raw frequency offset and wander (stability) is
+ * calculated. If the wander is less than the wander threshold
+ * the interval is increased; otherwise it is decreased.
+ */
+ ftemp = div_s64(((s64)(-freq_norm.nsec)) << NTP_SCALE_SHIFT,
+ freq_norm.sec);
+ delta = shift_right(ftemp - pps_freq, NTP_SCALE_SHIFT);
+ pps_freq = ftemp;
+ if (delta > PPS_MAXWANDER || delta < -PPS_MAXWANDER) {
+ pr_warning("hardpps: PPSWANDER: change=%ld\n", delta);
+ time_status |= STA_PPSWANDER;
+ pps_stbcnt++;
+ pps_dec_freq_interval();
+ } else { /* good sample */
+ pps_inc_freq_interval();
+ }
+
+ /* the stability metric is calculated as the average of recent
+ * frequency changes, but is used only for performance
+ * monitoring
+ */
+ delta_mod = delta;
+ if (delta_mod < 0)
+ delta_mod = -delta_mod;
+ pps_stabil += (div_s64(((s64)delta_mod) <<
+ (NTP_SCALE_SHIFT - SHIFT_USEC),
+ NSEC_PER_USEC) - pps_stabil) >> PPS_INTMIN;
+
+ /* if enabled, the system clock frequency is updated */
+ if ((time_status & STA_PPSFREQ) != 0 &&
+ (time_status & STA_FREQHOLD) == 0) {
+ time_freq = pps_freq;
+ ntp_update_frequency();
+ }
+
+ return delta;
+}
+
+/* correct REALTIME clock phase error against PPS signal */
+static void hardpps_update_phase(long error)
+{
+ long correction = -error;
+ long jitter;
+
+ /* add the sample to the median filter */
+ pps_phase_filter_add(correction);
+ correction = pps_phase_filter_get(&jitter);
+
+ /* Nominal jitter is due to PPS signal noise. If it exceeds the
+ * threshold, the sample is discarded; otherwise, if so enabled,
+ * the time offset is updated.
+ */
+ if (jitter > (pps_jitter << PPS_POPCORN)) {
+ pr_warning("hardpps: PPSJITTER: jitter=%ld, limit=%ld\n",
+ jitter, (pps_jitter << PPS_POPCORN));
+ time_status |= STA_PPSJITTER;
+ pps_jitcnt++;
+ } else if (time_status & STA_PPSTIME) {
+ /* correct the time using the phase offset */
+ time_offset = div_s64(((s64)correction) << NTP_SCALE_SHIFT,
+ NTP_INTERVAL_FREQ);
+ /* cancel running adjtime() */
+ time_adjust = 0;
+ }
+ /* update jitter */
+ pps_jitter += (jitter - pps_jitter) >> PPS_INTMIN;
+}
+
+/*
+ * hardpps() - discipline CPU clock oscillator to external PPS signal
+ *
+ * This routine is called at each PPS signal arrival in order to
+ * discipline the CPU clock oscillator to the PPS signal. It takes two
+ * parameters: REALTIME and MONOTONIC_RAW clock timestamps. The former
+ * is used to correct clock phase error and the latter is used to
+ * correct the frequency.
+ *
+ * This code is based on David Mills's reference nanokernel
+ * implementation. It was mostly rewritten but keeps the same idea.
+ */
+void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
+{
+ struct pps_normtime pts_norm, freq_norm;
+ unsigned long flags;
+
+ pts_norm = pps_normalize_ts(*phase_ts);
+
+ write_seqlock_irqsave(&xtime_lock, flags);
+
+ /* clear the error bits, they will be set again if needed */
+ time_status &= ~(STA_PPSJITTER | STA_PPSWANDER | STA_PPSERROR);
+
+ /* indicate signal presence */
+ time_status |= STA_PPSSIGNAL;
+ pps_valid = PPS_VALID;
+
+ /* when called for the first time,
+ * just start the frequency interval */
+ if (unlikely(pps_fbase.tv_sec == 0)) {
+ pps_fbase = *raw_ts;
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+ return;
+ }
+
+ /* ok, now we have a base for frequency calculation */
+ freq_norm = pps_normalize_ts(timespec_sub(*raw_ts, pps_fbase));
+
+ /* check that the signal is in the range
+ * [1s - MAXFREQ us, 1s + MAXFREQ us], otherwise reject it */
+ if ((freq_norm.sec == 0) ||
+ (freq_norm.nsec > MAXFREQ * freq_norm.sec) ||
+ (freq_norm.nsec < -MAXFREQ * freq_norm.sec)) {
+ time_status |= STA_PPSJITTER;
+ /* restart the frequency calibration interval */
+ pps_fbase = *raw_ts;
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+ pr_err("hardpps: PPSJITTER: bad pulse\n");
+ return;
+ }
+
+ /* signal is ok */
+
+ /* check if the current frequency interval is finished */
+ if (freq_norm.sec >= (1 << pps_shift)) {
+ pps_calcnt++;
+ /* restart the frequency calibration interval */
+ pps_fbase = *raw_ts;
+ hardpps_update_freq(freq_norm);
+ }
+
+ hardpps_update_phase(pts_norm.nsec);
+
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+}
+EXPORT_SYMBOL(hardpps);
+
+#endif /* CONFIG_NTP_PPS */
+
static int __init ntp_tick_adj_setup(char *str)
{
ntp_tick_adj = simple_strtol(str, NULL, 0);
--
1.7.1
Using device index as a pointer needs some unnecessary work to be done
every time the pointer is needed (in irq handler for example).
Using a direct pointer is much more easy (and safe as well).
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 30 ++++-----
drivers/pps/clients/pps-ldisc.c | 33 +++++-----
drivers/pps/kapi.c | 124 ++++++++-----------------------------
drivers/pps/pps.c | 22 ++-----
include/linux/pps_kernel.h | 21 +++----
5 files changed, 70 insertions(+), 160 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index e1bdd8b..64cba1d 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -31,7 +31,7 @@
* Global variables
*/
-static int source;
+static struct pps_device *pps;;
static struct timer_list ktimer;
/*
@@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
pr_info("PPS event at %lu\n", jiffies);
- pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
+ pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
}
@@ -56,12 +56,11 @@ static void pps_ktimer_event(unsigned long ptr)
* The echo function
*/
-static void pps_ktimer_echo(int source, int event, void *data)
+static void pps_ktimer_echo(struct pps_device *pps, int event, void *data)
{
- pr_info("echo %s %s for source %d\n",
+ dev_info(pps->dev, "echo %s %s\n",
event & PPS_CAPTUREASSERT ? "assert" : "",
- event & PPS_CAPTURECLEAR ? "clear" : "",
- source);
+ event & PPS_CAPTURECLEAR ? "clear" : "");
}
/*
@@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = {
static void __exit pps_ktimer_exit(void)
{
- del_timer_sync(&ktimer);
- pps_unregister_source(source);
+ dev_info(pps->dev, "ktimer PPS source unregistered\n");
- pr_info("ktimer PPS source unregistered\n");
+ del_timer_sync(&ktimer);
+ pps_unregister_source(pps);
}
static int __init pps_ktimer_init(void)
{
- int ret;
-
- ret = pps_register_source(&pps_ktimer_info,
+ pps = pps_register_source(&pps_ktimer_info,
PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
- if (ret < 0) {
+ if (pps == NULL) {
printk(KERN_ERR "cannot register ktimer source\n");
- return ret;
+ return -ENOMEM;
}
- source = ret;
setup_timer(&ktimer, pps_ktimer_event, 0);
mod_timer(&ktimer, jiffies + HZ);
- pr_info("ktimer PPS source registered at %d\n", source);
+ dev_info(pps->dev, "ktimer PPS source registered\n");
- return 0;
+ return 0;
}
module_init(pps_ktimer_init);
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 20fc9f7..d7e1a27 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -29,7 +29,7 @@
static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
struct pps_event_time *ts)
{
- int id = (long)tty->disc_data;
+ struct pps_device *pps = (struct pps_device *)tty->disc_data;
struct pps_event_time __ts;
/* First of all we get the time stamp... */
@@ -40,11 +40,11 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
ts = &__ts;
/* Now do the PPS event report */
- pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
+ pps_event(pps, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
NULL);
- pr_debug("PPS %s at %lu on source #%d\n",
- status ? "assert" : "clear", jiffies, id);
+ dev_dbg(pps->dev, "PPS %s at %lu\n", status ? "assert" : "clear",
+ jiffies);
}
static int (*alias_n_tty_open)(struct tty_struct *tty);
@@ -54,7 +54,7 @@ static int pps_tty_open(struct tty_struct *tty)
struct pps_source_info info;
struct tty_driver *drv = tty->driver;
int index = tty->index + drv->name_base;
- int ret;
+ struct pps_device *pps;
info.owner = THIS_MODULE;
info.dev = NULL;
@@ -64,20 +64,19 @@ static int pps_tty_open(struct tty_struct *tty)
PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
PPS_CANWAIT | PPS_TSFMT_TSPEC;
- ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
+ pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
- if (ret < 0) {
+ if (pps == NULL) {
pr_err("cannot register PPS source \"%s\"\n", info.path);
- return ret;
+ return -ENOMEM;
}
- tty->disc_data = (void *)(long)ret;
+ tty->disc_data = pps;
/* Should open N_TTY ldisc too */
- ret = alias_n_tty_open(tty);
- if (ret < 0)
- pps_unregister_source((long)tty->disc_data);
+ if (alias_n_tty_open(tty) < 0)
+ pps_unregister_source(pps);
- pr_info("PPS source #%d \"%s\" added\n", ret, info.path);
+ dev_info(pps->dev, "source \"%s\" added\n", info.path);
return 0;
}
@@ -86,12 +85,12 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
static void pps_tty_close(struct tty_struct *tty)
{
- int id = (long)tty->disc_data;
+ struct pps_device *pps = (struct pps_device *)tty->disc_data;
- pps_unregister_source(id);
- alias_n_tty_close(tty);
+ dev_info(pps->dev, "removed\n");
- pr_info("PPS source #%d removed\n", id);
+ pps_unregister_source(pps);
+ alias_n_tty_close(tty);
}
static struct tty_ldisc_ops pps_ldisc_ops;
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index b431d83..568223b 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -32,11 +32,11 @@
#include <linux/slab.h>
/*
- * Global variables
+ * Local variables
*/
-DEFINE_SPINLOCK(pps_idr_lock);
-DEFINE_IDR(pps_idr);
+static DEFINE_SPINLOCK(pps_idr_lock);
+static DEFINE_IDR(pps_idr);
/*
* Local functions
@@ -60,60 +60,6 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
* Exported functions
*/
-/* pps_get_source - find a PPS source
- * @source: the PPS source ID.
- *
- * This function is used to find an already registered PPS source into the
- * system.
- *
- * The function returns NULL if found nothing, otherwise it returns a pointer
- * to the PPS source data struct (the refcounter is incremented by 1).
- */
-
-struct pps_device *pps_get_source(int source)
-{
- struct pps_device *pps;
- unsigned long flags;
-
- spin_lock_irqsave(&pps_idr_lock, flags);
-
- pps = idr_find(&pps_idr, source);
- if (pps != NULL)
- atomic_inc(&pps->usage);
-
- spin_unlock_irqrestore(&pps_idr_lock, flags);
-
- return pps;
-}
-
-/* pps_put_source - free the PPS source data
- * @pps: a pointer to the PPS source.
- *
- * This function is used to free a PPS data struct if its refcount is 0.
- */
-
-void pps_put_source(struct pps_device *pps)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&pps_idr_lock, flags);
- BUG_ON(atomic_read(&pps->usage) == 0);
-
- if (!atomic_dec_and_test(&pps->usage)) {
- pps = NULL;
- goto exit;
- }
-
- /* No more reference to the PPS source. We can safely remove the
- * PPS data struct.
- */
- idr_remove(&pps_idr, pps->id);
-
-exit:
- spin_unlock_irqrestore(&pps_idr_lock, flags);
- kfree(pps);
-}
-
/* pps_register_source - add a PPS source in the system
* @info: the PPS info struct
* @default_params: the default PPS parameters of the new source
@@ -122,10 +68,11 @@ exit:
* source is described by info's fields and it will have, as default PPS
* parameters, the ones specified into default_params.
*
- * The function returns, in case of success, the PPS source ID.
+ * The function returns, in case of success, the PPS device. Otherwise NULL.
*/
-int pps_register_source(struct pps_source_info *info, int default_params)
+struct pps_device *pps_register_source(struct pps_source_info *info,
+ int default_params)
{
struct pps_device *pps;
int id;
@@ -168,7 +115,6 @@ int pps_register_source(struct pps_source_info *info, int default_params)
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
- atomic_set(&pps->usage, 1);
/* Get new ID for the new PPS source */
if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
@@ -211,7 +157,7 @@ int pps_register_source(struct pps_source_info *info, int default_params)
pr_info("new PPS source %s at ID %d\n", info->name, id);
- return id;
+ return pps;
free_idr:
spin_lock_irq(&pps_idr_lock);
@@ -224,38 +170,31 @@ kfree_pps:
pps_register_source_exit:
printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
- return err;
+ return NULL;
}
EXPORT_SYMBOL(pps_register_source);
/* pps_unregister_source - remove a PPS source from the system
- * @source: the PPS source ID
+ * @pps: the PPS source
*
* This function is used to remove a previously registered PPS source from
* the system.
*/
-void pps_unregister_source(int source)
+void pps_unregister_source(struct pps_device *pps)
{
- struct pps_device *pps;
+ unsigned int id = pps->id;
+ pps_unregister_cdev(pps);
+
spin_lock_irq(&pps_idr_lock);
- pps = idr_find(&pps_idr, source);
-
- if (!pps) {
- BUG();
- spin_unlock_irq(&pps_idr_lock);
- return;
- }
+ idr_remove(&pps_idr, pps->id);
spin_unlock_irq(&pps_idr_lock);
-
- pps_unregister_cdev(pps);
- pps_put_source(pps);
}
EXPORT_SYMBOL(pps_unregister_source);
/* pps_event - register a PPS event into the system
- * @source: the PPS source ID
+ * @pps: the PPS device
* @ts: the event timestamp
* @event: the event type
* @data: userdef pointer
@@ -263,30 +202,24 @@ EXPORT_SYMBOL(pps_unregister_source);
* This function is used by each PPS client in order to register a new
* PPS event into the system (it's usually called inside an IRQ handler).
*
- * If an echo function is associated with the PPS source it will be called
+ * If an echo function is associated with the PPS device it will be called
* as:
- * pps->info.echo(source, event, data);
+ * pps->info.echo(pps, event, data);
*/
-
-void pps_event(int source, struct pps_event_time *ts, int event, void *data)
+void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
+ void *data)
{
- struct pps_device *pps;
unsigned long flags;
int captured = 0;
struct pps_ktime ts_real;
if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
- printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
- event, source);
+ dev_err(pps->dev, "unknown event (%x)\n", event);
return;
}
- pps = pps_get_source(source);
- if (!pps)
- return;
-
- pr_debug("PPS event on source %d at %ld.%09ld\n",
- pps->id, ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
+ dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
+ ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
timespec_to_pps_ktime(&ts_real, ts->ts_real);
@@ -294,7 +227,7 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Must call the echo function? */
if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
- pps->info.echo(source, event, data);
+ pps->info.echo(pps, event, data);
/* Check the event */
pps->current_mode = pps->params.mode;
@@ -308,8 +241,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Save the time stamp */
pps->assert_tu = ts_real;
pps->assert_sequence++;
- pr_debug("capture assert seq #%u for source %d\n",
- pps->assert_sequence, source);
+ dev_dbg(pps->dev, "capture assert seq #%u\n",
+ pps->assert_sequence);
captured = ~0;
}
@@ -323,8 +256,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Save the time stamp */
pps->clear_tu = ts_real;
pps->clear_sequence++;
- pr_debug("capture clear seq #%u for source %d\n",
- pps->clear_sequence, source);
+ dev_dbg(pps->dev, "capture clear seq #%u\n",
+ pps->clear_sequence);
captured = ~0;
}
@@ -338,8 +271,5 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
}
spin_unlock_irqrestore(&pps->lock, flags);
-
- /* Now we can release the PPS source for (possible) deregistration */
- pps_put_source(pps);
}
EXPORT_SYMBOL(pps_event);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index cb24147..89f478b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -204,12 +204,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
struct pps_device, cdev);
- int found;
-
- found = pps_get_source(pps->id) != 0;
- if (!found)
- return -ENODEV;
-
file->private_data = pps;
return 0;
@@ -217,11 +211,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
static int pps_cdev_release(struct inode *inode, struct file *file)
{
- struct pps_device *pps = file->private_data;
-
- /* Free the PPS source and wake up (possible) deregistration */
- pps_put_source(pps);
-
return 0;
}
@@ -242,22 +231,23 @@ static const struct file_operations pps_cdev_fops = {
int pps_register_cdev(struct pps_device *pps)
{
int err;
+ dev_t devt;
+
+ devt = MKDEV(MAJOR(pps_devt), pps->id);
- pps->devno = MKDEV(MAJOR(pps_devt), pps->id);
cdev_init(&pps->cdev, &pps_cdev_fops);
pps->cdev.owner = pps->info.owner;
- err = cdev_add(&pps->cdev, pps->devno, 1);
+ err = cdev_add(&pps->cdev, devt, 1);
if (err) {
printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
return err;
}
- pps->dev = device_create(pps_class, pps->info.dev, pps->devno, NULL,
+ pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
"pps%d", pps->id);
if (IS_ERR(pps->dev))
goto del_cdev;
- dev_set_drvdata(pps->dev, pps);
pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
MAJOR(pps_devt), pps->id);
@@ -272,7 +262,7 @@ del_cdev:
void pps_unregister_cdev(struct pps_device *pps)
{
- device_destroy(pps_class, pps->devno);
+ device_destroy(pps_class, pps->dev->devt);
cdev_del(&pps->cdev);
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 32aa676..1e0f249 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -31,13 +31,16 @@
* Global defines
*/
+struct pps_device;
+
/* The specific PPS source info */
struct pps_source_info {
char name[PPS_MAX_NAME_LEN]; /* simbolic name */
char path[PPS_MAX_NAME_LEN]; /* path of connected device */
int mode; /* PPS's allowed mode */
- void (*echo)(int source, int event, void *data); /* PPS echo function */
+ void (*echo)(struct pps_device *pps,
+ int event, void *data); /* PPS echo function */
struct module *owner;
struct device *dev;
@@ -65,34 +68,26 @@ struct pps_device {
unsigned int id; /* PPS source unique ID */
struct cdev cdev;
struct device *dev;
- int devno;
struct fasync_struct *async_queue; /* fasync method */
spinlock_t lock;
-
- atomic_t usage; /* usage count */
};
/*
* Global variables
*/
-extern spinlock_t pps_idr_lock;
-extern struct idr pps_idr;
-
extern struct device_attribute pps_attrs[];
/*
* Exported functions
*/
-struct pps_device *pps_get_source(int source);
-extern void pps_put_source(struct pps_device *pps);
-extern int pps_register_source(struct pps_source_info *info,
- int default_params);
-extern void pps_unregister_source(int source);
+extern struct pps_device *pps_register_source(
+ struct pps_source_info *info, int default_params);
+extern void pps_unregister_source(struct pps_device *pps);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
-extern void pps_event(int source, struct pps_event_time *ts, int event,
+extern void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
void *data);
static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
--
1.7.1
Since we now have direct pointers to struct pps_device everywhere it's
easy to use dev_* functions to print messages instead of plain printks.
Where dev_* cannot be used printks are converted to pr_*.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 4 ++--
drivers/pps/kapi.c | 14 +++++++-------
drivers/pps/pps.c | 24 ++++++++++++------------
3 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index 64cba1d..9d27239 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -45,7 +45,7 @@ static void pps_ktimer_event(unsigned long ptr)
/* First of all we get the time stamp... */
pps_get_ts(&ts);
- pr_info("PPS event at %lu\n", jiffies);
+ dev_info(pps->dev, "PPS event at %lu\n", jiffies);
pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
@@ -94,7 +94,7 @@ static int __init pps_ktimer_init(void)
pps = pps_register_source(&pps_ktimer_info,
PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
if (pps == NULL) {
- printk(KERN_ERR "cannot register ktimer source\n");
+ pr_err("cannot register ktimer source\n");
return -ENOMEM;
}
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 568223b..eddf41e 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -80,20 +80,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
/* Sanity checks */
if ((info->mode & default_params) != default_params) {
- printk(KERN_ERR "pps: %s: unsupported default parameters\n",
+ pr_err("pps: %s: unsupported default parameters\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
}
if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
info->echo == NULL) {
- printk(KERN_ERR "pps: %s: echo function is not defined\n",
+ pr_err("pps: %s: echo function is not defined\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
}
if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
- printk(KERN_ERR "pps: %s: unspecified time format\n",
+ pr_err("pps: %s: unspecified time format\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
@@ -138,7 +138,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
if (id >= PPS_MAX_SOURCES) {
spin_unlock_irq(&pps_idr_lock);
- printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
+ pr_err("pps: %s: too many PPS sources in the system\n",
info->name);
err = -EBUSY;
goto free_idr;
@@ -150,12 +150,12 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
- printk(KERN_ERR "pps: %s: unable to create char device\n",
+ pr_err("pps: %s: unable to create char device\n",
info->name);
goto free_idr;
}
- pr_info("new PPS source %s at ID %d\n", info->name, id);
+ dev_info(pps->dev, "new PPS source %s\n", info->name);
return pps;
@@ -168,7 +168,7 @@ kfree_pps:
kfree(pps);
pps_register_source_exit:
- printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
+ pr_err("pps: %s: unable to register source\n", info->name);
return NULL;
}
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 89f478b..a5ca1c0 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -67,7 +67,7 @@ static long pps_cdev_ioctl(struct file *file,
switch (cmd) {
case PPS_GETPARAMS:
- pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_GETPARAMS\n");
spin_lock_irq(&pps->lock);
@@ -83,7 +83,7 @@ static long pps_cdev_ioctl(struct file *file,
break;
case PPS_SETPARAMS:
- pr_debug("PPS_SETPARAMS: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_SETPARAMS\n");
/* Check the capabilities */
if (!capable(CAP_SYS_TIME))
@@ -93,14 +93,14 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return -EFAULT;
if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
- pr_debug("capture mode unspecified (%x)\n",
+ dev_dbg(pps->dev, "capture mode unspecified (%x)\n",
params.mode);
return -EINVAL;
}
/* Check for supported capabilities */
if ((params.mode & ~pps->info.mode) != 0) {
- pr_debug("unsupported capabilities (%x)\n",
+ dev_dbg(pps->dev, "unsupported capabilities (%x)\n",
params.mode);
return -EINVAL;
}
@@ -113,7 +113,7 @@ static long pps_cdev_ioctl(struct file *file,
/* Restore the read only parameters */
if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
/* section 3.3 of RFC 2783 interpreted */
- pr_debug("time format unspecified (%x)\n",
+ dev_dbg(pps->dev, "time format unspecified (%x)\n",
params.mode);
pps->params.mode |= PPS_TSFMT_TSPEC;
}
@@ -126,7 +126,7 @@ static long pps_cdev_ioctl(struct file *file,
break;
case PPS_GETCAP:
- pr_debug("PPS_GETCAP: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_GETCAP\n");
err = put_user(pps->info.mode, iuarg);
if (err)
@@ -138,7 +138,7 @@ static long pps_cdev_ioctl(struct file *file,
struct pps_fdata fdata;
unsigned int ev;
- pr_debug("PPS_FETCH: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_FETCH\n");
err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
if (err)
@@ -153,7 +153,7 @@ static long pps_cdev_ioctl(struct file *file,
else {
unsigned long ticks;
- pr_debug("timeout %lld.%09d\n",
+ dev_dbg(pps->dev, "timeout %lld.%09d\n",
(long long) fdata.timeout.sec,
fdata.timeout.nsec);
ticks = fdata.timeout.sec * HZ;
@@ -171,7 +171,7 @@ static long pps_cdev_ioctl(struct file *file,
/* Check for pending signals */
if (err == -ERESTARTSYS) {
- pr_debug("pending signal caught\n");
+ dev_dbg(pps->dev, "pending signal caught\n");
return -EINTR;
}
@@ -240,7 +240,7 @@ int pps_register_cdev(struct pps_device *pps)
err = cdev_add(&pps->cdev, devt, 1);
if (err) {
- printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
+ pr_err("pps: %s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
return err;
}
@@ -282,14 +282,14 @@ static int __init pps_init(void)
pps_class = class_create(THIS_MODULE, "pps");
if (!pps_class) {
- printk(KERN_ERR "pps: failed to allocate class\n");
+ pr_err("pps: failed to allocate class\n");
return -ENOMEM;
}
pps_class->dev_attrs = pps_attrs;
err = alloc_chrdev_region(&pps_devt, 0, PPS_MAX_SOURCES, "pps");
if (err < 0) {
- printk(KERN_ERR "pps: failed to allocate char device region\n");
+ pr_err("pps: failed to allocate char device region\n");
goto remove_class;
}
--
1.7.1
Add a helper function to gather timestamps. This way clients don't have
to duplicate it.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 9 ++-------
drivers/pps/clients/pps-ldisc.c | 18 ++++++------------
drivers/pps/kapi.c | 19 ++++++++++++-------
include/linux/pps_kernel.h | 19 ++++++++++++++++++-
include/linux/serial_core.h | 5 +++--
include/linux/tty_ldisc.h | 5 +++--
6 files changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index e7ef5b8..e1bdd8b 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -40,18 +40,13 @@ static struct timer_list ktimer;
static void pps_ktimer_event(unsigned long ptr)
{
- struct timespec __ts;
- struct pps_ktime ts;
+ struct pps_event_time ts;
/* First of all we get the time stamp... */
- getnstimeofday(&__ts);
+ pps_get_ts(&ts);
pr_info("PPS event at %lu\n", jiffies);
- /* ... and translate it to PPS time data struct */
- ts.sec = __ts.tv_sec;
- ts.nsec = __ts.tv_nsec;
-
pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 8e1932d..20fc9f7 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -27,26 +27,20 @@
#define PPS_TTY_MAGIC 0x0001
static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
- struct timespec *ts)
+ struct pps_event_time *ts)
{
int id = (long)tty->disc_data;
- struct timespec __ts;
- struct pps_ktime pps_ts;
+ struct pps_event_time __ts;
/* First of all we get the time stamp... */
- getnstimeofday(&__ts);
+ pps_get_ts(&__ts);
/* Does caller give us a timestamp? */
- if (ts) { /* Yes. Let's use it! */
- pps_ts.sec = ts->tv_sec;
- pps_ts.nsec = ts->tv_nsec;
- } else { /* No. Do it ourself! */
- pps_ts.sec = __ts.tv_sec;
- pps_ts.nsec = __ts.tv_nsec;
- }
+ if (!ts) /* No. Do it ourself! */
+ ts = &__ts;
/* Now do the PPS event report */
- pps_event(id, &pps_ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
+ pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
NULL);
pr_debug("PPS %s at %lu on source #%d\n",
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 3f89f5e..b431d83 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -268,11 +268,12 @@ EXPORT_SYMBOL(pps_unregister_source);
* pps->info.echo(source, event, data);
*/
-void pps_event(int source, struct pps_ktime *ts, int event, void *data)
+void pps_event(int source, struct pps_event_time *ts, int event, void *data)
{
struct pps_device *pps;
unsigned long flags;
int captured = 0;
+ struct pps_ktime ts_real;
if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
@@ -284,8 +285,10 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
if (!pps)
return;
- pr_debug("PPS event on source %d at %llu.%06u\n",
- pps->id, (unsigned long long) ts->sec, ts->nsec);
+ pr_debug("PPS event on source %d at %ld.%09ld\n",
+ pps->id, ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
+
+ timespec_to_pps_ktime(&ts_real, ts->ts_real);
spin_lock_irqsave(&pps->lock, flags);
@@ -299,10 +302,11 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
(pps->params.mode & PPS_CAPTUREASSERT)) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETASSERT)
- pps_add_offset(ts, &pps->params.assert_off_tu);
+ pps_add_offset(&ts_real,
+ &pps->params.assert_off_tu);
/* Save the time stamp */
- pps->assert_tu = *ts;
+ pps->assert_tu = ts_real;
pps->assert_sequence++;
pr_debug("capture assert seq #%u for source %d\n",
pps->assert_sequence, source);
@@ -313,10 +317,11 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
(pps->params.mode & PPS_CAPTURECLEAR)) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETCLEAR)
- pps_add_offset(ts, &pps->params.clear_off_tu);
+ pps_add_offset(&ts_real,
+ &pps->params.clear_off_tu);
/* Save the time stamp */
- pps->clear_tu = *ts;
+ pps->clear_tu = ts_real;
pps->clear_sequence++;
pr_debug("capture clear seq #%u for source %d\n",
pps->clear_sequence, source);
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c3aed4b..32aa676 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -43,6 +43,10 @@ struct pps_source_info {
struct device *dev;
};
+struct pps_event_time {
+ struct timespec ts_real;
+};
+
/* The main struct */
struct pps_device {
struct pps_source_info info; /* PSS source info */
@@ -88,7 +92,20 @@ extern int pps_register_source(struct pps_source_info *info,
extern void pps_unregister_source(int source);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
-extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
+extern void pps_event(int source, struct pps_event_time *ts, int event,
+ void *data);
+
+static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
+ struct timespec ts)
+{
+ kt->sec = ts.tv_sec;
+ kt->nsec = ts.tv_nsec;
+}
+
+static inline void pps_get_ts(struct pps_event_time *ts)
+{
+ getnstimeofday(&ts->ts_real);
+}
#endif /* LINUX_PPS_KERNEL_H */
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index f10db6e..17b809a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -196,6 +196,7 @@
#include <linux/tty.h>
#include <linux/mutex.h>
#include <linux/sysrq.h>
+#include <linux/pps_kernel.h>
struct uart_port;
struct serial_struct;
@@ -497,10 +498,10 @@ uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
struct uart_state *state = uport->state;
struct tty_port *port = &state->port;
struct tty_ldisc *ld = tty_ldisc_ref(port->tty);
- struct timespec ts;
+ struct pps_event_time ts;
if (ld && ld->ops->dcd_change)
- getnstimeofday(&ts);
+ pps_get_ts(&ts);
uport->icount.dcd++;
#ifdef CONFIG_HARD_PPS
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 526d66f..763e061 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -101,7 +101,7 @@
* any pending driver I/O is completed.
*
* void (*dcd_change)(struct tty_struct *tty, unsigned int status,
- * struct timespec *ts)
+ * struct pps_event_time *ts)
*
* Tells the discipline that the DCD pin has changed its status and
* the relative timestamp. Pointer ts can be NULL.
@@ -109,6 +109,7 @@
#include <linux/fs.h>
#include <linux/wait.h>
+#include <linux/pps_kernel.h>
struct tty_ldisc_ops {
int magic;
@@ -143,7 +144,7 @@ struct tty_ldisc_ops {
char *fp, int count);
void (*write_wakeup)(struct tty_struct *);
void (*dcd_change)(struct tty_struct *, unsigned int,
- struct timespec *);
+ struct pps_event_time *);
struct module *owner;
--
1.7.1
Here are some very trivial fixes combined:
* add macro definitions to protect header file from including several times
* remove declaration for an unexistent array
* fix typos
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 2 +-
include/linux/pps_kernel.h | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 1aa02db..55f3961 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -324,7 +324,7 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
captured = ~0;
}
- /* Wake up iif captured somthing */
+ /* Wake up if captured something */
if (captured) {
pps->go = ~0;
wake_up_interruptible(&pps->queue);
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index e0a193f..c930d11 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -18,6 +18,9 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#ifndef LINUX_PPS_KERNEL_H
+#define LINUX_PPS_KERNEL_H
+
#include <linux/pps.h>
#include <linux/cdev.h>
@@ -71,7 +74,6 @@ struct pps_device {
extern spinlock_t pps_idr_lock;
extern struct idr pps_idr;
-extern struct timespec pps_irq_ts[];
extern struct device_attribute pps_attrs[];
@@ -87,3 +89,6 @@ extern void pps_unregister_source(int source);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
+
+#endif /* LINUX_PPS_KERNEL_H */
+
--
1.7.1
There was a race in PPS_FETCH ioctl handler when several processes want
to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
sleep most of the time in the system call.
With the old approach when the first process waiting on the pps queue
is waken up it makes new system call right away and zeroes pps->go. So
other processes continue to sleep. This is a clear race condition
because of the global 'go' variable.
With the new approach pps->last_ev holds some value increasing at each
PPS event. PPS_FETCH ioctl handler saves current value to the local
variable at the very beginning so it can safely check that there is a
new event by just comparing both variables.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 4 ++--
drivers/pps/pps.c | 10 +++++++---
include/linux/pps_kernel.h | 2 +-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 55f3961..3f89f5e 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
/* Wake up if captured something */
if (captured) {
- pps->go = ~0;
- wake_up_interruptible(&pps->queue);
+ pps->last_ev++;
+ wake_up_interruptible_all(&pps->queue);
kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
}
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index c76afb9..cb24147 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -136,6 +136,7 @@ static long pps_cdev_ioctl(struct file *file,
case PPS_FETCH: {
struct pps_fdata fdata;
+ unsigned int ev;
pr_debug("PPS_FETCH: source %d\n", pps->id);
@@ -143,11 +144,12 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return -EFAULT;
- pps->go = 0;
+ ev = pps->last_ev;
/* Manage the timeout */
if (fdata.timeout.flags & PPS_TIME_INVALID)
- err = wait_event_interruptible(pps->queue, pps->go);
+ err = wait_event_interruptible(pps->queue,
+ ev < pps->last_ev);
else {
unsigned long ticks;
@@ -159,7 +161,9 @@ static long pps_cdev_ioctl(struct file *file,
if (ticks != 0) {
err = wait_event_interruptible_timeout(
- pps->queue, pps->go, ticks);
+ pps->queue,
+ ev < pps->last_ev,
+ ticks);
if (err == 0)
return -ETIMEDOUT;
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c930d11..c3aed4b 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -55,7 +55,7 @@ struct pps_device {
struct pps_ktime clear_tu;
int current_mode; /* PPS mode at event time */
- int go; /* PPS event is arrived? */
+ unsigned int last_ev; /* last PPS event id */
wait_queue_head_t queue; /* PPS event queue */
unsigned int id; /* PPS source unique ID */
--
1.7.1
Bitwise conjunction is distributive so we can simplify some conditions.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 220ab08..0335b2c 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -180,8 +180,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
/* Check the event */
pps->current_mode = pps->params.mode;
- if ((event & PPS_CAPTUREASSERT) &
- (pps->params.mode & PPS_CAPTUREASSERT)) {
+ if (event & pps->params.mode & PPS_CAPTUREASSERT) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETASSERT)
pps_add_offset(&ts_real,
@@ -195,8 +194,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
captured = ~0;
}
- if ((event & PPS_CAPTURECLEAR) &
- (pps->params.mode & PPS_CAPTURECLEAR)) {
+ if (event & pps->params.mode & PPS_CAPTURECLEAR) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETCLEAR)
pps_add_offset(&ts_real,
--
1.7.1
Since now idr is only used to manage char device id's and not used in
kernel API anymore it should be moved to pps.c. This also makes it
possible to release id only at actual device freeing so nobody can
register a pps device with the same id while our device is not freed
yet.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 53 +---------------------------------------------------
drivers/pps/pps.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 50 insertions(+), 54 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index eddf41e..041c27e 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -26,19 +26,11 @@
#include <linux/sched.h>
#include <linux/time.h>
#include <linux/spinlock.h>
-#include <linux/idr.h>
#include <linux/fs.h>
#include <linux/pps_kernel.h>
#include <linux/slab.h>
/*
- * Local variables
- */
-
-static DEFINE_SPINLOCK(pps_idr_lock);
-static DEFINE_IDR(pps_idr);
-
-/*
* Local functions
*/
@@ -75,7 +67,6 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
int default_params)
{
struct pps_device *pps;
- int id;
int err;
/* Sanity checks */
@@ -116,54 +107,18 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
- /* Get new ID for the new PPS source */
- if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
- err = -ENOMEM;
- goto kfree_pps;
- }
-
- spin_lock_irq(&pps_idr_lock);
-
- /* Now really allocate the PPS source.
- * After idr_get_new() calling the new source will be freely available
- * into the kernel.
- */
- err = idr_get_new(&pps_idr, pps, &id);
- if (err < 0) {
- spin_unlock_irq(&pps_idr_lock);
- goto kfree_pps;
- }
-
- id = id & MAX_ID_MASK;
- if (id >= PPS_MAX_SOURCES) {
- spin_unlock_irq(&pps_idr_lock);
-
- pr_err("pps: %s: too many PPS sources in the system\n",
- info->name);
- err = -EBUSY;
- goto free_idr;
- }
- pps->id = id;
-
- spin_unlock_irq(&pps_idr_lock);
-
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
pr_err("pps: %s: unable to create char device\n",
info->name);
- goto free_idr;
+ goto kfree_pps;
}
dev_info(pps->dev, "new PPS source %s\n", info->name);
return pps;
-free_idr:
- spin_lock_irq(&pps_idr_lock);
- idr_remove(&pps_idr, id);
- spin_unlock_irq(&pps_idr_lock);
-
kfree_pps:
kfree(pps);
@@ -183,13 +138,7 @@ EXPORT_SYMBOL(pps_register_source);
void pps_unregister_source(struct pps_device *pps)
{
- unsigned int id = pps->id;
-
pps_unregister_cdev(pps);
-
- spin_lock_irq(&pps_idr_lock);
- idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
}
EXPORT_SYMBOL(pps_unregister_source);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index a5ca1c0..d81f13b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -29,6 +29,7 @@
#include <linux/cdev.h>
#include <linux/poll.h>
#include <linux/pps_kernel.h>
+#include <linux/slab.h>
/*
* Local variables
@@ -37,6 +38,9 @@
static dev_t pps_devt;
static struct class *pps_class;
+static DEFINE_SPINLOCK(pps_idr_lock);
+static DEFINE_IDR(pps_idr);
+
/*
* Char device methods
*/
@@ -228,11 +232,47 @@ static const struct file_operations pps_cdev_fops = {
.release = pps_cdev_release,
};
+static void pps_device_destruct(struct device *dev)
+{
+ struct pps_device *pps = dev_get_drvdata(dev);
+
+ /* release id here to protect others from using it while it's
+ * still in use */
+ spin_lock_irq(&pps_idr_lock);
+ idr_remove(&pps_idr, pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
+ kfree(dev);
+}
+
int pps_register_cdev(struct pps_device *pps)
{
int err;
dev_t devt;
-
+
+ /* Get new ID for the new PPS source */
+ if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
+ return -ENOMEM;
+
+ /* Now really allocate the PPS source.
+ * After idr_get_new() calling the new source will be freely available
+ * into the kernel.
+ */
+ spin_lock_irq(&pps_idr_lock);
+ err = idr_get_new(&pps_idr, pps, &pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
+ if (err < 0)
+ return err;
+
+ pps->id &= MAX_ID_MASK;
+ if (pps->id >= PPS_MAX_SOURCES) {
+ pr_err("pps: %s: too many PPS sources in the system\n",
+ pps->info.name);
+ err = -EBUSY;
+ goto free_idr;
+ }
+
devt = MKDEV(MAJOR(pps_devt), pps->id);
cdev_init(&pps->cdev, &pps_cdev_fops);
@@ -242,13 +282,15 @@ int pps_register_cdev(struct pps_device *pps)
if (err) {
pr_err("pps: %s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
- return err;
+ goto free_idr;
}
pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
"pps%d", pps->id);
if (IS_ERR(pps->dev))
goto del_cdev;
+ pps->dev->release = pps_device_destruct;
+
pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
MAJOR(pps_devt), pps->id);
@@ -257,6 +299,11 @@ int pps_register_cdev(struct pps_device *pps)
del_cdev:
cdev_del(&pps->cdev);
+free_idr:
+ spin_lock_irq(&pps_idr_lock);
+ idr_remove(&pps_idr, pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
return err;
}
--
1.7.1
This handler should be called from an IRQ handler. It uses per-device
workqueue internally.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 2 +-
drivers/pps/clients/pps-ldisc.c | 2 +-
drivers/pps/kapi.c | 95 ++++++++++++++++++++++++++++++++++++--
drivers/pps/pps.c | 14 +++++-
include/linux/pps_kernel.h | 16 ++++++-
5 files changed, 119 insertions(+), 10 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index 9d27239..61d0800 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
dev_info(pps->dev, "PPS event at %lu\n", jiffies);
- pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
+ pps_event_irq(pps, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
}
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index d7e1a27..3e64042 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -40,7 +40,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
ts = &__ts;
/* Now do the PPS event report */
- pps_event(pps, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
+ pps_event_irq(pps, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
NULL);
dev_dbg(pps->dev, "PPS %s at %lu\n", status ? "assert" : "clear",
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 041c27e..d5f18ce 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -31,9 +31,19 @@
#include <linux/slab.h>
/*
+ * Global variables
+ */
+
+/* PPS event workqueue */
+struct workqueue_struct *pps_event_workqueue;
+
+/*
* Local functions
*/
+static void assert_work_func(struct work_struct *work);
+static void clear_work_func(struct work_struct *work);
+
static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
{
ts->nsec += offset->nsec;
@@ -107,6 +117,9 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
+ INIT_WORK(&pps->assert_work, assert_work_func);
+ INIT_WORK(&pps->clear_work, clear_work_func);
+
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
@@ -148,11 +161,12 @@ EXPORT_SYMBOL(pps_unregister_source);
* @event: the event type
* @data: userdef pointer
*
- * This function is used by each PPS client in order to register a new
- * PPS event into the system (it's usually called inside an IRQ handler).
+ * This function is used by PPS clients in order to register a new
+ * PPS event into the system. It should not be called from an IRQ
+ * handler. Use pps_event_irq instead.
*
- * If an echo function is associated with the PPS device it will be called
- * as:
+ * If an echo function is associated with the PPS device it will be
+ * called as:
* pps->info.echo(pps, event, data);
*/
void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
@@ -222,3 +236,76 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
spin_unlock_irqrestore(&pps->lock, flags);
}
EXPORT_SYMBOL(pps_event);
+
+/* Async event handlers */
+
+static void assert_work_func(struct work_struct *work)
+{
+ struct pps_device *pps = container_of(work,
+ struct pps_device, assert_work);
+
+ pps_event(pps, &pps->assert_work_ts, PPS_CAPTUREASSERT,
+ pps->assert_work_data);
+}
+
+static void clear_work_func(struct work_struct *work)
+{
+ struct pps_device *pps = container_of(work,
+ struct pps_device, clear_work);
+
+ pps_event(pps, &pps->clear_work_ts, PPS_CAPTURECLEAR,
+ pps->clear_work_data);
+}
+
+/* pps_event_irq - register a PPS event for deffered handling using
+ * workqueue
+ *
+ * @pps: the PPS device
+ * @ts: the event timestamp
+ * @event: the event type
+ * @data: userdef pointer
+ *
+ * This function is used by PPS clients in order to register a new
+ * PPS event into the system. It should be called from an IRQ handler
+ * only.
+ *
+ * If an echo function is associated with the PPS device it will be
+ * called as:
+ * pps->info.echo(pps, event, data);
+ */
+void pps_event_irq(struct pps_device *pps, struct pps_event_time *ts,
+ int event, void *data)
+{
+ /* check event type */
+ BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
+
+ if (event & PPS_CAPTUREASSERT) {
+ if (work_pending(&pps->assert_work)) {
+ dev_err(pps->dev, "deferred assert edge work haven't"
+ " been handled within a second\n");
+ /* FIXME: do something more intelligent
+ * then just exit */
+ } else {
+ /* now we can copy data safely */
+ pps->assert_work_ts = *ts;
+ pps->assert_work_data = data;
+
+ queue_work(pps_event_workqueue, &pps->assert_work);
+ }
+ }
+ if (event & PPS_CAPTURECLEAR) {
+ if (work_pending(&pps->clear_work)) {
+ dev_err(pps->dev, "deferred clear edge work haven't"
+ " been handled within a second\n");
+ /* FIXME: do something more intelligent
+ * then just exit */
+ } else {
+ /* now we can copy data safely */
+ pps->clear_work_ts = *ts;
+ pps->clear_work_data = data;
+
+ queue_work(pps_event_workqueue, &pps->clear_work);
+ }
+ }
+}
+EXPORT_SYMBOL(pps_event_irq);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index d81f13b..81adb33 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -319,18 +319,26 @@ void pps_unregister_cdev(struct pps_device *pps)
static void __exit pps_exit(void)
{
- class_destroy(pps_class);
unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
+ class_destroy(pps_class);
+ destroy_workqueue(pps_event_workqueue);
}
static int __init pps_init(void)
{
int err;
+ pps_event_workqueue = create_rt_workqueue("pps");
+ if (!pps_event_workqueue) {
+ pr_err("pps: failed to create workqueue\n");
+ return -ENOMEM;
+ }
+
pps_class = class_create(THIS_MODULE, "pps");
if (!pps_class) {
pr_err("pps: failed to allocate class\n");
- return -ENOMEM;
+ err = -ENOMEM;
+ goto destroy_workqueue;
}
pps_class->dev_attrs = pps_attrs;
@@ -348,6 +356,8 @@ static int __init pps_init(void)
remove_class:
class_destroy(pps_class);
+destroy_workqueue:
+ destroy_workqueue(pps_event_workqueue);
return err;
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 1e0f249..5af0498 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -26,6 +26,7 @@
#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/time.h>
+#include <linux/workqueue.h>
/*
* Global defines
@@ -70,6 +71,13 @@ struct pps_device {
struct device *dev;
struct fasync_struct *async_queue; /* fasync method */
spinlock_t lock;
+
+ struct work_struct assert_work;
+ struct work_struct clear_work;
+ struct pps_event_time assert_work_ts;
+ struct pps_event_time clear_work_ts;
+ void *assert_work_data;
+ void *clear_work_data;
};
/*
@@ -78,6 +86,8 @@ struct pps_device {
extern struct device_attribute pps_attrs[];
+extern struct workqueue_struct *pps_event_workqueue;
+
/*
* Exported functions
*/
@@ -87,8 +97,10 @@ extern struct pps_device *pps_register_source(
extern void pps_unregister_source(struct pps_device *pps);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
-extern void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
- void *data);
+extern void pps_event(struct pps_device *pps,
+ struct pps_event_time *ts, int event, void *data);
+extern void pps_event_irq(struct pps_device *pps,
+ struct pps_event_time *ts, int event, void *data);
static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
struct timespec ts)
--
1.7.1
MONOTONIC_RAW clock timestamps are ideally suited for frequency
calculation and also fit well into the original NTP hardpps design. Now
phase and frequency can be adjusted separately: the former based on
REALTIME clock and the latter based on MONOTONIC_RAW clock.
A new function getnstime_raw_and_real is added to timekeeping subsystem
to capture both timestamps at the same time and atomically.
Signed-off-by: Alexander Gordeev <[email protected]>
---
include/linux/pps_kernel.h | 3 ++-
include/linux/time.h | 2 ++
kernel/time/timekeeping.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 5af0498..39fc712 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -48,6 +48,7 @@ struct pps_source_info {
};
struct pps_event_time {
+ struct timespec ts_raw;
struct timespec ts_real;
};
@@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
static inline void pps_get_ts(struct pps_event_time *ts)
{
- getnstimeofday(&ts->ts_real);
+ getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
}
#endif /* LINUX_PPS_KERNEL_H */
diff --git a/include/linux/time.h b/include/linux/time.h
index ea3559f..1da8a7b 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -143,6 +143,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
extern int do_getitimer(int which, struct itimerval *value);
extern void getnstimeofday(struct timespec *tv);
extern void getrawmonotonic(struct timespec *ts);
+extern void getnstime_raw_and_real(struct timespec *ts_raw,
+ struct timespec *ts_real);
extern void getboottime(struct timespec *ts);
extern void monotonic_to_bootbased(struct timespec *ts);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index caf8d4d..ef37b14 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
EXPORT_SYMBOL_GPL(ktime_get_ts);
/**
+ * getnstime_raw_and_real - Returns both the time of day an raw
+ * monotonic time in a timespec format
+ * @ts_mono_raw: pointer to the timespec to be set to raw
+ * monotonic time
+ * @ts_real: pointer to the timespec to be set to the time
+ * of day
+ */
+void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
+{
+ unsigned long seq;
+ s64 nsecs_raw, nsecs_real;
+
+ WARN_ON(timekeeping_suspended);
+
+ do {
+ seq = read_seqbegin(&xtime_lock);
+
+ *ts_raw = raw_time;
+ *ts_real = xtime;
+
+ nsecs_raw = timekeeping_get_ns_raw();
+ nsecs_real = timekeeping_get_ns();
+
+ /* If arch requires, add in gettimeoffset() */
+ nsecs_real += arch_gettimeoffset();
+
+ } while (read_seqretry(&xtime_lock, seq));
+
+ timespec_add_ns(ts_raw, nsecs_raw);
+ timespec_add_ns(ts_real, nsecs_real);
+}
+EXPORT_SYMBOL(getnstime_raw_and_real);
+
+/**
* do_gettimeofday - Returns the time of day in a timeval
* @tv: pointer to the timeval to be set
*
--
1.7.1
This way less overhead is involved when running production kernel.
If you want to debug a pps client module please define DEBUG to enable
the checks.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 33 ++++++++++-----------------------
1 files changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 8c557c4..220ab08 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -80,25 +80,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
int err;
/* Sanity checks */
- if ((info->mode & default_params) != default_params) {
- pr_err("pps: %s: unsupported default parameters\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
- if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
- info->echo == NULL) {
- pr_err("pps: %s: echo function is not defined\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
- if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
- pr_err("pps: %s: unspecified time format\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
+
+ /* default_params should be supported */
+ BUG_ON((info->mode & default_params) != default_params);
+ /* echo function should be defined if we are asked to call it */
+ BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
+ info->echo == NULL);
+ /* time format should be specified */
+ BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
/* Allocate memory for the new PPS source struct */
pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
@@ -175,10 +164,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
int captured = 0;
struct pps_ktime ts_real;
- if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
- dev_err(pps->dev, "unknown event (%x)\n", event);
- return;
- }
+ /* check event type */
+ BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
--
1.7.1
Add an optional feature of PPSAPI, kernel consumer support, which uses
the added hardpps() function.
Signed-off-by: Alexander Gordeev <[email protected]>
---
Documentation/ioctl/ioctl-number.txt | 2 +-
drivers/pps/Kconfig | 1 +
drivers/pps/kapi.c | 23 ++++++++++++
drivers/pps/pps.c | 62 +++++++++++++++++++++++++++++++++-
include/linux/pps.h | 7 ++++
include/linux/pps_kernel.h | 6 +++
6 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index dd5806f..e507f84 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -248,7 +248,7 @@ Code Seq#(hex) Include File Comments
'p' 40-7F linux/nvram.h
'p' 80-9F linux/ppdev.h user-space parport
<mailto:[email protected]>
-'p' A1-A4 linux/pps.h LinuxPPS
+'p' A1-A5 linux/pps.h LinuxPPS
<mailto:[email protected]>
'q' 00-1F linux/serio.h
'q' 80-FF linux/telephony.h Internet PhoneJACK, Internet LineJACK
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 33c9126..4823e47 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -7,6 +7,7 @@ menu "PPS support"
config PPS
tristate "PPS support"
depends on EXPERIMENTAL
+ select NTP_PPS
---help---
PPS (Pulse Per Second) is a special pulse provided by some GPS
antennae. Userland can use it to get a high-precision time
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 0335b2c..02f4dd1 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -25,6 +25,7 @@
#include <linux/init.h>
#include <linux/sched.h>
#include <linux/time.h>
+#include <linux/timex.h>
#include <linux/spinlock.h>
#include <linux/fs.h>
#include <linux/pps_kernel.h>
@@ -37,6 +38,12 @@
/* PPS event workqueue */
struct workqueue_struct *pps_event_workqueue;
+/* state variables to bind kernel consumer */
+/* PPS API (RFC 2783): current source and mode for ``kernel consumer'' */
+DEFINE_SPINLOCK(pps_kc_hardpps_lock);
+void *pps_kc_hardpps_dev; /* some unique pointer to device */
+int pps_kc_hardpps_mode; /* mode bits for kernel consumer */
+
/*
* Local functions
*/
@@ -140,6 +147,16 @@ EXPORT_SYMBOL(pps_register_source);
void pps_unregister_source(struct pps_device *pps)
{
+ spin_lock(&pps_kc_hardpps_lock);
+ if (pps == pps_kc_hardpps_dev) {
+ pps_kc_hardpps_mode = 0;
+ pps_kc_hardpps_dev = NULL;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "unbound kernel consumer"
+ " on device removal\n");
+ } else
+ spin_unlock(&pps_kc_hardpps_lock);
+
pps_unregister_cdev(pps);
}
EXPORT_SYMBOL(pps_unregister_source);
@@ -209,6 +226,12 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
captured = ~0;
}
+ /* Pass some events to kernel consumer if activated */
+ spin_lock(&pps_kc_hardpps_lock);
+ if (pps == pps_kc_hardpps_dev && event & pps_kc_hardpps_mode)
+ hardpps(&ts->ts_real, &ts->ts_raw);
+ spin_unlock(&pps_kc_hardpps_lock);
+
/* Wake up if captured something */
if (captured) {
pps->last_ev++;
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 4f02718..c288b72 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -196,9 +196,69 @@ static long pps_cdev_ioctl(struct file *file,
break;
}
+ case PPS_KC_BIND: {
+ struct pps_bind_args bind_args;
+
+ dev_dbg(pps->dev, "PPS_KC_BIND\n");
+
+ /* Check the capabilities */
+ if (!capable(CAP_SYS_TIME))
+ return -EPERM;
+
+ if (copy_from_user(&bind_args, uarg,
+ sizeof(struct pps_bind_args)))
+ return -EFAULT;
+
+ /* Check for supported capabilities */
+ if ((bind_args.edge & ~pps->info.mode) != 0) {
+ dev_err(pps->dev, "unsupported capabilities (%x)\n",
+ bind_args.edge);
+ return -EINVAL;
+ }
+
+ /* Validate parameters roughly */
+ if (bind_args.tsformat != PPS_TSFMT_TSPEC ||
+ (bind_args.edge & ~PPS_CAPTUREBOTH) != 0 ||
+ bind_args.consumer != PPS_KC_HARDPPS) {
+ dev_err(pps->dev, "invalid kernel consumer bind"
+ " parameters (%x)\n", bind_args.edge);
+ return -EINVAL;
+ }
+
+ /* Check if another consumer is already bound */
+ spin_lock(&pps_kc_hardpps_lock);
+
+ if (bind_args.edge == 0)
+ if (pps_kc_hardpps_dev == pps) {
+ pps_kc_hardpps_mode = 0;
+ pps_kc_hardpps_dev = NULL;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "unbound kernel"
+ " consumer\n");
+ } else {
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_err(pps->dev, "selected kernel consumer"
+ " is not bound\n");
+ return -EINVAL;
+ }
+ else
+ if (pps_kc_hardpps_dev == NULL ||
+ pps_kc_hardpps_dev == pps) {
+ pps_kc_hardpps_mode = bind_args.edge;
+ pps_kc_hardpps_dev = pps;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "bound kernel consumer: "
+ "edge=0x%x\n", bind_args.edge);
+ } else {
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_err(pps->dev, "another kernel consumer"
+ " is already bound\n");
+ return -EINVAL;
+ }
+ break;
+ }
default:
return -ENOTTY;
- break;
}
return 0;
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 0194ab0..a9bb1d9 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -114,11 +114,18 @@ struct pps_fdata {
struct pps_ktime timeout;
};
+struct pps_bind_args {
+ int tsformat; /* format of time stamps */
+ int edge; /* selected event type */
+ int consumer; /* selected kernel consumer */
+};
+
#include <linux/ioctl.h>
#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_KC_BIND _IOW('p', 0xa5, struct pps_bind_args *)
#endif /* _PPS_H_ */
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 39fc712..10e10c8 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -87,6 +87,12 @@ struct pps_device {
extern struct device_attribute pps_attrs[];
+/* state variables to bind kernel consumer */
+/* PPS API (RFC 2783): current source and mode for ``kernel consumer'' */
+extern spinlock_t pps_kc_hardpps_lock;
+extern void *pps_kc_hardpps_dev; /* some unique pointer to device */
+extern int pps_kc_hardpps_mode; /* mode bits for kernel consumer */
+
extern struct workqueue_struct *pps_event_workqueue;
/*
--
1.7.1
Now all PPS spin locks are never used in interrupt context so we can
safely replace spin_lock_irq*/spin_unlock_irq* with plain
spin_lock/spin_unlock.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 5 ++---
drivers/pps/pps.c | 24 ++++++++++++------------
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index d5f18ce..8c557c4 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -172,7 +172,6 @@ EXPORT_SYMBOL(pps_unregister_source);
void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
void *data)
{
- unsigned long flags;
int captured = 0;
struct pps_ktime ts_real;
@@ -186,7 +185,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
timespec_to_pps_ktime(&ts_real, ts->ts_real);
- spin_lock_irqsave(&pps->lock, flags);
+ spin_lock(&pps->lock);
/* Must call the echo function? */
if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
@@ -233,7 +232,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
}
- spin_unlock_irqrestore(&pps->lock, flags);
+ spin_unlock(&pps->lock);
}
EXPORT_SYMBOL(pps_event);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 81adb33..4f02718 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -73,12 +73,12 @@ static long pps_cdev_ioctl(struct file *file,
case PPS_GETPARAMS:
dev_dbg(pps->dev, "PPS_GETPARAMS\n");
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
/* Get the current parameters */
params = pps->params;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
err = copy_to_user(uarg, ¶ms, sizeof(struct pps_kparams));
if (err)
@@ -109,7 +109,7 @@ static long pps_cdev_ioctl(struct file *file,
return -EINVAL;
}
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
/* Save the new parameters */
pps->params = params;
@@ -125,7 +125,7 @@ static long pps_cdev_ioctl(struct file *file,
pps->params.mode |= PPS_CANWAIT;
pps->params.api_version = PPS_API_VERS;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
break;
@@ -180,7 +180,7 @@ static long pps_cdev_ioctl(struct file *file,
}
/* Return the fetched timestamp */
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
fdata.info.assert_sequence = pps->assert_sequence;
fdata.info.clear_sequence = pps->clear_sequence;
@@ -188,7 +188,7 @@ static long pps_cdev_ioctl(struct file *file,
fdata.info.clear_tu = pps->clear_tu;
fdata.info.current_mode = pps->current_mode;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
if (err)
@@ -238,9 +238,9 @@ static void pps_device_destruct(struct device *dev)
/* release id here to protect others from using it while it's
* still in use */
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
kfree(dev);
}
@@ -258,9 +258,9 @@ int pps_register_cdev(struct pps_device *pps)
* After idr_get_new() calling the new source will be freely available
* into the kernel.
*/
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
err = idr_get_new(&pps_idr, pps, &pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
if (err < 0)
return err;
@@ -300,9 +300,9 @@ del_cdev:
cdev_del(&pps->cdev);
free_idr:
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
return err;
}
--
1.7.1
Add parallel port PPS client. It uses a standard method for capturing
timestamps for assert edge transitions: getting a timestamp soon after
an interrupt has happened. This is not a very precise source of time
information due to interrupt handling delays. However, timestamps for
clear edge transitions are much more precise because the interrupt
handler continuously polls hardware port until the transition is done.
Hardware port operations require only about 1us so the maximum error
should not exceed this value. This was my primary goal when developing
this client.
Clear edge capture could be disabled using clear_wait parameter.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/Kconfig | 7 +
drivers/pps/clients/Makefile | 1 +
drivers/pps/clients/pps_parport.c | 244 +++++++++++++++++++++++++++++++++++++
3 files changed, 252 insertions(+), 0 deletions(-)
create mode 100644 drivers/pps/clients/pps_parport.c
diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
index 4e801bd..8520a7f 100644
--- a/drivers/pps/clients/Kconfig
+++ b/drivers/pps/clients/Kconfig
@@ -22,4 +22,11 @@ config PPS_CLIENT_LDISC
If you say yes here you get support for a PPS source connected
with the CD (Carrier Detect) pin of your serial port.
+config PPS_CLIENT_PARPORT
+ tristate "Parallel port PPS client"
+ depends on PPS && PARPORT
+ help
+ If you say yes here you get support for a PPS source connected
+ with the interrupt pin of your parallel port.
+
endif
diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
index 812c9b1..42517da 100644
--- a/drivers/pps/clients/Makefile
+++ b/drivers/pps/clients/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_PPS_CLIENT_KTIMER) += pps-ktimer.o
obj-$(CONFIG_PPS_CLIENT_LDISC) += pps-ldisc.o
+obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
ifeq ($(CONFIG_PPS_DEBUG),y)
EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
new file mode 100644
index 0000000..71d8958
--- /dev/null
+++ b/drivers/pps/clients/pps_parport.c
@@ -0,0 +1,244 @@
+/*
+ * pps_parport.c -- kernel parallel port PPS client
+ *
+ *
+ * Copyright (C) 2009 Alexander Gordeev <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+
+/*
+ * TODO:
+ * implement echo over SEL pin
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/irqnr.h>
+#include <linux/time.h>
+#include <linux/parport.h>
+#include <linux/pps_kernel.h>
+
+#define DRVNAME "pps_parport"
+#define DRVDESC "parallel port PPS client"
+
+/* module parameters */
+
+#define CLEAR_WAIT_MAX 100
+#define CLEAR_WAIT_MAX_ERRORS 5
+
+static unsigned int clear_wait = 100;
+MODULE_PARM_DESC(clear_wait,
+ "Maximum number of port reads when polling for signal clear,"
+ " zero turns clear edge capture off entirely");
+module_param(clear_wait, uint, 0);
+
+
+/* internal per port structure */
+struct pps_client_pp {
+ struct pardevice *pardev; /* parport device */
+ struct pps_device *pps; /* PPS device */
+ unsigned int cw; /* port clear timeout */
+ unsigned int cw_err; /* number of timeouts */
+};
+
+#define SIGNAL_IS_SET(port) \
+ ((port->ops->read_status(port) & PARPORT_STATUS_ACK) != 0)
+
+/* parport interrupt handler */
+static void parport_irq(void *handle)
+{
+ struct pps_event_time ts_assert, ts_clear;
+ struct pps_client_pp *dev = handle;
+ struct parport *port = dev->pardev->port;
+ unsigned int i;
+ unsigned long flags;
+
+ /* first of all we get the time stamp... */
+ pps_get_ts(&ts_assert);
+
+ if (dev->cw == 0)
+ /* clear edge capture disabled */
+ goto out_assert;
+
+ /* try capture the clear edge */
+ local_irq_save(flags);
+ /* check the signal (no signal means the pulse is lost this time) */
+ if (!SIGNAL_IS_SET(port)) {
+ local_irq_restore(flags);
+ dev_err(dev->pps->dev, "lost the signal\n");
+ goto out_assert;
+ }
+
+ /* poll the port until the signal is unset */
+ for (i = dev->cw; i; i--)
+ if (!SIGNAL_IS_SET(port)) {
+ pps_get_ts(&ts_clear);
+ local_irq_restore(flags);
+ dev->cw_err = 0;
+ goto out_both;
+ }
+ local_irq_restore(flags);
+
+ /* timeout */
+ dev->cw_err++;
+ if (dev->cw_err >= CLEAR_WAIT_MAX_ERRORS) {
+ dev_err(dev->pps->dev, "disabled clear edge capture after %d"
+ " timeouts\n", dev->cw_err);
+ dev->cw = 0;
+ dev->cw_err = 0;
+ }
+
+out_assert:
+ /* fire assert event */
+ pps_event_irq(dev->pps, &ts_assert,
+ PPS_CAPTUREASSERT, NULL);
+ return;
+
+out_both:
+ /* fire assert event */
+ pps_event_irq(dev->pps, &ts_assert,
+ PPS_CAPTUREASSERT, NULL);
+ /* fire clear event */
+ pps_event_irq(dev->pps, &ts_clear,
+ PPS_CAPTURECLEAR, NULL);
+ return;
+}
+
+/* the PPS echo function */
+static void pps_echo(struct pps_device *pps, int event, void *data)
+{
+ dev_info(pps->dev, "echo %s %s\n",
+ event & PPS_CAPTUREASSERT ? "assert" : "",
+ event & PPS_CAPTURECLEAR ? "clear" : "");
+}
+
+static void parport_attach(struct parport *port)
+{
+ struct pps_client_pp *device;
+ struct pps_source_info info = {
+ .name = DRVNAME,
+ .path = "",
+ .mode = PPS_CAPTUREBOTH | \
+ PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
+ PPS_ECHOASSERT | PPS_ECHOCLEAR | \
+ PPS_CANWAIT | PPS_TSFMT_TSPEC,
+ .echo = pps_echo,
+ .owner = THIS_MODULE,
+ .dev = NULL
+ };
+
+ device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
+ if (!device) {
+ pr_err(DRVNAME ": memory allocation failed, not attaching\n");
+ return;
+ }
+
+ device->pardev = parport_register_device(port, DRVNAME,
+ NULL, NULL, parport_irq, 0, device);
+ if (!device->pardev) {
+ pr_err(DRVNAME ": couldn't register with %s\n", port->name);
+ goto err_free;
+ }
+
+ if (parport_claim_or_block(device->pardev) < 0) {
+ pr_err(DRVNAME ": couldn't claim %s\n", port->name);
+ goto err_unregister_dev;
+ }
+
+ device->pps = pps_register_source(&info,
+ PPS_CAPTUREBOTH | PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
+ if (device->pps == NULL) {
+ pr_err(DRVNAME ": couldn't register PPS source\n");
+ goto err_release_dev;
+ }
+
+ device->cw = clear_wait;
+
+ port->ops->enable_irq(port);
+
+ pr_info(DRVNAME ": attached to %s\n", port->name);
+
+ return;
+
+err_release_dev:
+ parport_release(device->pardev);
+err_unregister_dev:
+ parport_unregister_device(device->pardev);
+err_free:
+ kfree(device);
+}
+
+static void parport_detach(struct parport *port)
+{
+ struct pardevice *pardev = port->cad;
+ struct pps_client_pp *device;
+
+ /* FIXME: oooh, this is ugly! */
+ if (strcmp(pardev->name, DRVNAME))
+ /* not our port */
+ return;
+
+ device = pardev->private;
+
+ port->ops->disable_irq(port);
+ pps_unregister_source(device->pps);
+ parport_release(pardev);
+ parport_unregister_device(pardev);
+ kfree(device);
+}
+
+static struct parport_driver pps_parport_driver = {
+ .name = DRVNAME,
+ .attach = parport_attach,
+ .detach = parport_detach,
+};
+
+/* module staff */
+
+static int __init pps_parport_init(void)
+{
+ int ret;
+
+ pr_info(DRVNAME ": " DRVDESC "\n");
+
+ if (clear_wait > CLEAR_WAIT_MAX) {
+ pr_err(DRVNAME ": clear_wait value should be not"
+ " greater then %d\n", CLEAR_WAIT_MAX);
+ return -EINVAL;
+ }
+
+ ret = parport_register_driver(&pps_parport_driver);
+ if (ret) {
+ pr_err(DRVNAME ": unable to register with parport\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit pps_parport_exit(void)
+{
+ parport_unregister_driver(&pps_parport_driver);
+}
+
+module_init(pps_parport_init);
+module_exit(pps_parport_exit);
+
+MODULE_AUTHOR("Alexander Gordeev <[email protected]>");
+MODULE_DESCRIPTION(DRVDESC);
+MODULE_LICENSE("GPL");
--
1.7.1
Add PPS signal generator which utilizes STROBE pin of a parallel port to
send PPS signals. It uses parport abstraction layer and hrtimers to
precisely control the signal.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/Kconfig | 2 +
drivers/pps/Makefile | 2 +-
drivers/pps/generators/Kconfig | 17 ++
drivers/pps/generators/Makefile | 9 +
drivers/pps/generators/pps_gen_parport.c | 274 ++++++++++++++++++++++++++++++
5 files changed, 303 insertions(+), 1 deletions(-)
create mode 100644 drivers/pps/generators/Kconfig
create mode 100644 drivers/pps/generators/Makefile
create mode 100644 drivers/pps/generators/pps_gen_parport.c
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 4823e47..79bda60 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -40,4 +40,6 @@ config NTP_PPS
source drivers/pps/clients/Kconfig
+source drivers/pps/generators/Kconfig
+
endmenu
diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
index 98960dd..1906eb7 100644
--- a/drivers/pps/Makefile
+++ b/drivers/pps/Makefile
@@ -4,6 +4,6 @@
pps_core-y := pps.o kapi.o sysfs.o
obj-$(CONFIG_PPS) := pps_core.o
-obj-y += clients/
+obj-y += clients/ generators/
ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
new file mode 100644
index 0000000..5fbd614
--- /dev/null
+++ b/drivers/pps/generators/Kconfig
@@ -0,0 +1,17 @@
+#
+# PPS generators configuration
+#
+
+if PPS
+
+comment "PPS generators support"
+
+config PPS_GENERATOR_PARPORT
+ tristate "Parallel port PPS signal generator"
+ depends on PARPORT != n && GENERIC_TIME
+ help
+ If you say yes here you get support for a PPS signal generator which
+ utilizes STROBE pin of a parallel port to send PPS signals. It uses
+ parport abstraction layer and hrtimers to precisely control the signal.
+
+endif
diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
new file mode 100644
index 0000000..303304a
--- /dev/null
+++ b/drivers/pps/generators/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for PPS generators.
+#
+
+obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
+
+ifeq ($(CONFIG_PPS_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
new file mode 100644
index 0000000..be819d2
--- /dev/null
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -0,0 +1,274 @@
+/*
+ * pps_gen_parport.c -- kernel parallel port PPS signal generator
+ *
+ *
+ * Copyright (C) 2009 Alexander Gordeev <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+
+/*
+ * TODO:
+ * fix issues when realtime clock is adjusted in a leap
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/time.h>
+#include <linux/hrtimer.h>
+#include <linux/parport.h>
+
+#define DRVNAME "pps_gen_parport"
+#define DRVDESC "parallel port PPS signal generator"
+
+#define SIGNAL 0
+#define NO_SIGNAL PARPORT_CONTROL_STROBE
+
+/* module parameters */
+
+#define SEND_DELAY_MAX 300000000
+
+static unsigned int send_delay = 30000;
+MODULE_PARM_DESC(delay,
+ "Delay between setting and dropping the signal (ns)");
+module_param_named(delay, send_delay, uint, 0);
+
+
+#define SAFETY_INTERVAL 3000 /* set the hrtimer earlier for safety (ns) */
+
+/* internal per port structure */
+struct pps_generator_pp {
+ struct pardevice *pardev; /* parport device */
+ struct hrtimer timer;
+ long port_write_time; /* calibrated port write time (ns) */
+};
+
+static struct pps_generator_pp device = {
+ .pardev = NULL,
+};
+
+static int attached;
+
+/* calibrated time between a hrtimer event and the reaction */
+static long hrtimer_error = SAFETY_INTERVAL;
+
+/* the kernel hrtimer event */
+static enum hrtimer_restart hrtimer_event(struct hrtimer *timer)
+{
+ struct timespec expire_time, ts1, ts2, ts3, dts;
+ struct pps_generator_pp *dev;
+ struct parport *port;
+ long lim, delta;
+ unsigned long flags;
+
+ /* NB: approx time with blocked interrupts =
+ send_delay + 3 * SAFETY_INTERVAL */
+ local_irq_save(flags);
+
+ /* first of all we get the time stamp... */
+ getnstimeofday(&ts1);
+ expire_time = ktime_to_timespec(timer->_expires);
+ dev = container_of(timer, struct pps_generator_pp, timer);
+ lim = NSEC_PER_SEC - send_delay - dev->port_write_time;
+
+ /* check if we are late */
+ if (expire_time.tv_sec != ts1.tv_sec || ts1.tv_nsec > lim) {
+ local_irq_restore(flags);
+ pr_err(DRVNAME ": we are late this time %ld.%09ld\n",
+ ts1.tv_sec, ts1.tv_nsec);
+ goto done;
+ }
+
+ /* busy loop until the time is right for an assert edge */
+ do {
+ getnstimeofday(&ts2);
+ } while (expire_time.tv_sec == ts2.tv_sec && ts2.tv_nsec < lim);
+
+ /* set the signal */
+ port = dev->pardev->port;
+ port->ops->write_control(port, SIGNAL);
+
+ /* busy loop until the time is right for a clear edge */
+ lim = NSEC_PER_SEC - dev->port_write_time;
+ do {
+ getnstimeofday(&ts2);
+ } while (expire_time.tv_sec == ts2.tv_sec && ts2.tv_nsec < lim);
+
+ /* unset the signal */
+ port->ops->write_control(port, NO_SIGNAL);
+
+ getnstimeofday(&ts3);
+
+ local_irq_restore(flags);
+
+ /* update calibrated port write time */
+ dts = timespec_sub(ts3, ts2);
+ dev->port_write_time =
+ (dev->port_write_time + timespec_to_ns(&dts)) >> 1;
+
+done:
+ /* update calibrated hrtimer error */
+ dts = timespec_sub(ts1, expire_time);
+ delta = timespec_to_ns(&dts);
+ /* If the new error value is bigger then the old, use the new
+ * value, if not then slowly move towards the new value. This
+ * way it should be safe in bad conditions and efficient in
+ * good conditions.
+ */
+ if (delta >= hrtimer_error)
+ hrtimer_error = delta;
+ else
+ hrtimer_error = (3 * hrtimer_error + delta) >> 2;
+
+ /* update the hrtimer expire time */
+ hrtimer_set_expires(timer,
+ ktime_set(expire_time.tv_sec + 1,
+ NSEC_PER_SEC - (send_delay +
+ dev->port_write_time + SAFETY_INTERVAL +
+ 2 * hrtimer_error)));
+
+ return HRTIMER_RESTART;
+}
+
+/* calibrate port write time */
+#define PORT_NTESTS_SHIFT 5
+static void calibrate_port(struct pps_generator_pp *dev)
+{
+ struct parport *port = dev->pardev->port;
+ int i;
+ long acc = 0;
+
+ for (i = 0; i < (1 << PORT_NTESTS_SHIFT); i++) {
+ struct timespec a, b;
+ unsigned long irq_flags;
+
+ local_irq_save(irq_flags);
+ getnstimeofday(&a);
+ port->ops->write_control(port, NO_SIGNAL);
+ getnstimeofday(&b);
+ local_irq_restore(irq_flags);
+
+ b = timespec_sub(b, a);
+ acc += timespec_to_ns(&b);
+ }
+
+ dev->port_write_time = acc >> PORT_NTESTS_SHIFT;
+ pr_info(DRVNAME ": port write takes %ldns\n", dev->port_write_time);
+}
+
+static inline ktime_t next_intr_time(struct pps_generator_pp *dev)
+{
+ struct timespec ts;
+
+ getnstimeofday(&ts);
+
+ return ktime_set(ts.tv_sec +
+ ((ts.tv_nsec > 990 * NSEC_PER_MSEC) ? 1 : 0),
+ NSEC_PER_SEC - (send_delay +
+ dev->port_write_time + 3 * SAFETY_INTERVAL));
+}
+
+static void parport_attach(struct parport *port)
+{
+ if (attached) {
+ /* we already have a port */
+ return;
+ }
+
+ device.pardev = parport_register_device(port, DRVNAME,
+ NULL, NULL, NULL, 0, &device);
+ if (!device.pardev) {
+ pr_err(DRVNAME ": couldn't register with %s\n", port->name);
+ return;
+ }
+
+ if (parport_claim_or_block(device.pardev) < 0) {
+ pr_err(DRVNAME ": couldn't claim %s\n", port->name);
+ goto err_unregister_dev;
+ }
+
+ pr_info(DRVNAME ": attached to %s\n", port->name);
+ attached = 1;
+
+ calibrate_port(&device);
+
+ hrtimer_init(&device.timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
+ device.timer.function = hrtimer_event;
+#ifdef CONFIG_PREEMPT_RT
+ /* hrtimer interrupt will run in the interrupt context with this */
+ device.timer.irqsafe = 1;
+#endif
+
+ hrtimer_start(&device.timer, next_intr_time(&device), HRTIMER_MODE_ABS);
+
+ return;
+
+err_unregister_dev:
+ parport_unregister_device(device.pardev);
+}
+
+static void parport_detach(struct parport *port)
+{
+ if (port->cad != device.pardev)
+ return; /* not our port */
+
+ hrtimer_cancel(&device.timer);
+ parport_release(device.pardev);
+ parport_unregister_device(device.pardev);
+}
+
+static struct parport_driver pps_gen_parport_driver = {
+ .name = DRVNAME,
+ .attach = parport_attach,
+ .detach = parport_detach,
+};
+
+/* module staff */
+
+static int __init pps_gen_parport_init(void)
+{
+ int ret;
+
+ pr_info(DRVNAME ": " DRVDESC "\n");
+
+ if (send_delay > SEND_DELAY_MAX) {
+ pr_err(DRVNAME ": delay value should be not"
+ " greater then %d\n", SEND_DELAY_MAX);
+ return -EINVAL;
+ }
+
+ ret = parport_register_driver(&pps_gen_parport_driver);
+ if (ret) {
+ pr_err(DRVNAME ": unable to register with parport\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit pps_gen_parport_exit(void)
+{
+ parport_unregister_driver(&pps_gen_parport_driver);
+ pr_info(DRVNAME ": hrtimer avg error is %ldns\n", hrtimer_error);
+}
+
+module_init(pps_gen_parport_init);
+module_exit(pps_gen_parport_exit);
+
+MODULE_AUTHOR("Alexander Gordeev <[email protected]>");
+MODULE_DESCRIPTION(DRVDESC);
+MODULE_LICENSE("GPL");
--
1.7.1
On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> This commit adds hardpps() implementation based upon the original one
> from the NTPv4 reference kernel code from David Mills. However, it is
> highly optimized towards very fast syncronization and maximum stickness
> to PPS signal. The typical error is less then a microsecond.
> To make it sync faster I had to throw away exponential phase filter so
> that the full phase offset is corrected immediately. Then I also had to
> throw away median phase filter because it gives a bigger error itself
> if used without exponential filter.
> Maybe we will find an appropriate filtering scheme in the future but
> it's not necessary if the signal quality is ok.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
[snip]
> +#ifdef CONFIG_NTP_PPS
> +
> +struct pps_normtime {
> + __kernel_time_t sec; /* seconds */
> + long nsec; /* nanoseconds */
> +};
I don't quite remember the history here (it may be I suggested you use
this instead of overloading a timespec? I honestly don't recall), but
could you add some extra context in a comment here for what a
pps_normtime structure represents and why its used instead of a
timespec? The comment below sort of hints at it, but it would be useful
if it was more explicit.
> +/* normalize the timestamp so that nsec is in the
> + ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
> +static inline struct pps_normtime pps_normalize_ts(struct timespec ts)
> +{
> + struct pps_normtime norm = {
> + .sec = ts.tv_sec,
> + .nsec = ts.tv_nsec
> + };
> +
> + if (norm.nsec > (NSEC_PER_SEC >> 1)) {
> + norm.nsec -= NSEC_PER_SEC;
> + norm.sec++;
> + }
> +
> + return norm;
> +}
Otherwise the code looks pretty good to me.
Acked-by: John Stultz <[email protected]>
thanks
-john
On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> MONOTONIC_RAW clock timestamps are ideally suited for frequency
> calculation and also fit well into the original NTP hardpps design. Now
> phase and frequency can be adjusted separately: the former based on
> REALTIME clock and the latter based on MONOTONIC_RAW clock.
> A new function getnstime_raw_and_real is added to timekeeping subsystem
> to capture both timestamps at the same time and atomically.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> include/linux/pps_kernel.h | 3 ++-
> include/linux/time.h | 2 ++
> kernel/time/timekeeping.c | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 5af0498..39fc712 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -48,6 +48,7 @@ struct pps_source_info {
> };
>
> struct pps_event_time {
> + struct timespec ts_raw;
> struct timespec ts_real;
> };
>
> @@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
>
> static inline void pps_get_ts(struct pps_event_time *ts)
> {
> - getnstimeofday(&ts->ts_real);
> + getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
> }
>
> #endif /* LINUX_PPS_KERNEL_H */
> diff --git a/include/linux/time.h b/include/linux/time.h
> index ea3559f..1da8a7b 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -143,6 +143,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
> extern int do_getitimer(int which, struct itimerval *value);
> extern void getnstimeofday(struct timespec *tv);
> extern void getrawmonotonic(struct timespec *ts);
> +extern void getnstime_raw_and_real(struct timespec *ts_raw,
> + struct timespec *ts_real);
> extern void getboottime(struct timespec *ts);
> extern void monotonic_to_bootbased(struct timespec *ts);
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index caf8d4d..ef37b14 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> /**
> + * getnstime_raw_and_real - Returns both the time of day an raw
> + * monotonic time in a timespec format
> + * @ts_mono_raw: pointer to the timespec to be set to raw
> + * monotonic time
> + * @ts_real: pointer to the timespec to be set to the time
> + * of day
> + */
> +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> +{
> + unsigned long seq;
> + s64 nsecs_raw, nsecs_real;
> +
> + WARN_ON(timekeeping_suspended);
> +
> + do {
> + seq = read_seqbegin(&xtime_lock);
> +
> + *ts_raw = raw_time;
> + *ts_real = xtime;
> +
> + nsecs_raw = timekeeping_get_ns_raw();
> + nsecs_real = timekeeping_get_ns();
> +
> + /* If arch requires, add in gettimeoffset() */
> + nsecs_real += arch_gettimeoffset();
Hmm. So this brings up an interesting point. If the system is an one
which uses arch_gettimeoffset() (which means it doesn't have
clocksources), I'm not sure if this function would return what you'd
expect.
The issue is that jiffies is the basic clocksource that is in use on
these systems, but the arch_gettimeoffset() function returns an
inter-tick time value (usually by reading the interrupt generating
decrementer or whatnot). So in this case, you'd get a
CLOCK_MONOTONIC_RAW time that is based off the raw jiffies value at the
last tick, and CLOCK_REALTIME time value at the exact time of the call.
That offset between the two would then vary depending on when in the
inter-tick period the call was made.
The two solutions would be:
1) Add the arch_gettimeoffset() value to nsecs_raw as well. Should be
safe, as any freq variance from the raw freq will be limited to 1 tick
length and won't accumulate.
2) Don't add arch_gettimeoffset to either value, providing the raw and
real time values at the last tick.
I'd probably go with #1.
Otherwise, the code looks good.
thanks
-john
sigh. The amount of inlining which this patch does is nutty.
But I don't think I'll bother making a fuss over it.
On Thu, 5 Aug 2010 01:06:50 +0400
Alexander Gordeev <[email protected]> wrote:
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> /**
> + * getnstime_raw_and_real - Returns both the time of day an raw
> + * monotonic time in a timespec format
> + * @ts_mono_raw: pointer to the timespec to be set to raw
> + * monotonic time
> + * @ts_real: pointer to the timespec to be set to the time
> + * of day
> + */
> +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> +{
> + unsigned long seq;
> + s64 nsecs_raw, nsecs_real;
> +
> + WARN_ON(timekeeping_suspended);
I suspect that if this warning ever triggers, it'll trigger at some
high frequency making a complete mess all over the floor.
WARN_ON_ONCE, perhaps? Or just remove it?
On Thu, 5 Aug 2010 01:06:52 +0400
Alexander Gordeev <[email protected]> wrote:
> Add parallel port PPS client. It uses a standard method for capturing
> timestamps for assert edge transitions: getting a timestamp soon after
> an interrupt has happened. This is not a very precise source of time
> information due to interrupt handling delays. However, timestamps for
> clear edge transitions are much more precise because the interrupt
> handler continuously polls hardware port until the transition is done.
> Hardware port operations require only about 1us so the maximum error
> should not exceed this value. This was my primary goal when developing
> this client.
> Clear edge capture could be disabled using clear_wait parameter.
>
>
> ...
>
> +
> +#define SIGNAL_IS_SET(port) \
> + ((port->ops->read_status(port) & PARPORT_STATUS_ACK) != 0)
This could (and hence should) be implemented in a regular old C
function.
>
> ...
>
> +{
> + struct pps_client_pp *device;
> + struct pps_source_info info = {
> + .name = DRVNAME,
> + .path = "",
> + .mode = PPS_CAPTUREBOTH | \
> + PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> + PPS_ECHOASSERT | PPS_ECHOCLEAR | \
> + PPS_CANWAIT | PPS_TSFMT_TSPEC,
> + .echo = pps_echo,
> + .owner = THIS_MODULE,
> + .dev = NULL
> + };
> +
> + device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
> + if (!device) {
> + pr_err(DRVNAME ": memory allocation failed, not attaching\n");
> + return;
eww. parport_driver.attach() returns void so there's no way to
propagate the error code back. That sucks a bit.
>
> ...
>
On 08/04/2010 04:26 PM, Andrew Morton wrote:
> sigh. The amount of inlining which this patch does is nutty.
Well one could ask about the rationale behind it all and hope that it
wasn't purely gratuitous.
David Daney
>
> But I don't think I'll bother making a fuss over it.
On Thu, 5 Aug 2010 01:06:53 +0400
Alexander Gordeev <[email protected]> wrote:
> Add PPS signal generator which utilizes STROBE pin of a parallel port to
> send PPS signals. It uses parport abstraction layer and hrtimers to
> precisely control the signal.
>
> ..
>
> +static unsigned int send_delay = 30000;
> +MODULE_PARM_DESC(delay,
> + "Delay between setting and dropping the signal (ns)");
> +module_param_named(delay, send_delay, uint, 0);
This code doesn't look easy to configure and use. For example, if some
random person tries to get it going, how does he even know that this
module parameter exists, let alone how to use it?
Please review Documentation/pps/pps.txt and check that it accurately
and completely describes how to use the pps code after your changes,
thanks.
On Wed, 04 Aug 2010 16:39:13 -0700
David Daney <[email protected]> wrote:
> On 08/04/2010 04:26 PM, Andrew Morton wrote:
> > sigh. The amount of inlining which this patch does is nutty.
>
> Well one could ask about the rationale behind it all and hope that it
> wasn't purely gratuitous.
>
Rationale needs rethinking, because removing every one of those inlines
makes no change at all to the generated code.
On Thu, Aug 05, 2010 at 01:06:40AM +0400, Alexander Gordeev wrote:
> There was a race in PPS_FETCH ioctl handler when several processes want
> to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> sleep most of the time in the system call.
> With the old approach when the first process waiting on the pps queue
> is waken up it makes new system call right away and zeroes pps->go. So
> other processes continue to sleep. This is a clear race condition
> because of the global 'go' variable.
> With the new approach pps->last_ev holds some value increasing at each
> PPS event. PPS_FETCH ioctl handler saves current value to the local
> variable at the very beginning so it can safely check that there is a
> new event by just comparing both variables.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 4 ++--
> drivers/pps/pps.c | 10 +++++++---
> include/linux/pps_kernel.h | 2 +-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 55f3961..3f89f5e 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
>
> /* Wake up if captured something */
> if (captured) {
> - pps->go = ~0;
> - wake_up_interruptible(&pps->queue);
> + pps->last_ev++;
> + wake_up_interruptible_all(&pps->queue);
What happens if pps->last_ev overflows? Seems to me it would freeze
pps.
Cheers,
Vita
On Thu, Aug 05, 2010 at 01:06:39AM +0400, Alexander Gordeev wrote:
> Move variable declarations where they are used in pps_cdev_ioctl.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/pps.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index ca5183b..c76afb9 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -61,8 +61,6 @@ static long pps_cdev_ioctl(struct file *file,
> {
> struct pps_device *pps = file->private_data;
> struct pps_kparams params;
> - struct pps_fdata fdata;
> - unsigned long ticks;
> void __user *uarg = (void __user *) arg;
> int __user *iuarg = (int __user *) arg;
> int err;
> @@ -136,7 +134,9 @@ static long pps_cdev_ioctl(struct file *file,
>
> break;
>
> - case PPS_FETCH:
> + case PPS_FETCH: {
> + struct pps_fdata fdata;
> +
> pr_debug("PPS_FETCH: source %d\n", pps->id);
>
> err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
> @@ -149,6 +149,8 @@ static long pps_cdev_ioctl(struct file *file,
> if (fdata.timeout.flags & PPS_TIME_INVALID)
> err = wait_event_interruptible(pps->queue, pps->go);
> else {
> + unsigned long ticks;
> +
> pr_debug("timeout %lld.%09d\n",
> (long long) fdata.timeout.sec,
> fdata.timeout.nsec);
> @@ -185,7 +187,7 @@ static long pps_cdev_ioctl(struct file *file,
> return -EFAULT;
>
> break;
> -
> + }
> default:
> return -ENOTTY;
> break;
> --
> 1.7.1
I don't like such definitions but I know it is just a stylistic
problem, so it's ok for me. :)
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Aug 05, 2010 at 01:06:38AM +0400, Alexander Gordeev wrote:
> Here are some very trivial fixes combined:
> * add macro definitions to protect header file from including several times
> * remove declaration for an unexistent array
> * fix typos
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 2 +-
> include/linux/pps_kernel.h | 7 ++++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 1aa02db..55f3961 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -324,7 +324,7 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
> captured = ~0;
> }
>
> - /* Wake up iif captured somthing */
> + /* Wake up if captured something */
> if (captured) {
> pps->go = ~0;
> wake_up_interruptible(&pps->queue);
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index e0a193f..c930d11 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -18,6 +18,9 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#ifndef LINUX_PPS_KERNEL_H
> +#define LINUX_PPS_KERNEL_H
> +
> #include <linux/pps.h>
>
> #include <linux/cdev.h>
> @@ -71,7 +74,6 @@ struct pps_device {
>
> extern spinlock_t pps_idr_lock;
> extern struct idr pps_idr;
> -extern struct timespec pps_irq_ts[];
>
> extern struct device_attribute pps_attrs[];
>
> @@ -87,3 +89,6 @@ extern void pps_unregister_source(int source);
> extern int pps_register_cdev(struct pps_device *pps);
> extern void pps_unregister_cdev(struct pps_device *pps);
> extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
> +
> +#endif /* LINUX_PPS_KERNEL_H */
> +
> --
> 1.7.1
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Aug 05, 2010 at 01:06:40AM +0400, Alexander Gordeev wrote:
> There was a race in PPS_FETCH ioctl handler when several processes want
> to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> sleep most of the time in the system call.
> With the old approach when the first process waiting on the pps queue
> is waken up it makes new system call right away and zeroes pps->go. So
> other processes continue to sleep. This is a clear race condition
> because of the global 'go' variable.
> With the new approach pps->last_ev holds some value increasing at each
> PPS event. PPS_FETCH ioctl handler saves current value to the local
> variable at the very beginning so it can safely check that there is a
> new event by just comparing both variables.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 4 ++--
> drivers/pps/pps.c | 10 +++++++---
> include/linux/pps_kernel.h | 2 +-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 55f3961..3f89f5e 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
>
> /* Wake up if captured something */
> if (captured) {
> - pps->go = ~0;
> - wake_up_interruptible(&pps->queue);
> + pps->last_ev++;
> + wake_up_interruptible_all(&pps->queue);
>
> kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
> }
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index c76afb9..cb24147 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -136,6 +136,7 @@ static long pps_cdev_ioctl(struct file *file,
>
> case PPS_FETCH: {
> struct pps_fdata fdata;
> + unsigned int ev;
>
> pr_debug("PPS_FETCH: source %d\n", pps->id);
>
> @@ -143,11 +144,12 @@ static long pps_cdev_ioctl(struct file *file,
> if (err)
> return -EFAULT;
>
> - pps->go = 0;
> + ev = pps->last_ev;
>
> /* Manage the timeout */
> if (fdata.timeout.flags & PPS_TIME_INVALID)
> - err = wait_event_interruptible(pps->queue, pps->go);
> + err = wait_event_interruptible(pps->queue,
> + ev < pps->last_ev);
> else {
> unsigned long ticks;
>
> @@ -159,7 +161,9 @@ static long pps_cdev_ioctl(struct file *file,
>
> if (ticks != 0) {
> err = wait_event_interruptible_timeout(
> - pps->queue, pps->go, ticks);
> + pps->queue,
> + ev < pps->last_ev,
> + ticks);
> if (err == 0)
> return -ETIMEDOUT;
> }
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index c930d11..c3aed4b 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -55,7 +55,7 @@ struct pps_device {
> struct pps_ktime clear_tu;
> int current_mode; /* PPS mode at event time */
>
> - int go; /* PPS event is arrived? */
> + unsigned int last_ev; /* last PPS event id */
> wait_queue_head_t queue; /* PPS event queue */
>
> unsigned int id; /* PPS source unique ID */
> --
> 1.7.1
Yes. It was added when we passed from ioctl to unlocked_ioctl...
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Aug 05, 2010 at 01:06:41AM +0400, Alexander Gordeev wrote:
> Add a helper function to gather timestamps. This way clients don't have
> to duplicate it.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/clients/pps-ktimer.c | 9 ++-------
> drivers/pps/clients/pps-ldisc.c | 18 ++++++------------
> drivers/pps/kapi.c | 19 ++++++++++++-------
> include/linux/pps_kernel.h | 19 ++++++++++++++++++-
> include/linux/serial_core.h | 5 +++--
> include/linux/tty_ldisc.h | 5 +++--
> 6 files changed, 44 insertions(+), 31 deletions(-)
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Aug 05, 2010 at 01:06:42AM +0400, Alexander Gordeev wrote:
> Using device index as a pointer needs some unnecessary work to be done
> every time the pointer is needed (in irq handler for example).
> Using a direct pointer is much more easy (and safe as well).
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/clients/pps-ktimer.c | 30 ++++-----
> drivers/pps/clients/pps-ldisc.c | 33 +++++-----
> drivers/pps/kapi.c | 124 ++++++++-----------------------------
> drivers/pps/pps.c | 22 ++-----
> include/linux/pps_kernel.h | 21 +++----
> 5 files changed, 70 insertions(+), 160 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> index e1bdd8b..64cba1d 100644
> --- a/drivers/pps/clients/pps-ktimer.c
> +++ b/drivers/pps/clients/pps-ktimer.c
> @@ -31,7 +31,7 @@
> * Global variables
> */
>
> -static int source;
> +static struct pps_device *pps;;
> static struct timer_list ktimer;
>
> /*
> @@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
>
> pr_info("PPS event at %lu\n", jiffies);
>
> - pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
> + pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
>
> mod_timer(&ktimer, jiffies + HZ);
> }
> @@ -56,12 +56,11 @@ static void pps_ktimer_event(unsigned long ptr)
> * The echo function
> */
>
> -static void pps_ktimer_echo(int source, int event, void *data)
> +static void pps_ktimer_echo(struct pps_device *pps, int event, void *data)
> {
> - pr_info("echo %s %s for source %d\n",
> + dev_info(pps->dev, "echo %s %s\n",
> event & PPS_CAPTUREASSERT ? "assert" : "",
> - event & PPS_CAPTURECLEAR ? "clear" : "",
> - source);
> + event & PPS_CAPTURECLEAR ? "clear" : "");
> }
>
> /*
> @@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = {
>
> static void __exit pps_ktimer_exit(void)
> {
> - del_timer_sync(&ktimer);
> - pps_unregister_source(source);
> + dev_info(pps->dev, "ktimer PPS source unregistered\n");
>
> - pr_info("ktimer PPS source unregistered\n");
> + del_timer_sync(&ktimer);
> + pps_unregister_source(pps);
> }
>
> static int __init pps_ktimer_init(void)
> {
> - int ret;
> -
> - ret = pps_register_source(&pps_ktimer_info,
> + pps = pps_register_source(&pps_ktimer_info,
> PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> - if (ret < 0) {
> + if (pps == NULL) {
> printk(KERN_ERR "cannot register ktimer source\n");
> - return ret;
> + return -ENOMEM;
> }
> - source = ret;
>
> setup_timer(&ktimer, pps_ktimer_event, 0);
> mod_timer(&ktimer, jiffies + HZ);
>
> - pr_info("ktimer PPS source registered at %d\n", source);
> + dev_info(pps->dev, "ktimer PPS source registered\n");
>
> - return 0;
> + return 0;
> }
>
> module_init(pps_ktimer_init);
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index 20fc9f7..d7e1a27 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -29,7 +29,7 @@
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> struct pps_event_time *ts)
> {
> - int id = (long)tty->disc_data;
> + struct pps_device *pps = (struct pps_device *)tty->disc_data;
> struct pps_event_time __ts;
>
> /* First of all we get the time stamp... */
> @@ -40,11 +40,11 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> ts = &__ts;
>
> /* Now do the PPS event report */
> - pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> + pps_event(pps, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> NULL);
>
> - pr_debug("PPS %s at %lu on source #%d\n",
> - status ? "assert" : "clear", jiffies, id);
> + dev_dbg(pps->dev, "PPS %s at %lu\n", status ? "assert" : "clear",
> + jiffies);
> }
>
> static int (*alias_n_tty_open)(struct tty_struct *tty);
> @@ -54,7 +54,7 @@ static int pps_tty_open(struct tty_struct *tty)
> struct pps_source_info info;
> struct tty_driver *drv = tty->driver;
> int index = tty->index + drv->name_base;
> - int ret;
> + struct pps_device *pps;
>
> info.owner = THIS_MODULE;
> info.dev = NULL;
> @@ -64,20 +64,19 @@ static int pps_tty_open(struct tty_struct *tty)
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> PPS_CANWAIT | PPS_TSFMT_TSPEC;
>
> - ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
> + pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> - if (ret < 0) {
> + if (pps == NULL) {
> pr_err("cannot register PPS source \"%s\"\n", info.path);
> - return ret;
> + return -ENOMEM;
> }
> - tty->disc_data = (void *)(long)ret;
> + tty->disc_data = pps;
>
> /* Should open N_TTY ldisc too */
> - ret = alias_n_tty_open(tty);
> - if (ret < 0)
> - pps_unregister_source((long)tty->disc_data);
> + if (alias_n_tty_open(tty) < 0)
> + pps_unregister_source(pps);
>
> - pr_info("PPS source #%d \"%s\" added\n", ret, info.path);
> + dev_info(pps->dev, "source \"%s\" added\n", info.path);
>
> return 0;
> }
> @@ -86,12 +85,12 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
>
> static void pps_tty_close(struct tty_struct *tty)
> {
> - int id = (long)tty->disc_data;
> + struct pps_device *pps = (struct pps_device *)tty->disc_data;
>
> - pps_unregister_source(id);
> - alias_n_tty_close(tty);
> + dev_info(pps->dev, "removed\n");
>
> - pr_info("PPS source #%d removed\n", id);
> + pps_unregister_source(pps);
> + alias_n_tty_close(tty);
> }
>
> static struct tty_ldisc_ops pps_ldisc_ops;
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index b431d83..568223b 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -32,11 +32,11 @@
> #include <linux/slab.h>
>
> /*
> - * Global variables
> + * Local variables
> */
>
> -DEFINE_SPINLOCK(pps_idr_lock);
> -DEFINE_IDR(pps_idr);
> +static DEFINE_SPINLOCK(pps_idr_lock);
> +static DEFINE_IDR(pps_idr);
>
> /*
> * Local functions
> @@ -60,60 +60,6 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
> * Exported functions
> */
>
> -/* pps_get_source - find a PPS source
> - * @source: the PPS source ID.
> - *
> - * This function is used to find an already registered PPS source into the
> - * system.
> - *
> - * The function returns NULL if found nothing, otherwise it returns a pointer
> - * to the PPS source data struct (the refcounter is incremented by 1).
> - */
> -
> -struct pps_device *pps_get_source(int source)
> -{
> - struct pps_device *pps;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&pps_idr_lock, flags);
> -
> - pps = idr_find(&pps_idr, source);
> - if (pps != NULL)
> - atomic_inc(&pps->usage);
> -
> - spin_unlock_irqrestore(&pps_idr_lock, flags);
> -
> - return pps;
> -}
> -
> -/* pps_put_source - free the PPS source data
> - * @pps: a pointer to the PPS source.
> - *
> - * This function is used to free a PPS data struct if its refcount is 0.
> - */
> -
> -void pps_put_source(struct pps_device *pps)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&pps_idr_lock, flags);
> - BUG_ON(atomic_read(&pps->usage) == 0);
> -
> - if (!atomic_dec_and_test(&pps->usage)) {
> - pps = NULL;
> - goto exit;
> - }
> -
> - /* No more reference to the PPS source. We can safely remove the
> - * PPS data struct.
> - */
> - idr_remove(&pps_idr, pps->id);
> -
> -exit:
> - spin_unlock_irqrestore(&pps_idr_lock, flags);
> - kfree(pps);
> -}
If you remove these functions you can't be sure anymore that nobodies
may call pps_event() over a non existent device...
> /* pps_register_source - add a PPS source in the system
> * @info: the PPS info struct
> * @default_params: the default PPS parameters of the new source
> @@ -122,10 +68,11 @@ exit:
> * source is described by info's fields and it will have, as default PPS
> * parameters, the ones specified into default_params.
> *
> - * The function returns, in case of success, the PPS source ID.
> + * The function returns, in case of success, the PPS device. Otherwise NULL.
> */
>
> -int pps_register_source(struct pps_source_info *info, int default_params)
> +struct pps_device *pps_register_source(struct pps_source_info *info,
> + int default_params)
> {
> struct pps_device *pps;
> int id;
> @@ -168,7 +115,6 @@ int pps_register_source(struct pps_source_info *info, int default_params)
>
> init_waitqueue_head(&pps->queue);
> spin_lock_init(&pps->lock);
> - atomic_set(&pps->usage, 1);
>
> /* Get new ID for the new PPS source */
> if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> @@ -211,7 +157,7 @@ int pps_register_source(struct pps_source_info *info, int default_params)
>
> pr_info("new PPS source %s at ID %d\n", info->name, id);
>
> - return id;
> + return pps;
>
> free_idr:
> spin_lock_irq(&pps_idr_lock);
> @@ -224,38 +170,31 @@ kfree_pps:
> pps_register_source_exit:
> printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
>
> - return err;
> + return NULL;
> }
> EXPORT_SYMBOL(pps_register_source);
>
> /* pps_unregister_source - remove a PPS source from the system
> - * @source: the PPS source ID
> + * @pps: the PPS source
> *
> * This function is used to remove a previously registered PPS source from
> * the system.
> */
>
> -void pps_unregister_source(int source)
> +void pps_unregister_source(struct pps_device *pps)
> {
> - struct pps_device *pps;
> + unsigned int id = pps->id;
>
> + pps_unregister_cdev(pps);
> +
> spin_lock_irq(&pps_idr_lock);
> - pps = idr_find(&pps_idr, source);
> -
> - if (!pps) {
> - BUG();
> - spin_unlock_irq(&pps_idr_lock);
> - return;
> - }
> + idr_remove(&pps_idr, pps->id);
> spin_unlock_irq(&pps_idr_lock);
> -
> - pps_unregister_cdev(pps);
> - pps_put_source(pps);
> }
> EXPORT_SYMBOL(pps_unregister_source);
>
> /* pps_event - register a PPS event into the system
> - * @source: the PPS source ID
> + * @pps: the PPS device
> * @ts: the event timestamp
> * @event: the event type
> * @data: userdef pointer
> @@ -263,30 +202,24 @@ EXPORT_SYMBOL(pps_unregister_source);
> * This function is used by each PPS client in order to register a new
> * PPS event into the system (it's usually called inside an IRQ handler).
> *
> - * If an echo function is associated with the PPS source it will be called
> + * If an echo function is associated with the PPS device it will be called
> * as:
> - * pps->info.echo(source, event, data);
> + * pps->info.echo(pps, event, data);
> */
> -
> -void pps_event(int source, struct pps_event_time *ts, int event, void *data)
> +void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> + void *data)
> {
> - struct pps_device *pps;
> unsigned long flags;
> int captured = 0;
> struct pps_ktime ts_real;
>
> if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> - printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
> - event, source);
> + dev_err(pps->dev, "unknown event (%x)\n", event);
> return;
> }
>
> - pps = pps_get_source(source);
> - if (!pps)
> - return;
> -
> - pr_debug("PPS event on source %d at %ld.%09ld\n",
> - pps->id, ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
> + dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
> + ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
By dropping pps_get_source you may be here by a call from (i.e.) a
serial port driver whose doesn't know if your PPS source is gone or
not...
I don't understand how your modifications may resolve this problem.
> timespec_to_pps_ktime(&ts_real, ts->ts_real);
>
> @@ -294,7 +227,7 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
>
> /* Must call the echo function? */
> if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
> - pps->info.echo(source, event, data);
> + pps->info.echo(pps, event, data);
>
> /* Check the event */
> pps->current_mode = pps->params.mode;
> @@ -308,8 +241,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
> /* Save the time stamp */
> pps->assert_tu = ts_real;
> pps->assert_sequence++;
> - pr_debug("capture assert seq #%u for source %d\n",
> - pps->assert_sequence, source);
> + dev_dbg(pps->dev, "capture assert seq #%u\n",
> + pps->assert_sequence);
>
> captured = ~0;
> }
> @@ -323,8 +256,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
> /* Save the time stamp */
> pps->clear_tu = ts_real;
> pps->clear_sequence++;
> - pr_debug("capture clear seq #%u for source %d\n",
> - pps->clear_sequence, source);
> + dev_dbg(pps->dev, "capture clear seq #%u\n",
> + pps->clear_sequence);
>
> captured = ~0;
> }
> @@ -338,8 +271,5 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
> }
>
> spin_unlock_irqrestore(&pps->lock, flags);
> -
> - /* Now we can release the PPS source for (possible) deregistration */
> - pps_put_source(pps);
> }
> EXPORT_SYMBOL(pps_event);
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index cb24147..89f478b 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -204,12 +204,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
> {
> struct pps_device *pps = container_of(inode->i_cdev,
> struct pps_device, cdev);
> - int found;
> -
> - found = pps_get_source(pps->id) != 0;
> - if (!found)
> - return -ENODEV;
> -
> file->private_data = pps;
>
> return 0;
> @@ -217,11 +211,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
>
> static int pps_cdev_release(struct inode *inode, struct file *file)
> {
> - struct pps_device *pps = file->private_data;
> -
> - /* Free the PPS source and wake up (possible) deregistration */
> - pps_put_source(pps);
> -
> return 0;
> }
>
> @@ -242,22 +231,23 @@ static const struct file_operations pps_cdev_fops = {
> int pps_register_cdev(struct pps_device *pps)
> {
> int err;
> + dev_t devt;
> +
> + devt = MKDEV(MAJOR(pps_devt), pps->id);
>
> - pps->devno = MKDEV(MAJOR(pps_devt), pps->id);
> cdev_init(&pps->cdev, &pps_cdev_fops);
> pps->cdev.owner = pps->info.owner;
>
> - err = cdev_add(&pps->cdev, pps->devno, 1);
> + err = cdev_add(&pps->cdev, devt, 1);
> if (err) {
> printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
> pps->info.name, MAJOR(pps_devt), pps->id);
> return err;
> }
> - pps->dev = device_create(pps_class, pps->info.dev, pps->devno, NULL,
> + pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
> "pps%d", pps->id);
> if (IS_ERR(pps->dev))
> goto del_cdev;
> - dev_set_drvdata(pps->dev, pps);
>
> pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
> MAJOR(pps_devt), pps->id);
> @@ -272,7 +262,7 @@ del_cdev:
>
> void pps_unregister_cdev(struct pps_device *pps)
> {
> - device_destroy(pps_class, pps->devno);
> + device_destroy(pps_class, pps->dev->devt);
> cdev_del(&pps->cdev);
> }
>
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 32aa676..1e0f249 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -31,13 +31,16 @@
> * Global defines
> */
>
> +struct pps_device;
> +
> /* The specific PPS source info */
> struct pps_source_info {
> char name[PPS_MAX_NAME_LEN]; /* simbolic name */
> char path[PPS_MAX_NAME_LEN]; /* path of connected device */
> int mode; /* PPS's allowed mode */
>
> - void (*echo)(int source, int event, void *data); /* PPS echo function */
> + void (*echo)(struct pps_device *pps,
> + int event, void *data); /* PPS echo function */
>
> struct module *owner;
> struct device *dev;
> @@ -65,34 +68,26 @@ struct pps_device {
> unsigned int id; /* PPS source unique ID */
> struct cdev cdev;
> struct device *dev;
> - int devno;
> struct fasync_struct *async_queue; /* fasync method */
> spinlock_t lock;
> -
> - atomic_t usage; /* usage count */
> };
>
> /*
> * Global variables
> */
>
> -extern spinlock_t pps_idr_lock;
> -extern struct idr pps_idr;
> -
> extern struct device_attribute pps_attrs[];
>
> /*
> * Exported functions
> */
>
> -struct pps_device *pps_get_source(int source);
> -extern void pps_put_source(struct pps_device *pps);
> -extern int pps_register_source(struct pps_source_info *info,
> - int default_params);
> -extern void pps_unregister_source(int source);
> +extern struct pps_device *pps_register_source(
> + struct pps_source_info *info, int default_params);
> +extern void pps_unregister_source(struct pps_device *pps);
> extern int pps_register_cdev(struct pps_device *pps);
> extern void pps_unregister_cdev(struct pps_device *pps);
> -extern void pps_event(int source, struct pps_event_time *ts, int event,
> +extern void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> void *data);
>
> static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
> --
> 1.7.1
>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
Hi Vitezslav,
В Thu, 5 Aug 2010 07:19:29 +0200
Vitezslav Samel <[email protected]> пишет:
> On Thu, Aug 05, 2010 at 01:06:40AM +0400, Alexander Gordeev wrote:
> > There was a race in PPS_FETCH ioctl handler when several processes want
> > to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> > sleep most of the time in the system call.
> > With the old approach when the first process waiting on the pps queue
> > is waken up it makes new system call right away and zeroes pps->go. So
> > other processes continue to sleep. This is a clear race condition
> > because of the global 'go' variable.
> > With the new approach pps->last_ev holds some value increasing at each
> > PPS event. PPS_FETCH ioctl handler saves current value to the local
> > variable at the very beginning so it can safely check that there is a
> > new event by just comparing both variables.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/pps/kapi.c | 4 ++--
> > drivers/pps/pps.c | 10 +++++++---
> > include/linux/pps_kernel.h | 2 +-
> > 3 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > index 55f3961..3f89f5e 100644
> > --- a/drivers/pps/kapi.c
> > +++ b/drivers/pps/kapi.c
> > @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
> >
> > /* Wake up if captured something */
> > if (captured) {
> > - pps->go = ~0;
> > - wake_up_interruptible(&pps->queue);
> > + pps->last_ev++;
> > + wake_up_interruptible_all(&pps->queue);
>
> What happens if pps->last_ev overflows? Seems to me it would freeze
> pps.
Yes, it will freeze the fds (if they don't use timeouts). But in normal
circumstances, i.e. when pps_event is called twice a second, it will
overflow after ~68 years of uninterrupted work. Well, it's the same
kind of problem as an overflow of struct timespec. I thought it's not
actually a problem. Should I use u64 instead of unsigned int or add a
runtime check somewhere?
--
Alexander
On Thu, Aug 05, 2010 at 02:19:51PM +0400, Alexander Gordeev wrote:
> Hi Vitezslav,
>
> В Thu, 5 Aug 2010 07:19:29 +0200
> Vitezslav Samel <[email protected]> пишет:
>
> > On Thu, Aug 05, 2010 at 01:06:40AM +0400, Alexander Gordeev wrote:
> > > There was a race in PPS_FETCH ioctl handler when several processes want
> > > to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> > > sleep most of the time in the system call.
> > > With the old approach when the first process waiting on the pps queue
> > > is waken up it makes new system call right away and zeroes pps->go. So
> > > other processes continue to sleep. This is a clear race condition
> > > because of the global 'go' variable.
> > > With the new approach pps->last_ev holds some value increasing at each
> > > PPS event. PPS_FETCH ioctl handler saves current value to the local
> > > variable at the very beginning so it can safely check that there is a
> > > new event by just comparing both variables.
> > >
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> > > ---
> > > drivers/pps/kapi.c | 4 ++--
> > > drivers/pps/pps.c | 10 +++++++---
> > > include/linux/pps_kernel.h | 2 +-
> > > 3 files changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > index 55f3961..3f89f5e 100644
> > > --- a/drivers/pps/kapi.c
> > > +++ b/drivers/pps/kapi.c
> > > @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
> > >
> > > /* Wake up if captured something */
> > > if (captured) {
> > > - pps->go = ~0;
> > > - wake_up_interruptible(&pps->queue);
> > > + pps->last_ev++;
> > > + wake_up_interruptible_all(&pps->queue);
> >
> > What happens if pps->last_ev overflows? Seems to me it would freeze
> > pps.
>
> Yes, it will freeze the fds (if they don't use timeouts). But in normal
> circumstances, i.e. when pps_event is called twice a second, it will
> overflow after ~68 years of uninterrupted work. Well, it's the same
> kind of problem as an overflow of struct timespec. I thought it's not
> actually a problem. Should I use u64 instead of unsigned int or add a
> runtime check somewhere?
If we're using 1PPS it's ~68 years, but someone is trying 5PPS now
(it would overflow in ~13.6 years) - what if someone tries e.g. 100PPS?
It's not the same as overflow of struct timespec! I think it deserves
some treatment.
Cheers,
Vita
В Thu, 5 Aug 2010 11:32:36 +0200
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Aug 05, 2010 at 01:06:42AM +0400, Alexander Gordeev wrote:
> > Using device index as a pointer needs some unnecessary work to be done
> > every time the pointer is needed (in irq handler for example).
> > Using a direct pointer is much more easy (and safe as well).
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
[snip]
>
> If you remove these functions you can't be sure anymore that nobodies
> may call pps_event() over a non existent device...
[snip]
> By dropping pps_get_source you may be here by a call from (i.e.) a
> serial port driver whose doesn't know if your PPS source is gone or
> not...
>
> I don't understand how your modifications may resolve this problem.
Well, this can happen only if PPS client module calls pps_event before
calling pps_register_source() or after pps_unregister_source(). This
means that it's broken! If we try to handle/workaround broken clients it
affects performance. So we have to choose what is the priority:
security or performance. My guru told me I shouldn't bother too much
about broken kernel-space code which my code interacts with. If it's
broken it should be fixed. Some assertions enabled by DEBUG define are
enough. For me it makes sense but I don't know what should I check here?
Well I can add something like that to pps_event:
BUG_ON((pps == NULL) || (pps_get_source(pps->id) != pps));
where pps_get_source is:
static inline struct pps_device *pps_get_source(int source)
{
struct pps_device *pps;
unsigned long flags;
spin_lock_irqsave(&pps_idr_lock, flags);
pps = idr_find(&pps_idr, source);
spin_unlock_irqrestore(&pps_idr_lock, flags);
return pps;
}
BTW, while looking at the code to answer your question I've found a
bug: struct pps_device was not kfree'd on device destruction. The fix
will appear soon. Perhaps with an assertion above if you like it.
--
Alexander
В Wed, 04 Aug 2010 15:49:47 -0700
john stultz <[email protected]> пишет:
> On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> > This commit adds hardpps() implementation based upon the original one
> > from the NTPv4 reference kernel code from David Mills. However, it is
> > highly optimized towards very fast syncronization and maximum stickness
> > to PPS signal. The typical error is less then a microsecond.
> > To make it sync faster I had to throw away exponential phase filter so
> > that the full phase offset is corrected immediately. Then I also had to
> > throw away median phase filter because it gives a bigger error itself
> > if used without exponential filter.
> > Maybe we will find an appropriate filtering scheme in the future but
> > it's not necessary if the signal quality is ok.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
>
> [snip]
>
> > +#ifdef CONFIG_NTP_PPS
> > +
> > +struct pps_normtime {
> > + __kernel_time_t sec; /* seconds */
> > + long nsec; /* nanoseconds */
> > +};
>
> I don't quite remember the history here (it may be I suggested you use
> this instead of overloading a timespec? I honestly don't recall), but
> could you add some extra context in a comment here for what a
> pps_normtime structure represents and why its used instead of a
> timespec? The comment below sort of hints at it, but it would be useful
> if it was more explicit.
Yes, you asked me to do this. :)
Sure, I'll add an explicit comment.
> > +/* normalize the timestamp so that nsec is in the
> > + ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
> > +static inline struct pps_normtime pps_normalize_ts(struct timespec ts)
> > +{
> > + struct pps_normtime norm = {
> > + .sec = ts.tv_sec,
> > + .nsec = ts.tv_nsec
> > + };
> > +
> > + if (norm.nsec > (NSEC_PER_SEC >> 1)) {
> > + norm.nsec -= NSEC_PER_SEC;
> > + norm.sec++;
> > + }
> > +
> > + return norm;
> > +}
>
> Otherwise the code looks pretty good to me.
>
> Acked-by: John Stultz <[email protected]>
Thanks!
--
Alexander
В Wed, 04 Aug 2010 16:03:57 -0700
john stultz <[email protected]> пишет:
> On Thu, 2010-08-05 at 01:06 +0400, Alexander Gordeev wrote:
> > MONOTONIC_RAW clock timestamps are ideally suited for frequency
> > calculation and also fit well into the original NTP hardpps design. Now
> > phase and frequency can be adjusted separately: the former based on
> > REALTIME clock and the latter based on MONOTONIC_RAW clock.
> > A new function getnstime_raw_and_real is added to timekeeping subsystem
> > to capture both timestamps at the same time and atomically.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > include/linux/pps_kernel.h | 3 ++-
> > include/linux/time.h | 2 ++
> > kernel/time/timekeeping.c | 34 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 38 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> > index 5af0498..39fc712 100644
> > --- a/include/linux/pps_kernel.h
> > +++ b/include/linux/pps_kernel.h
> > @@ -48,6 +48,7 @@ struct pps_source_info {
> > };
> >
> > struct pps_event_time {
> > + struct timespec ts_raw;
> > struct timespec ts_real;
> > };
> >
> > @@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
> >
> > static inline void pps_get_ts(struct pps_event_time *ts)
> > {
> > - getnstimeofday(&ts->ts_real);
> > + getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
> > }
> >
> > #endif /* LINUX_PPS_KERNEL_H */
> > diff --git a/include/linux/time.h b/include/linux/time.h
> > index ea3559f..1da8a7b 100644
> > --- a/include/linux/time.h
> > +++ b/include/linux/time.h
> > @@ -143,6 +143,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
> > extern int do_getitimer(int which, struct itimerval *value);
> > extern void getnstimeofday(struct timespec *tv);
> > extern void getrawmonotonic(struct timespec *ts);
> > +extern void getnstime_raw_and_real(struct timespec *ts_raw,
> > + struct timespec *ts_real);
> > extern void getboottime(struct timespec *ts);
> > extern void monotonic_to_bootbased(struct timespec *ts);
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index caf8d4d..ef37b14 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> > EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > /**
> > + * getnstime_raw_and_real - Returns both the time of day an raw
> > + * monotonic time in a timespec format
> > + * @ts_mono_raw: pointer to the timespec to be set to raw
> > + * monotonic time
> > + * @ts_real: pointer to the timespec to be set to the time
> > + * of day
> > + */
> > +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> > +{
> > + unsigned long seq;
> > + s64 nsecs_raw, nsecs_real;
> > +
> > + WARN_ON(timekeeping_suspended);
> > +
> > + do {
> > + seq = read_seqbegin(&xtime_lock);
> > +
> > + *ts_raw = raw_time;
> > + *ts_real = xtime;
> > +
> > + nsecs_raw = timekeeping_get_ns_raw();
> > + nsecs_real = timekeeping_get_ns();
> > +
> > + /* If arch requires, add in gettimeoffset() */
> > + nsecs_real += arch_gettimeoffset();
>
> Hmm. So this brings up an interesting point. If the system is an one
> which uses arch_gettimeoffset() (which means it doesn't have
> clocksources), I'm not sure if this function would return what you'd
> expect.
>
> The issue is that jiffies is the basic clocksource that is in use on
> these systems, but the arch_gettimeoffset() function returns an
> inter-tick time value (usually by reading the interrupt generating
> decrementer or whatnot). So in this case, you'd get a
> CLOCK_MONOTONIC_RAW time that is based off the raw jiffies value at the
> last tick, and CLOCK_REALTIME time value at the exact time of the call.
>
> That offset between the two would then vary depending on when in the
> inter-tick period the call was made.
>
>
> The two solutions would be:
> 1) Add the arch_gettimeoffset() value to nsecs_raw as well. Should be
> safe, as any freq variance from the raw freq will be limited to 1 tick
> length and won't accumulate.
>
> 2) Don't add arch_gettimeoffset to either value, providing the raw and
> real time values at the last tick.
>
> I'd probably go with #1.
>
> Otherwise, the code looks good.
Ok, thanks for the point! I'll use #1 then.
--
Alexander
В Wed, 4 Aug 2010 16:29:07 -0700
Andrew Morton <[email protected]> пишет:
> On Thu, 5 Aug 2010 01:06:50 +0400
> Alexander Gordeev <[email protected]> wrote:
>
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -286,6 +286,40 @@ void ktime_get_ts(struct timespec *ts)
> > EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > /**
> > + * getnstime_raw_and_real - Returns both the time of day an raw
> > + * monotonic time in a timespec format
> > + * @ts_mono_raw: pointer to the timespec to be set to raw
> > + * monotonic time
> > + * @ts_real: pointer to the timespec to be set to the time
> > + * of day
> > + */
> > +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> > +{
> > + unsigned long seq;
> > + s64 nsecs_raw, nsecs_real;
> > +
> > + WARN_ON(timekeeping_suspended);
>
> I suspect that if this warning ever triggers, it'll trigger at some
> high frequency making a complete mess all over the floor.
>
> WARN_ON_ONCE, perhaps? Or just remove it?
Well, getnstime_raw_and_real() is actually just a merge of
getnstimeofday() and getrawmonotonic(). The warning came from
getnstimeofday(). Usually this code should be called once a second but
the frequency can be higher. IMHO other functions like getnstimeofday()
and ktime_get_ts() would be a bigger problem anyway because they have
the same checks.
So I'm ok with either choice.
Hmm, will use WARN_ON_ONCE then if nobody objects.
--
Alexander
On Thu, Aug 05, 2010 at 03:42:31PM +0400, Alexander Gordeev wrote:
> ?? Thu, 5 Aug 2010 11:32:36 +0200
> Rodolfo Giometti <[email protected]> ??????????:
>
> > On Thu, Aug 05, 2010 at 01:06:42AM +0400, Alexander Gordeev wrote:
> > > Using device index as a pointer needs some unnecessary work to be done
> > > every time the pointer is needed (in irq handler for example).
> > > Using a direct pointer is much more easy (and safe as well).
> > >
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> [snip]
> >
> > If you remove these functions you can't be sure anymore that nobodies
> > may call pps_event() over a non existent device...
>
> [snip]
>
> > By dropping pps_get_source you may be here by a call from (i.e.) a
> > serial port driver whose doesn't know if your PPS source is gone or
> > not...
> >
> > I don't understand how your modifications may resolve this problem.
>
> Well, this can happen only if PPS client module calls pps_event before
> calling pps_register_source() or after pps_unregister_source(). This
> means that it's broken! If we try to handle/workaround broken clients it
Suppose we are using pps-ldisc client. How can you assure that
nobodies may execute pps_tty_close() while you are into the
pps_event() related to the same serial port?
You can't disable serial interrupts in order to avoid
pps_tty_dcd_change calls...
> affects performance. So we have to choose what is the priority:
> security or performance. My guru told me I shouldn't bother too much
> about broken kernel-space code which my code interacts with. If it's
> broken it should be fixed. Some assertions enabled by DEBUG define are
> enough. For me it makes sense but I don't know what should I check here?
I'm sorry but I disagree with you. Kernel code can't allow userland
programs to corrupt it!
We are not discussing about security or performance but about
reliability.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
В Wed, 4 Aug 2010 16:34:30 -0700
Andrew Morton <[email protected]> пишет:
> On Thu, 5 Aug 2010 01:06:52 +0400
> Alexander Gordeev <[email protected]> wrote:
>
> > Add parallel port PPS client. It uses a standard method for capturing
> > timestamps for assert edge transitions: getting a timestamp soon after
> > an interrupt has happened. This is not a very precise source of time
> > information due to interrupt handling delays. However, timestamps for
> > clear edge transitions are much more precise because the interrupt
> > handler continuously polls hardware port until the transition is done.
> > Hardware port operations require only about 1us so the maximum error
> > should not exceed this value. This was my primary goal when developing
> > this client.
> > Clear edge capture could be disabled using clear_wait parameter.
> >
> >
> > ...
> >
> > +
> > +#define SIGNAL_IS_SET(port) \
> > + ((port->ops->read_status(port) & PARPORT_STATUS_ACK) != 0)
>
> This could (and hence should) be implemented in a regular old C
> function.
Ok, thanks!
> >
> > ...
> >
> > +{
> > + struct pps_client_pp *device;
> > + struct pps_source_info info = {
> > + .name = DRVNAME,
> > + .path = "",
> > + .mode = PPS_CAPTUREBOTH | \
> > + PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> > + PPS_ECHOASSERT | PPS_ECHOCLEAR | \
> > + PPS_CANWAIT | PPS_TSFMT_TSPEC,
> > + .echo = pps_echo,
> > + .owner = THIS_MODULE,
> > + .dev = NULL
> > + };
> > +
> > + device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
> > + if (!device) {
> > + pr_err(DRVNAME ": memory allocation failed, not attaching\n");
> > + return;
>
> eww. parport_driver.attach() returns void so there's no way to
> propagate the error code back. That sucks a bit.
IMHO, the whole parport thing currently sucks quite a bit.
--
Alexander
В Thu, 5 Aug 2010 13:07:50 +0200
Vitezslav Samel <[email protected]> пишет:
> On Thu, Aug 05, 2010 at 02:19:51PM +0400, Alexander Gordeev wrote:
> > Hi Vitezslav,
> >
> > В Thu, 5 Aug 2010 07:19:29 +0200
> > Vitezslav Samel <[email protected]> пишет:
> >
> > > On Thu, Aug 05, 2010 at 01:06:40AM +0400, Alexander Gordeev wrote:
> > > > There was a race in PPS_FETCH ioctl handler when several processes want
> > > > to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> > > > sleep most of the time in the system call.
> > > > With the old approach when the first process waiting on the pps queue
> > > > is waken up it makes new system call right away and zeroes pps->go. So
> > > > other processes continue to sleep. This is a clear race condition
> > > > because of the global 'go' variable.
> > > > With the new approach pps->last_ev holds some value increasing at each
> > > > PPS event. PPS_FETCH ioctl handler saves current value to the local
> > > > variable at the very beginning so it can safely check that there is a
> > > > new event by just comparing both variables.
> > > >
> > > > Signed-off-by: Alexander Gordeev <[email protected]>
> > > > ---
> > > > drivers/pps/kapi.c | 4 ++--
> > > > drivers/pps/pps.c | 10 +++++++---
> > > > include/linux/pps_kernel.h | 2 +-
> > > > 3 files changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > > index 55f3961..3f89f5e 100644
> > > > --- a/drivers/pps/kapi.c
> > > > +++ b/drivers/pps/kapi.c
> > > > @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
> > > >
> > > > /* Wake up if captured something */
> > > > if (captured) {
> > > > - pps->go = ~0;
> > > > - wake_up_interruptible(&pps->queue);
> > > > + pps->last_ev++;
> > > > + wake_up_interruptible_all(&pps->queue);
> > >
> > > What happens if pps->last_ev overflows? Seems to me it would freeze
> > > pps.
> >
> > Yes, it will freeze the fds (if they don't use timeouts). But in normal
> > circumstances, i.e. when pps_event is called twice a second, it will
> > overflow after ~68 years of uninterrupted work. Well, it's the same
> > kind of problem as an overflow of struct timespec. I thought it's not
> > actually a problem. Should I use u64 instead of unsigned int or add a
> > runtime check somewhere?
>
> If we're using 1PPS it's ~68 years, but someone is trying 5PPS now
> (it would overflow in ~13.6 years) - what if someone tries e.g. 100PPS?
> It's not the same as overflow of struct timespec! I think it deserves
> some treatment.
You are right. :)
I'll use u64 here then which should be enough for sure, ok?
Thanks for the note!
--
Alexander
В Thu, 5 Aug 2010 14:31:47 +0200
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Aug 05, 2010 at 03:42:31PM +0400, Alexander Gordeev wrote:
> > ?? Thu, 5 Aug 2010 11:32:36 +0200
> > Rodolfo Giometti <[email protected]> ??????????:
> >
> > > On Thu, Aug 05, 2010 at 01:06:42AM +0400, Alexander Gordeev wrote:
> > > > Using device index as a pointer needs some unnecessary work to be done
> > > > every time the pointer is needed (in irq handler for example).
> > > > Using a direct pointer is much more easy (and safe as well).
> > > >
> > > > Signed-off-by: Alexander Gordeev <[email protected]>
> > [snip]
> > >
> > > If you remove these functions you can't be sure anymore that nobodies
> > > may call pps_event() over a non existent device...
> >
> > [snip]
> >
> > > By dropping pps_get_source you may be here by a call from (i.e.) a
> > > serial port driver whose doesn't know if your PPS source is gone or
> > > not...
> > >
> > > I don't understand how your modifications may resolve this problem.
> >
> > Well, this can happen only if PPS client module calls pps_event before
> > calling pps_register_source() or after pps_unregister_source(). This
> > means that it's broken! If we try to handle/workaround broken clients it
>
> Suppose we are using pps-ldisc client. How can you assure that
> nobodies may execute pps_tty_close() while you are into the
> pps_event() related to the same serial port?
>
> You can't disable serial interrupts in order to avoid
> pps_tty_dcd_change calls...
Hmm, yes, I see...
But this is custom problem of only one client. I think it should be
fixed in place instead of trying to fix it in the subsystem.
Are you 100% sure dcd_change can be called before open or after close?
Then I'll try to deal with this.
> > affects performance. So we have to choose what is the priority:
> > security or performance. My guru told me I shouldn't bother too much
> > about broken kernel-space code which my code interacts with. If it's
> > broken it should be fixed. Some assertions enabled by DEBUG define are
> > enough. For me it makes sense but I don't know what should I check here?
>
> I'm sorry but I disagree with you. Kernel code can't allow userland
> programs to corrupt it!
>
> We are not discussing about security or performance but about
> reliability.
Sure, now I see the problem (in the pps-ldisc).
--
Alexander
On Mon, Aug 09, 2010 at 11:53:43AM +0400, Alexander Gordeev wrote:
>
> Hmm, yes, I see...
> But this is custom problem of only one client. I think it should be
> fixed in place instead of trying to fix it in the subsystem.
I agree with you but I had no luck in doing it in the past... :'(
Maybe you can ask some help to the serial port maintainers or you can
try in pushing the PPS_IRQ_EVENTS solution (see my repository) into
main tree, in fact that solution could resolve two problems at once:
* this one, since we simply read PPS data into an array in RAM, and
* the weak PPS resolution in recording PPS timestamps into normal IRQ
* handlers.
However, the big issue on this solution is about the call of
gettimeofday() for each IRQs into the system (see old mails into this
list about this topic) which slows down the machine's performance.
A workaround (as suggested by Alan Cox if I well remember) could be
adding a flag for each IRQs in order to know if the timestamp must be
recorded or not (testing a flag is not a bit issue for the machine's
performance).
> Are you 100% sure dcd_change can be called before open or after close?
> Then I'll try to deal with this.
Before the open there are no problems since the line discipline is
off, the problem is during the close.
> > > affects performance. So we have to choose what is the priority:
> > > security or performance. My guru told me I shouldn't bother too much
> > > about broken kernel-space code which my code interacts with. If it's
> > > broken it should be fixed. Some assertions enabled by DEBUG define are
> > > enough. For me it makes sense but I don't know what should I check here?
> >
> > I'm sorry but I disagree with you. Kernel code can't allow userland
> > programs to corrupt it!
> >
> > We are not discussing about security or performance but about
> > reliability.
>
> Sure, now I see the problem (in the pps-ldisc).
Yes, the problem is there (and globally in all drivers whose not
directly take care off PPS issues). Unluckely the pps-ldisc is the
most currently used by PPS users...
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it