Received-SPF: pass (google.com: domain of linux-kernel+bounces-151002-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Mahesh Bandewar <maheshb@google.com>, 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>
Cc: Jonathan Corbet <corbet@lwn.net>, John Stultz <jstultz@google.com>,
 Mahesh Bandewar <mahesh@bandewar.net>, Mahesh Bandewar
 <maheshb@google.com>
Subject: Re: [PATCHv2 next] ptp: update gettimex64 to provide ts optionally
 in mono-raw base.
In-Reply-To: <20240418042706.1261473-1-maheshb@google.com>
Date: Fri, 19 Apr 2024 06:56:43 +0200
Message-ID: <87cyqmx850.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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