| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF2d9jjeYACm3ueLPjiYqNMBXrJ3U2dnWqKx-AbgRWLVLz+qUw@mail.gmail.com>
Date: Fri, 19 Apr 2024 15:32:37 -0700
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Netdev <netdev@...r.kernel.org>, Linux <linux-kernel@...r.kernel.org>,
David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
Richard Cochran <richardcochran@...il.com>, Arnd Bergmann <arnd@...db.de>,
Sagi Maimon <maimon.sagi@...il.com>, Jonathan Corbet <corbet@....net>, John Stultz <jstultz@...gle.com>,
Mahesh Bandewar <mahesh@...dewar.net>
Subject: Re: [PATCHv2 next] ptp: update gettimex64 to provide ts optionally in
mono-raw base.
On Thu, Apr 18, 2024 at 9:56 PM Thomas Gleixner <tglx@...utronix.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..
Powered by blists - more mailing lists