[LinuxPPS] Re: OK, I found an error, please fix.

[Rodolfo Giometti]

On Sun, Oct 21, 2007 at 06:00:24PM -0600, clemens at dwf.com wrote:
> 
> OK, I found the error.

Good job! :)

> In kapi.c/pps_event, you call pps_add_offset to add any offset that has been
> provided.
> 
> The arguments to pps_add_offset are of type pps_ktime, defined as
> 
> struct pps_ktime {
>        __u64 sec;
>        __u32 nsec;
>        __u32 flags;
> };
> 
> in pps.h .
> 
> (1) FIRST PROBLEM: Offset can be negative so it cant be of type pps_ktime.
> If negative, its value has been screwed up by the time you get here.
> 
> In pps_add_offset, you add the offset to the time, putting the result back
> in ts->nsec. You then normalize the result if the number of ns has gone
> above NSEC_PER_SEC (this works) or if it goes below ZERO (this doesnt work)
> since ts->nsec is unsigned and the result will never be below ZERO.
> 
> (2) SECOND PROBLEM: ts->nsec is UNSIGNED, so the test
> 
>         } else if (ts->nsec < 0) {
> 
> always fails.
> 
> In fact, my offset is negative, and Im sure that this is what is biting
> me in the ass.  Im not sure how I seem to get several seconds off but Ill worry
> about that when the above is fixed.
> 
> Seems you need EITHER to NOT make ktime up out of unsigned's or you need
> a parallel structure with signed variables in it.
> 
> Ill wait to see what your solution is.

Since offsets are related with struct timespec which is defined as follow:

struct timespec {
        time_t  tv_sec;         /* seconds */
        long    tv_nsec;        /* nanoseconds */
};

where time_t is long, I suppose the right-thing(C) to do is to use
signed data. So, please, try this patch:

diff --git a/include/linux/pps.h b/include/linux/pps.h
index 5bdb593..1501793 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -54,8 +54,8 @@
  *                                                     [David Woodhouse]
  */
 struct pps_ktime {
-       __u64 sec;
-       __u32 nsec;
+       __s64 sec;
+       __s32 nsec;
        __u32 flags;
 };
 #define PPS_TIME_INVALID       (1<<0)  /* used to specify timeout==NULL */

> AND, I seem to remember some old NTP code where the the IF and ELSE in the
> pps_add_offset normalization  were in fact while loops, with the thought that they
> should only be gone thru once,   ... but just in case ... they would get the right answer.

About this try this patch:

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 67290d5..3546dab 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -44,10 +44,11 @@ static DEFINE_IDR(pps_idr);
 static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
 {
        ts->nsec += offset->nsec;
-       if (ts->nsec >= NSEC_PER_SEC) {
+       while (ts->nsec >= NSEC_PER_SEC) {
                ts->nsec -= NSEC_PER_SEC;
                ts->sec++;
-       } else if (ts->nsec < 0) {
+       }
+       while (ts->nsec < 0) {
                ts->nsec += NSEC_PER_SEC;
                ts->sec--;
        }

Currently I have no time to test these patches, so please, try them
and report if they work. :)

Thanks a lot for your time,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127