[PATCH v7 net-next 0/7] ptp: Add PHC timestamp quality attributes

[PATCH v7 net-next 0/7] ptp: Add PHC timestamp quality attributes

[Arthur Kiyanovski]

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

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

[Arthur Kiyanovski]

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 <amitbern@amazon.com>
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
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(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index dc23cd708cfe..da6792102ce7 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2010 OMICRON electronics GmbH
  */
+#include <linux/clocksource_ids.h>
 #include <linux/compat.h>
 #include <linux/module.h>
 #include <linux/posix-clock.h>
@@ -190,6 +191,10 @@ static long ptp_clock_getcaps(struct ptp_clock *ptp, void __user *arg)
 		.cross_timestamping	= ptp->info->getcrosststamp != NULL,
 		.adjust_phase		= ptp->info->adjphase != NULL &&
 					  ptp->info->getmaxphase != NULL,
+		.extended_attrs		= ptp->info->gettimexattrs64 != NULL ||
+					  ptp->info->gettimex64 != NULL,
+		.precise_attrs		= ptp->info->getcrosststampattrs != NULL ||
+					  ptp->info->getcrosststamp != NULL,
 	};
 
 	if (caps.adjust_phase)
@@ -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;
+	int err;
 
 	if (!gettimex_fn)
 		return -EOPNOTSUPP;
