This is a rework of Richards PTP support series core code. The PTP
driver patches are unchanged and not included in this series.
The reason for this rework is that I got scared when reviewing:
[PATCH V10 09/15] posix clocks: cleanup the CLOCK_DISPTACH macro
The patch is really too large and the risk of wreckage too high. So
instead of whipping Richard through another round I reworked the
series in the following way:
1) Split the CLOCK_DISPATCH cleanup in fine grained steps.
That allowed further cleanups and got rid of 200 lines of code and
made a lot of functions static.
It also fixes subtle changes to the error return codes which happened
in the large all in one overhaul (EINVAL vs. ENOTSUP).
2) Move the patches which add new functionality after the cleanup.
It does not make sense to add new functionality into the old scheme
first and then clean it up.
Richard, can you please run that through your testing ? The PTP
drivers apply on top of that.
Thanks,
tglx
On Tue, Feb 01, 2011 at 01:50:55PM -0000, Thomas Gleixner wrote:
> The reason for this rework is that I got scared when reviewing:
> [PATCH V10 09/15] posix clocks: cleanup the CLOCK_DISPTACH macro
I am glad that you are taking the blame/credit ;)
> The patch is really too large and the risk of wreckage too high. So
> instead of whipping Richard through another round I reworked the
> series in the following way:
>
> 1) Split the CLOCK_DISPATCH cleanup in fine grained steps.
I didn't test or compile each of the little steps, but I have looked
the differences between where I landed with my V10 and this series.
> That allowed further cleanups and got rid of 200 lines of code and
> made a lot of functions static.
Yes, it looks even nicer than before, but I'll comment on a few small
points.
> Richard, can you please run that through your testing ? The PTP
> drivers apply on top of that.
I'll run my PTP tests today.
Thanks,
Richard
On Tue, Feb 01, 2011 at 01:50:55PM -0000, Thomas Gleixner wrote:
>
> Richard, can you please run that through your testing ? The PTP
> drivers apply on top of that.
My PTP convergence tests ran fine with the gianfar and phyter clocks.
Tested-by: Richard Cochran <[email protected]>
Commit-ID: 094aa1881fdc1b8889b442eb3511b31f3ec2b762
Gitweb: http://git.kernel.org/tip/094aa1881fdc1b8889b442eb3511b31f3ec2b762
Author: Richard Cochran <[email protected]>
AuthorDate: Tue, 1 Feb 2011 13:52:20 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 2 Feb 2011 15:28:18 +0100
ntp: Add ADJ_SETOFFSET mode bit
This patch adds a new mode bit into the timex structure. When set, the bit
instructs the kernel to add the given time value to the current time.
Signed-off-by: Richard Cochran <[email protected]>
Acked-by: John Stultz <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
include/linux/timex.h | 3 ++-
kernel/time/ntp.c | 11 +++++++++++
2 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/include/linux/timex.h b/include/linux/timex.h
index d23999f..aa60fe7 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -73,7 +73,7 @@ struct timex {
long tolerance; /* clock frequency tolerance (ppm)
* (read only)
*/
- struct timeval time; /* (read only) */
+ struct timeval time; /* (read only, except for ADJ_SETOFFSET) */
long tick; /* (modified) usecs between clock ticks */
long ppsfreq; /* pps frequency (scaled ppm) (ro) */
@@ -102,6 +102,7 @@ struct timex {
#define ADJ_STATUS 0x0010 /* clock status */
#define ADJ_TIMECONST 0x0020 /* pll time constant */
#define ADJ_TAI 0x0080 /* set TAI offset */
+#define ADJ_SETOFFSET 0x0100 /* add 'time' to current time */
#define ADJ_MICRO 0x1000 /* select microsecond resolution */
#define ADJ_NANO 0x2000 /* select nanosecond resolution */
#define ADJ_TICK 0x4000 /* tick value */
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index ed8cfdf..5ac5932 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -648,6 +648,17 @@ int do_adjtimex(struct timex *txc)
hrtimer_cancel(&leap_timer);
}
+ if (txc->modes & ADJ_SETOFFSET) {
+ struct timespec delta;
+ if ((unsigned long)txc->time.tv_usec >= NSEC_PER_SEC)
+ return -EINVAL;
+ delta.tv_sec = txc->time.tv_sec;
+ delta.tv_nsec = txc->time.tv_usec;
+ if (!(txc->modes & ADJ_NANO))
+ delta.tv_nsec *= 1000;
+ timekeeping_inject_offset(&delta);
+ }
+
getnstimeofday(&ts);
write_seqlock_irq(&xtime_lock);
I just noticed a problem with this code. As is, the code will not
detect if the microseconds field (the non-ADJ_NANO case) is out of
range. However, the call to timekeeping_inject_offset() would silently
fail.
This patch corrects the problem by letting timekeeping_inject_offset()
do the check and catching the return code.
Author: Richard Cochran <[email protected]>
Date: Fri Feb 18 09:54:19 2011 +0100
ntp: remove useless and incorrect parameter check
The ADJ_SETOFFSET code redundantly checks the range of the nanoseconds
field of the time value. This field is checked again in the subsequent
call to timekeeping_inject_offset(). Also, as is, the check will not
detect whether the number of microseconds is out of range.
This patch simply lets timekeeping_inject_offset() do the error checking.
Signed-off-by: Richard Cochran <[email protected]>
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 34d1b64..d62f35a 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -648,13 +648,13 @@ int do_adjtimex(struct timex *txc)
if (txc->modes & ADJ_SETOFFSET) {
struct timespec delta;
- if ((unsigned long)txc->time.tv_usec >= NSEC_PER_SEC)
- return -EINVAL;
delta.tv_sec = txc->time.tv_sec;
delta.tv_nsec = txc->time.tv_usec;
if (!(txc->modes & ADJ_NANO))
delta.tv_nsec *= 1000;
- timekeeping_inject_offset(&delta);
+ result = timekeeping_inject_offset(&delta);
+ if (result)
+ return result;
}
getnstimeofday(&ts);
Commit-ID: db1c1cce4a653dcbe6949c72ae7b9f42cab1b929
Gitweb: http://git.kernel.org/tip/db1c1cce4a653dcbe6949c72ae7b9f42cab1b929
Author: Richard Cochran <[email protected]>
AuthorDate: Fri, 18 Feb 2011 10:07:25 +0100
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 18 Feb 2011 17:01:12 +0100
ntp: Remove redundant and incorrect parameter check
The ADJ_SETOFFSET code redundantly checks the range of the nanoseconds
field of the time value. This field is checked again in the subsequent
call to timekeeping_inject_offset(). Also, as is, the check will not
detect whether the number of microseconds is out of range.
Let timekeeping_inject_offset() do the error checking.
Signed-off-by: Richard Cochran <[email protected]>
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/time/ntp.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 5ac5932..5f1bb8e 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -650,13 +650,13 @@ int do_adjtimex(struct timex *txc)
if (txc->modes & ADJ_SETOFFSET) {
struct timespec delta;
- if ((unsigned long)txc->time.tv_usec >= NSEC_PER_SEC)
- return -EINVAL;
delta.tv_sec = txc->time.tv_sec;
delta.tv_nsec = txc->time.tv_usec;
if (!(txc->modes & ADJ_NANO))
delta.tv_nsec *= 1000;
- timekeeping_inject_offset(&delta);
+ result = timekeeping_inject_offset(&delta);
+ if (result)
+ return result;
}
getnstimeofday(&ts);
On Tue, Feb 01, 2011 at 01:50:55PM -0000, Thomas Gleixner wrote:
> This is a rework of Richards PTP support series core code. The PTP
> driver patches are unchanged and not included in this series.
>
> The reason for this rework is that I got scared when reviewing:
> [PATCH V10 09/15] posix clocks: cleanup the CLOCK_DISPTACH macro
>
> The patch is really too large and the risk of wreckage too high. So
> instead of whipping Richard through another round I reworked the
> series in the following way:
>
> 1) Split the CLOCK_DISPATCH cleanup in fine grained steps.
>
> That allowed further cleanups and got rid of 200 lines of code and
> made a lot of functions static.
>
> It also fixes subtle changes to the error return codes which happened
> in the large all in one overhaul (EINVAL vs. ENOTSUP).
>
> 2) Move the patches which add new functionality after the cleanup.
>
> It does not make sense to add new functionality into the old scheme
> first and then clean it up.
>
> Richard, can you please run that through your testing ? The PTP
> drivers apply on top of that.
i am a bit puzzled how a software ptp clock would fit into this
framework. for some avb use-cases we could get away with a ptp clock
thats only accurate to a few 100us.
from a few quick glances it seems, that if userspace is able to create a
ptp clock driven by normal timers and the kernel allows for timestamping
packets using that clock, a modified ptpd could do the trick.
i am not sure, how much of this should be happening in userspace though.
--
torben Hohn
>> Richard, can you please run that through your testing ? The PTP
>> drivers apply on top of that.
>
>i am a bit puzzled how a software ptp clock would fit into this
>framework. for some avb use-cases we could get away with a ptp clock
>thats only accurate to a few 100us.
>
>from a few quick glances it seems, that if userspace is able to create a
>ptp clock driven by normal timers and the kernel allows for timestamping
>packets using that clock, a modified ptpd could do the trick.
>
>i am not sure, how much of this should be happening in userspace though.
The point of the PHC patch set is to support special hardware clocks.
After much discussion (see the links in the V12 patch set) we have come
up with an API that will work both with software only (ie normal system
time) as well as with hardware clocks.
The application just uses: clockid_t id = CLOCK_REALTIME
if it wants to use software time stamping with the normal system clock.
I have some patches for the ptpd that show how the API works, and I
will be reposting those to the ptpd.sf.net in the next few days.
HTH,
Richard
On Tue, Mar 01, 2011 at 07:50:55PM +0100, Richard Cochran wrote:
> >> Richard, can you please run that through your testing ? The PTP
> >> drivers apply on top of that.
> >
> >i am a bit puzzled how a software ptp clock would fit into this
> >framework. for some avb use-cases we could get away with a ptp clock
> >thats only accurate to a few 100us.
> >
> >from a few quick glances it seems, that if userspace is able to create a
> >ptp clock driven by normal timers and the kernel allows for timestamping
> >packets using that clock, a modified ptpd could do the trick.
> >
> >i am not sure, how much of this should be happening in userspace though.
>
> The point of the PHC patch set is to support special hardware clocks.
> After much discussion (see the links in the V12 patch set) we have come
> up with an API that will work both with software only (ie normal system
> time) as well as with hardware clocks.
>
> The application just uses: clockid_t id = CLOCK_REALTIME
> if it wants to use software time stamping with the normal system clock.
this assumes, that the ptp master clock is actually suitable to be
CLOCK_REALTIME. i am not sure if this assumption holds true, when the
master clock is an audio clock, of some cheap soundcard.
in that case, we need to relate the audio clock to CLOCK_REALTIME.
thats surely possible, but we end up with 2 cascaded clock servos.
additionally userspace must then manage to get the master clocks
relations from the ptpd to the app that plays out the audio.
if i was using a h/w clock, i could just make the audio playing app
query the h/w ptp clock using the right clockid.
so it looks like i would end up with a different api on the audio side.
>
> I have some patches for the ptpd that show how the API works, and I
> will be reposting those to the ptpd.sf.net in the next few days.
>
> HTH,
> Richard
>
--
torben Hohn