Received-SPF: pass (google.com: domain of linux-kernel+bounces-152007-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240418042706.1261473-1-maheshb@google.com> <87cyqmx850.ffs@tglx>
In-Reply-To: <87cyqmx850.ffs@tglx>
From: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= <maheshb@google.com>
Date: Fri, 19 Apr 2024 15:32:37 -0700
Message-ID: <CAF2d9jjeYACm3ueLPjiYqNMBXrJ3U2dnWqKx-AbgRWLVLz+qUw@mail.gmail.com>
Subject: Re: [PATCHv2 next] ptp: update gettimex64 to provide ts optionally in
 mono-raw base.
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Netdev <netdev@vger.kernel.org>, Linux <linux-kernel@vger.kernel.org>, 
	David Miller <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, 
	Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, 
	Richard Cochran <richardcochran@gmail.com>, Arnd Bergmann <arnd@arndb.de>, 
	Sagi Maimon <maimon.sagi@gmail.com>, Jonathan Corbet <corbet@lwn.net>, John Stultz <jstultz@google.com>, 
	Mahesh Bandewar <mahesh@bandewar.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 18, 2024 at 9:56=E2=80=AFPM 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 =3D=3D 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 ti=
mestamp
> >   * @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_cl=
ock.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..