@@ -363,23 +405,13 @@ static long ptp_sys_offset_extended(struct ptp_clock *ptp, void __user *arg,
 	if (extoff->n_samples > PTP_MAX_SAMPLES || extoff->rsv[0] || extoff->rsv[1])
 		return -EINVAL;
 
-	switch (extoff->clockid) {
-	case CLOCK_REALTIME:
-	case CLOCK_MONOTONIC:
-	case CLOCK_MONOTONIC_RAW:
-		break;
-	case CLOCK_AUX ... CLOCK_AUX_LAST:
-		if (IS_ENABLED(CONFIG_POSIX_AUX_CLOCKS))
-			break;
-		fallthrough;
-	default:
-		return -EINVAL;
-	}
+	err = ptp_validate_sys_offset_clockid(extoff->clockid);
+	if (err)
+		return err;
 
 	sts.clockid = extoff->clockid;
 	for (unsigned int i = 0; i < extoff->n_samples; i++) {
 		struct timespec64 ts;
-		int err;
 
 		err = gettimex_fn(ptp->info, &ts, &sts);
 		if (err)
@@ -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;
+	default:
+		/* CSID_X86_KVM_CLK is deliberately mapped to unknown:
+		 * kvmclock is not a raw hardware counter.
+		 */
+		return PTP_COUNTER_UNKNOWN;
+	}
+}
+
+static void ptp_fill_sys_counter(struct ptp_sys_time *st, u64 cycles,
+				 enum clocksource_ids cs_id)
+{
+	st->sys_counter_id = ptp_counter_id_from_csid(cs_id);
+	st->sys_counter = st->sys_counter_id == PTP_COUNTER_UNKNOWN ? 0 : cycles;
+}
+
+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;
+	unsigned int n_samples;
+	int err;
+
+	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)
+		return err;
+
+	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 (unsigned int i = 0; i < n_samples; i++) {
+		struct ptp_system_timestamp sts = { .clockid = request.clock_id };
+		struct ptp_timestamp *tstamp = &data->timestamps[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;
+
+		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);
+	}
+
+	return copy_to_user(arg, data,
+			    struct_size(data, timestamps, n_samples)) ? -EFAULT : 0;
+}
+
+static long ptp_sys_offset_precise_attrs(struct ptp_clock *ptp, void __user *arg)
+{
+	struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
+	struct system_device_crosststamp xtstamp = {};
+	struct ptp_attrs_request request;
+	struct ptp_clock_attrs att = {};
+	struct ptp_timestamp *tstamp;
+	struct timespec64 ts;
+	int err;
+
+	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)
+		return err;
+
+	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);
+	else if (ptp->info->getcrosststamp)
+		err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
+	else
+		return -EOPNOTSUPP;
+
+	if (err)
+		return err;
+
+	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;
+
+	return copy_to_user(arg, data,
+			    struct_size(data, timestamps, 1)) ? -EFAULT : 0;
+}
+
 static long ptp_sys_offset(struct ptp_clock *ptp, void __user *arg)
 {
 	struct ptp_sys_offset *sysoff __free(kfree) = NULL;
@@ -539,11 +715,17 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 		return ptp_sys_offset_precise(ptp, argptr,
 					      ptp->info->getcrosststamp);
 
+	case PTP_SYS_OFFSET_PRECISE_ATTRS:
+		return ptp_sys_offset_precise_attrs(ptp, argptr);
+
 	case PTP_SYS_OFFSET_EXTENDED:
 	case PTP_SYS_OFFSET_EXTENDED2:
 		return ptp_sys_offset_extended(ptp, argptr,
 					       ptp->info->gettimex64);
 
+	case PTP_SYS_OFFSET_EXTENDED_ATTRS:
+		return ptp_sys_offset_extended_attrs(ptp, argptr);
+
 	case PTP_SYS_OFFSET:
 	case PTP_SYS_OFFSET2:
 		return ptp_sys_offset(ptp, argptr);
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 4111342d64f0..27c0cd1eba59 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -113,7 +113,9 @@ static int ptp_clock_gettime(struct posix_clock *pc, struct timespec64 *tp)
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	int err;
 
-	if (ptp->info->gettimex64)
+	if (ptp->info->gettimexattrs64)
+		err = ptp->info->gettimexattrs64(ptp->info, tp, NULL, NULL);
+	else if (ptp->info->gettimex64)
 		err = ptp->info->gettimex64(ptp->info, tp, NULL);
 	else
 		err = ptp->info->gettime64(ptp->info, tp);
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 36a27a910595..9bf2b6066e78 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -123,11 +123,24 @@ struct ptp_system_timestamp {
  *               reading the lowest bits of the PHC timestamp and the second
  *               reading immediately follows that.
  *
+ * @gettimexattrs64:  Same as @gettimex64, but also fills @att (if not NULL)
+ *                    with the maximum error bound for the returned PHC
+ *                    timestamp in nanoseconds, the timescale for the returned
+ *                    PHC timestamp and the clock's qualitative synchronization
+ *                    status. As for @gettimex64, @sts may be NULL; @att may
+ *                    be NULL independently.
+ *
  * @getcrosststamp:  Reads the current time from the hardware clock and
  *                   system clock simultaneously.
  *                   parameter cts: Contains timestamp (device,system) pair,
  *                   where system time is realtime and monotonic.
  *
+ * @getcrosststampattrs:  Same as @getcrosststamp, but also fills @att (if not
+ *                        NULL) with the maximum error bound for the returned
+ *                        PHC timestamp in nanoseconds, the timescale for the
+ *                        returned PHC timestamp and the clock's qualitative
+ *                        synchronization status.
+ *
  * @settime64:  Set the current time on the hardware clock.
  *              parameter ts: Time value to set.
  *
@@ -209,8 +222,15 @@ struct ptp_clock_info {
 	int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
 	int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
 			  struct ptp_system_timestamp *sts);
+	int (*gettimexattrs64)(struct ptp_clock_info *ptp,
+			       struct timespec64 *ts,
+			       struct ptp_system_timestamp *sts,
+			       struct ptp_clock_attrs *att);
 	int (*getcrosststamp)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
+	int (*getcrosststampattrs)(struct ptp_clock_info *ptp,
+				   struct system_device_crosststamp *cts,
+				   struct ptp_clock_attrs *att);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
 	int (*getcycles64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
 	int (*getcyclesx64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 46d45f902486..98a86c5621d4 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -79,6 +79,149 @@
  */
 #define PTP_PEROUT_V1_VALID_FLAGS	(0)
 
+/*
+ * Clock status values for struct ptp_clock_attrs.status
+ */
+enum ptp_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,
+
+	/*
+	 * Clock is drifting but remains within acceptable error bounds;
+	 * error_bound is valid and can be trusted.
+	 */
+	PTP_CLOCK_STATUS_HOLDOVER     = 3,
+
+	/*
+	 * Clock is free-running: not currently disciplined toward a reference
+	 * (unlike HOLDOVER), but coasting on a known oscillator. error_bound
+	 * remains valid and can be trusted, and typically grows over time.
+	 */
+	PTP_CLOCK_STATUS_FREE_RUNNING = 4,
+
+	/*
+	 * Clock is considered broken (e.g. the oscillator is faulty or
+	 * abnormally unstable): error_bound cannot be trusted. A clock that is
+	 * merely unsynchronized or resynchronizing should report
+	 * PTP_CLOCK_STATUS_UNKNOWN or PTP_CLOCK_STATUS_INITIALIZING instead.
+	 */
+	PTP_CLOCK_STATUS_UNRELIABLE   = 5
+};
+
+/*
+ * Clock timescale values for struct ptp_clock_attrs.timescale.
+ *
+ * These definitions describe the mathematical properties and reference
+ * epochs of the timescale provided by the PHC.
+ *
+ * Discipline: Describes the frequency/phase steering behavior.
+ * Continuity: Describes whether the timeline is uninterrupted.
+ */
+enum ptp_clock_timescale {
+	/* Unknown or unspecified timescale */
+	PTP_TIMESCALE_UNKNOWN = 0,
+
+	/********************* Absolute Atomic Timescales *********************
+	 * These timescales are continuous, monotonic standards based on atomic
+	 * physics. They do not experience phase jumps.
+	 **********************************************************************/
+
+	/**
+	 * International Atomic Time (TAI)
+	 * Epoch: 1958-01-01 00:00:00.
+	 * Continuity: Strictly monotonic and continuous; no leap seconds.
+	 * Discipline: Primary atomic reference; no phase jumps.
+	 */
+	PTP_TIMESCALE_TAI = 1,
+
+	/**
+	 * Terrestrial Time (TT)
+	 * Epoch: 1958-01-01 00:00:00.
+	 * Continuity: Strictly monotonic and continuous; no leap seconds.
+	 * Discipline: Defined as TAI + 32.184s constant offset.
+	 */
+	PTP_TIMESCALE_TT = 2,
+
+	/**
+	 * Global Positioning System (GPS) Time
+	 * Epoch: 1980-01-06 00:00:00.
+	 * Continuity: Strictly monotonic and continuous; no leap seconds.
+	 * Discipline: Defined by the GPS constellation; fixed offset from TAI.
+	 */
+	PTP_TIMESCALE_GPS = 3,
+
+	/****************** UTC-Based Timescales (Civil Time) *****************
+	 * These timescales are derived from TAI but adjusted to align with
+	 * the Earth's rotation, primarily through leap seconds.
+	 **********************************************************************/
+
+	/**
+	 * Coordinated Universal Time (UTC) - Wall-clock (CLOCK_REALTIME)
+	 * Epoch: 1970-01-01 00:00:00 (Unix epoch).
+	 * Continuity: Discontinuous; subject to 1-second leap second
+	 *             phase jumps.
+	 * Discipline: Frequency steered; incorporates leap second corrections.
+	 *
+	 * Note: Leap-smeared UTC MUST NOT be advertised as PTP_TIMESCALE_UTC.
+	 * Smear algorithms are not standardized and the resulting timescale
+	 * is ambiguous. Implementations using smeared UTC MUST advertise
+	 * PTP_TIMESCALE_UNKNOWN or PTP_TIMESCALE_PROPRIETARY instead.
+	 */
+	PTP_TIMESCALE_UTC = 4,
+
+	/**
+	 * POSIX Time (Unix Time)
+	 * Epoch: 1970-01-01 00:00:00.
+	 * Continuity: Discontinuous; leap seconds handled by
+	 *             repeating/skipping values.
+	 * Discipline: Follows UTC frequency steering and phase jumps.
+	 */
+	PTP_TIMESCALE_POSIX = 5,
+
+	/****************** System-Relative Monotonic Clocks ******************
+	 * These timescales are relative to a system event (like boot)
+	 * and are not synchronized to an external atomic standard.
+	 **********************************************************************/
+
+	/**
+	 * Monotonic System Clock (CLOCK_MONOTONIC)
+	 * Epoch: Arbitrary (System boot time).
+	 * Continuity: Strictly monotonic; no leap seconds.
+	 * Discipline: Frequency steered to match system reference;
+	 *             does not advance during suspend.
+	 */
+	PTP_TIMESCALE_MONOTONIC = 6,
+
+	/**
+	 * Raw Monotonic System Clock (CLOCK_MONOTONIC_RAW)
+	 * Epoch: Arbitrary (System boot time).
+	 * Continuity: Strictly monotonic; no leap seconds.
+	 * Discipline: Raw hardware oscillator; no frequency steering
+	 *             or discipline.
+	 */
+	PTP_TIMESCALE_MONOTONIC_RAW = 7,
+
+	/**
+	 * Boot Time System Clock (CLOCK_BOOTTIME)
+	 * Epoch: Arbitrary (System boot time).
+	 * Continuity: Strictly monotonic and continuous; no leap seconds.
+	 * Discipline: Frequency steered to match system reference;
+	 *             advances during suspend.
+	 */
+	PTP_TIMESCALE_BOOTTIME = 8,
+
+	/********************** Vendor-Specific Timescale *********************/
+
+	/* A proprietary or vendor-specific timescale with custom rules. */
+	PTP_TIMESCALE_PROPRIETARY = 9,
+};
+
 /*
  * struct ptp_clock_time - represents a time value
  *
@@ -94,6 +237,136 @@ struct ptp_clock_time {
 	__u32 reserved;
 };
 
+/*
+ * Hardware counter identifiers for struct ptp_sys_time.sys_counter_id
+ */
+enum ptp_counter_id {
+	/* Counter value not available or type not specified */
+	PTP_COUNTER_UNKNOWN = 0,
+
+	/* x86 Time Stamp Counter (TSC) */
+	PTP_COUNTER_X86_TSC = 1,
+
+	/* ARM Generic Timer virtual counter */
+	PTP_COUNTER_ARM_ARCH = 2,
+};
+
+/* Valid flags for struct ptp_clock_attrs.valid */
+#define PTP_ATTRS_VALID_ERROR_BOUND	(1 << 0)
+#define PTP_ATTRS_VALID_TIMESCALE	(1 << 1)
+#define PTP_ATTRS_VALID_STATUS		(1 << 2)
+
+/**
+ * struct ptp_clock_attrs - quality attributes for a PHC timestamp
+ *
+ * @valid:       Bitmask of PTP_ATTRS_VALID_* indicating which fields
+ *               are populated. Zero means no attributes available.
+ * @error_bound: Maximum error (an upper bound, in nanoseconds) between the
+ *               returned device_time and true time on the advertised
+ *               @timescale; a worst-case bound, not a statistical estimate.
+ *               Valid only when PTP_ATTRS_VALID_ERROR_BOUND is set, and must
+ *               not be trusted when @status is PTP_CLOCK_STATUS_UNKNOWN or
+ *               PTP_CLOCK_STATUS_UNRELIABLE.
+ * @timescale:   Clock timescale (enum ptp_clock_timescale). Valid only
+ *               when PTP_ATTRS_VALID_TIMESCALE is set.
+ * @status:      Synchronization status (enum ptp_clock_status). Valid
+ *               only when PTP_ATTRS_VALID_STATUS is set. Transitions between
+ *               states are device-specific; there are no kernel-defined
+ *               thresholds relating @status to @error_bound.
+ * @rsv:         Reserved for future use, must be zero.
+ */
+struct ptp_clock_attrs {
+	__u32 valid;
+	__u32 error_bound;
+	__u32 timescale;
+	__u32 status;
+	__u32 rsv[4];
+};
+
+/**
+ * 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.
+ * @sys_counter:    Raw clocksource counter value (0 = unavailable).
+ * @sys_counter_id: Identifies the counter (enum ptp_counter_id).
+ * @rsv:            Reserved for future use, must be zero.
+ */
+struct ptp_sys_time {
+	__s64 sys_time;
+	__s64 sys_rawtime;
+	__u64 sys_counter;
+	__u32 sys_counter_id;
+	__u32 rsv;
+};
+
+/**
+ * struct ptp_dev_time - device timestamp with quality attributes
+ *
+ * @device_time: PHC timestamp value.
+ * @attrs:       Quality attributes for this timestamp.
+ */
+struct ptp_dev_time {
+	struct ptp_clock_time device_time;
+	struct ptp_clock_attrs attrs;
+};
+
+/**
+ * struct ptp_timestamp - a complete timestamp sample
+ *
+ * @systime:      System time snapshot; shares storage with @pre_systime,
+ *                used by PTP_SYS_OFFSET_PRECISE_ATTRS.
+ * @pre_systime:  System time read right before the device read, used by
+ *                PTP_SYS_OFFSET_EXTENDED_ATTRS.
+ * @devtime:      Device timestamp with its quality attributes.
+ * @post_systime: System time read right after the device read, used by
+ *                PTP_SYS_OFFSET_EXTENDED_ATTRS.
+ *
+ * For PTP_SYS_OFFSET_EXTENDED_ATTRS: pre_systime and post_systime bracket
+ * the device read (ABA sandwich).
+ * For PTP_SYS_OFFSET_PRECISE_ATTRS: only systime (union with pre_systime)
+ * is meaningful; post_systime is zeroed.
+ */
+struct ptp_timestamp {
+	union {
+		struct ptp_sys_time systime;
+		struct ptp_sys_time pre_systime;
+	};
+	struct ptp_dev_time devtime;
+	struct ptp_sys_time post_systime;
+};
+
+/**
+ * struct ptp_attrs_request - request parameters for attrs ioctls
+ *
+ * @valid:       Bitmask for future request extensions. Must be zero for now.
+ * @clock_id:    Clock base for system timestamps (CLOCK_REALTIME, etc).
+ * @num_samples: Number of timestamp samples requested.
+ *               For PTP_SYS_OFFSET_PRECISE_ATTRS must be 1.
+ * @rsv:         Reserved for future use, must be zero.
+ */
+struct ptp_attrs_request {
+	__u32 valid;
+	__kernel_clockid_t clock_id;
+	__u32 num_samples;
+	__u32 rsv[3];
+};
+
+/**
+ * struct ptp_sys_offset_attrs - unified data structure for attrs ioctls
+ *
+ * @request:    Request parameters (see struct ptp_attrs_request).
+ * @timestamps: Array of returned samples; holds request.num_samples entries.
+ *
+ * Used by both PTP_SYS_OFFSET_EXTENDED_ATTRS and
+ * PTP_SYS_OFFSET_PRECISE_ATTRS. Userspace allocates space for
+ * request.num_samples entries in the timestamps array.
+ */
+struct ptp_sys_offset_attrs {
+	struct ptp_attrs_request request;
+	struct ptp_timestamp timestamps[];
+};
+
 struct ptp_clock_caps {
 	int max_adj;   /* Maximum frequency adjustment in parts per billon. */
 	int n_alarm;   /* Number of programmable alarms. */
@@ -106,7 +379,11 @@ struct ptp_clock_caps {
 	/* Whether the clock supports adjust phase */
 	int adjust_phase;
 	int max_phase_adj; /* Maximum phase adjustment in nanoseconds. */
-	int rsv[11];       /* Reserved for future use. */
+	/* Whether the clock supports extended timestamps with attributes */
+	int extended_attrs;
+	/* Whether the clock supports precise cross-timestamps with attributes */
+	int precise_attrs;
+	int rsv[9];       /* Reserved for future use. */
 };
 
 struct ptp_extts_request {
@@ -252,6 +529,10 @@ struct ptp_pin_desc {
 	_IOWR(PTP_CLK_MAGIC, 21, struct ptp_sys_offset_precise)
 #define PTP_SYS_OFFSET_EXTENDED_CYCLES \
 	_IOWR(PTP_CLK_MAGIC, 22, struct ptp_sys_offset_extended)
+#define PTP_SYS_OFFSET_PRECISE_ATTRS \
+	_IOWR(PTP_CLK_MAGIC, 23, struct ptp_sys_offset_attrs)
+#define PTP_SYS_OFFSET_EXTENDED_ATTRS \
+	_IOWR(PTP_CLK_MAGIC, 24, struct ptp_sys_offset_attrs)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occurred. */
-- 
2.47.3

[PATCH v7 net-next 2/7] selftests/ptp: Extract print_system_timestamp helper in testptp

[Arthur Kiyanovski]

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 <dwmw@amazon.co.uk>
---
 tools/testing/selftests/ptp/testptp.c | 76 +++++++++++++--------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index ed1e2886ba3c..d603d9f82237 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -153,6 +153,34 @@ static void usage(char *progname)
 		progname, PTP_MAX_SAMPLES);
 }
 
+static void print_system_timestamp(int sample_num, __kernel_clockid_t clockid,
+				   long long sec, unsigned int nsec,
+				   const char *when)
+{
+	const char *name;
+
+	switch (clockid) {
+	case CLOCK_REALTIME:
+		name = "real";
+		break;
+	case CLOCK_MONOTONIC:
+		name = "monotonic";
+		break;
+	case CLOCK_MONOTONIC_RAW:
+		name = "monotonic-raw";
+		break;
+	default:
+		name = NULL;
+		break;
+	}
+
+	if (!name)
+		return;
+
+	printf("sample #%2d: %s time %s: %lld.%09u\n",
+	       sample_num, name, when, sec, nsec);
+}
+
 int main(int argc, char *argv[])
 {
 	struct ptp_clock_caps caps;
@@ -608,46 +636,18 @@ int main(int argc, char *argv[])
 			       getextended);
 
 			for (i = 0; i < getextended; i++) {
-				switch (ext_clockid) {
-				case CLOCK_REALTIME:
-					printf("sample #%2d: real time before: %lld.%09u\n",
-					       i, soe->ts[i][0].sec,
-					       soe->ts[i][0].nsec);
-					break;
-				case CLOCK_MONOTONIC:
-					printf("sample #%2d: monotonic time before: %lld.%09u\n",
-					       i, soe->ts[i][0].sec,
-					       soe->ts[i][0].nsec);
-					break;
-				case CLOCK_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) {
-				case CLOCK_REALTIME:
-					printf("            real time after: %lld.%09u\n",
-					       soe->ts[i][2].sec,
-					       soe->ts[i][2].nsec);
-					break;
-				case CLOCK_MONOTONIC:
-					printf("            monotonic time after: %lld.%09u\n",
-					       soe->ts[i][2].sec,
-					       soe->ts[i][2].nsec);
-					break;
-				case CLOCK_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");
 			}
 		}
 
-- 
2.47.3

[PATCH v7 net-next 3/7] selftests/ptp: Add testptp support for attributes ioctls

[Arthur Kiyanovski]

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 <amitbern@amazon.com>
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
 tools/testing/selftests/ptp/testptp.c | 124 +++++++++++++++++++++++++-
 1 file changed, 120 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index d603d9f82237..62b1d0340d71 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -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);
@@ -174,8 +176,10 @@ static void print_system_timestamp(int sample_num, __kernel_clockid_t clockid,
 		break;
 	}
 
-	if (!name)
+	if (!name) {
+		printf("sample #%2d: unknown clock %d\n", sample_num, clockid);
 		return;
+	}
 
 	printf("sample #%2d: %s time %s: %lld.%09u\n",
 	       sample_num, name, when, sec, nsec);
@@ -194,6 +198,7 @@ int main(int argc, char *argv[])
 	struct ptp_sys_offset *sysoff;
 	struct ptp_sys_offset_extended *soe;
 	struct ptp_sys_offset_precise *xts;
+	struct ptp_sys_offset_attrs *attrs_data;
 
 	char *progname;
 	unsigned int i;
@@ -215,6 +220,7 @@ int main(int argc, char *argv[])
 	int pct_offset = 0;
 	int getextended = 0;
 	int getcross = 0;
+	int use_attrs = 0;
 	int n_samples = 0;
 	int pin_index = -1, pin_func;
 	int pps = -1;
@@ -232,7 +238,8 @@ int main(int argc, char *argv[])
 
 	progname = strrchr(argv[0], '/');
 	progname = progname ? 1+progname : argv[0];
-	while (EOF != (c = getopt(argc, argv, "cd:e:E:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xy:z"))) {
+	while (EOF != (c = getopt(argc, argv,
+				  "acd:e:E:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xy:z"))) {
 		switch (c) {
 		case 'c':
 			capabilities = 1;
@@ -317,6 +324,9 @@ int main(int argc, char *argv[])
 				return -1;
 			}
 			break;
+		case 'a':
+			use_attrs = 1;
+			break;
 		case 'X':
 			getcross = 1;
 			break;
@@ -373,6 +383,8 @@ int main(int argc, char *argv[])
 			       "  %d programmable pins\n"
 			       "  %d cross timestamping\n"
 			       "  %d adjust_phase\n"
+			       "  %d extended_attrs\n"
+			       "  %d precise_attrs\n"
 			       "  %d maximum phase adjustment (ns)\n",
 			       caps.max_adj,
 			       caps.n_alarm,
@@ -382,6 +394,8 @@ int main(int argc, char *argv[])
 			       caps.n_pins,
 			       caps.cross_timestamping,
 			       caps.adjust_phase,
+			       caps.extended_attrs,
+			       caps.precise_attrs,
 			       caps.max_phase_adj);
 		}
 	}
@@ -619,7 +633,7 @@ int main(int argc, char *argv[])
 		free(sysoff);
 	}
 
-	if (getextended) {
+	if (getextended && !use_attrs) {
 		soe = calloc(1, sizeof(*soe));
 		if (!soe) {
 			perror("calloc");
@@ -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);
+			}
+		}
+
+		free(attrs_data);
+	}
+
+	if (getcross && !use_attrs) {
 		xts = calloc(1, sizeof(*xts));
 		if (!xts) {
 			perror("calloc");
@@ -677,6 +744,55 @@ int main(int argc, char *argv[])
 		free(xts);
 	}
 
+	if (getcross && use_attrs) {
+		attrs_data = calloc(1, sizeof(*attrs_data) +
+				    sizeof(struct ptp_timestamp));
+		if (!attrs_data) {
+			perror("calloc");
+			return -1;
+		}
+
+		attrs_data->request.num_samples = 1;
+		/* precise crosstimestamp supports only CLOCK_REALTIME/AUX */
+		attrs_data->request.clock_id = CLOCK_REALTIME;
+
+		if (ioctl(fd, PTP_SYS_OFFSET_PRECISE_ATTRS, attrs_data)) {
+			perror("PTP_SYS_OFFSET_PRECISE_ATTRS");
+		} else {
+			struct ptp_timestamp *ts = &attrs_data->timestamps[0];
+
+			puts("precise attrs crosstimestamp request okay");
+			printf("device time: %lld.%09u\n",
+			       ts->devtime.device_time.sec,
+			       ts->devtime.device_time.nsec);
+			printf("system time: %lld ns\n",
+			       (long long)ts->systime.sys_time);
+			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);
+			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");
+		}
+
+		free(attrs_data);
+	}
+
 	if (channel >= 0) {
 		if (ioctl(fd, PTP_MASK_CLEAR_ALL)) {
 			perror("PTP_MASK_CLEAR_ALL");
-- 
2.47.3

[PATCH v7 net-next 4/7] ptp: ptp_vmclock: Implement attributes ioctls

[Arthur Kiyanovski]

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(-)

diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c
index bb0e14bac9f2..0cd449999139 100644
--- a/drivers/ptp/ptp_vmclock.c
+++ b/drivers/ptp/ptp_vmclock.c
@@ -53,6 +53,17 @@ struct vmclock_state {
 	char *name;
 };
 
+/**
+ * struct vmclock_crosststamp_ctx - context for get_device_system_crosststamp()
+ * @st: vmclock device state
+ * @attrs: optional output for PTP clock attributes, populated inside the
+ *         seq_count loop for a consistent snapshot with the timestamp
+ */
+struct vmclock_crosststamp_ctx {
+	struct vmclock_state *st;
+	struct ptp_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)
 	return false;
 }
 
+static uint8_t vmclock_get_ptp_timescale(uint8_t vmclock_time_type)
+{
+	switch (vmclock_time_type) {
+	case VMCLOCK_TIME_UTC:
+		return PTP_TIMESCALE_UTC;
+	case VMCLOCK_TIME_TAI:
+		return PTP_TIMESCALE_TAI;
+	case VMCLOCK_TIME_MONOTONIC:
+		return PTP_TIMESCALE_MONOTONIC;
+	default:
+		return PTP_TIMESCALE_UNKNOWN;
+	}
+}
+
+static uint8_t vmclock_get_ptp_status(uint8_t vmclock_status)
+{
+	switch (vmclock_status) {
+	case VMCLOCK_STATUS_UNKNOWN:
+		return PTP_CLOCK_STATUS_UNKNOWN;
+	case VMCLOCK_STATUS_INITIALIZING:
+		return PTP_CLOCK_STATUS_INITIALIZING;
+	case VMCLOCK_STATUS_SYNCHRONIZED:
+		return PTP_CLOCK_STATUS_SYNCED;
+	case VMCLOCK_STATUS_FREERUNNING:
+		return PTP_CLOCK_STATUS_FREE_RUNNING;
+	case VMCLOCK_STATUS_UNRELIABLE:
+		return PTP_CLOCK_STATUS_UNRELIABLE;
+	default:
+		return PTP_CLOCK_STATUS_UNKNOWN;
+	}
+}
+
+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);
+
+		/* 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;
+	}
+	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;
+}
+
 static int vmclock_get_crosststamp(struct vmclock_state *st,
 				   struct ptp_system_timestamp *sts,
 				   struct system_counterval_t *system_counter,
-				   struct timespec64 *tspec)
+				   struct timespec64 *tspec,
+				   struct ptp_clock_attrs *attrs)
 {
 	ktime_t deadline = ktime_add(ktime_get(), VMCLOCK_MAX_WAIT);
 	uint64_t cycle, delta, seq, frac_sec;
+	uint8_t clock_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;
-
 		/*
 		 * When invoked for gettimex64(), fill in the pre/post system
 		 * times. The simple case is when system time is based on the
@@ -164,6 +272,17 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,
 		if (!tai_adjust(st->clk, &tspec->tv_sec))
 			return -EINVAL;
 
+		/*
+		 * Capture clock state inside the seq_count loop for a
+		 * consistent snapshot with the timestamp. The attrs path
+		 * reports it to userspace via the status field; the legacy
+		 * path saves it for the UNRELIABLE check after the loop.
+		 */
+		if (attrs)
+			vmclock_populate_ptp_attributes(st, attrs, delta);
+		else
+			clock_status = st->clk->clock_status;
+
 		/*
 		 * This pairs with a write barrier in the hypervisor
 		 * which populates this structure.
@@ -181,6 +300,17 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,
 		system_counter->cs_id = st->cs_id;
 	}
 
+	/*
+	 * If attrs is set, attributes were already populated inside the
+	 * seq_count loop. Return success even for UNRELIABLE - the attrs
+	 * ioctl can report the status to userspace.
+	 */
+	if (attrs)
+		return 0;
+
+	if (clock_status == VMCLOCK_STATUS_UNRELIABLE)
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -193,7 +323,8 @@ static int vmclock_get_crosststamp(struct vmclock_state *st,
 static int vmclock_get_crosststamp_kvmclock(struct vmclock_state *st,
 					    struct ptp_system_timestamp *sts,
 					    struct system_counterval_t *system_counter,
-					    struct timespec64 *tspec)
+					    struct timespec64 *tspec,
+					    struct ptp_clock_attrs *attrs)
 {
 	struct pvclock_vcpu_time_info *pvti = this_cpu_pvti();
 	unsigned int pvti_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,
 				   struct system_counterval_t *system_counter,
 				   void *ctx)
 {
-	struct vmclock_state *st = ctx;
+	struct vmclock_crosststamp_ctx *vctx = ctx;
+	struct vmclock_state *st = vctx->st;
 	struct timespec64 tspec;
 	int ret;
 
 #ifdef SUPPORT_KVMCLOCK
 	if (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,
 	return ret;
 }
 
-static int ptp_vmclock_getcrosststamp(struct ptp_clock_info *ptp,
-				      struct system_device_crosststamp *xtstamp)
+static int ptp_vmclock_do_getcrosststamp(struct vmclock_crosststamp_ctx *vctx,
+					 struct system_device_crosststamp *xtstamp)
 {
-	struct vmclock_state *st = container_of(ptp, struct vmclock_state,
-						ptp_clock_info);
-	int ret = get_device_system_crosststamp(ptp_vmclock_get_time_fn, st,
+	int ret = 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,
 	 * timestamp that get_device_system_crosststamp() *can* handle.
 	 */
 	if (ret == -ENODEV) {
+		struct vmclock_state *st = vctx->st;
 		struct system_time_snapshot systime_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);
 		}
 	}
 #endif
 	return ret;
 }
 
