From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:31:55
This series adds quality attributes to PTP Hardware Clock (PHC)
timestamps, allowing userspace to obtain error bound, clock status,
timescale, and system counter values alongside timestamps in a single
call.
Motivation
----------
The existing PTP APIs return timestamps without any indication of
their quality. Applications that need clock accuracy and
synchronization status commonly rely on external tools such as
ptp4l, which implement synchronization logic and can export their
measurement of clock accuracy. For managed PHC devices - such as
the ENA network adapter, whose clock is synchronized by the device
without userspace involvement - these tools are not available, and
the existing APIs lack a way to report quality metrics to consumers
of time.
This was previously proposed as an RFC [1] with a single ioctl.
Based on community feedback, the design was reworked to cover both
the extended (multi-sample) and precise (cross-timestamp) paths.
Design
------
The UAPI was redesigned based on Thomas Gleixner's proposal [2]:
- A unified data structure (struct ptp_sys_offset_attrs) is used
for both extended and precise ioctls.
- A u32 valid bitmask in struct ptp_clock_attrs indicates which
attributes are populated, replacing sentinel values. Drivers
set only the bits for attributes they provide.
- System counter values (cycles + counter_id) are carried in
struct ptp_sys_time alongside each system timestamp. These are
populated by the timekeeping core cross-timestamp infrastructure,
which is now merged in net-next [3] - drivers do not fill them.
This series therefore applies directly to net-next with no
out-of-tree dependency.
- Graceful degradation: the attrs ioctls work even on devices
without attrs callbacks, falling back to gettimex64 /
getcrosststamp and returning attrs.valid = 0. The capability
bits reflect this fallback so cap-respecting userspace does not
skip the ioctl on base-only drivers.
A capability flag is added to ptp_clock_caps so userspace can
discover attributes support.
Patches 2-3 add testptp support for the new ioctls.
Patch 4 implements the attributes for ptp_vmclock, reporting
error bound, clock status, and timescale.
Patches 5-7 implement the attributes for the ENA driver,
reporting error bound from the device's PHC layer.
v7:
- ptp_vmclock: only set PTP_ATTRS_VALID_ERROR_BOUND when the
hypervisor flags the base error valid
(VMCLOCK_FLAG_TIME_MAXERROR_VALID); otherwise the bit stays clear
so userspace sees "not provided" rather than a UINT_MAX that looks
like a real ~4.29 s bound. (Simon Horman, sashiko)
- ptp_vmclock: drop the counter_period_shift >= 128 guard added in
v3. counter_period_shift is read from the shared page and passed
to the same helper on the main timestamp path in
vmclock_get_crosststamp() with no bounds check, so guarding only
the new attrs path diverged from the rest of the driver. With the
guard gone the attrs path reads the field once, so the divergent
double-read the review flagged no longer exists. If the shift
should be bounds-checked, that is a fix to the existing driver and
better handled on its own. (Simon Horman, sashiko)
- ptp_vmclock: clear att->valid and att->error_bound at the top of
vmclock_populate_ptp_attributes(). The function runs once per
seq_count retry iteration and only ORs into att->valid, so a
discarded torn read could otherwise leave
PTP_ATTRS_VALID_ERROR_BOUND set together with a stale error_bound
that is not part of the accepted snapshot. (sashiko)
- ptp_vmclock: drop David Woodhouse's Reviewed-by from patch 4 as
the patch changed.
- No UAPI changes; patches 1-3 and 5-7 are unchanged from v6.
v6:
- ptp: reject a non-zero reserved field (request.rsv[]) in the
attrs ioctls, matching ptp_sys_offset_extended(). (Jakub Kicinski)
- ptp: zero-initialize the per-sample system-timestamp snapshot in
ptp_sys_offset_extended_attrs() so no uninitialized or stale stack
data can reach userspace. (Jakub Kicinski)
- ptp: translate the kernel enum clocksource_ids to the UAPI enum
ptp_counter_id before returning sys_counter_id; sources that are
not a raw hardware counter (e.g. kvmclock) are reported as
PTP_COUNTER_UNKNOWN with sys_counter = 0. (Jakub Kicinski,
David Woodhouse)
- ptp: initialize the precise cross-timestamp struct and set its
clock_id from the validated request; validate clock_id against a
precise-path allowlist (CLOCK_REALTIME + CLOCK_AUX) so unsupported
clocks return -EINVAL instead of triggering a WARN in
get_device_system_crosststamp(). (Jakub Kicinski, David Woodhouse)
- ptp: make the extended_attrs/precise_attrs capability bits reflect
the base-callback fallback (gettimexattrs64 || gettimex64,
getcrosststampattrs || getcrosststamp) and document the fallback
and the att.valid semantics. (Jakub Kicinski)
- ptp: echo the full request header back from the attrs ioctls so
valid/clock_id/num_samples/rsv round-trip. (Jakub Kicinski)
- ptp: clarify the FREE_RUNNING vs UNRELIABLE and HOLDOVER status
comments and the @error_bound kernel-doc (a maximum bound, not a
statistical estimate; not to be trusted when status is UNKNOWN or
UNRELIABLE). (Carolina Jubran)
- ptp: document that both @sts and @att may be NULL in the
gettimexattrs64 kernel-doc. (Jakub Kicinski)
- selftests/ptp: print the returned system counter value and id
(sys_counter, sys_counter_id) in the attrs output.
- Exposing the underlying hw_csid/hw_cycles for derived clocksources
(kvmclock, ART, ...) is left to a follow-up; the UAPI leaves room
to add them without an ABI change. (David Woodhouse)
v5:
- ptp: simplify ptp_sys_offset_{extended,precise}_attrs() to copy
the request header onto the stack and allocate the result buffer
once, instead of memdup_user() + kfree() + kzalloc(). (Saeed
Bshara)
- ptp: deduplicate the kernel-doc for gettimexattrs64 /
getcrosststampattrs by referencing the base gettimex64 /
getcrosststamp callbacks. (Saeed Bshara)
- ptp: add missing kernel-doc member descriptions for struct
ptp_timestamp and struct ptp_sys_offset_attrs.
- selftests/ptp: map the clockid to a name and print once instead
of three duplicated printfs. (Saeed Bshara)
- selftests/ptp: make -a a modifier on -x/-X instead of an
exclusive option; drop -A (use -X -a); trim the unknown-clock
output; print "not reported" for attributes the device does not
report. (Saeed Bshara)
- ptp_vmclock: scope the local 'st' inside the SUPPORT_KVMCLOCK
block to fix an unused-variable warning on !CONFIG_KVM_GUEST.
(kernel test robot)
- No UAPI changes; the ioctl structures are unchanged from v4.
v4:
- Complete UAPI redesign per Thomas Gleixner's proposal [2]:
unified data structure with u32 valid bitmask, system counter
in ptp_sys_time (populated by core, not drivers), graceful
degradation for devices without attrs callbacks. (Thomas Gleixner,
David Woodhouse)
- Counter values moved from driver attrs callback to timekeeping
core infrastructure - drivers no longer set counter_id or
counter_value.
- Flexible array member for timestamps[] (kernel bounds the copy,
userspace allocates for num_samples requested).
- Drop separate ptp_clock_attributes kernel struct - driver
callbacks fill the UAPI ptp_clock_attrs directly.
v3:
- Remove patch 5/8 from v2 (return-code bugfix) - sent separately
as [PATCH net] to the net tree.
- Zero-initialize struct ptp_clock_attributes in PTP core ioctl
handlers to prevent stack leak of unset fields. (Simon Horman,
sashiko)
- ptp_vmclock: validate counter_period_shift < 128 to prevent
undefined behavior on untrusted hypervisor input. (sashiko)
- ptp_vmclock: add overflow check on err_hi * NSEC_PER_SEC to
prevent silent wraparound producing erroneously small error
bound. (sashiko)
- ptp_vmclock: report PTP_TIMESCALE_TAI after tai_adjust() to
avoid timescale mismatch. (sashiko)
- ENA: set counter_id = 0, counter_value = 0 in gettimexattrs64
for defense-in-depth. (sashiko)
v2:
- Fix build bisectability: move ena_com.c consumer updates into
patch 6/8 and ena_phc.c caller update into patch 7/8 so each
patch compiles independently.
- Add missing Cc for Amit Bernstein (co-author of ENA patches).
[1] https://lore.kernel.org/netdev/20250724115657.150-1-darinzon@amazon.com/
[2] https://lore.kernel.org/all/87se7ht25o.ffs@tglx/
[3] https://lore.kernel.org/all/20260526165826.392227559@kernel.org/
Arthur Kiyanovski (7):
ptp: Add ioctls for PHC timestamps with quality attributes
selftests/ptp: Extract print_system_timestamp helper in testptp
selftests/ptp: Add testptp support for attributes ioctls
ptp: ptp_vmclock: Implement attributes ioctls
net: ena: Update PHC admin interface for error bound support
net: ena: Add error bound to PHC communication layer
net: ena: Implement gettimexattrs64 callback for PTP attributes
.../device_drivers/ethernet/amazon/ena.rst | 2 +
.../net/ethernet/amazon/ena/ena_admin_defs.h | 17 +-
drivers/net/ethernet/amazon/ena/ena_com.c | 51 ++--
drivers/net/ethernet/amazon/ena/ena_com.h | 5 +-
drivers/net/ethernet/amazon/ena/ena_debugfs.c | 3 +
drivers/net/ethernet/amazon/ena/ena_phc.c | 61 +++-
drivers/ptp/ptp_chardev.c | 208 ++++++++++++-
drivers/ptp/ptp_clock.c | 4 +-
drivers/ptp/ptp_vmclock.c | 199 +++++++++++-
include/linux/ptp_clock_kernel.h | 20 ++
include/uapi/linux/ptp_clock.h | 283 +++++++++++++++++-
tools/testing/selftests/ptp/testptp.c | 198 +++++++++---
12 files changed, 941 insertions(+), 110 deletions(-)
base-commit: 548b86839f7fb819a4d6c83b71c73ec378d24275
--
2.47.3
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:13
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; reported as 0 with
counter_id PTP_COUNTER_UNKNOWN when the counter source cannot be
identified
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.
The _ATTRS ioctls fall back to the base timestamp callbacks (gettimex64 /
getcrosststamp) when a driver does not implement the corresponding attrs
callback. In that case the ioctl still returns the timestamps and the
system counter/timestamp fields and reports no quality attributes. The
extended_attrs / precise_attrs capability bits therefore indicate that the
ioctl is callable, while the per-timestamp att.valid bitmask indicates
which quality attributes were actually populated (zero when the driver
provides none).
For the precise ioctl only CLOCK_REALTIME and the auxiliary clocks are
supported, matching get_device_system_crosststamp(); other clock ids are
rejected with -EINVAL.
This implementation is based on the original RFC and the UAPI design
discussion linked below.
Link: https://lore.kernel.org/netdev/20250724115657.150-1-darinzon@amazon.com/
Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/
Co-developed-by: Amit Bernstein <redacted>
Signed-off-by: Amit Bernstein <redacted>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
drivers/ptp/ptp_chardev.c | 208 +++++++++++++++++++++--
drivers/ptp/ptp_clock.c | 4 +-
include/linux/ptp_clock_kernel.h | 20 +++
include/uapi/linux/ptp_clock.h | 283 ++++++++++++++++++++++++++++++-
4 files changed, 500 insertions(+), 15 deletions(-)
@@ -404,6 +436,150 @@ static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg,returncopy_to_user(arg,extoff,sizeof(*extoff))?-EFAULT:0;}+staticu32ptp_counter_id_from_csid(enumclocksource_idscs_id)+{+switch(cs_id){+caseCSID_X86_TSC_EARLY:+caseCSID_X86_TSC:+returnPTP_COUNTER_X86_TSC;+caseCSID_ARM_ARCH_COUNTER:+returnPTP_COUNTER_ARM_ARCH;+default:+/* CSID_X86_KVM_CLK is deliberately mapped to unknown:+*kvmclockisnotarawhardwarecounter.+*/+returnPTP_COUNTER_UNKNOWN;+}+}++staticvoidptp_fill_sys_counter(structptp_sys_time*st,u64cycles,+enumclocksource_idscs_id)+{+st->sys_counter_id=ptp_counter_id_from_csid(cs_id);+st->sys_counter=st->sys_counter_id==PTP_COUNTER_UNKNOWN?0:cycles;+}++staticlongptp_sys_offset_extended_attrs(structptp_clock*ptp,void__user*arg)+{+structptp_sys_offset_attrs*data__free(kfree)=NULL;+structptp_attrs_requestrequest;+unsignedintn_samples;+interr;++if(copy_from_user(&request,arg,sizeof(request)))+return-EFAULT;++if(request.valid||+!mem_is_zero(request.rsv,sizeof(request.rsv))||+request.num_samples>PTP_MAX_SAMPLES||+request.num_samples==0)+return-EINVAL;++err=ptp_validate_sys_offset_clockid(request.clock_id);+if(err)+returnerr;++n_samples=request.num_samples;++data=kzalloc(struct_size(data,timestamps,n_samples),GFP_KERNEL);+if(!data)+return-ENOMEM;++/* echo the request header back unchanged (ioctl is _IOWR) */+data->request=request;++for(unsignedinti=0;i<n_samples;i++){+structptp_system_timestampsts={.clockid=request.clock_id};+structptp_timestamp*tstamp=&data->timestamps[i];+structptp_clock_attrsatt={};+structtimespec64ts;++if(ptp->info->gettimexattrs64)+err=ptp->info->gettimexattrs64(ptp->info,&ts,&sts,&att);+elseif(ptp->info->gettimex64)+err=ptp->info->gettimex64(ptp->info,&ts,&sts);+else+return-EOPNOTSUPP;++if(err)+returnerr;++/* Filter out disabled or unavailable clocks */+if(!sts.pre_sts.valid||!sts.post_sts.valid)+return-EINVAL;++tstamp->pre_systime.sys_time=ktime_to_ns(sts.pre_sts.systime);+tstamp->pre_systime.sys_rawtime=ktime_to_ns(sts.pre_sts.monoraw);+ptp_fill_sys_counter(&tstamp->pre_systime,sts.pre_sts.cycles,+sts.pre_sts.cs_id);+tstamp->devtime.device_time.sec=ts.tv_sec;+tstamp->devtime.device_time.nsec=ts.tv_nsec;+tstamp->devtime.attrs=att;+tstamp->post_systime.sys_time=ktime_to_ns(sts.post_sts.systime);+tstamp->post_systime.sys_rawtime=ktime_to_ns(sts.post_sts.monoraw);+ptp_fill_sys_counter(&tstamp->post_systime,sts.post_sts.cycles,+sts.post_sts.cs_id);+}++returncopy_to_user(arg,data,+struct_size(data,timestamps,n_samples))?-EFAULT:0;+}++staticlongptp_sys_offset_precise_attrs(structptp_clock*ptp,void__user*arg)+{+structptp_sys_offset_attrs*data__free(kfree)=NULL;+structsystem_device_crosststampxtstamp={};+structptp_attrs_requestrequest;+structptp_clock_attrsatt={};+structptp_timestamp*tstamp;+structtimespec64ts;+interr;++if(copy_from_user(&request,arg,sizeof(request)))+return-EFAULT;++if(request.valid||+!mem_is_zero(request.rsv,sizeof(request.rsv))||+request.num_samples!=1)+return-EINVAL;++err=ptp_validate_precise_clockid(request.clock_id);+if(err)+returnerr;++xtstamp.clock_id=request.clock_id;++data=kzalloc(struct_size(data,timestamps,1),GFP_KERNEL);+if(!data)+return-ENOMEM;++/* echo the request header back unchanged (ioctl is _IOWR) */+data->request=request;+tstamp=&data->timestamps[0];++if(ptp->info->getcrosststampattrs)+err=ptp->info->getcrosststampattrs(ptp->info,&xtstamp,&att);+elseif(ptp->info->getcrosststamp)+err=ptp->info->getcrosststamp(ptp->info,&xtstamp);+else+return-EOPNOTSUPP;++if(err)+returnerr;++ts=ktime_to_timespec64(xtstamp.device);+tstamp->systime.sys_time=ktime_to_ns(xtstamp.sys_systime);+tstamp->systime.sys_rawtime=ktime_to_ns(xtstamp.sys_monoraw);+ptp_fill_sys_counter(&tstamp->systime,xtstamp.sys_counter.cycles,+xtstamp.sys_counter.cs_id);+tstamp->devtime.device_time.sec=ts.tv_sec;+tstamp->devtime.device_time.nsec=ts.tv_nsec;+tstamp->devtime.attrs=att;++returncopy_to_user(arg,data,+struct_size(data,timestamps,1))?-EFAULT:0;+}+staticlongptp_sys_offset(structptp_clock*ptp,void__user*arg){structptp_sys_offset*sysoff__free(kfree)=NULL;
@@ -539,11 +715,17 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,returnptp_sys_offset_precise(ptp,argptr,ptp->info->getcrosststamp);+casePTP_SYS_OFFSET_PRECISE_ATTRS:+returnptp_sys_offset_precise_attrs(ptp,argptr);+casePTP_SYS_OFFSET_EXTENDED:casePTP_SYS_OFFSET_EXTENDED2:returnptp_sys_offset_extended(ptp,argptr,ptp->info->gettimex64);+casePTP_SYS_OFFSET_EXTENDED_ATTRS:+returnptp_sys_offset_extended_attrs(ptp,argptr);+casePTP_SYS_OFFSET:casePTP_SYS_OFFSET2:returnptp_sys_offset(ptp,argptr);
@@ -79,6 +79,149 @@*/#define PTP_PEROUT_V1_VALID_FLAGS (0)+/*+*Clockstatusvaluesforstructptp_clock_attrs.status+*/+enumptp_clock_status{+/* Clock synchronization status cannot be reliably determined */+PTP_CLOCK_STATUS_UNKNOWN=0,++/* Clock is acquiring synchronization */+PTP_CLOCK_STATUS_INITIALIZING=1,++/* Clock is synchronized and maintained accurately by the device */+PTP_CLOCK_STATUS_SYNCED=2,++/*+*Clockisdriftingbutremainswithinacceptableerrorbounds;+*error_boundisvalidandcanbetrusted.+*/+PTP_CLOCK_STATUS_HOLDOVER=3,++/*+*Clockisfree-running:notcurrentlydisciplinedtowardareference+*(unlikeHOLDOVER),butcoastingonaknownoscillator.error_bound+*remainsvalidandcanbetrusted,andtypicallygrowsovertime.+*/+PTP_CLOCK_STATUS_FREE_RUNNING=4,++/*+*Clockisconsideredbroken(e.g.theoscillatorisfaultyor+*abnormallyunstable):error_boundcannotbetrusted.Aclockthatis+*merelyunsynchronizedorresynchronizingshouldreport+*PTP_CLOCK_STATUS_UNKNOWNorPTP_CLOCK_STATUS_INITIALIZINGinstead.+*/+PTP_CLOCK_STATUS_UNRELIABLE=5+};++/*+*Clocktimescalevaluesforstructptp_clock_attrs.timescale.+*+*Thesedefinitionsdescribethemathematicalpropertiesandreference+*epochsofthetimescaleprovidedbythePHC.+*+*Discipline:Describesthefrequency/phasesteeringbehavior.+*Continuity:Describeswhetherthetimelineisuninterrupted.+*/+enumptp_clock_timescale{+/* Unknown or unspecified timescale */+PTP_TIMESCALE_UNKNOWN=0,++/********************* Absolute Atomic Timescales *********************+*Thesetimescalesarecontinuous,monotonicstandardsbasedonatomic+*physics.Theydonotexperiencephasejumps.+**********************************************************************/++/**+*InternationalAtomicTime(TAI)+*Epoch:1958-01-0100:00:00.+*Continuity:Strictlymonotonicandcontinuous;noleapseconds.+*Discipline:Primaryatomicreference;nophasejumps.+*/+PTP_TIMESCALE_TAI=1,++/**+*TerrestrialTime(TT)+*Epoch:1958-01-0100:00:00.+*Continuity:Strictlymonotonicandcontinuous;noleapseconds.+*Discipline:DefinedasTAI+32.184sconstantoffset.+*/+PTP_TIMESCALE_TT=2,++/**+*GlobalPositioningSystem(GPS)Time+*Epoch:1980-01-0600:00:00.+*Continuity:Strictlymonotonicandcontinuous;noleapseconds.+*Discipline:DefinedbytheGPSconstellation;fixedoffsetfromTAI.+*/+PTP_TIMESCALE_GPS=3,++/****************** UTC-Based Timescales (Civil Time) *****************+*ThesetimescalesarederivedfromTAIbutadjustedtoalignwith+*theEarth'srotation,primarilythroughleapseconds.+**********************************************************************/++/**+*CoordinatedUniversalTime(UTC)-Wall-clock(CLOCK_REALTIME)+*Epoch:1970-01-0100:00:00(Unixepoch).+*Continuity:Discontinuous;subjectto1-secondleapsecond+*phasejumps.+*Discipline:Frequencysteered;incorporatesleapsecondcorrections.+*+*Note:Leap-smearedUTCMUSTNOTbeadvertisedasPTP_TIMESCALE_UTC.+*Smearalgorithmsarenotstandardizedandtheresultingtimescale+*isambiguous.ImplementationsusingsmearedUTCMUSTadvertise+*PTP_TIMESCALE_UNKNOWNorPTP_TIMESCALE_PROPRIETARYinstead.+*/+PTP_TIMESCALE_UTC=4,++/**+*POSIXTime(UnixTime)+*Epoch:1970-01-0100:00:00.+*Continuity:Discontinuous;leapsecondshandledby+*repeating/skippingvalues.+*Discipline:FollowsUTCfrequencysteeringandphasejumps.+*/+PTP_TIMESCALE_POSIX=5,++/****************** System-Relative Monotonic Clocks ******************+*Thesetimescalesarerelativetoasystemevent(likeboot)+*andarenotsynchronizedtoanexternalatomicstandard.+**********************************************************************/++/**+*MonotonicSystemClock(CLOCK_MONOTONIC)+*Epoch:Arbitrary(Systemboottime).+*Continuity:Strictlymonotonic;noleapseconds.+*Discipline:Frequencysteeredtomatchsystemreference;+*doesnotadvanceduringsuspend.+*/+PTP_TIMESCALE_MONOTONIC=6,++/**+*RawMonotonicSystemClock(CLOCK_MONOTONIC_RAW)+*Epoch:Arbitrary(Systemboottime).+*Continuity:Strictlymonotonic;noleapseconds.+*Discipline:Rawhardwareoscillator;nofrequencysteering+*ordiscipline.+*/+PTP_TIMESCALE_MONOTONIC_RAW=7,++/**+*BootTimeSystemClock(CLOCK_BOOTTIME)+*Epoch:Arbitrary(Systemboottime).+*Continuity:Strictlymonotonicandcontinuous;noleapseconds.+*Discipline:Frequencysteeredtomatchsystemreference;+*advancesduringsuspend.+*/+PTP_TIMESCALE_BOOTTIME=8,++/********************** Vendor-Specific Timescale *********************/++/* A proprietary or vendor-specific timescale with custom rules. */+PTP_TIMESCALE_PROPRIETARY=9,+};+/**structptp_clock_time-representsatimevalue*
@@ -94,6 +237,136 @@ struct ptp_clock_time {__u32reserved;};+/*+*Hardwarecounteridentifiersforstructptp_sys_time.sys_counter_id+*/+enumptp_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)++/**+*structptp_clock_attrs-qualityattributesforaPHCtimestamp+*+*@valid:BitmaskofPTP_ATTRS_VALID_*indicatingwhichfields+*arepopulated.Zeromeansnoattributesavailable.+*@error_bound:Maximumerror(anupperbound,innanoseconds)betweenthe+*returneddevice_timeandtruetimeontheadvertised+*@timescale;aworst-casebound,notastatisticalestimate.+*ValidonlywhenPTP_ATTRS_VALID_ERROR_BOUNDisset,andmust+*notbetrustedwhen@statusisPTP_CLOCK_STATUS_UNKNOWNor+*PTP_CLOCK_STATUS_UNRELIABLE.+*@timescale:Clocktimescale(enumptp_clock_timescale).Validonly+*whenPTP_ATTRS_VALID_TIMESCALEisset.+*@status:Synchronizationstatus(enumptp_clock_status).Valid+*onlywhenPTP_ATTRS_VALID_STATUSisset.Transitionsbetween+*statesaredevice-specific;therearenokernel-defined+*thresholdsrelating@statusto@error_bound.+*@rsv:Reservedforfutureuse,mustbezero.+*/+structptp_clock_attrs{+__u32valid;+__u32error_bound;+__u32timescale;+__u32status;+__u32rsv[4];+};++/**+*structptp_sys_time-systemtimesnapshotwithcountervalue+*+*@sys_time:Systemtimeinnanoseconds(clockselectedbyrequest).+*@sys_rawtime:CLOCK_MONOTONIC_RAWtimeinnanoseconds.+*@sys_counter:Rawclocksourcecountervalue(0=unavailable).+*@sys_counter_id:Identifiesthecounter(enumptp_counter_id).+*@rsv:Reservedforfutureuse,mustbezero.+*/+structptp_sys_time{+__s64sys_time;+__s64sys_rawtime;+__u64sys_counter;+__u32sys_counter_id;+__u32rsv;+};++/**+*structptp_dev_time-devicetimestampwithqualityattributes+*+*@device_time:PHCtimestampvalue.+*@attrs:Qualityattributesforthistimestamp.+*/+structptp_dev_time{+structptp_clock_timedevice_time;+structptp_clock_attrsattrs;+};++/**+*structptp_timestamp-acompletetimestampsample+*+*@systime:Systemtimesnapshot;sharesstoragewith@pre_systime,+*usedbyPTP_SYS_OFFSET_PRECISE_ATTRS.+*@pre_systime:Systemtimereadrightbeforethedeviceread,usedby+*PTP_SYS_OFFSET_EXTENDED_ATTRS.+*@devtime:Devicetimestampwithitsqualityattributes.+*@post_systime:Systemtimereadrightafterthedeviceread,usedby+*PTP_SYS_OFFSET_EXTENDED_ATTRS.+*+*ForPTP_SYS_OFFSET_EXTENDED_ATTRS:pre_systimeandpost_systimebracket+*thedeviceread(ABAsandwich).+*ForPTP_SYS_OFFSET_PRECISE_ATTRS:onlysystime(unionwithpre_systime)+*ismeaningful;post_systimeiszeroed.+*/+structptp_timestamp{+union{+structptp_sys_timesystime;+structptp_sys_timepre_systime;+};+structptp_dev_timedevtime;+structptp_sys_timepost_systime;+};++/**+*structptp_attrs_request-requestparametersforattrsioctls+*+*@valid:Bitmaskforfuturerequestextensions.Mustbezerofornow.+*@clock_id:Clockbaseforsystemtimestamps(CLOCK_REALTIME,etc).+*@num_samples:Numberoftimestampsamplesrequested.+*ForPTP_SYS_OFFSET_PRECISE_ATTRSmustbe1.+*@rsv:Reservedforfutureuse,mustbezero.+*/+structptp_attrs_request{+__u32valid;+__kernel_clockid_tclock_id;+__u32num_samples;+__u32rsv[3];+};++/**+*structptp_sys_offset_attrs-unifieddatastructureforattrsioctls+*+*@request:Requestparameters(seestructptp_attrs_request).+*@timestamps:Arrayofreturnedsamples;holdsrequest.num_samplesentries.+*+*UsedbybothPTP_SYS_OFFSET_EXTENDED_ATTRSand+*PTP_SYS_OFFSET_PRECISE_ATTRS.Userspaceallocatesspacefor+*request.num_samplesentriesinthetimestampsarray.+*/+structptp_sys_offset_attrs{+structptp_attrs_requestrequest;+structptp_timestamptimestamps[];+};+structptp_clock_caps{intmax_adj;/* Maximum frequency adjustment in parts per billon. */intn_alarm;/* Number of programmable alarms. */
@@ -106,7 +379,11 @@ struct ptp_clock_caps {/* Whether the clock supports adjust phase */intadjust_phase;intmax_phase_adj;/* Maximum phase adjustment in nanoseconds. */-intrsv[11];/* Reserved for future use. */+/* Whether the clock supports extended timestamps with attributes */+intextended_attrs;+/* Whether the clock supports precise cross-timestamps with attributes */+intprecise_attrs;+intrsv[9];/* Reserved for future use. */};structptp_extts_request{
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:20
Extract the repeated switch-on-clockid pattern used for printing
system timestamps into a reusable helper function. This removes
code duplication in the -x (PTP_SYS_OFFSET_EXTENDED) output path
and prepares for additional callers.
The "after" timestamp lines now include the sample number prefix
for consistency with the "before" lines, slightly changing the
output format.
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <redacted>
---
tools/testing/selftests/ptp/testptp.c | 76 +++++++++++++--------------
1 file changed, 38 insertions(+), 38 deletions(-)
@@ -608,46 +636,18 @@ int main(int argc, char *argv[])getextended);for(i=0;i<getextended;i++){-switch(ext_clockid){-caseCLOCK_REALTIME:-printf("sample #%2d: real time before: %lld.%09u\n",-i,soe->ts[i][0].sec,-soe->ts[i][0].nsec);-break;-caseCLOCK_MONOTONIC:-printf("sample #%2d: monotonic time before: %lld.%09u\n",-i,soe->ts[i][0].sec,-soe->ts[i][0].nsec);-break;-caseCLOCK_MONOTONIC_RAW:-printf("sample #%2d: monotonic-raw time before: %lld.%09u\n",-i,soe->ts[i][0].sec,-soe->ts[i][0].nsec);-break;-default:-break;-}+print_system_timestamp(i,ext_clockid,+soe->ts[i][0].sec,+soe->ts[i][0].nsec,+"before");+printf(" phc time: %lld.%09u\n",soe->ts[i][1].sec,soe->ts[i][1].nsec);-switch(ext_clockid){-caseCLOCK_REALTIME:-printf(" real time after: %lld.%09u\n",-soe->ts[i][2].sec,-soe->ts[i][2].nsec);-break;-caseCLOCK_MONOTONIC:-printf(" monotonic time after: %lld.%09u\n",-soe->ts[i][2].sec,-soe->ts[i][2].nsec);-break;-caseCLOCK_MONOTONIC_RAW:-printf(" monotonic-raw time after: %lld.%09u\n",-soe->ts[i][2].sec,-soe->ts[i][2].nsec);-break;-default:-break;-}++print_system_timestamp(i,ext_clockid,+soe->ts[i][2].sec,+soe->ts[i][2].nsec,+"after");}}
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:28
Add support for testing the new PTP_SYS_OFFSET_EXTENDED_ATTRS and
PTP_SYS_OFFSET_PRECISE_ATTRS ioctls in the testptp utility.
Add a "-a" modifier that requests clock quality attributes
(error_bound, status, timescale) alongside the existing extended (-x)
and precise (-X) offset requests:
-x N -a: extended offset with attributes (N samples)
-X -a: precise cross-timestamp with attributes
When the device does not report a given attribute, print an explicit
"not reported" line so all attributes are always accounted for.
Also print the system counter value and its identifier (sys_counter,
sys_counter_id) returned in struct ptp_sys_time, so the counter source
is visible; a sys_counter_id of 0 (UNKNOWN) with sys_counter 0 indicates
an unavailable or unidentified counter source.
These options allow testing and validation of PHC devices that provide
clock quality information alongside timestamps.
Also display the new extended_attrs/precise_attrs capabilities in the -c
output, and update print_system_timestamp to print unrecognized clock
types instead of silently dropping them.
Co-developed-by: Amit Bernstein <redacted>
Signed-off-by: Amit Bernstein <redacted>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
tools/testing/selftests/ptp/testptp.c | 124 +++++++++++++++++++++++++-
1 file changed, 120 insertions(+), 4 deletions(-)
@@ -148,6 +148,8 @@ static void usage(char *progname)" -T val set the ptp clock time to 'val' seconds\n"" -x val get an extended ptp clock time with the desired number of samples (up to %d)\n"" -X get a ptp clock cross timestamp\n"+" -a also report clock quality attributes (error_bound,\n"+" status, timescale); use together with -x or -X\n"" -y val pre/post tstamp timebase to use {realtime|monotonic|monotonic-raw}\n"" -z test combinations of rising/falling external time stamp flags\n",progname,PTP_MAX_SAMPLES);
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:33
Implement the gettimexattrs64 and getcrosststampattrs callbacks in the
ptp_vmclock driver to provide clock quality attributes through the new
PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS ioctls.
The ptp_vmclock device exposes:
- error_bound: Derived from time_maxerror_nanosec, accumulated with
counter frequency error (counter_period_maxerror_rate_frac_sec) over
elapsed counter ticks
- clock_status: Mapped from the device's clock_status field
- timescale: Always reports TAI (UTC sources are converted by tai_adjust()
before the attributes are populated)
The legacy ioctls return -EINVAL when clock_status is UNRELIABLE since
they have no way to communicate clock state to userspace. The attrs
ioctls have a status field for this purpose, so they treat UNRELIABLE
as success and let userspace check the status field.
To avoid a race where the hypervisor could update clock_status between
the timestamp call and the UNRELIABLE check, the clock state is captured
inside the seq_count loop for a consistent snapshot with the timestamp.
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
drivers/ptp/ptp_vmclock.c | 199 +++++++++++++++++++++++++++++++++++---
1 file changed, 183 insertions(+), 16 deletions(-)
@@ -53,6 +53,17 @@ struct vmclock_state {char*name;};+/**+*structvmclock_crosststamp_ctx-contextforget_device_system_crosststamp()+*@st:vmclockdevicestate+*@attrs:optionaloutputforPTPclockattributes,populatedinsidethe+*seq_countloopforaconsistentsnapshotwiththetimestamp+*/+structvmclock_crosststamp_ctx{+structvmclock_state*st;+structptp_clock_attrs*attrs;+};+#define VMCLOCK_MAX_WAIT ms_to_ktime(100)/* Require at least the flags field to be present. All else can be optional. */
@@ -95,13 +106,113 @@ static bool tai_adjust(struct vmclock_abi *clk, uint64_t *sec)returnfalse;}+staticuint8_tvmclock_get_ptp_timescale(uint8_tvmclock_time_type)+{+switch(vmclock_time_type){+caseVMCLOCK_TIME_UTC:+returnPTP_TIMESCALE_UTC;+caseVMCLOCK_TIME_TAI:+returnPTP_TIMESCALE_TAI;+caseVMCLOCK_TIME_MONOTONIC:+returnPTP_TIMESCALE_MONOTONIC;+default:+returnPTP_TIMESCALE_UNKNOWN;+}+}++staticuint8_tvmclock_get_ptp_status(uint8_tvmclock_status)+{+switch(vmclock_status){+caseVMCLOCK_STATUS_UNKNOWN:+returnPTP_CLOCK_STATUS_UNKNOWN;+caseVMCLOCK_STATUS_INITIALIZING:+returnPTP_CLOCK_STATUS_INITIALIZING;+caseVMCLOCK_STATUS_SYNCHRONIZED:+returnPTP_CLOCK_STATUS_SYNCED;+caseVMCLOCK_STATUS_FREERUNNING:+returnPTP_CLOCK_STATUS_FREE_RUNNING;+caseVMCLOCK_STATUS_UNRELIABLE:+returnPTP_CLOCK_STATUS_UNRELIABLE;+default:+returnPTP_CLOCK_STATUS_UNKNOWN;+}+}++staticvoidvmclock_populate_ptp_attributes(structvmclock_state*st,+structptp_clock_attrs*att,+uint64_tdelta)+{+uint64_tmaxerror_ns=UINT_MAX;+boolhave_error_bound=false;++if(!att)+return;++/* Called once per seq_count retry, so don't accumulate stale bits */+att->valid=0;+att->error_bound=0;++/* Only calculate if the base error is flagged as valid+*bythehypervisor.+*/+if(VMCLOCK_FIELD_PRESENT(st->clk,time_maxerror_nanosec)&&+(le64_to_cpu(st->clk->flags)&VMCLOCK_FLAG_TIME_MAXERROR_VALID)){+have_error_bound=true;+maxerror_ns=le64_to_cpu(st->clk->time_maxerror_nanosec);++/* If frequency error is also valid, accumulate it+*overthedelta.+*/+if(VMCLOCK_FIELD_PRESENT(st->clk,counter_period_maxerror_rate_frac_sec)&&+(le64_to_cpu(st->clk->flags)&VMCLOCK_FLAG_PERIOD_MAXERROR_VALID)){+uint64_tmaxerror_rate,err_hi,err_frac,growth_ns;++maxerror_rate=le64_to_cpu(st->clk->counter_period_maxerror_rate_frac_sec);+err_frac=mul_u64_u64_shr_add_u64(&err_hi,delta,+maxerror_rate,+st->clk->counter_period_shift,+0);++if(err_hi>=U64_MAX/NSEC_PER_SEC){+maxerror_ns=U64_MAX;+gotosaturate;+}++growth_ns=(err_hi*NSEC_PER_SEC)++mul_u64_u64_shr(err_frac,NSEC_PER_SEC,64);++/* Guard against overflow */+if(U64_MAX-growth_ns<maxerror_ns)+maxerror_ns=U64_MAX;+else+maxerror_ns+=growth_ns;+}+}++saturate:+if(have_error_bound){+att->error_bound=(maxerror_ns>UINT_MAX)?+UINT_MAX:(uint32_t)maxerror_ns;+att->valid|=PTP_ATTRS_VALID_ERROR_BOUND;+}+att->timescale=vmclock_get_ptp_timescale(st->clk->time_type);+/* tai_adjust() already converted UTC to TAI before we're called */+if(st->clk->time_type==VMCLOCK_TIME_UTC)+att->timescale=PTP_TIMESCALE_TAI;+att->valid|=PTP_ATTRS_VALID_TIMESCALE;+att->status=vmclock_get_ptp_status(st->clk->clock_status);+att->valid|=PTP_ATTRS_VALID_STATUS;+}+staticintvmclock_get_crosststamp(structvmclock_state*st,structptp_system_timestamp*sts,structsystem_counterval_t*system_counter,-structtimespec64*tspec)+structtimespec64*tspec,+structptp_clock_attrs*attrs){ktime_tdeadline=ktime_add(ktime_get(),VMCLOCK_MAX_WAIT);uint64_tcycle,delta,seq,frac_sec;+uint8_tclock_status=VMCLOCK_STATUS_UNKNOWN;#ifdef CONFIG_X86/*
@@ -121,9 +232,6 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,*/virt_rmb();-if(st->clk->clock_status==VMCLOCK_STATUS_UNRELIABLE)-return-EINVAL;-/**Wheninvokedforgettimex64(),fillinthepre/postsystem*times.Thesimplecaseiswhensystemtimeisbasedonthe
@@ -164,6 +272,17 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,if(!tai_adjust(st->clk,&tspec->tv_sec))return-EINVAL;+/*+*Captureclockstateinsidetheseq_countloopfora+*consistentsnapshotwiththetimestamp.Theattrspath+*reportsittouserspaceviathestatusfield;thelegacy+*pathsavesitfortheUNRELIABLEcheckaftertheloop.+*/+if(attrs)+vmclock_populate_ptp_attributes(st,attrs,delta);+else+clock_status=st->clk->clock_status;+/**Thispairswithawritebarrierinthehypervisor*whichpopulatesthisstructure.
@@ -181,6 +300,17 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,system_counter->cs_id=st->cs_id;}+/*+*Ifattrsisset,attributeswerealreadypopulatedinsidethe+*seq_countloop.ReturnsuccessevenforUNRELIABLE-theattrs+*ioctlcanreportthestatustouserspace.+*/+if(attrs)+return0;++if(clock_status==VMCLOCK_STATUS_UNRELIABLE)+return-EINVAL;+return0;}
@@ -193,7 +323,8 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,staticintvmclock_get_crosststamp_kvmclock(structvmclock_state*st,structptp_system_timestamp*sts,structsystem_counterval_t*system_counter,-structtimespec64*tspec)+structtimespec64*tspec,+structptp_clock_attrs*attrs){structpvclock_vcpu_time_info*pvti=this_cpu_pvti();unsignedintpvti_ver;
@@ -204,7 +335,8 @@ static int vmclock_get_crosststamp_kvmclock(struct vmclock_state *st,do{pvti_ver=pvclock_read_begin(pvti);-ret=vmclock_get_crosststamp(st,sts,system_counter,tspec);+ret=vmclock_get_crosststamp(st,sts,system_counter,tspec,+attrs);if(ret)break;
@@ -233,17 +365,19 @@ static int ptp_vmclock_get_time_fn(ktime_t *device_time,structsystem_counterval_t*system_counter,void*ctx){-structvmclock_state*st=ctx;+structvmclock_crosststamp_ctx*vctx=ctx;+structvmclock_state*st=vctx->st;structtimespec64tspec;intret;#ifdef SUPPORT_KVMCLOCKif(READ_ONCE(st->sys_cs_id)==CSID_X86_KVM_CLK)ret=vmclock_get_crosststamp_kvmclock(st,NULL,system_counter,-&tspec);+&tspec,vctx->attrs);else#endif-ret=vmclock_get_crosststamp(st,NULL,system_counter,&tspec);+ret=vmclock_get_crosststamp(st,NULL,system_counter,&tspec,+vctx->attrs);if(!ret)*device_time=timespec64_to_ktime(tspec);
@@ -251,12 +385,10 @@ static int ptp_vmclock_get_time_fn(ktime_t *device_time,returnret;}-staticintptp_vmclock_getcrosststamp(structptp_clock_info*ptp,-structsystem_device_crosststamp*xtstamp)+staticintptp_vmclock_do_getcrosststamp(structvmclock_crosststamp_ctx*vctx,+structsystem_device_crosststamp*xtstamp){-structvmclock_state*st=container_of(ptp,structvmclock_state,-ptp_clock_info);-intret=get_device_system_crosststamp(ptp_vmclock_get_time_fn,st,+intret=get_device_system_crosststamp(ptp_vmclock_get_time_fn,vctx,NULL,xtstamp);#ifdef SUPPORT_KVMCLOCK/*
@@ -265,6 +397,7 @@ static int ptp_vmclock_getcrosststamp(struct ptp_clock_info *ptp,*timestampthatget_device_system_crosststamp()*can*handle.*/if(ret==-ENODEV){+structvmclock_state*st=vctx->st;structsystem_time_snapshotsystime_snapshot;ktime_get_snapshot_id(CLOCK_REALTIME,&systime_snapshot);
@@ -273,13 +406,23 @@ static int ptp_vmclock_getcrosststamp(struct ptp_clock_info *ptp,systime_snapshot.cs_id==CSID_X86_KVM_CLK){WRITE_ONCE(st->sys_cs_id,systime_snapshot.cs_id);ret=get_device_system_crosststamp(ptp_vmclock_get_time_fn,-st,NULL,xtstamp);+vctx,NULL,xtstamp);}}#endifreturnret;}+staticintptp_vmclock_getcrosststamp(structptp_clock_info*ptp,+structsystem_device_crosststamp*xtstamp)+{+structvmclock_state*st=container_of(ptp,structvmclock_state,+ptp_clock_info);+structvmclock_crosststamp_ctxvctx={.st=st};++returnptp_vmclock_do_getcrosststamp(&vctx,xtstamp);+}+/**PTPclockoperations*/
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:43
Extend the ENA admin interface to support error bound.
Add error_bound to the PHC response structure.
Introduce a feature version mechanism to indicate device supports
error_bound, and add an error flag for error_bound retrieval failures.
This enables the driver to retrieve error_bound information from the
device alongside timestamps.
Co-developed-by: Amit Bernstein <redacted>
Signed-off-by: Amit Bernstein <redacted>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <redacted>
---
.../net/ethernet/amazon/ena/ena_admin_defs.h | 17 +++++++++++------
drivers/net/ethernet/amazon/ena/ena_com.c | 11 ++++++-----
2 files changed, 17 insertions(+), 11 deletions(-)
@@ -128,12 +128,14 @@ enum ena_admin_get_stats_scope {ENA_ADMIN_ETH_TRAFFIC=1,};-enumena_admin_phc_type{-ENA_ADMIN_PHC_TYPE_READLESS=0,+enumena_admin_phc_feature_version{+/* Readless with error_bound */+ENA_ADMIN_PHC_FEATURE_VERSION_0=0,};enumena_admin_phc_error_flags{ENA_ADMIN_PHC_ERROR_FLAG_TIMESTAMP=BIT(0),+ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND=BIT(1),};/* ENA SRD configuration for ENI */
@@ -1035,10 +1037,10 @@ struct ena_admin_queue_ext_feature_desc {};structena_admin_feature_phc_desc{-/* PHC type as defined in enum ena_admin_get_phc_type,-*usedonlyforGETcommand.+/* PHC version as defined in enum ena_admin_phc_feature_version,+*usedonlyforGETcommandasmaxsupportedPHCversionbythedevice.*/-u8type;+u8version;/* Reserved - MBZ */u8reserved1[3];
@@ -1224,7 +1226,10 @@ struct ena_admin_phc_resp {/* PHC timestamp (nsec) */u64timestamp;-u8reserved2[12];+u8reserved2[8];++/* Timestamp error limit (nsec) */+u32error_bound;/* Bit field of enum ena_admin_phc_error_flags */u32error_flags;
@@ -306,6 +306,8 @@ PHC errors must remain below 1% of all PHC requests to maintain the desired leve**phc_err_dv** | Number of failed get time attempts due to device errors (entering into block state).**phc_err_ts** | Number of failed get time attempts due to timestamp errors (entering into block state),| This occurs if driver exceeded the request limit or device received an invalid timestamp.+**phc_err_eb** | Number of failed get time attempts due to error bound errors (entering into block state),+| This occurs if device received an excessively high or invalid error bound. ================= ====================================================== PHC timeouts:
@@ -1828,6 +1830,8 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)*aPHCerror,thisoccursifdevice:*-exceededthegettimerequestlimit*-receivedaninvalidtimestamp+*-receivedanexcessivelyhigherrorbound+*-receivedaninvaliderrorbound*/netdev_err(ena_dev->net_device,"PHC get time request 0x%x failed (error 0x%x)\n",
@@ -1835,9 +1839,11 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)resp->error_flags);phc->stats.phc_err_ts+=!!(resp->error_flags&ENA_ADMIN_PHC_ERROR_FLAG_TIMESTAMP);+phc->stats.phc_err_eb+=!!(resp->error_flags&+ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND);}else{/* Device updated req_id during blocking time-*withvalidtimestamp+*withvalidtimestampanderrorbound*/phc->stats.phc_exp++;}
@@ -1864,9 +1870,9 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)/* Stalling until the device updates req_id */while(1){if(unlikely(ktime_after(ktime_get(),expire_time))){-/* Gave up waiting for updated req_id, PHC enters into-*blockedstateuntilpassingblockingtime,-*duringthistimeanygetPHCtimestampwillfailwith+/* Gave up waiting for updated req_id,+*PHCentersintoblockedstateuntilpassingblocking+*time,duringthistime,anyrequestwillfailwith*devicebusyerror*/ret=-EBUSY;
@@ -1881,20 +1887,21 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)continue;}-/* Ensure PHC payload (timestamp, error_flags) is read-*afterreq_idupdateisobserved+/* Ensure PHC payload (timestamp, error_bound, error_flags)+*isreadafterreq_idupdateisobserved*/dma_rmb();/* req_id was updated by the device which indicates that-*PHCtimestampanderror_flagsareupdatedtoo,-*checkingerrorsbeforeretrievingtimestamp+*PHCtimestamp,error_boundanderror_flagsareupdatedtoo,+*checkingerrorflagsbeforeretrievingtimestampand+*error_boundvalues*/if(unlikely(resp->error_flags&ENA_PHC_ERROR_FLAGS)){-/* Retrieved invalid PHC timestamp, PHC enters into-*blockedstateuntilpassingblockingtime,-*duringthistimeanygetPHCtimestamprequests-*willfailwithdevicebusyerror+/* Retrieved timestamp or error bound errors,+*PHCentersintoblockedstateuntilpassingblocking+*time,duringthistime,anyrequestwillfailwith+*devicebusyerror*/ret=-EBUSY;break;
@@ -1902,12 +1909,15 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)/* PHC timestamp value is returned to the caller */*timestamp=resp->timestamp;+if(error_bound)+*error_bound=resp->error_bound;/* Update statistic on valid PHC timestamp retrieval */phc->stats.phc_cnt++;/* This indicates PHC state is active */phc->system_time=zero_system_time;+break;}
From: Arthur Kiyanovski <akiyano@amazon.com> Date: 2026-09-09 07:32:57
Implement the gettimexattrs64 callback in the ENA driver to support
the PTP_SYS_OFFSET_EXTENDED_ATTRS ioctl.
This enables applications to retrieve PHC timestamps with quality
attributes through the standard PTP ioctl interface.
The ENA device currently reports only error_bound (valid bit set).
Other attributes are not reported (valid bits unset).
Co-developed-by: Amit Bernstein <redacted>
Signed-off-by: Amit Bernstein <redacted>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <redacted>
---
drivers/net/ethernet/amazon/ena/ena_phc.c | 58 +++++++++++++++++++----
1 file changed, 48 insertions(+), 10 deletions(-)
@@ -25,6 +25,43 @@ static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,return-EOPNOTSUPP;}+staticintena_phc_gettimexattrs64(structptp_clock_info*clock_info,+structtimespec64*ts,+structptp_system_timestamp*sts,+structptp_clock_attrs*att)+{+structena_phc_info*phc_info=+container_of(clock_info,structena_phc_info,clock_info);+u32error_bound_nsec;+unsignedlongflags;+u64timestamp_nsec;+intrc;++spin_lock_irqsave(&phc_info->lock,flags);++ptp_read_system_prets(sts);++rc=ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,+×tamp_nsec,+&error_bound_nsec);++ptp_read_system_postts(sts);++spin_unlock_irqrestore(&phc_info->lock,flags);++if(rc)+returnrc;++*ts=ns_to_timespec64(timestamp_nsec);++if(att){+att->error_bound=error_bound_nsec;+att->valid|=PTP_ATTRS_VALID_ERROR_BOUND;+}++return0;+}+staticintena_phc_gettimex64(structptp_clock_info*clock_info,structtimespec64*ts,structptp_system_timestamp*sts)
@@ -62,16 +99,17 @@ static int ena_phc_settime64(struct ptp_clock_info *clock_info,}staticstructptp_clock_infoena_ptp_clock_info={-.owner=THIS_MODULE,-.n_alarm=0,-.n_ext_ts=0,-.n_per_out=0,-.pps=0,-.adjtime=ena_phc_adjtime,-.adjfine=ena_phc_adjfine,-.gettimex64=ena_phc_gettimex64,-.settime64=ena_phc_settime64,-.enable=ena_phc_feature_enable,+.owner=THIS_MODULE,+.n_alarm=0,+.n_ext_ts=0,+.n_per_out=0,+.pps=0,+.adjtime=ena_phc_adjtime,+.adjfine=ena_phc_adjfine,+.gettimexattrs64=ena_phc_gettimexattrs64,+.gettimex64=ena_phc_gettimex64,+.settime64=ena_phc_settime64,+.enable=ena_phc_feature_enable,};/* Enable/Disable PHC by the kernel, affects on the next init flow */
Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider:
Critical: 0 · High: 0 (1) · Medium: 5 · Low: 3
- [Medium] `struct ptp_sys_time.sys_rawtime` is documented in the new
UAPI as "CLOCK_MONOTONIC_RAW time in nanoseconds", but both new ioctls…
- [Medium] The new `ptp_clock_caps` fields are documented in the UAPI as
"Whether the clock supports extended timestamps with attributes" /…
- [Medium] `ptp_counter_id_from_csid()` maps every
`CSID_ARM_ARCH_COUNTER` snapshot to `PTP_COUNTER_ARM_ARCH`, which the…
- [Medium] `ptp_fill_sys_counter()` derives the exported
`sys_counter`/`sys_counter_id` pair from `struct…
- [Medium] `ptp_counter_id_from_csid()`/`ptp_fill_sys_counter()` inspect
only the timekeeper clocksource id (`cs_id`) and its `cycles`,…
- [Low] Incomplete integration of the two new driver callbacks.
- [Low] Concern that the new UAPI has no in-tree producer of the quality
attributes and no test coverage, making the ~280 lines of new frozen…
- [Low] The commit message presents an exhaustive-looking list of the new
clock-status states ("unknown, initializing, synchronized, free-…
Pre-existing issues:
- [High] `ptp_sys_offset_extended()` (the function this patch refactors)
declares `struct ptp_system_timestamp sts;` without initialization and…
--
ptp: Add ioctls for PHC timestamps with quality attributes
This adds PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS,
which return PHC timestamps together with an error bound, clock status,
timescale and a raw system counter value. Two new driver callbacks
(gettimexattrs64, getcrosststampattrs) are introduced, and the new ioctls
fall back to gettimex64 / getcrosststamp when a driver does not implement
them.
Link: https://lore.kernel.org/netdev/20250724115657.150-1-darinzon@amazon.com/
Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/
A few questions on the commit message and the diff below.
2. clock_status: Synchronization state (unknown, initializing,
synchronized, free-running, unreliable)
[Severity: Low]
This list looks exhaustive, but enum ptp_clock_status in the uapi header
also defines PTP_CLOCK_STATUS_HOLDOVER = 3, which is described as
"drifting but remains within acceptable error bounds" and is distinct from
FREE_RUNNING.
Could the enumeration in the commit message include holdover so it matches
the new interface?
[Severity: Medium]
Do these capability bits match what the header says they mean? The uapi
side of this patch documents them as:
/* Whether the clock supports extended timestamps with attributes */
int extended_attrs;
/* Whether the clock supports precise cross-timestamps with attributes */
int precise_attrs;
Here extended_attrs is set whenever gettimex64 exists, which is the case
for nearly every in-tree PHC driver (ptp_ocp, ptp_netc, dfl_tod,
virtio_rtc, hns3, nxp-c45, ...), and none of those can ever produce
attributes, so att.valid stays 0 for every sample.
The commit message explains the intended meaning ("the ioctl is
callable"), but the header comment that userspace authors read says
"supports ... with attributes". Could the header comments be reworded to
match the implementation?
Related: cross_timestamping is still keyed on getcrosststamp only, while
precise_attrs was extended to getcrosststampattrs. For a driver that
implements only getcrosststampattrs, cross_timestamping would report 0
while precise_attrs reports 1. Is that intended?
quoted hunk
@@ -347,11 +352,48 @@ typedef int (*ptp_gettimex_fn)(struct ptp_clock_info *, struct timespec64 *, struct ptp_system_timestamp *);+static int ptp_validate_sys_offset_clockid(__kernel_clockid_t clockid)+{+ switch (clockid) {+ case CLOCK_REALTIME:+ case CLOCK_MONOTONIC:+ case CLOCK_MONOTONIC_RAW:+ return 0;+ case CLOCK_AUX ... CLOCK_AUX_LAST:+ if (IS_ENABLED(CONFIG_POSIX_AUX_CLOCKS))+ return 0;+ fallthrough;+ default:+ return -EINVAL;+ }+}++/*+ * Validate clock_id for the precise crosststamp path.+ * get_device_system_crosststamp() supports only CLOCK_REALTIME and the+ * AUX clocks, so anything else (incl. the monotonic clocks accepted for+ * the extended path) must be rejected here to avoid its WARN_ON_ONCE().+ */+static int ptp_validate_precise_clockid(__kernel_clockid_t clockid)+{+ switch (clockid) {+ case CLOCK_REALTIME:+ return 0;+ case CLOCK_AUX ... CLOCK_AUX_LAST:+ if (IS_ENABLED(CONFIG_POSIX_AUX_CLOCKS))+ return 0;+ fallthrough;+ default:+ return -EINVAL;+ }+}+ static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg, ptp_gettimex_fn gettimex_fn) { struct ptp_sys_offset_extended *extoff __free(kfree) = NULL; struct ptp_system_timestamp sts;
^^^^
[Severity: High]
This isn't a bug introduced by this patch (the declaration is unchanged),
but since the function is being reworked here it may be a good place to
fix it: sts is uninitialized and only sts.clockid is assigned, yet the
loop below decides what to hand back to userspace with:
drivers/ptp/ptp_chardev.c:ptp_sys_offset_extended() {
...
/* Filter out disabled or unavailable clocks */
if (!sts.pre_sts.valid || !sts.post_sts.valid)
return -EINVAL;
...
}
Can this read uninitialized stack bytes? valid is only ever set by
ptp_read_system_prets() / ptp_read_system_postts(), and two in-tree
.gettimex64 implementations never touch sts at all and return 0:
hclge_ptp_gettimex() (hns3, assigned to .gettimex64)
nxp_c45_ptp_gettimex64() (nxp-c45-tja11xx, assigned to .gettimex64)
If both leftover valid bytes happen to be non-zero, the guard passes and
extoff->ts[i][0] / ts[i][2] are filled from uninitialized sts.pre_sts.systime
and sts.post_sts.systime and then copied out with copy_to_user(). Would
initializing it the same way the new sibling does be worth folding in?
struct ptp_system_timestamp sts = { .clockid = request.clock_id };
[ ... ]
quoted hunk
@@ -404,6 +436,150 @@ static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg, return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0; }+static u32 ptp_counter_id_from_csid(enum clocksource_ids cs_id)+{+ switch (cs_id) {+ case CSID_X86_TSC_EARLY:+ case CSID_X86_TSC:+ return PTP_COUNTER_X86_TSC;+ case CSID_ARM_ARCH_COUNTER:+ return PTP_COUNTER_ARM_ARCH;
[Severity: Medium]
PTP_COUNTER_ARM_ARCH is documented in the uapi part of this patch as the
"ARM Generic Timer virtual counter", but can CSID_ARM_ARCH_COUNTER tell
the two counters apart?
drivers/clocksource/arm_arch_timer.c uses one clocksource id for both
accessors:
static struct clocksource clocksource_counter = {
.name = "arch_sys_counter",
.id = CSID_ARM_ARCH_COUNTER,
and arch_counter_register() picks the register at runtime:
if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI ||
arch_timer_uses_ppi == ARCH_TIMER_HYP_VIRT_PPI) {
... rd = arch_counter_get_cntvct;
} else {
... rd = arch_counter_get_cntpct;
On a configuration that takes the cntpct branch, the ioctl would export
CNTPCT cycles labelled as the virtual counter. The two differ by CNTVOFF
(often zero at EL2, so the values may coincide in practice), and userspace
normally can only read CNTVCT.
Should the enum comment drop the "virtual" wording, or should the mapping
distinguish the two?
quoted hunk
+ default:+ /* CSID_X86_KVM_CLK is deliberately mapped to unknown:+ * kvmclock is not a raw hardware counter.+ */+ return PTP_COUNTER_UNKNOWN;+ }+}+
[Severity: Medium]
Does this drop a counter value that the snapshot already carries?
ktime_get_snapshot_id() also fills hw_csid / hw_cycles for derived
clocksources, and kvmclock populates them with the raw TSC:
arch/x86/kernel/kvmclock.c:kvm_clock_get_cycles_snapshot() {
...
chs->hw_cycles = tsc;
chs->hw_csid = CSID_X86_TSC;
...
}
The Hyper-V TSC page clocksource does the same, and it reports
CSID_GENERIC, so it also lands in this default case.
The result is that on an x86 guest using kvm-clock (the default) or the
Hyper-V TSC page, the new ioctls report sys_counter = 0 with
sys_counter_id = PTP_COUNTER_UNKNOWN even though the paired TSC value is
sitting in the same snapshot. That is the environment the third use case
in the commit message targets ("VMMs that need raw system counter values
paired with PTP timestamps").
There is already precedent for the fallback in this subsystem:
drivers/ptp/ptp_vmclock.c:vmclock_get_crosststamp() {
...
} else if (sts->pre_sts.hw_csid == st->cs_id &&
sts->pre_sts.hw_cycles) {
cycle = sts->pre_sts.hw_cycles;
...
}
Could ptp_fill_sys_counter() consult hw_csid / hw_cycles when cs_id is a
derived clocksource? As it stands userspace also cannot distinguish "no
hardware counter exists" from "one was captured and discarded".
[Severity: Medium]
Should this path also look at xtstamp.sys_counter.use_nsecs? cycles is
only a cycle count when use_nsecs is false, and the timekeeping core skips
the conversion when the ids match:
kernel/time/timekeeping.c:convert_base_to_cs() {
...
/* The timestamp was taken from the time keeper clock source */
if (cs->id == scv->cs_id)
return true;
...
}
mlx5 and idpf both report a nanosecond value with the timekeeper's id:
*sys_counterval = (struct system_counterval_t) {
.cycles = host,
.cs_id = IS_ENABLED(CONFIG_X86) ? CSID_X86_ART :
CSID_ARM_ARCH_COUNTER,
.use_nsecs = true,
};
On arm64 the timekeeper id is CSID_ARM_ARCH_COUNTER, so cycles stays in
nanoseconds while cs_id still says arch counter, and
PTP_SYS_OFFSET_PRECISE_ATTRS would export that nanosecond value as
sys_counter with sys_counter_id = PTP_COUNTER_ARM_ARCH.
Whether such a sample survives timestamp_in_interval() depends on the
generic timer frequency being close to 1 GHz (mandated by ARMv8.6 ECV), so
this may be hard to hit, but nothing in the new code inspects use_nsecs.
Would reporting PTP_COUNTER_UNKNOWN when use_nsecs is set be safer?
[Severity: Low]
Correction to the finding id above: gpt-5-6-sol-2-14.
Here gettimexattrs64 is treated as a self-sufficient time read, and the
kernel-doc added by this patch calls it "Same as @gettimex64, but also
fills @att". Can a driver that implements only gettimexattrs64 register?
ptp_clock_register() is unchanged:
drivers/ptp/ptp_clock.c:ptp_clock_register() {
if (WARN_ON_ONCE(info->n_alarm > PTP_MAX_ALARMS ||
(!info->gettimex64 && !info->gettime64) ||
!info->settime64))
return ERR_PTR(-EINVAL);
...
}
If such a driver adds gettime64 to get past that check, the other core
dispatch sites still do not know about the new callback:
ptp_ioctl() PTP_SYS_OFFSET_EXTENDED -> ptp_sys_offset_extended(..., gettimex64)
ptp_ioctl() PTP_SYS_OFFSET_PRECISE -> ptp_sys_offset_precise(..., getcrosststamp)
ptp_sys_offset() -> gettimex64 else gettime64
ptp_clock_register() -> getcyclesx64 = gettimex64,
getcrosscycles = getcrosststamp
so PTP_CLOCK_GETCAPS would advertise extended_attrs = 1 while
PTP_SYS_OFFSET_EXTENDED returns -EOPNOTSUPP, PTP_SYS_OFFSET silently drops
to the gettime64 path with no pre/post sandwich, and the _CYCLES ioctls
plus vclock cycle support lose their shims.
Both in-tree implementers added later in the series (ptp_vmclock and ena)
populate gettimex64 as well, so nothing is broken today. Should the
kernel-doc state that the base callback stays mandatory, or should
registration and the remaining dispatch sites learn about the attrs
callbacks?
[Severity: Low]
Taken on its own, this commit adds a new frozen uapi with no in-tree
producer of the quality attributes and no test coverage, so the two new
callbacks are unused at this point in the tree.
Later patches in the same series set .gettimexattrs64 /
.getcrosststampattrs in ptp_vmclock, .gettimexattrs64 in ena, and add a
testptp -a modifier exercising both ioctls, so this looks covered by the
series as a whole. Noting it only for the ordering.
@@ -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,+};
[ ... ]
quoted hunk
+/**+ * struct ptp_sys_time - system time snapshot with counter value+ *+ * @sys_time: System time in nanoseconds (clock selected by request).+ * @sys_rawtime: CLOCK_MONOTONIC_RAW time in nanoseconds.
[Severity: Medium]
Is sys_rawtime always CLOCK_MONOTONIC_RAW? Both new ioctls accept clock_id
in the CLOCK_AUX ... CLOCK_AUX_LAST range via
ptp_validate_sys_offset_clockid() and ptp_validate_precise_clockid(), and
for those ids the value stored here is the AUX-relative raw time:
tstamp->pre_systime.sys_rawtime = ktime_to_ns(sts.pre_sts.monoraw);
tstamp->systime.sys_rawtime = ktime_to_ns(xtstamp.sys_monoraw);
The core documents the AUX semantics explicitly:
kernel/time/timekeeping.c:ktime_get_snapshot_id() {
...
* MONOTONIC_RAW(AUX$N) = CLOCK_MONOTONIC_RAW(system) + offset(AUX$N)
* The offset is established when a AUX clock is initialized, but it is
* currently not accessible.
...
}
include/linux/timekeeping.h repeats that for both
struct system_time_snapshot.monoraw and
struct system_device_crosststamp.sys_monoraw.
An application that follows this comment and compares sys_rawtime against
clock_gettime(CLOCK_MONOTONIC_RAW) would be off by an offset it cannot
query. Could this comment carry the same AUX qualification the core
headers do?
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] The new `-x N -a` (PTP_SYS_OFFSET_EXTENDED_ATTRS) print loop
in `main()` (tools/testing/selftests/ptp/testptp.c lines 671-722)…
- [Low] In `main()` of tools/testing/selftests/ptp/testptp.c, the new
`-a` modifier is accepted and sets `use_attrs = 1` (line 327-329) but…
--
selftests/ptp: Add testptp support for attributes ioctls
This adds a "-a" modifier to testptp so that the new
PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS ioctls can
be exercised alongside the existing -x and -X requests. It also prints the
new extended_attrs/precise_attrs capabilities and reports unrecognized
clock ids in print_system_timestamp().
A couple of questions below.
@@ -148,6 +148,8 @@ static void usage(char *progname)" -T val set the ptp clock time to 'val' seconds\n"" -x val get an extended ptp clock time with the desired number of samples (up to %d)\n"" -X get a ptp clock cross timestamp\n"+" -a also report clock quality attributes (error_bound,\n"+" status, timescale); use together with -x or -X\n"" -y val pre/post tstamp timebase to use {realtime|monotonic|monotonic-raw}\n"" -z test combinations of rising/falling external time stamp flags\n",progname,PTP_MAX_SAMPLES);
[ ... ]
quoted hunk
@@ -317,6 +324,9 @@ int main(int argc, char *argv[]) return -1; } break;+ case 'a':+ use_attrs = 1;+ break;
[Severity: Low]
The usage text states that -a must be used "together with -x or -X", but is
that requirement enforced anywhere in main()?
Both new request blocks are gated on a companion option:
if (getextended && use_attrs) {
...
if (getcross && use_attrs) {
and the two legacy blocks are now additionally gated on !use_attrs, so
"testptp -d /dev/ptp0 -a" (or -a combined with something unrelated like -k
or -g) issues no attrs ioctl, prints nothing about attributes, and exits 0.
main() already diagnoses the analogous dependency for -w and -H:
if (pulsewidth >= 0 && perout < 0) {
puts("-w can only be specified together with -p");
return -1;
}
if (perout_phase >= 0 && perout < 0) {
puts("-H can only be specified together with -p");
return -1;
}
Would it make sense to add the equivalent check for -a without -x or -X?
case 'X':
getcross = 1;
break;
[ ... ]
quoted hunk
@@ -654,7 +668,60 @@ int main(int argc, char *argv[]) free(soe); }- if (getcross) {+ if (getextended && use_attrs) {+ attrs_data = calloc(1, sizeof(*attrs_data) ++ getextended * sizeof(struct ptp_timestamp));+ if (!attrs_data) {+ perror("calloc");+ return -1;+ }++ attrs_data->request.num_samples = getextended;+ attrs_data->request.clock_id = ext_clockid;++ if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED_ATTRS, attrs_data)) {+ perror("PTP_SYS_OFFSET_EXTENDED_ATTRS");+ } else {+ printf("extended attrs timestamp request returned %d samples\n",+ getextended);++ for (i = 0; i < getextended; i++) {+ struct ptp_timestamp *ts = &attrs_data->timestamps[i];++ printf(" sample #%u:\n", i);+ printf(" sys before: %lld ns\n",+ (long long)ts->pre_systime.sys_time);+ printf(" sys_counter_id: %u\n",+ ts->pre_systime.sys_counter_id);+ printf(" sys_counter: %llu\n",+ (unsigned long long)ts->pre_systime.sys_counter);+ printf(" phc time: %lld.%09u\n",+ ts->devtime.device_time.sec,+ ts->devtime.device_time.nsec);+ if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_ERROR_BOUND)+ printf(" error_bound: %u ns\n",+ ts->devtime.attrs.error_bound);+ else+ printf(" error_bound: not reported\n");+ if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_STATUS)+ printf(" status: %u\n",+ ts->devtime.attrs.status);+ else+ printf(" status: not reported\n");+ if (ts->devtime.attrs.valid & PTP_ATTRS_VALID_TIMESCALE)+ printf(" timescale: %u\n",+ ts->devtime.attrs.timescale);+ else+ printf(" timescale: not reported\n");+ printf(" sys after: %lld ns\n",+ (long long)ts->post_systime.sys_time);
[Severity: Medium]
Should the post snapshot print the counter fields too?
This loop prints pre_systime.sys_time, pre_systime.sys_counter_id and
pre_systime.sys_counter, but for the post snapshot only sys_time. It also
never prints sys_rawtime for either snapshot, although the kernel side fills
all four fields of both snapshots in ptp_sys_offset_extended_attrs():
drivers/ptp/ptp_chardev.c:ptp_sys_offset_extended_attrs() {
...
tstamp->post_systime.sys_time = ktime_to_ns(sts.post_sts.systime);
tstamp->post_systime.sys_rawtime = ktime_to_ns(sts.post_sts.monoraw);
ptp_fill_sys_counter(&tstamp->post_systime, sts.post_sts.cycles,
sts.post_sts.cs_id);
...
}
The -X -a printer added in the same patch does surface those fields:
printf("raw time: %lld ns\n",
(long long)ts->systime.sys_rawtime);
printf("sys_counter_id: %u\n",
ts->systime.sys_counter_id);
printf("sys_counter: %llu\n",
(unsigned long long)ts->systime.sys_counter);
so the two new consumers of struct ptp_sys_time disagree about which
returned fields are shown. As it stands, "-x N -a" output is identical
whether the post-read counter source is correctly identified or left at
zero.
The commit message says:
Also print the system counter value and its identifier (sys_counter,
sys_counter_id) returned in struct ptp_sys_time, so the counter source
is visible
Does that hold for the extended path, given that post_systime.sys_counter
and post_systime.sys_counter_id are not printed?
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 4 · Low: 0
- [Medium] vmclock_populate_ptp_attributes()
(drivers/ptp/ptp_vmclock.c:193-196) clamps the computed 64-bit worst-…
- [Medium] vmclock_populate_ptp_attributes() sets have_error_bound (and
therefore PTP_ATTRS_VALID_ERROR_BOUND) based on…
- [Medium] The driver advertises PTP_TIMESCALE_TAI for vmclock devices
(drivers/ptp/ptp_vmclock.c:114-115 and the UTC-converted case at…
- [Medium] vmclock_populate_ptp_attributes() introduces the first
*runtime* uses of VMCLOCK_FIELD_PRESENT (drivers/ptp/ptp_vmclock.c:158…
--
ptp: ptp_vmclock: Implement attributes ioctls
This adds gettimexattrs64 and getcrosststampattrs to ptp_vmclock so the
new PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS
ioctls can report an error bound, a clock status and a timescale. The
attributes are filled in inside the seq_count loop, and the UNRELIABLE
check that used to abort the read is moved after the loop so that only
the legacy ioctls still fail with -EINVAL.
A few questions about the new attribute code below.
+static void vmclock_populate_ptp_attributes(struct vmclock_state *st,+ struct ptp_clock_attrs *att,+ uint64_t delta)+{+ uint64_t maxerror_ns = UINT_MAX;+ bool have_error_bound = false;++ if (!att)+ return;++ /* Called once per seq_count retry, so don't accumulate stale bits */+ att->valid = 0;+ att->error_bound = 0;++ /* Only calculate if the base error is flagged as valid+ * by the hypervisor.+ */+ if (VMCLOCK_FIELD_PRESENT(st->clk, time_maxerror_nanosec) &&+ (le64_to_cpu(st->clk->flags) & VMCLOCK_FLAG_TIME_MAXERROR_VALID)) {+ have_error_bound = true;+ maxerror_ns = le64_to_cpu(st->clk->time_maxerror_nanosec);
[Severity: Medium]
Is VMCLOCK_FIELD_PRESENT safe to evaluate at ioctl time? The macro reads
the size field straight out of the shared mapping:
drivers/ptp/ptp_vmclock.c:
#define VMCLOCK_FIELD_PRESENT(_c, _f) \
(le32_to_cpu((_c)->size) >= (offsetof(struct vmclock_abi, _f) + \
sizeof((_c)->_f)))
and size is only compared against the mapping length once, in
vmclock_probe():
st->clk = devm_memremap(dev, st->res.start, resource_size(&st->res),
MEMREMAP_WB | MEMREMAP_DEC);
...
if (le32_to_cpu(st->clk->magic) != VMCLOCK_MAGIC ||
le32_to_cpu(st->clk->size) > resource_size(&st->res) ||
Before this patch VMCLOCK_FIELD_PRESENT was only used at probe, right
after that comparison, so the checked and used values were the same
snapshot. These new call sites run long afterwards, and the hypervisor
can raise size in shared memory at any point.
PTP registration only requires size to cover time_frac_sec, i.e. 88
bytes, while time_maxerror_nanosec sits at bytes 96..103 of struct
vmclock_abi. With a resource of 88..103 bytes and a later size write of
104 or more, does the dereference of st->clk->time_maxerror_nanosec then
read past the end of the devm_memremap()ed region, and publish whatever
it finds with PTP_ATTRS_VALID_ERROR_BOUND set?
The seq_count loop does not cover this, since it only detects hypervisor
updates and never re-validates size against resource_size(&st->res).
Would snapshotting the validated size at probe time (e.g. a clk_size
field in struct vmclock_state) and using that at runtime be preferable?
[Severity: Medium]
Should have_error_bound depend on VMCLOCK_FLAG_TIME_MAXERROR_VALID alone?
time_maxerror_nanosec bounds the error of the reference time at
counter_value, while counter_period_maxerror_rate_frac_sec bounds the
error accumulated while extrapolating over the elapsed ticks. The two
validity flags are independent bits in
include/uapi/linux/vmclock-abi.h:
#define VMCLOCK_FLAG_PERIOD_MAXERROR_VALID (1 << 4)
#define VMCLOCK_FLAG_TIME_MAXERROR_VALID (1 << 6)
The timestamp handed back to userspace is taken at counter_value + delta.
When VMCLOCK_FLAG_PERIOD_MAXERROR_VALID is clear and delta is non-zero,
the extrapolation error is unknown, yet the code below still sets
PTP_ATTRS_VALID_ERROR_BOUND on the reference-time error only. With the
50 PPM counter tolerance the vmclock ABI header itself mentions, tens of
seconds of delta already add milliseconds that are not reflected in the
reported bound. Would it be more correct to leave
PTP_ATTRS_VALID_ERROR_BOUND clear in that case?
quoted hunk
++ /* If frequency error is also valid, accumulate it+ * over the delta.+ */+ if (VMCLOCK_FIELD_PRESENT(st->clk, counter_period_maxerror_rate_frac_sec) &&+ (le64_to_cpu(st->clk->flags) & VMCLOCK_FLAG_PERIOD_MAXERROR_VALID)) {+ uint64_t maxerror_rate, err_hi, err_frac, growth_ns;++ maxerror_rate = le64_to_cpu(st->clk->counter_period_maxerror_rate_frac_sec);+ err_frac = mul_u64_u64_shr_add_u64(&err_hi, delta,+ maxerror_rate,+ st->clk->counter_period_shift,+ 0);++ if (err_hi >= U64_MAX / NSEC_PER_SEC) {+ maxerror_ns = U64_MAX;+ goto saturate;+ }++ growth_ns = (err_hi * NSEC_PER_SEC) ++ mul_u64_u64_shr(err_frac, NSEC_PER_SEC, 64);++ /* Guard against overflow */+ if (U64_MAX - growth_ns < maxerror_ns)+ maxerror_ns = U64_MAX;+ else+ maxerror_ns += growth_ns;+ }+ }++saturate:+ if (have_error_bound) {+ att->error_bound = (maxerror_ns > UINT_MAX) ?+ UINT_MAX : (uint32_t)maxerror_ns;+ att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;+ }
[Severity: Medium]
Can this report a bound that is smaller than the real one? The clamp to
UINT_MAX caps the value at roughly 4.295 s, and PTP_ATTRS_VALID_ERROR_BOUND
is still set, so userspace has no way to tell that saturation happened.
The kernel-doc for struct ptp_clock_attrs in include/uapi/linux/ptp_clock.h
describes the field as:
* @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.
and defines no saturation sentinel, only allowing the value to be
distrusted when @status is UNKNOWN or UNRELIABLE. A hypervisor
publishing time_maxerror_nanosec above 0xffffffff (a legal __le64 value)
with status SYNCHRONIZED, or a long enough delta in the growth
accumulation above, both end up at 0xffffffff with the valid bit set,
including the explicit maxerror_ns = U64_MAX branches.
Would it be better to leave PTP_ATTRS_VALID_ERROR_BOUND clear when the
value does not fit in __u32, or to define a saturation convention in the
uAPI? Related to that, the maxerror_ns = UINT_MAX initializer looks dead
now, since it is overwritten whenever have_error_bound becomes true.
quoted hunk
+ att->timescale = vmclock_get_ptp_timescale(st->clk->time_type);+ /* tai_adjust() already converted UTC to TAI before we're called */+ if (st->clk->time_type == VMCLOCK_TIME_UTC)+ att->timescale = PTP_TIMESCALE_TAI;+ att->valid |= PTP_ATTRS_VALID_TIMESCALE;
[Severity: Medium]
Which epoch does PTP_TIMESCALE_TAI imply here? The two headers that are
in tree after this series disagree.
include/uapi/linux/vmclock-abi.h:
#define VMCLOCK_TIME_TAI 1 /* Since 1970-01-01 00:00:00z */
and tspec->tv_sec is built directly from time_sec, so the value returned
to userspace is Unix-epoch based in both the TAI and the tai_adjust()
converted UTC case.
include/uapi/linux/ptp_clock.h, as added by "ptp: Add ioctls for PHC
timestamps with quality attributes" in this series, documents:
/**
* International Atomic Time (TAI)
* Epoch: 1958-01-01 00:00:00.
...
PTP_TIMESCALE_TAI = 1,
in explicit contrast to PTP_TIMESCALE_UTC/POSIX which it documents with
the 1970 epoch. A consumer following the enum documentation would be off
by about 378.7 million seconds. The commit message also states:
- timescale: Always reports TAI (UTC sources are converted by tai_adjust()
before the attributes are populated)
Should the enum documentation be corrected to the Unix epoch that PHC
timestamps actually use, or is the driver expected to shift the value?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 0
- [High] The commit message states it "Introduce[s] a feature version
mechanism to indicate device supports error_bound", but the mechanism…
- [Medium] Version handshake is implemented as an exact-equality test
against a field this patch documents as the device's *maximum*…
--
net: ena: Update PHC admin interface for error bound support
This extends the ENA admin definitions with an error_bound field in the PHC
response structure, replaces the PHC type enum with a PHC feature version
enum, and adds an error flag for error_bound retrieval failures. The
handshake in ena_com_phc_config() is updated to request and check the new
version field.
[Severity: High]
The commit message says:
"Introduce a feature version mechanism to indicate device supports
error_bound"
Can this mechanism actually indicate that? ENA_ADMIN_PHC_FEATURE_VERSION_0
is 0, which is the same wire value as the removed ENA_ADMIN_PHC_TYPE_READLESS
(also 0), and ena_com_phc_config() still sends 0 in
feat_common.feature_version and still accepts a response value of 0:
ret = ena_com_get_feature(ena_dev,
&get_feat_resp,
ENA_ADMIN_PHC_CONFIG,
ENA_ADMIN_PHC_FEATURE_VERSION_0);
...
if (get_feat_resp.u.phc.version != ENA_ADMIN_PHC_FEATURE_VERSION_0) {
So the bytes exchanged with the device look identical to the pre-patch
exchange, and firmware predating error_bound reports the same value as
firmware implementing it. I did not find another discriminator either:
struct ena_admin_acq_common_desc carries no feature_version echo, and
enum ena_admin_aq_caps_id has no PHC/error-bound capability bit.
Does that matter for the new field? error_bound is carved out of the
previously reserved bytes of the device-written DMA buffer
struct ena_admin_phc_resp:
If a device that only implements the old readless layout never writes those
bytes, the driver reads the zero left by the coherent allocation (or stale
data) and ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND is never set.
Later in this series, ena_phc_gettimexattrs64() publishes that value to
userspace as authoritative:
if (att) {
att->error_bound = error_bound_nsec;
att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;
}
which would report a validated 0 ns error bound on hardware that never
reported one. Would giving the error_bound-capable format a distinct value
(for example VERSION_1) and accepting version >= the requested one keep old
and new firmware distinguishable? Once value 0 is shipped as meaning
"readless with error_bound", that distinction cannot be recovered in
released kernels.
quoted hunk
@@ -1035,10 +1037,10 @@ struct ena_admin_queue_ext_feature_desc { }; struct ena_admin_feature_phc_desc {- /* PHC type as defined in enum ena_admin_get_phc_type,- * used only for GET command.+ /* PHC version as defined in enum ena_admin_phc_feature_version,+ * used only for GET command as max supported PHC version by the device. */- u8 type;+ u8 version; /* Reserved - MBZ */ u8 reserved1[3];
@@ -1694,10 +1694,11 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)returnret;}-/* Supporting only readless PHC retrieval */-if(get_feat_resp.u.phc.type!=ENA_ADMIN_PHC_TYPE_READLESS){+/* Supporting only PHC V0 (readless mode with error bound) */+if(get_feat_resp.u.phc.version!=ENA_ADMIN_PHC_FEATURE_VERSION_0){netdev_err(ena_dev->net_device,-"Unsupported PHC type, error: %d\n",+"Unsupported PHC version (0x%X), error: %d\n",+get_feat_resp.u.phc.version,-EOPNOTSUPP);return-EOPNOTSUPP;}
[Severity: Medium]
Should this be an ordering test rather than an equality test? The new
comment on the same byte in struct ena_admin_feature_phc_desc describes it
as a ceiling:
/* PHC version as defined in enum ena_admin_phc_feature_version,
* used only for GET command as max supported PHC version by the device.
*/
u8 version;
With that meaning, firmware that advertises a maximum of 1 while still
serving the V0 layout the driver explicitly asked for through
feat_common.feature_version = ENA_ADMIN_PHC_FEATURE_VERSION_0 fails the
"!=" test, ena_com_phc_config() returns -EOPNOTSUPP, and ena_phc_init()
takes the err_ena_com_phc_config path (ena_com_phc_destroy(),
ena_phc_enable(adapter, false), ena_devlink_disable_phc_param()), leaving
PHC/PTP unavailable on hardware that supports the requested interface.
The two comments in ena_admin_defs.h also seem to disagree about what the
device puts in the response. ena_admin_get_set_feature_common_desc says:
/* The driver specifies the max feature version it supports and the
* device responds with the currently supported feature version. The
* field is zero based
*/
u8 feature_version;
that is, a value clamped to what the driver requested, for which equality is
fine, while the new phc comment says the field carries the device maximum,
for which equality is wrong. Which contract applies here? If the device
clamps, could the new comment be reworded; if it reports its maximum, should
the check become something like
if (get_feat_resp.u.phc.version < ENA_ADMIN_PHC_FEATURE_VERSION_0)
or an explicit minimum-required-version comparison?
For reference, the existing precedent in ena_com_get_max_queue_ext() uses
equality (get_resp.u.max_queue_ext.version != ENA_FEATURE_MAX_QUEUE_EXT_VER),
but that field's comment is only "/* version */" and never claims to be a
maximum.
Also, "Unsupported PHC version (0x%X)" prints a u8 with %X; would %u be
clearer here, matching the zero based version numbering?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] ena_phc_gettimexattrs64()
(drivers/net/ethernet/amazon/ena/ena_phc.c:57-60) unconditionally…
- [Low] ena_phc_gettimexattrs64()
(drivers/net/ethernet/amazon/ena/ena_phc.c:28-62) is a verbatim…
--
net: ena: Implement gettimexattrs64 callback for PTP attributes
Adds ena_phc_gettimexattrs64() to the ENA PHC code and registers it in
ena_ptp_clock_info so the PTP_SYS_OFFSET_EXTENDED_ATTRS ioctl is supported.
The new callback reports the device error_bound value and sets the
corresponding valid bit; other attributes are left unreported.
A couple of questions below.
@@ -25,6 +25,43 @@ static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,return-EOPNOTSUPP;}+staticintena_phc_gettimexattrs64(structptp_clock_info*clock_info,+structtimespec64*ts,+structptp_system_timestamp*sts,+structptp_clock_attrs*att)+{+structena_phc_info*phc_info=+container_of(clock_info,structena_phc_info,clock_info);+u32error_bound_nsec;+unsignedlongflags;+u64timestamp_nsec;+intrc;++spin_lock_irqsave(&phc_info->lock,flags);++ptp_read_system_prets(sts);++rc=ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,+×tamp_nsec,+&error_bound_nsec);++ptp_read_system_postts(sts);++spin_unlock_irqrestore(&phc_info->lock,flags);++if(rc)+returnrc;++*ts=ns_to_timespec64(timestamp_nsec);+
[Severity: Low]
This isn't a bug, but the body above is a verbatim copy of
ena_phc_gettimex64(): same container_of(), same
spin_lock_irqsave(&phc_info->lock, flags) / ptp_read_system_prets() /
ena_com_phc_get_timestamp() / ptp_read_system_postts() /
spin_unlock_irqrestore() sequence, same rc handling and same
*ts = ns_to_timespec64(timestamp_nsec).
Since ena_phc_gettimexattrs64() already tolerates att == NULL (and is in
fact called with sts == NULL and att == NULL from ptp_clock_gettime() in
drivers/ptp/ptp_clock.c), could ena_phc_gettimex64() simply become:
static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
struct timespec64 *ts,
struct ptp_system_timestamp *sts)
{
return ena_phc_gettimexattrs64(clock_info, ts, sts, NULL);
}
Passing a non-NULL error_bound to ena_com_phc_get_timestamp() has no side
effect beyond storing the value, since that function guards the store with
if (error_bound). Keeping two copies of the PHC doorbell/poll sequence means
a later fix can land in only one of them.
[Severity: Medium]
Is PTP_ATTRS_VALID_ERROR_BOUND being advertised here without a device
capability gate?
The only capability check in the driver is in ena_com_phc_config(), which
accepts:
/* Supporting only PHC V0 (readless mode with error bound) */
if (get_feat_resp.u.phc.version != ENA_ADMIN_PHC_FEATURE_VERSION_0) {
ENA_ADMIN_PHC_FEATURE_VERSION_0 is 0, which is the same numeric value that
pre-series firmware reports for the old name ENA_ADMIN_PHC_TYPE_READLESS.
The preceding commit only renames u8 type to u8 version and re-documents 0
as "readless with error_bound"; it also carves u32 error_bound out of the
previously reserved (must-be-zero) bytes of struct ena_admin_phc_resp.
So can a PHC-capable device whose firmware predates error_bound still pass
that version check, leave the DMA bytes at zero, and have
ena_com_phc_get_timestamp() copy that zero out:
/* PHC timestamp value is returned to the caller */
*timestamp = resp->timestamp;
if (error_bound)
*error_bound = resp->error_bound;
In that case userspace receives error_bound = 0 ns with the validity bit
asserted, i.e. it is told the timestamp is exact, rather than being told the
attribute is unavailable, which is what the valid bitmask exists to express.
The device-side error signal does not seem to help here either:
ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND (checked via ENA_PHC_ERROR_FLAGS) is a
new flag that error_bound-unaware firmware would never set.
Would it make sense to gate the valid bit on a distinct feature version
value or an explicitly advertised device bit? Alternatively, if every
PHC-capable ENA firmware that reports version 0 does populate error_bound
and no older firmware exists in the field, could the commit message state
that?