From 7fa9cdd262f16dd9dd11820ff602d1df1736b760 Mon Sep 17 00:00:00 2001 From: Fred Wright Date: Wed, 21 Sep 2016 19:07:43 -0700 Subject: [PATCH] Fixes various flakiness in PPS handling. The pps_thread_activate() code uses a single structure to pass information to the monitor thread. The latter copies the data during its initialization, so there's no long-term conflict across threads, but there's no guarantee that the thread runs and gets far enough to do that before the structure is reused. Note that this bug is fairly unlikely to affect a multicore machine unless it's heavily loaded, but easily causes trouble on a single-core machine. A variety of strange PPS misbehaviors were observed on a Beaglebone Black in the presence of this bug, including various nonreproducible PPS initialization problems and failures, and duplicate PPS reports. This change uses the pps_thread field as a flag to communicate a "copy acknowledgment" back to pps_thread_activate(), which waits for it before proceeding. TESTED: Launched several times on a Beaglebone Black with KPPS support, and no longer observed flaky PPS startup in the logs, as well as no longer observing duplicate PPS events in gpsmon. Not tested on other platforms due to the lack of PPS availability. --- ppsthread.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ppsthread.c b/ppsthread.c index 96ce683..428f1c8 100644 --- a/ppsthread.c +++ b/ppsthread.c @@ -57,6 +57,7 @@ #include #include #include +#include #include #include /* pacifies OpenBSD's compiler */ @@ -70,7 +71,6 @@ // include unistd.h here as it is missing on older pps-tools releases. // 'close' is not defined otherwise. #include -#include #include #endif @@ -650,6 +650,9 @@ static void *gpsd_ppsmonitor(void *arg) #endif /* defined(HAVE_SYS_TIMEPPS_H) */ bool not_a_tty = false; + /* Acknowledge that we've grabbed the inner_context data */ + ((volatile struct inner_context_t *)arg)->pps_thread = NULL; + /* before the loop, figure out how we can detect edges: * TIOMCIWAIT, which is linux specifix * RFC2783, a.k.a kernel PPS (KPPS) @@ -1180,6 +1183,7 @@ void pps_thread_activate(volatile struct pps_thread_t *pps_thread) { int retval; pthread_t pt; + struct timespec start_delay = {0, 1000000}; /* 1 ms */ /* * FIXME: this launch code is not itself thread-safe! * It would be if inner_context could be auto, but the monitor @@ -1213,6 +1217,12 @@ void pps_thread_activate(volatile struct pps_thread_t *pps_thread) pps_thread->log_hook(pps_thread, THREAD_PROG, "PPS:%s thread %s\n", pps_thread->devicename, (retval==0) ? "launched" : "FAILED"); + /* The monitor thread may not run immediately, particularly on a single- + * core machine, so we need to wait for it to acknowledge its copying + * of the inner_context struct before proceeding. + */ + while (inner_context.pps_thread) + (void) nanosleep(&start_delay, NULL); } void pps_thread_deactivate(volatile struct pps_thread_t *pps_thread) -- 2.9.3