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

On 2026-07-27 10:03:28+01:00, David Woodhouse wrote:
> On Fri, 2026-07-17 at 06:59 +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 system counter (e.g. TSC ticks) captured by the
> >    timekeeping core alongside each system timestamp
> > 5. counter_id: Identifies the counter source (e.g. TSC, ARM arch counter)
> >
> > 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 system 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 original RFC and the UAPI design
> > discussion linked below.
> >
> > Link: https://lore.kernel.org/netdev/[email protected]/
> > Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/
> > Signed-off-by: Amit Bernstein <[email protected]>
> > Signed-off-by: Arthur Kiyanovski <[email protected]>
> 
> ...
> 
> > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user *arg)
> > +{
> > +	struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
> > +	struct ptp_attrs_request request;
> > +	struct ptp_system_timestamp sts;
> > +	unsigned int n_samples;
> > +	int err;
> > +
> > +	if (copy_from_user(&request, arg, sizeof(request)))
> > +		return -EFAULT;
> > +
> > +	if (request.valid ||
> > +	    request.num_samples > PTP_MAX_SAMPLES ||
> > +	    request.num_samples == 0)
> > +		return -EINVAL;
> > +
> > +	err = ptp_validate_sys_offset_clockid(request.clock_id);
> > +	if (err)
> > +		return err;
> > +
> > +	n_samples = request.num_samples;
> > +	sts.clockid = request.clock_id;
> > +
> > +	data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->request.num_samples = n_samples;
> > +
> > +	for (unsigned int i = 0; i < n_samples; i++) {
> > +		struct ptp_clock_attrs att = {};
> > +		struct timespec64 ts;
> > +
> > +		if (ptp->info->gettimexattrs64)
> > +			err = ptp->info->gettimexattrs64(ptp->info, &ts,
> > +							 &sts, &att);
> > +		else if (ptp->info->gettimex64)
> > +			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> > +		else
> > +			return -EOPNOTSUPP;
> > +
> > +		if (err)
> > +			return err;
> > +
> > +		/* Filter out disabled or unavailable clocks */
> > +		if (!sts.pre_sts.valid || !sts.post_sts.valid)
> > +			return -EINVAL;
> > +
> > +		data->timestamps[i].pre_systime.sys_time =
> > +			ktime_to_ns(sts.pre_sts.systime);
> > +		data->timestamps[i].pre_systime.sys_rawtime =
> > +			ktime_to_ns(sts.pre_sts.monoraw);
> > +		data->timestamps[i].pre_systime.sys_counter =
> > +			sts.pre_sts.cycles;
> 
> I hate all this line wrapping, btw. I'll defer to the net coding style
> if they insist, but my preference would just be just to have longer
> lines. Especially when it's a block of assignments like this, the
> wrapped form is *much* harder to read.
>

Agreed — I'll reduce the wrapping in v6 (a local temp var + a small helper
make that block read much better). Thanks.
 
> > +		data->timestamps[i].pre_systime.sys_counter_id =
> > +			sts.pre_sts.cs_id;
> 
> You added PTP_COUNTER_* constants as we discussed¹... but didn't you
> forget to *map* to them here?
> 
> ¹ https://lore.kernel.org/all/877boqtg3g.ffs@tglx/
>

Good catch — you're right, that's a bug. I'll add the csid→PTP_COUNTER_*
mapping in v6. 

> 
> Arguably, if the timestamps have a hw_csid then you should be returning
> that in place of the main one which will be kvmclock or hyperv
> nonsense. You can do that here for extended_attrs but precise_attrs
> will need a bit more work (as system_device_crosststamp doesn't have
> the hw_* fields). I won't randomise you further by asking you to handle
> that though. I'll do it in a follow-up.
> 

Thanks — I'll leave the hw_csid part for your follow-up as you offered. This
series exposes the base cs_id and leaves room to add it without an ABI
change.

> You can either map KVMCLOCK to unknown or give it a
> PTP_COUNTER_KVMCLOCK if you prefer. I suggest the former, as nobody who
> cares about time should be using kvmclock. And it means my promised
> follow-up will be providing counter data which previously wasn't
> provided, rather than *changing* what's provided.

Will do the former in v6 — map KVMCLOCK to UNKNOWN — so your follow-up adds
new counter data rather than changing existing values. Thanks.