+static int ptp_vmclock_getcrosststamp(struct ptp_clock_info *ptp,
+				      struct system_device_crosststamp *xtstamp)
+{
+	struct vmclock_state *st = container_of(ptp, struct vmclock_state,
+						ptp_clock_info);
+	struct vmclock_crosststamp_ctx vctx = { .st = st };
+
+	return ptp_vmclock_do_getcrosststamp(&vctx, xtstamp);
+}
+
 /*
  * PTP clock operations
  */
@@ -306,7 +449,29 @@ static int ptp_vmclock_gettimex(struct ptp_clock_info *ptp, struct timespec64 *t
 	struct vmclock_state *st = container_of(ptp, struct vmclock_state,
 						ptp_clock_info);
 
-	return vmclock_get_crosststamp(st, sts, NULL, ts);
+	return vmclock_get_crosststamp(st, sts, NULL, ts, NULL);
+}
+
+static int ptp_vmclock_gettimexattrs(struct ptp_clock_info *ptp,
+				     struct timespec64 *ts,
+				     struct ptp_system_timestamp *sts,
+				     struct ptp_clock_attrs *att)
+{
+	struct vmclock_state *st = container_of(ptp, struct vmclock_state,
+						ptp_clock_info);
+
+	return vmclock_get_crosststamp(st, sts, NULL, ts, att);
+}
+
+static int ptp_vmclock_getcrosststampattrs(struct ptp_clock_info *ptp,
+					   struct system_device_crosststamp *xtstamp,
+					   struct ptp_clock_attrs *att)
+{
+	struct vmclock_state *st = container_of(ptp, struct vmclock_state,
+						ptp_clock_info);
+	struct vmclock_crosststamp_ctx vctx = { .st = st, .attrs = att };
+
+	return ptp_vmclock_do_getcrosststamp(&vctx, xtstamp);
 }
 
 static int ptp_vmclock_enable(struct ptp_clock_info *ptp,
@@ -324,9 +489,11 @@ static const struct ptp_clock_info ptp_vmclock_info = {
 	.adjfine	= ptp_vmclock_adjfine,
 	.adjtime	= ptp_vmclock_adjtime,
 	.gettimex64	= ptp_vmclock_gettimex,
+	.gettimexattrs64 = ptp_vmclock_gettimexattrs,
 	.settime64	= ptp_vmclock_settime,
 	.enable		= ptp_vmclock_enable,
 	.getcrosststamp = ptp_vmclock_getcrosststamp,
+	.getcrosststampattrs = ptp_vmclock_getcrosststampattrs,
 };
 
 static struct ptp_clock *vmclock_ptp_register(struct device *dev,
-- 
2.47.3

[PATCH v7 net-next 5/7] net: ena: Update PHC admin interface for error bound support

[Arthur Kiyanovski]

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 <amitbern@amazon.com>
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../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(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index 898ecd96b96a..2d132c4bc590 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -128,12 +128,14 @@ enum ena_admin_get_stats_scope {
 	ENA_ADMIN_ETH_TRAFFIC                       = 1,
 };
 
-enum ena_admin_phc_type {
-	ENA_ADMIN_PHC_TYPE_READLESS                 = 0,
+enum ena_admin_phc_feature_version {
+	/* Readless with error_bound */
+	ENA_ADMIN_PHC_FEATURE_VERSION_0             = 0,
 };
 
 enum ena_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 {
 };
 
 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];
@@ -1224,7 +1226,10 @@ struct ena_admin_phc_resp {
 	/* PHC timestamp (nsec) */
 	u64 timestamp;
 
-	u8 reserved2[12];
+	u8 reserved2[8];
+
+	/* Timestamp error limit (nsec) */
+	u32 error_bound;
 
 	/* Bit field of enum ena_admin_phc_error_flags */
 	u32 error_flags;
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 297fb36ab8c1..40d1fd64bc34 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -1682,11 +1682,11 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)
 	struct ena_admin_set_feat_cmd set_feat_cmd;
 	int ret = 0;
 
-	/* Get device PHC default configuration */
+	/* Get default device PHC configuration */
 	ret = ena_com_get_feature(ena_dev,
 				  &get_feat_resp,
 				  ENA_ADMIN_PHC_CONFIG,
-				  0);
+				  ENA_ADMIN_PHC_FEATURE_VERSION_0);
 	if (unlikely(ret)) {
 		netdev_err(ena_dev->net_device,
 			   "Failed to get PHC feature configuration, error: %d\n",
@@ -1694,10 +1694,11 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)
 		return ret;
 	}
 
-	/* 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;
 	}
-- 
2.47.3

[PATCH v7 net-next 6/7] net: ena: Add error bound to PHC communication layer

[Arthur Kiyanovski]

Extend the ENA PHC communication layer to retrieve error bound from
the device.

Update ena_com_phc_get_timestamp() to retrieve error_bound alongside
timestamps.
Add error handling and statistics for error_bound retrieval failures.

Co-developed-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../device_drivers/ethernet/amazon/ena.rst    |  2 +
 drivers/net/ethernet/amazon/ena/ena_com.c     | 40 ++++++++++++-------
 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     |  3 +-
 5 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/Documentation/networking/device_drivers/ethernet/amazon/ena.rst b/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
index 14784a0a6a8a..ce9ba84bfd01 100644
--- a/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
+++ b/Documentation/networking/device_drivers/ethernet/amazon/ena.rst
@@ -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:
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 40d1fd64bc34..353cb88e880e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -45,7 +45,8 @@
 #define ENA_PHC_DEFAULT_EXPIRE_TIMEOUT_USEC 10
 #define ENA_PHC_DEFAULT_BLOCK_TIMEOUT_USEC 1000
 #define ENA_PHC_REQ_ID_OFFSET 0xDEAD
-#define ENA_PHC_ERROR_FLAGS (ENA_ADMIN_PHC_ERROR_FLAG_TIMESTAMP)
+#define ENA_PHC_ERROR_FLAGS (ENA_ADMIN_PHC_ERROR_FLAG_TIMESTAMP | \
+			     ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND)
 
 /*****************************************************************************/
 /*****************************************************************************/
@@ -1726,7 +1727,7 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)
 	if (phc->expire_timeout_usec > phc->block_timeout_usec)
 		phc->expire_timeout_usec = phc->block_timeout_usec;
 
-	/* Prepare PHC feature command */
+	/* Prepare PHC config feature command */
 	memset(&set_feat_cmd, 0x0, sizeof(set_feat_cmd));
 	set_feat_cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
 	set_feat_cmd.feat_common.feature_id = ENA_ADMIN_PHC_CONFIG;
@@ -1781,7 +1782,8 @@ void ena_com_phc_destroy(struct ena_com_dev *ena_dev)
 	phc->virt_addr = NULL;
 }
 
