Re: [PATCH v6 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 6318 bytes --]

On Tue, 2026-08-04 at 19:59 +0200, Maciek Machnikowski wrote:
> 
> What is the practical use of UNRELIABLE vs UNKNOWN.

It's "known bad" vs. "take it with a pinch of salt and make your own
mind up".

> I think it can be simplified to UNRELIABLE, SYNCED and HOLDOVER. All
> clocks will start in UNRELIABLE, switch to SYNCED whenever the clock
> gets synchronized correctly and enter HOLDOVER if it was at the SYNCED
> state and report the last synced for that state. Status INITIALIZING has
> no practical meaning and requires more work from the APIs, and
> HOLDOVER->FREE_RUNNING state transition is undefined (what is
> "acceptable" for one application won't be acceptable for another)


It feels like we're deep into bikeshedding at this point. We *have* a
set of clock status fields defined in the VMClock specification:
https://uapi-group.org/specifications/specs/vmclock/

The best time to have this kind of discussion would have been when we
were working on that. In general the VMClock fields are largely based
on virtio-rtc and designed to be identical to it, but I believe the
status field is one of the ones that virtio-rtc dropped (or perhaps
never had).

We could absolutely have Linux define its own set of status values —
but it seems like there would have to be a *significant* improvement of
the status quo in order to want to do so, *and* we'd need to be able to
map to/from the VMClock values.

Even if we could come up with tweaks which would be a better model,
having to do that mapping probably ends up being a net loss.

Unless we feel strongly enough about these changes that it's worth
revising the spec?

> Do we need this many timescales defined here? Can't we just use the
> notion of TIMESCALE_ARBITRARY and TIMESCALE_ABSOLUTE with the UTC
> offset? 

No, the difference between UTC and TAI is not (yet) just a constant
offset. And you can be on TAI without even knowing what the UTC offset
is — arguably one of the use cases for TAI is for when you don't *want*
to know or care about UTC.

> CLOCK_MONOTONIC / MONOTONIC_RAW are really ARBITRARY, is there
> any value of differentating between those two?

Yes, they behave very differently. The _RAW version doesn't get
disciplined.

And it's also important to have a clear value for any smeared-
leapsecond abomination which *doesn't* pretend it's UTC.



>  > +
> >  /*
> >   * struct ptp_clock_time - represents a time value
> >   *
> > @@ -94,6 +237,136 @@ struct ptp_clock_time {
> >  	__u32 reserved;
> >  };
> >  
> > +/*
> > + * Hardware counter identifiers for struct ptp_sys_time.sys_counter_id
> > + */
> > +enum ptp_counter_id {
> > +	/* Counter value not available or type not specified */
> > +	PTP_COUNTER_UNKNOWN = 0,
> > +
> > +	/* x86 Time Stamp Counter (TSC) */
> > +	PTP_COUNTER_X86_TSC = 1,
> > +
> > +	/* ARM Generic Timer virtual counter */
> > +	PTP_COUNTER_ARM_ARCH = 2,
> > +};
> > +
> > +/* Valid flags for struct ptp_clock_attrs.valid */
> > +#define PTP_ATTRS_VALID_ERROR_BOUND	(1 << 0)
> > +#define PTP_ATTRS_VALID_TIMESCALE	(1 << 1)
> > +#define PTP_ATTRS_VALID_STATUS		(1 << 2)
> > +
> > +/**
> > + * struct ptp_clock_attrs - quality attributes for a PHC timestamp
> > + *
> > + * @valid:       Bitmask of PTP_ATTRS_VALID_* indicating which fields
> > + *               are populated. Zero means no attributes available.
> > + * @error_bound: Maximum error (an upper bound, in nanoseconds) between the
> > + *               returned device_time and true time on the advertised
> > + *               @timescale; a worst-case bound, not a statistical estimate.
> > + *               Valid only when PTP_ATTRS_VALID_ERROR_BOUND is set, and must
> > + *               not be trusted when @status is PTP_CLOCK_STATUS_UNKNOWN or
> > + *               PTP_CLOCK_STATUS_UNRELIABLE.
> > + * @timescale:   Clock timescale (enum ptp_clock_timescale). Valid only
> > + *               when PTP_ATTRS_VALID_TIMESCALE is set.
> > + * @status:      Synchronization status (enum ptp_clock_status). Valid
> > + *               only when PTP_ATTRS_VALID_STATUS is set. Transitions between
> > + *               states are device-specific; there are no kernel-defined
> > + *               thresholds relating @status to @error_bound.
> > + * @rsv:         Reserved for future use, must be zero.
> > + */
> > +struct ptp_clock_attrs {
> > +	__u32 valid;
> > +	__u32 error_bound;
> > +	__u32 timescale;
> > +	__u32 status;
> > +	__u32 rsv[4];
> > +};
> This does not look like a practical model. Error bounds changes over
> time and will get stale by the time you transfer them over IOCTL.
> 
> I think a more practical approach would be to return error_bound
> alongside the anchor_timestamp at which it was measured. 

Huh, is that not exactly how they're used? They're passed in the
context of an actual timestamp ioctl.


> For example you
> can define master_offset for the RX timestamp of the Sync packet, or the
> offset for the last 1PPS signal you received. This should be
> complemented by some drift_factor that would define how fast it grows.
> 
> If you capture the time from the PHC you can use the formula of
> error_bound+(phc_time - anchor_timestamp)*drift_factor to interpolate
> the boundaries.

I guess we could add that too but it wasn't the primary use case here,
as I understand it. A user fetching a timestamp wants to know the
bounds of *that* timestamp — what is the latest, and the earliest, that
it could actually have been at the time in question. Tracking the
*ongoing* error bounds is a different game.

> Do we want to squash all errors into a single error_bound - it may make
> sense to add the hw_error and sync_error - hw_error would squash
> hw-based error bounds (such as clock resolution, GNSS precision) and
> sync_error (which would hold the dynamic part derived from the sync
> protocol)

Where does that end? Individually tracking an arbitrary number of error
hops all the way from the master / tier 1 clock? What's important is
the total resulting error, isn't it?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]