Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps for PTP_SYS_OFFSET_EXTENDED

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 02/05/2024 22:10, Mahesh Bandewar wrote:
> The ability to read the PHC (Physical Hardware Clock) alongside
> multiple system clocks is currently dependent on the specific
> hardware architecture. This limitation restricts the use of
> PTP_SYS_OFFSET_PRECISE to certain hardware configurations.
> 
> The generic soultion which would work across all architectures
> is to read the PHC along with the latency to perform PHC-read as
> offered by PTP_SYS_OFFSET_EXTENDED which provides pre and post
> timestamps.  However, these timestamps are currently limited
> to the CLOCK_REALTIME timebase. Since CLOCK_REALTIME is affected
> by NTP (or similar time synchronization services), it can
> experience significant jumps forward or backward. This hinders
> the precise latency measurements that PTP_SYS_OFFSET_EXTENDED
> is designed to provide.
> 
> This problem could be addressed by supporting MONOTONIC_RAW
> timestamps within PTP_SYS_OFFSET_EXTENDED. Unlike CLOCK_REALTIME
> or CLOCK_MONOTONIC, the MONOTONIC_RAW timebase is unaffected
> by NTP adjustments.
> 
> This enhancement can be implemented by utilizing one of the three
> reserved words within the PTP_SYS_OFFSET_EXTENDED struct to pass
> the clock-id for timestamps.  The current behavior aligns with
> clock-id for CLOCK_REALTIME timebase (value of 0), ensuring
> backward compatibility of the UAPI.
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

Hi Manesh!

Any chance there will be v5 soon with the comments addressed?
I don't see any strong objections to the patch, and it looks
like we can benefit from having MONOTONIC_RAW clock for our
project too.

Thanks,
Vadim

> ---
> v1 -> v2
>     * Code-style fixes.
> v2 -> v3
>     * Reword commit log
>     * Fix the compilation issue by using __kernel_clockid instead of clockid_t
>       which has kernel only scope.
> v3 -> v4
>     * Typo/comment fixes.
> 
>   drivers/ptp/ptp_chardev.c        |  7 +++++--
>   include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++----
>   include/uapi/linux/ptp_clock.h   | 27 +++++++++++++++++++++------
>   3 files changed, 52 insertions(+), 12 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..74ded5f95d95 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: clock-base used for capturing the system timestamps
>    */
>   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..5e3d70fbc499 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -155,13 +155,28 @@ struct ptp_sys_offset {
>   	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>   };
>   
> +/*
> + * ptp_sys_offset_extended - data structure for IOCTL operation
> + *			     PTP_SYS_OFFSET_EXTENDED
> + *
> + * @n_samples:	Desired number of measurements.
> + * @clockid:	clockid of a clock-base used for pre/post timestamps.
> + * @rsv:	Reserved for future use.
> + * @ts:		Array of samples in the form [pre-TS, PHC, post-TS]. The
> + *		kernel provides @n_samples.
> + *
> + * History:
> + * v1: Initial implementation.
> + *
> + * v2: Use the first word of the reserved-field for @clockid. That's
> + *     backward compatible since v1 expects all three reserved words
> + *     (@rsv[3]) to be 0 while the clockid (first word in v2) for
> + *     CLOCK_REALTIME is '0'.
> + */
>   struct ptp_sys_offset_extended {
> -	unsigned int n_samples; /* Desired number of measurements. */
> -	unsigned int rsv[3];    /* Reserved for future use. */
> -	/*
> -	 * Array of [system, phc, system] time stamps. The kernel will provide
> -	 * 3*n_samples time stamps.
> -	 */
> +	unsigned int n_samples;
> +	__kernel_clockid_t clockid;
> +	unsigned int rsv[2];
>   	struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
>   };
>