drivers/ptp/ptp_chardev.c | 7 +++++-- include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++---- include/uapi/linux/ptp_clock.h | 7 ++++++- 3 files changed, 37 insertions(+), 7 deletions(-)
The current implementation of PTP_SYS_OFFSET_EXTENDED provides
PHC reads in the form of [pre-TS, PHC, post-TS]. These pre and
post timestamps are useful to measure the width of the PHC read.
However, the current implementation provides these timestamps in
CLOCK_REALTIME only. Since CLOCK_REALTIME is disciplined by NTP
or NTP-like service(s), the value is subjected to change. This
makes some applications that are very sensitive to time change
have these timestamps delivered in different time-base.
This patch updates the gettimex64 / ioctl op PTP_SYS_OFFSET_EXTENDED
to provide these (sandwich) timestamps optionally in
CLOCK_MONOTONIC_RAW timebase while maintaining the default behavior
or giving them in CLOCK_REALTIME.
~# testptp -d /dev/ptp0 -x 3 -y raw
extended timestamp request returned 3 samples
sample # 0: mono-raw time before: 371.548640128
phc time: 371.579671788
mono-raw time after: 371.548640912
sample # 1: mono-raw time before: 371.548642104
phc time: 371.579673346
mono-raw time after: 371.548642490
sample # 2: mono-raw time before: 371.548643320
phc time: 371.579674652
mono-raw time after: 371.548643756
~# testptp -d /dev/ptp0 -x 3 -y real
extended timestamp request returned 3 samples
sample # 0: system time before: 1713243413.403474250
phc time: 385.699915490
system time after: 1713243413.403474948
sample # 1: system time before: 1713243413.403476220
phc time: 385.699917168
system time after: 1713243413.403476642
sample # 2: system time before: 1713243413.403477555
phc time: 385.699918442
system time after: 1713243413.403477961
~#
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1 -> v2
* Code-style fixes
drivers/ptp/ptp_chardev.c | 7 +++++--
include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
include/uapi/linux/ptp_clock.h | 7 ++++++-
3 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 7513018c9f9a..c109109c9e8e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -358,11 +358,14 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
extoff = NULL;
break;
}
- if (extoff->n_samples > PTP_MAX_SAMPLES
- || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+ if (extoff->n_samples > PTP_MAX_SAMPLES ||
+ extoff->rsv[0] || extoff->rsv[1] ||
+ (extoff->clockid != CLOCK_REALTIME &&
+ extoff->clockid != CLOCK_MONOTONIC_RAW)) {
err = -EINVAL;
break;
}
+ sts.clockid = extoff->clockid;
for (i = 0; i < extoff->n_samples; i++) {
err = ptp->info->gettimex64(ptp->info, &ts, &sts);
if (err)
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6e4b8206c7d0..7563da6db09b 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -47,10 +47,12 @@ struct system_device_crosststamp;
* struct ptp_system_timestamp - system time corresponding to a PHC timestamp
* @pre_ts: system timestamp before capturing PHC
* @post_ts: system timestamp after capturing PHC
+ * @clockid: clockid used for cpaturing timestamp
*/
struct ptp_system_timestamp {
struct timespec64 pre_ts;
struct timespec64 post_ts;
+ clockid_t clockid;
};
/**
@@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->pre_ts);
+ if (sts) {
+ switch (sts->clockid) {
+ case CLOCK_REALTIME:
+ ktime_get_real_ts64(&sts->pre_ts);
+ break;
+ case CLOCK_MONOTONIC_RAW:
+ ktime_get_raw_ts64(&sts->pre_ts);
+ break;
+ default:
+ break;
+ }
+ }
}
static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->post_ts);
+ if (sts) {
+ switch (sts->clockid) {
+ case CLOCK_REALTIME:
+ ktime_get_real_ts64(&sts->post_ts);
+ break;
+ case CLOCK_MONOTONIC_RAW:
+ ktime_get_raw_ts64(&sts->post_ts);
+ break;
+ default:
+ break;
+ }
+ }
}
#endif
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 053b40d642de..fc5825e72330 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -157,7 +157,12 @@ struct ptp_sys_offset {
struct ptp_sys_offset_extended {
unsigned int n_samples; /* Desired number of measurements. */
- unsigned int rsv[3]; /* Reserved for future use. */
+ /* The original implementation provided timestamps (always) in
+ * REALTIME clock-base. Since CLOCK_REALTIME is 0, adding
+ * clockid doesn't break backward compatibility.
+ */
+ clockid_t clockid; /* One of the supported clock-ids */
+ unsigned int rsv[2]; /* Reserved for future use. */
/*
* Array of [system, phc, system] time stamps. The kernel will provide
* 3*n_samples time stamps.
--
2.44.0.683.g7961c838ac-googFrom: Mahesh Bandewar > Sent: 18 April 2024 05:27 > > The current implementation of PTP_SYS_OFFSET_EXTENDED provides > PHC reads in the form of [pre-TS, PHC, post-TS]. These pre and > post timestamps are useful to measure the width of the PHC read. > However, the current implementation provides these timestamps in > CLOCK_REALTIME only. Since CLOCK_REALTIME is disciplined by NTP > or NTP-like service(s), the value is subjected to change. This > makes some applications that are very sensitive to time change > have these timestamps delivered in different time-base. ... Isn't using CLOCK_REALTIME just a big bug? As well as minor 'corrections' done by NTP it suffers from major time-warps that can jump in either direction by arbitrary amounts. If I understand the intent of the UAPI, a possibly solution is to get the offset between CLOCK_REALTIME and CLOCK_MONATONIC and ensure the same offset is added CLOCK_MONATONIC for the pre- and post- timestamps. This doesn't solve the problem of the NTP adjusted clock always running slightly slow or fast. The big NTP errors happen in the first (IIRC up to ~20 mins after boot) when the system clock is being synchronised. It really would be nice if those big adjustments didn't affect CLOCK_MONATONIC. (as an example try sending RTP audio every 20ms) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, Apr 21, 2024 at 11:27 AM David Laight <David.Laight@aculab.com> wrote: > > From: Mahesh Bandewar > > Sent: 18 April 2024 05:27 > > > > The current implementation of PTP_SYS_OFFSET_EXTENDED provides > > PHC reads in the form of [pre-TS, PHC, post-TS]. These pre and > > post timestamps are useful to measure the width of the PHC read. > > However, the current implementation provides these timestamps in > > CLOCK_REALTIME only. Since CLOCK_REALTIME is disciplined by NTP > > or NTP-like service(s), the value is subjected to change. This > > makes some applications that are very sensitive to time change > > have these timestamps delivered in different time-base. > ... > > Isn't using CLOCK_REALTIME just a big bug? > As well as minor 'corrections' done by NTP it suffers from > major time-warps that can jump in either direction by arbitrary amounts. > Yes, this arbitrary jump in either direction is a problem and hence the proposed update. However, since it's a UAPI and there could be use cases that are happy with the current implementation, we can't break them. Of course the use case that I'm bringing in (and probably what you have in mind) differs but backward compatibility needs to be maintained. > If I understand the intent of the UAPI, a possibly solution is > to get the offset between CLOCK_REALTIME and CLOCK_MONATONIC and > ensure the same offset is added CLOCK_MONATONIC for the pre- and > post- timestamps. > As I understand, the UAPI is to read the PHC and also use the pre/post timestamps to see the latency of that PHC read as well. CLOCK_REALTIME is disciplined by NTP and to certain extents CLOCK_MONOTONIC as well (only freq adjustments are applied). CLOCK_MONOTONIC_RAW is the only unaffected time-base and hence this proposal / enhancement. > This doesn't solve the problem of the NTP adjusted clock always > running slightly slow or fast. > The big NTP errors happen in the first (IIRC up to ~20 mins after boot) > when the system clock is being synchronised. Yes, a big step is a high possibility at the beginning (at boot) but smaller steps as well as ppm adjustments are real possibilities throughout and hence CLOCK_REALTIME and CLOCK_MONOTONIC are affected. By adding the timestamps in CLOCK_MONOTONIC_RAW (as proposed in this patch) should address this issue. > It really would be nice if those big adjustments didn't affect > CLOCK_MONATONIC. > (as an example try sending RTP audio every 20ms) > Hmm, probably this is out of context for this patch and probably a question for the time maintainers / experts? > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
On Mon, Apr 22 2024 at 15:04, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Sun, Apr 21, 2024 at 11:27 AM David Laight <David.Laight@aculab.com> wrote:
>>
>> Isn't using CLOCK_REALTIME just a big bug?
>> As well as minor 'corrections' done by NTP it suffers from
>> major time-warps that can jump in either direction by arbitrary amounts.
>>
> Yes, this arbitrary jump in either direction is a problem and hence
> the proposed update. However, since it's a UAPI and there could be use
> cases that are happy with the current implementation, we can't break
> them. Of course the use case that I'm bringing in (and probably what
> you have in mind) differs but backward compatibility needs to be
> maintained.
It depends on what you are trying to do. You cannot adjust
CLOCK_REALTIME/TAI without knowing the current time, right?
So just declaring that this is a big bug and a problem is as wrong as it
gets. It's obviously not the right thing for all use cases, but that
makes the legitimate use cases not wrong.
>> This doesn't solve the problem of the NTP adjusted clock always
>> running slightly slow or fast.
>> The big NTP errors happen in the first (IIRC up to ~20 mins after boot)
>> when the system clock is being synchronised.
>
> Yes, a big step is a high possibility at the beginning (at boot) but
> smaller steps as well as ppm adjustments are real possibilities
> throughout and hence CLOCK_REALTIME and CLOCK_MONOTONIC are affected.
> By adding the timestamps in CLOCK_MONOTONIC_RAW (as proposed in this
> patch) should address this issue.
>
>> It really would be nice if those big adjustments didn't affect
>> CLOCK_MONATONIC. (as an example try sending RTP audio every 20ms)
They don't affect CLOCK_MONATONIC at all because there is no such clock :)
> Hmm, probably this is out of context for this patch and probably a
> question for the time maintainers / experts?
The quantity of the initial frequency adjustments depends on the
accuracy of the initial clock frequency calibration which is on most
sane systems within +/- 500ppm.
500ppm of 20ms == 10us
If the clock calibration is off by a larger margin then that needs to be
fixed.
It's clearly documented that CLOCK_MONOTONIC and CLOCK_REALTIME (and
therefore CLOCK_BOOTTIME and CLOCK_TAI) are strictly based on the same
frequency and only differ by offsets. So there is nothing to fix and
change.
Thanks,
tglx
From: Thomas Gleixner > Sent: 23 April 2024 01:25 ... > >> It really would be nice if those big adjustments didn't affect > >> CLOCK_MONATONIC. (as an example try sending RTP audio every 20ms) > > They don't affect CLOCK_MONATONIC at all because there is no such clock :) > > > Hmm, probably this is out of context for this patch and probably a > > question for the time maintainers / experts? > > The quantity of the initial frequency adjustments depends on the > accuracy of the initial clock frequency calibration which is on most > sane systems within +/- 500ppm. > > 500ppm of 20ms == 10us > > If the clock calibration is off by a larger margin then that needs to be > fixed. The initial adjustment depends on the accuracy of the initial RTC value read from the local hardware. This is unlikely to be more accurate than 1 second and can easily be a few seconds out. Correcting this causes NTP to adjust the clock at its maximum drift rate for a while - I'm sure I've seen this happen for minutes. Once this completes there is a 'step change' in the frequency adjustment. Once the system has been running for a while the adjustments are minor. Time runs alternately fast and slow to maintain long term accuracy. This is noticeable if you use schedule_hrtimeout_range(,, HRTIMER_MODE_ABS) to synchronize to an accurate external clock [1]. (Without NTP it has to adjust for temperature changing the frequency.) David [1] Imagine some hardware that counts usecs after the 1-second GPS pulse. and a driver that adjusts a sleep to wake up when that count is between (say) 400 and 600. Using a timer and a single readl() is far faster than taking an interrupt. (In our case it is a 10ms pulse derived from the clock recovered from an E1/T1 telecoms link.) - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Apr 23 2024 at 09:22, David Laight wrote:
> From: Thomas Gleixner
>> The quantity of the initial frequency adjustments depends on the
>> accuracy of the initial clock frequency calibration which is on most
>> sane systems within +/- 500ppm.
>>
>> 500ppm of 20ms == 10us
>>
>> If the clock calibration is off by a larger margin then that needs to be
>> fixed.
>
> The initial adjustment depends on the accuracy of the initial RTC
> value read from the local hardware.
The initial value of CLOCK_REALTIME depends on the RTC value, but not
the initial frequency and the frequency is the only thing which matters
for CLOCK_MONOTONIC.
> This is unlikely to be more accurate than 1 second and can easily
> be a few seconds out.
Halfways sane RTCs drift in the range of +/- 5 seconds per day. But
that's not a problem if your NTP daemon is configured correctly because
it will step CLOCK_REALTIME right away when it's off more than 1 second.
Also correctly configured NTP daemons keep track of the drift and reuse
the last observed drift value which makes them stabilize quickly if the
initial frequency value which has been calibrated / determined by the
kernel is halfways accurate. It takes 5 seconds on my laptop from
starting chrony until it's stable.
Sure you can configure NTP in weird ways, but that's not a kernel
problem.
Thanks,
tglx
On Wed, Apr 17 2024 at 21:27, Mahesh Bandewar wrote:
Subject: ptp: update gettimex64 to provide ts optionally in mono-raw base.
Can we please have proper sentences without cryptic abbreviations? This
is not twatter or SMS.
Aside of that this is not about updating gettimex64(). The fact that
this is an UAPI change is the real important information. gettimex64()
is only the kernel side implementation detail.
ptp/ioctl: Support MONOTONIC_RAW timestamps forPTP_SYS_OFFSET_EXTENDED
or something like that.
> The current implementation of PTP_SYS_OFFSET_EXTENDED provides
> PHC reads in the form of [pre-TS, PHC, post-TS]. These pre and
> post timestamps are useful to measure the width of the PHC read.
> However, the current implementation provides these timestamps in
> CLOCK_REALTIME only. Since CLOCK_REALTIME is disciplined by NTP
> or NTP-like service(s), the value is subjected to change. This
> makes some applications that are very sensitive to time change
> have these timestamps delivered in different time-base.
The last sentence does not make any sense to me.
> This patch updates the gettimex64 / ioctl op PTP_SYS_OFFSET_EXTENDED
git grep 'This patch' Documentation/process/
> to provide these (sandwich) timestamps optionally in
> CLOCK_MONOTONIC_RAW timebase while maintaining the default behavior
> or giving them in CLOCK_REALTIME.
This change log lacks a proper explanation why this is needed and what's
the purpose and benefit.
Aside of that it fails to mention that using the currently unused
reserved field is correct because CLOCK_REALTIME == 0.
> ~# testptp -d /dev/ptp0 -x 3 -y raw
> extended timestamp request returned 3 samples
> sample # 0: mono-raw time before: 371.548640128
> phc time: 371.579671788
> mono-raw time after: 371.548640912
> sample # 1: mono-raw time before: 371.548642104
> phc time: 371.579673346
> mono-raw time after: 371.548642490
> sample # 2: mono-raw time before: 371.548643320
> phc time: 371.579674652
> mono-raw time after: 371.548643756
> ~# testptp -d /dev/ptp0 -x 3 -y real
> extended timestamp request returned 3 samples
> sample # 0: system time before: 1713243413.403474250
> phc time: 385.699915490
> system time after: 1713243413.403474948
> sample # 1: system time before: 1713243413.403476220
> phc time: 385.699917168
> system time after: 1713243413.403476642
> sample # 2: system time before: 1713243413.403477555
> phc time: 385.699918442
> system time after: 1713243413.403477961
That takes up a lot of space, but what's the actual value of this
information? Especially as there is no actual test case for this which
people can use to validate the changes.
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 6e4b8206c7d0..7563da6db09b 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -47,10 +47,12 @@ struct system_device_crosststamp;
> * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
> * @pre_ts: system timestamp before capturing PHC
> * @post_ts: system timestamp after capturing PHC
> + * @clockid: clockid used for cpaturing timestamp
cpaturing?
s/timestamp/the system timestamps/
Precision matters not only for PTP.
> */
> struct ptp_system_timestamp {
> struct timespec64 pre_ts;
> struct timespec64 post_ts;
> + clockid_t clockid;
> };
>
> /**
> @@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->pre_ts);
> + if (sts) {
> + switch (sts->clockid) {
> + case CLOCK_REALTIME:
> + ktime_get_real_ts64(&sts->pre_ts);
> + break;
> + case CLOCK_MONOTONIC_RAW:
> + ktime_get_raw_ts64(&sts->pre_ts);
> + break;
> + default:
> + break;
> + }
> + }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->post_ts);
> + if (sts) {
> + switch (sts->clockid) {
> + case CLOCK_REALTIME:
> + ktime_get_real_ts64(&sts->post_ts);
> + break;
> + case CLOCK_MONOTONIC_RAW:
> + ktime_get_raw_ts64(&sts->post_ts);
> + break;
> + default:
> + break;
> + }
> + }
> }
>
> #endif
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 053b40d642de..fc5825e72330 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -157,7 +157,12 @@ struct ptp_sys_offset {
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + /* The original implementation provided timestamps (always) in
> + * REALTIME clock-base. Since CLOCK_REALTIME is 0, adding
> + * clockid doesn't break backward compatibility.
> + */
This wants to be in the change log.
If you want to document the evolution of this data structure in a
comment, then 'original implementation' is not really the best wording
to use.
I'd rather see the documentation fixed so that it uses proper kernel doc
style for the whole data structure instead of having this mix of inline
and tail comments along with a properly structured version information.
/**
* ptp_sys_offset_extended - Data structure for IOCTL_PTP_.....
*
* @n_samples: Desired number of samples
* ....
* @...
*
* History:
* V1: Initial implementation
*
* V2: Use the first reserved field for @clockid. That's backwards
* compatible for V1 user space because CLOCK_REALTIME is 0 and
* the reserved fields must be 0.
*/
Or something like that. Hmm?
Thanks,
tglx
On Thu, Apr 18, 2024 at 9:56 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Apr 17 2024 at 21:27, Mahesh Bandewar wrote:
>
> Subject: ptp: update gettimex64 to provide ts optionally in mono-raw base.
>
> Can we please have proper sentences without cryptic abbreviations? This
> is not twatter or SMS.
>
character limit in the description is the limiting factor.
> Aside of that this is not about updating gettimex64(). The fact that
> this is an UAPI change is the real important information. gettimex64()
> is only the kernel side implementation detail.
>
> ptp/ioctl: Support MONOTONIC_RAW timestamps forPTP_SYS_OFFSET_EXTENDED
>
> or something like that.
>
ack.
> > The current implementation of PTP_SYS_OFFSET_EXTENDED provides
> > PHC reads in the form of [pre-TS, PHC, post-TS]. These pre and
> > post timestamps are useful to measure the width of the PHC read.
> > However, the current implementation provides these timestamps in
> > CLOCK_REALTIME only. Since CLOCK_REALTIME is disciplined by NTP
> > or NTP-like service(s), the value is subjected to change. This
> > makes some applications that are very sensitive to time change
> > have these timestamps delivered in different time-base.
>
> The last sentence does not make any sense to me.
>
> > This patch updates the gettimex64 / ioctl op PTP_SYS_OFFSET_EXTENDED
>
> git grep 'This patch' Documentation/process/
>
> > to provide these (sandwich) timestamps optionally in
> > CLOCK_MONOTONIC_RAW timebase while maintaining the default behavior
> > or giving them in CLOCK_REALTIME.
>
> This change log lacks a proper explanation why this is needed and what's
> the purpose and benefit.
>
> Aside of that it fails to mention that using the currently unused
> reserved field is correct because CLOCK_REALTIME == 0.
>
> > ~# testptp -d /dev/ptp0 -x 3 -y raw
> > extended timestamp request returned 3 samples
> > sample # 0: mono-raw time before: 371.548640128
> > phc time: 371.579671788
> > mono-raw time after: 371.548640912
> > sample # 1: mono-raw time before: 371.548642104
> > phc time: 371.579673346
> > mono-raw time after: 371.548642490
> > sample # 2: mono-raw time before: 371.548643320
> > phc time: 371.579674652
> > mono-raw time after: 371.548643756
> > ~# testptp -d /dev/ptp0 -x 3 -y real
> > extended timestamp request returned 3 samples
> > sample # 0: system time before: 1713243413.403474250
> > phc time: 385.699915490
> > system time after: 1713243413.403474948
> > sample # 1: system time before: 1713243413.403476220
> > phc time: 385.699917168
> > system time after: 1713243413.403476642
> > sample # 2: system time before: 1713243413.403477555
> > phc time: 385.699918442
> > system time after: 1713243413.403477961
>
> That takes up a lot of space, but what's the actual value of this
> information? Especially as there is no actual test case for this which
> people can use to validate the changes.
>
I'll polish the testptp.c changes and submit them later. But if this
is not adding any value, I can remove it from the log.
> > diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> > index 6e4b8206c7d0..7563da6db09b 100644
> > --- a/include/linux/ptp_clock_kernel.h
> > +++ b/include/linux/ptp_clock_kernel.h
> > @@ -47,10 +47,12 @@ struct system_device_crosststamp;
> > * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
> > * @pre_ts: system timestamp before capturing PHC
> > * @post_ts: system timestamp after capturing PHC
> > + * @clockid: clockid used for cpaturing timestamp
>
> cpaturing?
>
> s/timestamp/the system timestamps/
>
> Precision matters not only for PTP.
>
:) ack
> > */
> > struct ptp_system_timestamp {
> > struct timespec64 pre_ts;
> > struct timespec64 post_ts;
> > + clockid_t clockid;
> > };
> >
> > /**
> > @@ -457,14 +459,34 @@ static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
> >
> > static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> > {
> > - if (sts)
> > - ktime_get_real_ts64(&sts->pre_ts);
> > + if (sts) {
> > + switch (sts->clockid) {
> > + case CLOCK_REALTIME:
> > + ktime_get_real_ts64(&sts->pre_ts);
> > + break;
> > + case CLOCK_MONOTONIC_RAW:
> > + ktime_get_raw_ts64(&sts->pre_ts);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > }
> >
> > static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> > {
> > - if (sts)
> > - ktime_get_real_ts64(&sts->post_ts);
> > + if (sts) {
> > + switch (sts->clockid) {
> > + case CLOCK_REALTIME:
> > + ktime_get_real_ts64(&sts->post_ts);
> > + break;
> > + case CLOCK_MONOTONIC_RAW:
> > + ktime_get_raw_ts64(&sts->post_ts);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > }
> >
> > #endif
> > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> > index 053b40d642de..fc5825e72330 100644
> > --- a/include/uapi/linux/ptp_clock.h
> > +++ b/include/uapi/linux/ptp_clock.h
> > @@ -157,7 +157,12 @@ struct ptp_sys_offset {
> >
> > struct ptp_sys_offset_extended {
> > unsigned int n_samples; /* Desired number of measurements. */
> > - unsigned int rsv[3]; /* Reserved for future use. */
> > + /* The original implementation provided timestamps (always) in
> > + * REALTIME clock-base. Since CLOCK_REALTIME is 0, adding
> > + * clockid doesn't break backward compatibility.
> > + */
>
> This wants to be in the change log.
>
Ack
> If you want to document the evolution of this data structure in a
> comment, then 'original implementation' is not really the best wording
> to use.
>
> I'd rather see the documentation fixed so that it uses proper kernel doc
> style for the whole data structure instead of having this mix of inline
> and tail comments along with a properly structured version information.
>
> /**
> * ptp_sys_offset_extended - Data structure for IOCTL_PTP_.....
> *
> * @n_samples: Desired number of samples
> * ....
> * @...
> *
> * History:
> * V1: Initial implementation
> *
> * V2: Use the first reserved field for @clockid. That's backwards
> * compatible for V1 user space because CLOCK_REALTIME is 0 and
> * the reserved fields must be 0.
> */
>
> Or something like that. Hmm?
>
will attempt to add it.
> Thanks,
>
> tglx
Thanks for reviewing Thomas, I'll address them in the next revision.
--mahesh..
On Wed, 17 Apr 2024 21:27:06 -0700 Mahesh Bandewar wrote:
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -157,7 +157,12 @@ struct ptp_sys_offset {
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + /* The original implementation provided timestamps (always) in
> + * REALTIME clock-base. Since CLOCK_REALTIME is 0, adding
> + * clockid doesn't break backward compatibility.
> + */
> + clockid_t clockid; /* One of the supported clock-ids */
> + unsigned int rsv[2]; /* Reserved for future use. */
This suffers from a moderate inability to build:
usr/include/linux/ptp_clock.h:164:2: error: unknown type name 'clockid_t'
164 | clockid_t clockid; /* One of the supported clock-ids */
| ^
--
pw-bot: cr
On Thu, Apr 18, 2024 at 6:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 17 Apr 2024 21:27:06 -0700 Mahesh Bandewar wrote:
> > --- a/include/uapi/linux/ptp_clock.h
> > +++ b/include/uapi/linux/ptp_clock.h
> > @@ -157,7 +157,12 @@ struct ptp_sys_offset {
> >
> > struct ptp_sys_offset_extended {
> > unsigned int n_samples; /* Desired number of measurements. */
> > - unsigned int rsv[3]; /* Reserved for future use. */
> > + /* The original implementation provided timestamps (always) in
> > + * REALTIME clock-base. Since CLOCK_REALTIME is 0, adding
> > + * clockid doesn't break backward compatibility.
> > + */
> > + clockid_t clockid; /* One of the supported clock-ids */
> > + unsigned int rsv[2]; /* Reserved for future use. */
>
> This suffers from a moderate inability to build:
>
> usr/include/linux/ptp_clock.h:164:2: error: unknown type name 'clockid_t'
> 164 | clockid_t clockid; /* One of the supported clock-ids */
> | ^
Hmm, my bad, it didn't fail while I was testing/building. I should
watch the bot checks more carefully :(
I'll fix it in the next revision.
> --
> pw-bot: cr
© 2016 - 2024 Red Hat, Inc.