-int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)
+int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp,
+			      u32 *error_bound)
 {
 	const ktime_t zero_system_time = ktime_set(0, 0);
 	struct ena_com_phc_info *phc = &ena_dev->phc;
@@ -1828,6 +1830,8 @@ int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp)
 			 * a PHC error, this occurs if device:
 			 * - exceeded the get time request limit
 			 * - received an invalid timestamp
+			 * - received an excessively high error bound
+			 * - received an invalid error bound
 			 */
 			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
-			 * with valid timestamp
+			 * with valid timestamp and error bound
 			 */
 			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
-			 * blocked state until passing blocking time,
-			 * during this time any get PHC timestamp will fail with
+			/* Gave up waiting for updated req_id,
+			 * PHC enters into blocked state until passing blocking
+			 * time, during this time, any request will fail with
 			 * device busy error
 			 */
 			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
-		 * after req_id update is observed
+		/* Ensure PHC payload (timestamp, error_bound, error_flags)
+		 * is read after req_id update is observed
 		 */
 		dma_rmb();
 
 		/* req_id was updated by the device which indicates that
-		 * PHC timestamp and error_flags are updated too,
-		 * checking errors before retrieving timestamp
+		 * PHC timestamp, error_bound and error_flags are updated too,
+		 * checking error flags before retrieving timestamp and
+		 * error_bound values
 		 */
 		if (unlikely(resp->error_flags & ENA_PHC_ERROR_FLAGS)) {
-			/* Retrieved invalid PHC timestamp, PHC enters into
-			 * blocked state until passing blocking time,
-			 * during this time any get PHC timestamp requests
-			 * will fail with device busy error
+			/* Retrieved timestamp or error bound errors,
+			 * PHC enters into blocked state until passing blocking
+			 * time, during this time, any request will fail with
+			 * device busy error
 			 */
 			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;
 	}
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 64df2c48c9a6..fcbff1a9eb7a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -216,6 +216,7 @@ struct ena_com_stats_phc {
 	u64 phc_skp;
 	u64 phc_err_dv;
 	u64 phc_err_ts;
+	u64 phc_err_eb;
 };
 
 struct ena_com_admin_queue {
@@ -462,9 +463,11 @@ void ena_com_phc_destroy(struct ena_com_dev *ena_dev);
 /* ena_com_phc_get_timestamp - Retrieve PHC timestamp
  * @ena_dev: ENA communication layer struct
  * @timestamp: Retrieved PHC timestamp
+ * @error_bound: maximum possible deviation of the timestamp (nanosecond)
  * @return - 0 on success, negative value on failure
  */
-int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp);
+int ena_com_phc_get_timestamp(struct ena_com_dev *ena_dev, u64 *timestamp,
+			      u32 *error_bound);
 
 /* ena_com_set_mmio_read_mode - Enable/disable the indirect mmio reg read mechanism
  * @ena_dev: ENA communication layer struct
diff --git a/drivers/net/ethernet/amazon/ena/ena_debugfs.c b/drivers/net/ethernet/amazon/ena/ena_debugfs.c
index 46ed80986724..db9d1843996b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_debugfs.c
+++ b/drivers/net/ethernet/amazon/ena/ena_debugfs.c
@@ -32,6 +32,9 @@ static int phc_stats_show(struct seq_file *file, void *priv)
 	seq_printf(file,
 		   "phc_err_ts: %llu\n",
 		   adapter->ena_dev->phc.stats.phc_err_ts);
+	seq_printf(file,
+		   "phc_err_eb: %llu\n",
+		   adapter->ena_dev->phc.stats.phc_err_eb);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
index c2a3ff1ef645..2bcb5af564e2 100644
--- a/drivers/net/ethernet/amazon/ena/ena_phc.c
+++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
@@ -40,7 +40,8 @@ static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
 	ptp_read_system_prets(sts);
 
 	rc = ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,
-				       &timestamp_nsec);
+				       &timestamp_nsec,
+				       NULL);
 
 	ptp_read_system_postts(sts);
 
-- 
2.47.3

[PATCH v7 net-next 7/7] net: ena: Implement gettimexattrs64 callback for PTP attributes

[Arthur Kiyanovski]

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 <amitbern@amazon.com>
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/net/ethernet/amazon/ena/ena_phc.c | 58 +++++++++++++++++++----
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
index 2bcb5af564e2..725c36fe3f6e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_phc.c
+++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
@@ -25,6 +25,43 @@ static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,
 	return -EOPNOTSUPP;
 }
 
+static int ena_phc_gettimexattrs64(struct ptp_clock_info *clock_info,
+				   struct timespec64 *ts,
+				   struct ptp_system_timestamp *sts,
+				   struct ptp_clock_attrs *att)
+{
+	struct ena_phc_info *phc_info =
+		container_of(clock_info, struct ena_phc_info, clock_info);
+	u32 error_bound_nsec;
+	unsigned long flags;
+	u64 timestamp_nsec;
+	int rc;
+
+	spin_lock_irqsave(&phc_info->lock, flags);
+
+	ptp_read_system_prets(sts);
+
+	rc = ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,
+				       &timestamp_nsec,
+				       &error_bound_nsec);
+
+	ptp_read_system_postts(sts);
+
+	spin_unlock_irqrestore(&phc_info->lock, flags);
+
+	if (rc)
+		return rc;
+
+	*ts = ns_to_timespec64(timestamp_nsec);
+
+	if (att) {
+		att->error_bound = error_bound_nsec;
+		att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;
+	}
+
+	return 0;
+}
+
 static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
 			      struct timespec64 *ts,
 			      struct ptp_system_timestamp *sts)
@@ -62,16 +99,17 @@ static int ena_phc_settime64(struct ptp_clock_info *clock_info,
 }
 
 static struct ptp_clock_info ena_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 */
