Re: [RFC PATCH v2 0/8] timekeeping: Fix draft tracking precision and add feed-forward discipline via vmclock

[Thomas Gleixner <tglx@kernel.org>]

On Fri, May 22 2026 at 17:23, David Woodhouse wrote:
> On Fri, 2026-05-22 at 17:28 +0200, Thomas Gleixner wrote:
>> This raises an interesting question. Must any of the existing PTM using
>> drivers mplement that new extended getcrosstimestampattr() callback, in
>> order to expose the cycles/csid in attr or can you fallback to the
>> existing callback and have the rest of the fields 0?
>>
>> Same question arises if you change the pre/post timestamp helpers to
>> utilize ktime_get_snapshot_id(). All existing drivers which use them
>> will then automatically retrieve cs_cycles/cs_id.
>
> Taking those in reverse order... yes, this means that with a new
> variant of PTP_SYS_OFFSET_EXTENDED, userspace can see actual counter
> values even for the system parts of those ABA timestamps, even for non-
> PTM clocks, and discipline the TSC/archcounter against the external
> clock.

Correct.

> Currently I have userspace which literally does rdtsc() either side of
> calling the ioctl :)

Why am I not surprised? :)

> And PTM devices can be used with PTP_SYS_OFFSET_PRECISE, which goes
> through get_device_system_crosststamp() as described, and all just
> works? It's just that we now allow userspace to *see* the counter value
> that the driver was already generating.

A new variant of PRECISE

> So to your questions: although there's new userspace ioctl support, the
> *drivers* don't need any modification for that (as long as they use the
> standard prets/postts helpers).

Yes.

> The remaining question is the device timestamp part (the 'B' in the ABA
> sandwich) for PTP_SYS_OFFSET_EXTENDED with PTM-capable drivers. Should
> that get a counterval?

PTM-capable driver support cross timestamps, which will with a new
version of PTP_SYS_OFFSET_PRECISE expose the system counterval. No ABA
for that as it's hardware latched AB.

> I don't have a strong opinion. On one hand we'd have to find a way to
> convert it from PTM for devices where it actually *is* PTM, and that's
> what PTP_SYS_OFFSET_PRECISE is *for*.

Correct.

> But on the other hand, can't the conversion be a whole lot simpler than
> get_device_system_crosststamp() because it's not actually dealing with
> any timekeepers; it's basically only invoking convert_base_to_cs()?

But what for? If you have PTM, use PRECISE. There is _zero_ value of
having pre/post timestamps when the hardware already does the correlated
precise sampling, no?

> And the ioctl should support it *all* but just have a clear way of
> indicating that any of the optional fields including the attrs are
> *not* populated (or use 0/max values maybe?).

Yes.

> So no, I don't think any driver *has* to add any attr support in order
> to expose counter values to userspace. The only reason I asked Arthur
> to mix those things up was for the *userspace* API, to avoid adding yet
> another ioctl over and over again. And now I feel bad for doing so :)

I think you can create _one_ data structure variant, which fits both
EXTENDED_ATTR and PRECISE_ATTR:

struct attrs {
       u32	valid;
       u32	error_bound;
       ....
       u32	reserved[N];
};

@valid tells user space, which of the attributes has been filled in by
the driver. That avoids bounds based validity checks, which are a pain
as you might end up with different bounds for every attribute. Having a
valid flags field avoids that completely.

struct devtime {
        ptp_clock_time	device_time;
        struct attrs	attrs;
};

struct systime {
	u64	sys_systime;
        u64     sys_rawtime;
        u64     sys_counter;
        u32	sys_counter_id;
        u32	reserved;
};

Exposing @sys_counter_id requires to expose CSID_* in the user space ABI
reliably, as otherwise a kernel internal CSID enum change would blow up
the user space guess work. Your ptp_counter_id approach is error prone.

struct timestamp {
	union {
		struct systime		systime;
		struct systime		pre_systime;
	};
	struct devtime			devtime;
	struct systime			post_systime;
};

struct request {
	u32		valid;
	clockid_t	clock_id;
	unsigned int	num_samples;
        u32		reserved[N];
};

I rather have @valid here too. The 'zero the reserved' members approach
is a pain as new kernels have to map 0 to default behavior instead of
being free to make 0 mean what they intend. @valid allows you to use
other sizes than u32 for future fields. All you have to take care of is
to keep the existing fields at the same place as before.

struct ioctl_data {
	struct request		request;
        struct timestamp	timestamps[];
};

So for both PTP_SYS_OFFSET_EXTENDED_ATTRS and
PTP_SYS_OFFSET_PRECISE_ATTRS user space allocates enough space to
accomodate data::request::num_samples.

For PTP_SYS_OFFSET_PRECISE_ATTRS num_samples has to be 1 and
data::timestamps[0].post_systime is zeroed by the kernel because it has
no meaning.

So now in the kernel you do:

ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void __user *argptr)
{
        struct ioctl_data __user *data = argptr;
        struct request;

        if (copy_from_user(&request, &data->request, sizeof(request)))
        	return -EFAULT;

        if (!extattr_request_valid(request))        	
        	return -EINVAL;

        for (unsigned int i; i < request.num_samples; i++) {
        	struct ptp_system_timestamp sts = { .clock_id = request.clock_id, };
	        struct timestamp uts = { };
                struct timespec64 devts;

        	if (ptp->info->gettimex64_attr)
                	ret = ptp->info->gettimex64_attr(ptp->info, &dev_ts, &sts, &uts.attr);
                else if (ptp->info->gettimex64)
                	ret = ptp->info->gettimex64(ptp->info, &dev_ts, &sts);
                else
                	return -ENOTSUPP;

                if (ret)
                	return ret;

               uts.pre_systime = mangle(sts.pre_systime);
               uts.devtime.device_time = mangle(dev_ts);
               uts.post_systime = mangle(sts.post_systime);
               if (!copy_to_user(&data->timestamps[i], uts, sizeof(uts)))
               		return -EFAULT;
	}
        return 0;
}

ptp_sys_offset_precise_attrs(struct ptp_clock *ptp, void __user *argptr)
{
        struct ioctl_data __user *data = argptr;
        struct request;

        if (copy_from_user(&request, &data->request, sizeof(request)))
        	return -EFAULT;

        if (!preciseattr_request_valid(request))        	
        	return -EINVAL;

	struct system_device_crosststamp xtstamp = { .clock_id = request.clock_id, };
        struct timestamp uts = { };
        
        if (ptp->info->getcrosststamp_attr)
                ret = ptp->info->getcrosststamp_attr(ptp->info, &xtstamp, &uts.attr);
        else if (ptp->info->getcrosststamp)
              	ret = ptp->info->getcrosststamp(ptp->info, &xtstamp);
        else
              	return -ENOTSUPP;

        if (ret)
              	return ret;

        uts.systime = mangle(xtstamp.systime);
        uts.devtime.device_time = mangle(xtstamp.device);
        if (!copy_to_user(&data->timestamps[0], uts, sizeof(uts)))
        	return -EFAULT;
        return 0;
}

Or something like this, which immediately enables the functionality for
all drivers which implement the getcrosststamp() or the gettimex64()
callbacks with a unified user space data structure.

The attributes.valid bits are all zero and and once drivers implement
the _attr callback variants, those attributes supported by the driver
will magically appear with the corresponding valid bits set.

Hmm?

Thanks,

        tglx