[v3,3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

From: Christopher Hall <christopher.s.hall@intel.com>

This patch allows system and device time ("cross-timestamp") to be
performed by the driver. Currently, the cross-timestamping is performed
in the PTP_SYS_OFFSET ioctl.  The PTP clock driver reads gettimeofday()
and the gettime64() callback provided by the driver. The cross-timestamp
is best effort where the latency between the capture of system time
(getnstimeofday()) and the device time (driver callback) may be
significant.

This patch adds an additional callback getsynctime64(). Which will be
called when the driver is able to perform a more accurate, implementation
specific cross-timestamping.  For example, future network devices that
implement PCIE PTM will be able to precisely correlate the device clock
with the system clock with virtually zero latency between captures.
This added callback can be used by the driver to expose this functionality.

The callback, getsynctime64(), will only be called when defined and
n_samples == 1 because the driver returns only 1 cross-timestamp where
multiple samples cannot be chained together.

This patch also adds to the capabilities ioctl (PTP_CLOCK_GETCAPS),
allowing applications to query whether or not drivers implement the
getsynctime callback, providing more precise cross timestamping.

Commit Details:

Added additional callback to ptp_clock_info:

* getsynctime64()

This takes 2 arguments referring to system and device time

With this callback drivers may provide both system time and device time
to ensure precise correlation

Modified PTP_SYS_OFFSET ioctl in PTP clock driver to use the above
callback if it's available

Added capability (PTP_CLOCK_GETCAPS) for checking whether driver supports
cross timestamping

Added check for cross timestamping flag to testptp.c

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 Documentation/ptp/testptp.c      |  6 ++++--
 drivers/ptp/ptp_chardev.c        | 29 +++++++++++++++++++++--------
 include/linux/ptp_clock_kernel.h |  7 +++++++
 include/uapi/linux/ptp_clock.h   |  4 +++-
 4 files changed, 35 insertions(+), 11 deletions(-)

On Fri, 21 Aug 2015, Christopher S. Hall wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> This patch allows system and device time ("cross-timestamp") to be
> performed by the driver. Currently, the cross-timestamping is performed
> in the PTP_SYS_OFFSET ioctl.  The PTP clock driver reads gettimeofday()
> and the gettime64() callback provided by the driver. The cross-timestamp
> is best effort where the latency between the capture of system time
> (getnstimeofday()) and the device time (driver callback) may be
> significant.
> 
> This patch adds an additional callback getsynctime64(). Which will be
> called when the driver is able to perform a more accurate, implementation
> specific cross-timestamping.  For example, future network devices that
> implement PCIE PTM will be able to precisely correlate the device clock
> with the system clock with virtually zero latency between captures.
> This added callback can be used by the driver to expose this functionality.
> 
> The callback, getsynctime64(), will only be called when defined and
> n_samples == 1 because the driver returns only 1 cross-timestamp where
> multiple samples cannot be chained together.
> 
> This patch also adds to the capabilities ioctl (PTP_CLOCK_GETCAPS),
> allowing applications to query whether or not drivers implement the
> getsynctime callback, providing more precise cross timestamping.

That looks close to a proper changelog. A few nitpicks though.

Please avoid 'This patch does ...' phrases. We already know that this
is a patch.


> Commit Details:

Please get rid of this. It's useless noise.
 
> Added additional callback to ptp_clock_info:
> 
> * getsynctime64()

> @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			break;
>  		}
>  		pct = &sysoff->ts[0];
> -		for (i = 0; i < sysoff->n_samples; i++) {
> -			getnstimeofday64(&ts);
> +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&

The number of samples should be irrelevant for this sampling method.

> +		    ptp->info->getsynctime64(ptp->info, &ts, &systs) == 0) {

Why is this function taking struct timespec64 pointers? Just so every
driver which implements the callback needs to convert from u64 to
struct timespec64? That's simply wrong. Use u64 for both and do the
conversion in the ioctl.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Aug 22, 2015 at 10:33:48PM +0200, Thomas Gleixner wrote:
> > @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> >  			break;
> >  		}
> >  		pct = &sysoff->ts[0];
> > -		for (i = 0; i < sysoff->n_samples; i++) {
> > -			getnstimeofday64(&ts);
> > +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&
> 
> The number of samples should be irrelevant for this sampling method.

Chris had send me a preview of this before he posted, so I can explain
that test for one sample.

User space requests N (1 to 25) samples of the two clocks.  The kernel
is supposed to deliver that many samples.  This has always been the
documented behavior.  From ptp_clock.h:

  struct ptp_sys_offset {
	unsigned int n_samples; /* Desired number of measurements. */
	unsigned int rsv[3];    /* Reserved for future use. */
	/*
	 * Array of interleaved system/phc time stamps. The kernel
	 * will provide 2*n_samples + 1 time stamps, with the last
	 * one as a system time stamp.
	 */
	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
  };

So the kernel cannot simply change n_samples to 1.

I would prefer to have a new system call that compares any two posix
clock_t, but that is of course more work.

Allowing n_samples=1 as a special case is a kind of overloading of the
ioctl to support the new capability.  At least it preserves the
behavior of the interface from the user's perspective.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, 22 Aug 2015, Richard Cochran wrote:
> On Sat, Aug 22, 2015 at 10:33:48PM +0200, Thomas Gleixner wrote:
> > > @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> > >  			break;
> > >  		}
> > >  		pct = &sysoff->ts[0];
> > > -		for (i = 0; i < sysoff->n_samples; i++) {
> > > -			getnstimeofday64(&ts);
> > > +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&
> > 
> > The number of samples should be irrelevant for this sampling method.
> 
> Chris had send me a preview of this before he posted, so I can explain
> that test for one sample.
> 
> User space requests N (1 to 25) samples of the two clocks.  The kernel
> is supposed to deliver that many samples.  This has always been the
> documented behavior.  From ptp_clock.h:
> 
>   struct ptp_sys_offset {
> 	unsigned int n_samples; /* Desired number of measurements. */
> 	unsigned int rsv[3];    /* Reserved for future use. */
> 	/*
> 	 * Array of interleaved system/phc time stamps. The kernel
> 	 * will provide 2*n_samples + 1 time stamps, with the last
> 	 * one as a system time stamp.
> 	 */
> 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>   };
> 
> So the kernel cannot simply change n_samples to 1.
> 
> I would prefer to have a new system call that compares any two posix
> clock_t, but that is of course more work.
> 
> Allowing n_samples=1 as a special case is a kind of overloading of the
> ioctl to support the new capability.  At least it preserves the
> behavior of the interface from the user's perspective.

So why can't you take N samples from the synced hardware? It does not
make any sense to me to switch to the imprecise mode if nsamples > 1.

You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
-ENOSYS if hardware timestamping is not available and avoid the whole
nsamples dance for the case where we can get precise timestamps.

Thanks,

	tglx


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, Aug 23, 2015 at 10:15:00AM +0200, Thomas Gleixner wrote:
> So why can't you take N samples from the synced hardware? It does not
> make any sense to me to switch to the imprecise mode if nsamples > 1.

Ok, then I prefer to leave this "imprecise" method in place and ...
 
> You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
> -ENOSYS if hardware timestamping is not available and avoid the whole
> nsamples dance for the case where we can get precise timestamps.

have this for the new way.

By keeping the imprecise method, we will be able to run both methods
on the new hardware.  That will help to quantify how imprecise the old
method is.

Thanks,
Richard

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Sunday, August 23, 2015 4:26 AM
> To: Thomas Gleixner
> Cc: Hall, Christopher S; Kirsher, Jeffrey T; hpa@zytor.com;
> mingo@redhat.com; john.stultz@linaro.org; x86@kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; peterz@infradead.org
> Subject: Re: [PATCH v3 3/4] Add support for driver cross-timestamp to
> PTP_SYS_OFFSET ioctl
> 
> On Sun, Aug 23, 2015 at 10:15:00AM +0200, Thomas Gleixner wrote:
> > So why can't you take N samples from the synced hardware? It does not
> > make any sense to me to switch to the imprecise mode if nsamples > 1.
> 
> Ok, then I prefer to leave this "imprecise" method in place and ...
> 
> > You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
> > -ENOSYS if hardware timestamping is not available and avoid the whole
> > nsamples dance for the case where we can get precise timestamps.
> 
> have this for the new way.
> 
> By keeping the imprecise method, we will be able to run both methods
> on the new hardware.  That will help to quantify how imprecise the old
> method is.

This means: remove code changes from the PTP_SYS_OFFSET ioctl and call getsynctime64() from a new ioctl PTP_SYS_OFFSET_PRECISE.  Right?

And use the same type (struct ptp_sys_offset) for the new ioctl?  Or should a new simplified struct be used? Such as:

struct precise_ptp_sys_offset {
	struct ptp_clock_time device;
	struct ptp_clock_time system;
};

Does it make sense to keep the "cross-timestamp" capabilities flag as-is?

> 
> Thanks,
> Richard

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Aug 24, 2015 at 08:16:51PM +0000, Hall, Christopher S wrote:
> 
> This means: remove code changes from the PTP_SYS_OFFSET ioctl and call getsynctime64() from a new ioctl PTP_SYS_OFFSET_PRECISE.  Right?

Yes.
 
> And use the same type (struct ptp_sys_offset) for the new ioctl?  Or should a new simplified struct be used? Such as:
> 
> struct precise_ptp_sys_offset {
> 	struct ptp_clock_time device;
> 	struct ptp_clock_time system;
> };

I don't have a strong preference either way.  I would not mind reusing
the existing struct.

> Does it make sense to keep the "cross-timestamp" capabilities flag as-is?

Yes, indeed.
 

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html