-- 
2.47.3

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

[netdev-bot+sashiko]

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?

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index dc23cd708cfe..da6792102ce7 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c

[ ... ]

> @@ -190,6 +191,10 @@ static long ptp_clock_getcaps(struct ptp_clock *ptp, void __user *arg)
>  		.cross_timestamping	= ptp->info->getcrosststamp != NULL,
>  		.adjust_phase		= ptp->info->adjphase != NULL &&
>  					  ptp->info->getmaxphase != NULL,
> +		.extended_attrs		= ptp->info->gettimexattrs64 != NULL ||
> +					  ptp->info->gettimex64 != NULL,
> +		.precise_attrs		= ptp->info->getcrosststampattrs != NULL ||
> +					  ptp->info->getcrosststamp != NULL,
>  	};

[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?

> @@ -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 };

[ ... ]

> @@ -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?

> +	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".

> +static void ptp_fill_sys_counter(struct ptp_sys_time *st, u64 cycles,
> +				 enum clocksource_ids cs_id)
> +{
> +	st->sys_counter_id = ptp_counter_id_from_csid(cs_id);
> +	st->sys_counter = st->sys_counter_id == PTP_COUNTER_UNKNOWN ? 0 : cycles;
> +}
> +

[ ... ]

> +	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);

[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?

> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 4111342d64f0..27c0cd1eba59 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -113,7 +113,9 @@ static int ptp_clock_gettime(struct posix_clock *pc, struct timespec64 *tp)
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
>  	int err;
>  
> -	if (ptp->info->gettimex64)
> +	if (ptp->info->gettimexattrs64)
> +		err = ptp->info->gettimexattrs64(ptp->info, tp, NULL, NULL);
> +	else if (ptp->info->gettimex64)
>  		err = ptp->info->gettimex64(ptp->info, tp, NULL);
>  	else
>  		err = ptp->info->gettime64(ptp->info, tp);

[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?

> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 36a27a910595..9bf2b6066e78 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h

[ ... ]

> @@ -209,8 +222,15 @@ struct ptp_clock_info {
>  	int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
>  	int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
>  			  struct ptp_system_timestamp *sts);
> +	int (*gettimexattrs64)(struct ptp_clock_info *ptp,
> +			       struct timespec64 *ts,
> +			       struct ptp_system_timestamp *sts,
> +			       struct ptp_clock_attrs *att);
>  	int (*getcrosststamp)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
> +	int (*getcrosststampattrs)(struct ptp_clock_info *ptp,
> +				   struct system_device_crosststamp *cts,
> +				   struct ptp_clock_attrs *att);

[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.

> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 46d45f902486..98a86c5621d4 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h

[ ... ]

> @@ -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,
> +};

[ ... ]

> +/**
> + * 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?

> +	__u32 sys_counter_id;
> +	__u32 rsv;
> +};

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com

Re: [PATCH v7 net-next 3/7] selftests/ptp: Add testptp support for attributes ioctls

[netdev-bot+sashiko]

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.

> diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
> index d603d9f82237..62b1d0340d71 100644
> --- a/tools/testing/selftests/ptp/testptp.c
> +++ b/tools/testing/selftests/ptp/testptp.c
> @@ -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);

[ ... ]

> @@ -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;

[ ... ]

> @@ -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?

> +			}
> +		}
> +
> +		free(attrs_data);
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com

Re: [PATCH v7 net-next 4/7] ptp: ptp_vmclock: Implement attributes ioctls

[netdev-bot+sashiko]

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.

> diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c
> index bb0e14bac9f2..0cd449999139 100644
> --- a/drivers/ptp/ptp_vmclock.c
> +++ b/drivers/ptp/ptp_vmclock.c

[ ... ]

> @@ -95,13 +106,113 @@ static bool tai_adjust(struct vmclock_abi *clk, uint64_t *sec)
>  	return false;
>  }
>  
> +static uint8_t vmclock_get_ptp_timescale(uint8_t vmclock_time_type)
> +{
> +	switch (vmclock_time_type) {
> +	case VMCLOCK_TIME_UTC:
> +		return PTP_TIMESCALE_UTC;
> +	case VMCLOCK_TIME_TAI:
> +		return PTP_TIMESCALE_TAI;
> +	case VMCLOCK_TIME_MONOTONIC:
> +		return PTP_TIMESCALE_MONOTONIC;
> +	default:
> +		return PTP_TIMESCALE_UNKNOWN;
> +	}
> +}

[ ... ]

> +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?

> +
> +		/* 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.

> +	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

Re: [PATCH v7 net-next 5/7] net: ena: Update PHC admin interface for error bound support

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> index 898ecd96b96a..2d132c4bc590 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> @@ -128,12 +128,14 @@ enum ena_admin_get_stats_scope {
>  	ENA_ADMIN_ETH_TRAFFIC                       = 1,
>  };
>  
> -enum ena_admin_phc_type {
> -	ENA_ADMIN_PHC_TYPE_READLESS                 = 0,
> +enum ena_admin_phc_feature_version {
> +	/* Readless with error_bound */
> +	ENA_ADMIN_PHC_FEATURE_VERSION_0             = 0,
>  };

[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:

> @@ -1224,7 +1226,10 @@ struct ena_admin_phc_resp {
>  	/* PHC timestamp (nsec) */
>  	u64 timestamp;
>  
> -	u8 reserved2[12];
> +	u8 reserved2[8];
> +
> +	/* Timestamp error limit (nsec) */
> +	u32 error_bound;
>  
>  	/* Bit field of enum ena_admin_phc_error_flags */
>  	u32 error_flags;

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.

> @@ -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];

[ ... ]

> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 297fb36ab8c1..40d1fd64bc34 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -1694,10 +1694,11 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)
>  		return ret;
>  	}
>  
> -	/* 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

Re: [PATCH v7 net-next 7/7] net: ena: Implement gettimexattrs64 callback for PTP attributes

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
> index 2bcb5af564e2..725c36fe3f6e 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_phc.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
> @@ -25,6 +25,43 @@ static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int ena_phc_gettimexattrs64(struct ptp_clock_info *clock_info,
> +				   struct timespec64 *ts,
> +				   struct ptp_system_timestamp *sts,
> +				   struct ptp_clock_attrs *att)
> +{
> +	struct ena_phc_info *phc_info =
> +		container_of(clock_info, struct ena_phc_info, clock_info);
> +	u32 error_bound_nsec;
> +	unsigned long flags;
> +	u64 timestamp_nsec;
> +	int rc;
> +
> +	spin_lock_irqsave(&phc_info->lock, flags);
> +
> +	ptp_read_system_prets(sts);
> +
> +	rc = ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,
> +				       &timestamp_nsec,
> +				       &error_bound_nsec);
> +
> +	ptp_read_system_postts(sts);
> +
> +	spin_unlock_irqrestore(&phc_info->lock, flags);
> +
> +	if (rc)
> +		return rc;
> +
> +	*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.

> +	if (att) {
> +		att->error_bound = error_bound_nsec;
> +		att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;
> +	}

[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?

> +
> +	return 0;
> +}
> +
>  static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
>  			      struct timespec64 *ts,
>  			      struct ptp_system_timestamp *sts)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com