On Fri, 2026-05-15 at 16:40 +0000, Arthur Kiyanovski wrote:
> Introduce two new ioctls that extend existing PTP timestamp interfaces
> with clock quality information:
> 
> - PTP_SYS_OFFSET_EXTENDED_ATTRS: Extends PTP_SYS_OFFSET_EXTENDED
> - PTP_SYS_OFFSET_PRECISE_ATTRS: Extends PTP_SYS_OFFSET_PRECISE
> 
> These ioctls provide quality attributes alongside timestamps:
> 
> 1. error_bound: Maximum deviation from true time (nanoseconds), based
>    on device's internal clock state
> 2. clock_status: Synchronization state (unknown, initializing,
>    synchronized, free-running, unreliable)
> 3. timescale: Time reference (TAI, UTC, etc.)
> 4. counter_value: Raw hardware counter (e.g. TSC ticks) at the time of
>    the PHC reading, for feed-forward calibration use cases
> 5. counter_id: Identifies the hardware counter type (enum ptp_counter_id)
> 
> This supports three use cases:
> 
> 1. Managed PHC devices (e.g., ENA, vmclock) that maintain their own
>    synchronization and can report quality metrics directly to userspace
>    without requiring ptp4l
> 
> 2. Applications that need complete time quality information in a single
>    call, regardless of how the PHC is synchronized
> 
> 3. VMMs that need raw hardware counter values paired
>    with PTP timestamps for feed-forward clock calibration, avoiding the
>    feedback loop inherent in NTP-style synchronization
> 
> Timescale definitions use a Continuity/Discipline framework to describe
> timeline properties and steering behavior consistently across all
> entries.
> 
> This implementation is based on the RFC discussion linked below.
> 
> Link: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Amit Bernstein <[email protected]>
> Signed-off-by: Arthur Kiyanovski <[email protected]>

I confess I kind of hate the way we add entirely new ioctls,
progressing from PTP_SYS_OFFSET to PTP_SYS_OFFSET_EXTENDED and now to
PTP_SYS_OFFSET_EXTENDED_ATTRS.

The historical change from PTP_SYS_OFFSET to PTP_SYS_OFFSET_EXTENDED
wasn't just changing rsv[0] to clockid; it *also* changed the
subsequent array of timestamps from interleaved ABABABA to a set of
3-tuples ABA ABA ABA.

That long predates me paying attention to PTP, and I guess it makes
sense, even if it might have been possible to overload the same ioctl
with some kind of flags field to ask for the new mode.

Even so, part of me wants to insist that you try *really* hard to shoe-
horn the new fields into the existing ptp_sys_offset_extended, turning
another of the rsv[] words into a flags field in which the user *asks*
for the error information, and/or the pure cycle counts, which would be
appended at the end of the existing structs without a whole new ioctl.

As things stand though, I settled for looking at least a few weeks into
the future and asking that you add both the error bounds *and* the
cycle counts at the same time, so that at least this new ioctl handles
the use cases I can see on the horizon.

So I'm OK with the new ioctls.


However...

+               extoffattrs->ts[i][0].pct.sec = sts.pre_ts.tv_sec;
+               extoffattrs->ts[i][0].pct.nsec = sts.pre_ts.tv_nsec;
+               extoffattrs->ts[i][1].pct.sec = ts.tv_sec;
+               extoffattrs->ts[i][1].pct.nsec = ts.tv_nsec;
+               extoffattrs->ts[i][1].att.error_bound = att.error_bound;
+               extoffattrs->ts[i][1].att.timescale = att.timescale;
+               extoffattrs->ts[i][1].att.status = att.status;
+               extoffattrs->ts[i][1].att.counter_id = att.counter_id;
+               extoffattrs->ts[i][1].att.counter_value = att.counter_value;
+               extoffattrs->ts[i][2].pct.sec = sts.post_ts.tv_sec;
+               extoffattrs->ts[i][2].pct.nsec = sts.post_ts.tv_nsec;

... that's nice for the cases I personally care about most where we
have a counter reading at the precise moment of the external clock
reading which is ts[1], but what about the cases (like the patches now
being posted for GVE if I understand correctly) where we still have to
have the sandwiched ABA readings. Don't we want the counter values for
those (ts[0] and ts[2]) too?

Other minor nits:

+       /**
+        * International Atomic Time (TAI)
+        * Epoch: 1958-01-01 00:00:00.
+        * Continuity: Strictly monotonic and continuous; no leap seconds.
+        * Discipline: Primary atomic reference; no phase jumps.
+        */
+       PTP_TIMESCALE_TAI = 1,

The current TAI offset is 37. If I add 37 to a value with that 1958
epoch, I don't get a UTC value with the 1970 epoch you described for
UTC. It's cosmetic... but confusing. 

+/*
+ * ptp_sys_offset_extended_attrs - data structure for IOCTL operation
+ *                                PTP_SYS_OFFSET_EXTENDED_ATTRS
+ *
+ * @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].
+ *             Each sample consists of timestamp in the form [sec, nsec],
+ *             while the PHC sample also includes clock attributes in the form
+ *             [error_bound, timescale, status].
+ *
+ * Starting from kernel 6.12 and onwards, the first word of the reserved-field
+ * is used for @clockid. That's backward compatible since previous kernel
+ * expect all three reserved words (@rsv[3]) to be 0 while the clockid (first
+ * word in the new structure) for CLOCK_REALTIME is '0'.
+ */

I don't think we need the 'from kernel 6.12  and onwards' part for this one :)

Attachment: smime.p7s
Description: S/MIME cryptographic signature

Reply via email to