Received-SPF: pass (google.com: domain of linux-kernel+bounces-172678-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 7 May 2024 21:44:23 -0700
From: Richard Cochran <richardcochran@gmail.com>
To: Mahesh Bandewar <maheshb@google.com>
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>,
	Thomas Gleixner <tglx@linutronix.de>, 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>
Subject: Re: [PATCHv4 net-next] ptp/ioctl: support MONOTONIC_RAW timestamps
 for PTP_SYS_OFFSET_EXTENDED
Message-ID: <ZjsDJ-adNCBQIbG1@hoboy.vegasvil.org>
References: <20240502211047.2240237-1-maheshb@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240502211047.2240237-1-maheshb@google.com>

On Thu, May 02, 2024 at 02:10:47PM -0700, Mahesh Bandewar wrote:

> @@ -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;

Why not add CLOCK_MONOTONIC as well?
That would be useful in many cases.

> +/*
> + * 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'.

This is not really appropriate for a source code comment.  The
un-merged patch series iterations are preserved at lore.kernel in case
someone needs that.

The "backward compatible" information really wants to be in the commit
message.

Thanks,
Richard