[PATCH net v2 0/2] ptp: make virtual clock sampling and teardown failure-safe

[PATCH net v2 0/2] ptp: make virtual clock sampling and teardown failure-safe

[James Hilliard]

Keep virtual clock state intact when a physical clock cannot be sampled,
and retain elapsed time when errors postpone the next successful sample.
Drain virtual clock sysfs changes before parent teardown so failed
creation requests can safely unwind their newly registered children.

Assisted-by: Codex:gpt-6-astra

---
Changes in v2:
- Drain virtual clock sysfs changes before parent unregister walks children.
- Use full-width samples and overflow-safe scaling across long sampling gaps.
- Update the anchor for extended and cross-timestamp reads under the clock lock.
- Link to v1: https://patch.msgid.link/20260930-ptp-vclock-sampling-v1-1-c11bc16691ae@gmail.com

To: Richard Cochran <richardcochran@gmail.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Yangbo Lu <yangbo.lu@nxp.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
James Hilliard (2):
      ptp: drain virtual clock sysfs operations before unregistering children
      ptp: vclock: preserve time across failed physical clock samples

 drivers/ptp/ptp_clock.c   |   5 ++
 drivers/ptp/ptp_private.h |  10 ++--
 drivers/ptp/ptp_sysfs.c   |  14 ++++-
 drivers/ptp/ptp_vclock.c  | 148 ++++++++++++++++++++++++++++++----------------
 4 files changed, 121 insertions(+), 56 deletions(-)
---
base-commit: aaaaf87ea99b8766c9a8aa0e71aa42e6bc8a5320
change-id: 20260930-ptp-vclock-sampling-902f98408c93

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

[PATCH net v2 1/2] ptp: drain virtual clock sysfs operations before unregistering children

[James Hilliard]

Parent clock removal walks its virtual clocks before removing the
n_vclocks sysfs attribute. The mutex taken by ptp_vclock_in_use() is
released before that walk, so a concurrent sysfs deletion can pick the
same child and unregister and free its ptp_vclock a second time. A
reference held by the device iterator does not protect that separately
allocated virtual clock.

Remove n_vclocks before walking the children. Removing the attribute
prevents new stores and drains stores already running, without holding
n_vclocks_mux across a callback that needs that mutex. Virtual clocks
have no such attribute and must not remove the parent attribute when
being deleted by its active store.

This race was found by code inspection of virtual-clock registration
failure cleanup and parent removal.

Fixes: 5d43f951b1ac ("ptp: add ptp virtual clock driver framework")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/ptp/ptp_clock.c   | 5 +++++
 drivers/ptp/ptp_private.h | 1 +
 drivers/ptp/ptp_sysfs.c   | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 4111342d64f0..47ffc773065a 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -508,6 +508,11 @@ static int unregister_vclock(struct device *dev, void *data)
 
 int ptp_clock_unregister(struct ptp_clock *ptp)
 {
+	/*
+	 * Stop and drain virtual-clock creation and deletion before walking the
+	 * children. Do not hold n_vclocks_mux while waiting for sysfs callbacks.
+	 */
+	ptp_vclock_remove_sysfs(ptp);
 	if (ptp_vclock_in_use(ptp)) {
 		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index db4039d642b4..ec8633126d6b 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -169,6 +169,7 @@ extern const struct attribute_group *ptp_groups[];
 
 int ptp_populate_pin_groups(struct ptp_clock *ptp);
 void ptp_cleanup_pin_groups(struct ptp_clock *ptp);
+void ptp_vclock_remove_sysfs(struct ptp_clock *ptp);
 
 struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock);
 void ptp_vclock_unregister(struct ptp_vclock *vclock);
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index dc398c6b7528..53388b123198 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -263,6 +263,12 @@ static ssize_t n_vclocks_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(n_vclocks);
 
+void ptp_vclock_remove_sysfs(struct ptp_clock *ptp)
+{
+	if (!ptp->is_virtual_clock)
+		device_remove_file(&ptp->dev, &dev_attr_n_vclocks);
+}
+
 static ssize_t max_vclocks_show(struct device *dev,
 				struct device_attribute *attr, char *page)
 {

-- 
2.53.0

[PATCH net v2 2/2] ptp: vclock: preserve time across failed physical clock samples

[James Hilliard]

The cyclecounter read callback cannot report errors. Passing an
unsuccessful PHC read through it consumes an invalid sample, and a zero
sample followed by the real counter can add an extra 32-bit wrap to
virtual time. This was identified by code inspection of reset-time PHC
access and the virtual clock callers.

Read the parent clock before updating or initializing the virtual
counter, propagate failures from clock operations, and leave its state
unchanged on failure. Initialize it before publishing a new virtual
clock.

Simply skipping failed reads is insufficient: the next successful read
can arrive after the 32-bit counter has wrapped, while timestamp
conversion has an even shorter half-wrap window. Use the full physical
nanosecond sample with a wide frequency-adjustment multiply and retain
fractional nanoseconds. This also allows delayed packet timestamps to be
converted without truncating their distance from the last sample.
Serialize extended and cross-timestamp sampling with adjustments and
advance the anchor on each successful clock read.

The initial sample can also fail partway through a sysfs request to create
multiple virtual clocks. Unregister any clocks created by that request
and clear their index entries, preserving the previously installed clocks
and count. Otherwise the failed request leaves registered children that
are not included in n_vclocks. This cleanup also handles existing
allocation and registration failure paths.

Fixes: 5d43f951b1ac ("ptp: add ptp virtual clock driver framework")
Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/ptp/ptp_private.h |   9 +--
 drivers/ptp/ptp_sysfs.c   |   8 ++-
 drivers/ptp/ptp_vclock.c  | 148 ++++++++++++++++++++++++++++++----------------
 3 files changed, 109 insertions(+), 56 deletions(-)

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index ec8633126d6b..e08272335da0 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -71,17 +71,18 @@ struct ptp_clock {
 };
 
 #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
-#define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc)
 #define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work)
 
 struct ptp_vclock {
+	u64 cycles;
+	u64 nsec;
+	u64 frac;
+	u32 mult;
 	struct ptp_clock *pclock;
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
 	struct hlist_node vclock_hash_node;
-	struct cyclecounter cc;
-	struct timecounter tc;
-	struct mutex lock;	/* protects tc/cc */
+	struct mutex lock;	/* protects the virtual counter */
 };
 
 /*
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 53388b123198..fabc1c9b1752 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -225,7 +225,7 @@ static ssize_t n_vclocks_store(struct device *dev,
 		for (i = 0; i < num - ptp->n_vclocks; i++) {
 			vclock = ptp_vclock_register(ptp);
 			if (!vclock)
-				goto out;
+				goto err_register;
 
 			*(ptp->vclock_index + ptp->n_vclocks + i) =
 				vclock->clock->index;
@@ -257,6 +257,12 @@ static ssize_t n_vclocks_store(struct device *dev,
 	mutex_unlock(&ptp->n_vclocks_mux);
 
 	return count;
+err_register:
+	num = i;
+	if (num)
+		device_for_each_child_reverse(dev, &num, unregister_vclock);
+	for (num = 0; num < i; num++)
+		ptp->vclock_index[ptp->n_vclocks + num] = -1;
 out:
 	mutex_unlock(&ptp->n_vclocks_mux);
 	return err;
diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 84cb527f59cc..52201c6b4d1f 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -6,10 +6,12 @@
  */
 #include <linux/slab.h>
 #include <linux/hashtable.h>
+#include <linux/math64.h>
 #include "ptp_private.h"
 
 #define PTP_VCLOCK_CC_SHIFT		31
 #define PTP_VCLOCK_CC_MULT		(1 << PTP_VCLOCK_CC_SHIFT)
+#define PTP_VCLOCK_FRAC_MASK		((1ULL << PTP_VCLOCK_CC_SHIFT) - 1)
 #define PTP_VCLOCK_FADJ_SHIFT		9
 #define PTP_VCLOCK_FADJ_DENOMINATOR	15625ULL
 #define PTP_VCLOCK_REFRESH_INTERVAL	(HZ * 2)
@@ -42,21 +44,74 @@ static void ptp_vclock_hash_del(struct ptp_vclock *vclock)
 	synchronize_srcu(&vclock_srcu);
 }
 
+/*
+ * Physical clock samples already contain full-width nanoseconds. Do not
+ * truncate them to a 32-bit counter: failed reads can postpone a refresh
+ * beyond its wrap period. Use a wide multiply and retain fractional ns.
+ * Packet timestamps may precede the last sample; conversion must not move
+ * the clock's anchor in that case. The caller holds vclock->lock.
+ */
+static u64 ptp_vclock_convert(struct ptp_vclock *vclock, u64 cycles, u64 *frac)
+{
+	u64 delta = cycles - vclock->cycles;
+	bool backwards = delta > S64_MAX;
+	u64 nsec, rem;
+
+	if (backwards)
+		delta = -delta;
+	nsec = mul_u64_u32_shr(delta, vclock->mult, PTP_VCLOCK_CC_SHIFT);
+	rem = (delta * vclock->mult) & PTP_VCLOCK_FRAC_MASK;
+	if (backwards) {
+		nsec = vclock->nsec - nsec - (rem > *frac);
+		*frac = (*frac - rem) & PTP_VCLOCK_FRAC_MASK;
+	} else {
+		rem += *frac;
+		nsec += vclock->nsec + (rem >> PTP_VCLOCK_CC_SHIFT);
+		*frac = rem & PTP_VCLOCK_FRAC_MASK;
+	}
+
+	return nsec;
+}
+
+static void ptp_vclock_update(struct ptp_vclock *vclock, u64 cycles)
+{
+	vclock->nsec = ptp_vclock_convert(vclock, cycles, &vclock->frac);
+	vclock->cycles = cycles;
+}
+
+/* The caller holds vclock->lock, or has not published the clock yet. */
+static int ptp_vclock_sample(struct ptp_vclock *vclock, u64 *cycles)
+{
+	struct ptp_clock *ptp = vclock->pclock;
+	struct timespec64 ts;
+	int err;
+
+	err = ptp->info->getcycles64(ptp->info, &ts);
+	if (!err)
+		*cycles = timespec64_to_ns(&ts);
+	return err;
+}
+
 static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
+	u64 cycles;
 	s64 adj;
+	int err;
 
 	adj = (s64)scaled_ppm << PTP_VCLOCK_FADJ_SHIFT;
 	adj = div_s64(adj, PTP_VCLOCK_FADJ_DENOMINATOR);
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	timecounter_read(&vclock->tc);
-	vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj;
+	err = ptp_vclock_sample(vclock, &cycles);
+	if (!err) {
+		ptp_vclock_update(vclock, cycles);
+		vclock->mult = PTP_VCLOCK_CC_MULT + adj;
+	}
 	mutex_unlock(&vclock->lock);
 
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_adjtime(struct ptp_clock_info *ptp, s64 delta)
@@ -65,7 +120,7 @@ static int ptp_vclock_adjtime(struct ptp_clock_info *ptp, s64 delta)
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	timecounter_adjtime(&vclock->tc, delta);
+	vclock->nsec += delta;
 	mutex_unlock(&vclock->lock);
 
 	return 0;
@@ -75,15 +130,19 @@ static int ptp_vclock_gettime(struct ptp_clock_info *ptp,
 			      struct timespec64 *ts)
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
-	u64 ns;
+	u64 cycles;
+	int err;
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	ns = timecounter_read(&vclock->tc);
+	err = ptp_vclock_sample(vclock, &cycles);
+	if (!err) {
+		ptp_vclock_update(vclock, cycles);
+		*ts = ns_to_timespec64(vclock->nsec);
+	}
 	mutex_unlock(&vclock->lock);
-	*ts = ns_to_timespec64(ns);
 
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
@@ -94,34 +153,37 @@ static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
 	struct ptp_clock *pptp = vclock->pclock;
 	struct timespec64 pts;
 	int err;
-	u64 ns;
-
-	err = pptp->info->getcyclesx64(pptp->info, &pts, sts);
-	if (err)
-		return err;
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	ns = timecounter_cyc2time(&vclock->tc, timespec64_to_ns(&pts));
+	err = pptp->info->getcyclesx64(pptp->info, &pts, sts);
+	if (!err) {
+		ptp_vclock_update(vclock, timespec64_to_ns(&pts));
+		*ts = ns_to_timespec64(vclock->nsec);
+	}
 	mutex_unlock(&vclock->lock);
 
-	*ts = ns_to_timespec64(ns);
-
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_settime(struct ptp_clock_info *ptp,
 			      const struct timespec64 *ts)
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
-	u64 ns = timespec64_to_ns(ts);
+	u64 cycles;
+	int err;
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	timecounter_init(&vclock->tc, &vclock->cc, ns);
+	err = ptp_vclock_sample(vclock, &cycles);
+	if (!err) {
+		vclock->cycles = cycles;
+		vclock->nsec = timespec64_to_ns(ts);
+		vclock->frac = 0;
+	}
 	mutex_unlock(&vclock->lock);
 
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
@@ -130,20 +192,17 @@ static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
 	struct ptp_clock *pptp = vclock->pclock;
 	int err;
-	u64 ns;
-
-	err = pptp->info->getcrosscycles(pptp->info, xtstamp);
-	if (err)
-		return err;
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	ns = timecounter_cyc2time(&vclock->tc, ktime_to_ns(xtstamp->device));
+	err = pptp->info->getcrosscycles(pptp->info, xtstamp);
+	if (!err) {
+		ptp_vclock_update(vclock, ktime_to_ns(xtstamp->device));
+		xtstamp->device = ns_to_ktime(vclock->nsec);
+	}
 	mutex_unlock(&vclock->lock);
 
-	xtstamp->device = ns_to_ktime(ns);
-
-	return 0;
+	return err;
 }
 
 static long ptp_vclock_refresh(struct ptp_clock_info *ptp)
@@ -171,24 +230,6 @@ static const struct ptp_clock_info ptp_vclock_info = {
 	.do_aux_work	= ptp_vclock_refresh,
 };
 
-static u64 ptp_vclock_read(struct cyclecounter *cc)
-{
-	struct ptp_vclock *vclock = cc_to_vclock(cc);
-	struct ptp_clock *ptp = vclock->pclock;
-	struct timespec64 ts = {};
-
-	ptp->info->getcycles64(ptp->info, &ts);
-
-	return timespec64_to_ns(&ts);
-}
-
-static const struct cyclecounter ptp_vclock_cc = {
-	.read	= ptp_vclock_read,
-	.mask	= CYCLECOUNTER_MASK(32),
-	.mult	= PTP_VCLOCK_CC_MULT,
-	.shift	= PTP_VCLOCK_CC_SHIFT,
-};
-
 struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 {
 	struct ptp_vclock *vclock;
@@ -205,7 +246,7 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 		vclock->info.gettime64 = ptp_vclock_gettime;
 	if (pclock->info->getcrosscycles)
 		vclock->info.getcrosststamp = ptp_vclock_getcrosststamp;
-	vclock->cc = ptp_vclock_cc;
+	vclock->mult = PTP_VCLOCK_CC_MULT;
 
 	snprintf(vclock->info.name, PTP_CLOCK_NAME_LEN, "ptp%d_virt",
 		 pclock->index);
@@ -214,6 +255,11 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	mutex_init(&vclock->lock);
 
+	if (ptp_vclock_sample(vclock, &vclock->cycles)) {
+		kfree(vclock);
+		return NULL;
+	}
+
 	vclock->clock = ptp_clock_register(&vclock->info, &pclock->dev);
 	if (IS_ERR_OR_NULL(vclock->clock)) {
 		kfree(vclock);
@@ -222,7 +268,6 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	ptp_vclock_set_subclass(vclock->clock);
 
-	timecounter_init(&vclock->tc, &vclock->cc, 0);
 	ptp_schedule_worker(vclock->clock, PTP_VCLOCK_REFRESH_INTERVAL);
 
 	ptp_vclock_hash_add(vclock);
@@ -280,7 +325,7 @@ ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index)
 	struct ptp_vclock *vclock;
 	u64 vclock_ns = 0;
 	int srcu_idx;
-	u64 ns;
+	u64 ns, frac;
 
 	ns = ktime_to_ns(*hwtstamp);
 
@@ -293,7 +338,8 @@ ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index)
 
 		if (mutex_lock_interruptible(&vclock->lock))
 			break;
-		vclock_ns = timecounter_cyc2time(&vclock->tc, ns);
+		frac = vclock->frac;
+		vclock_ns = ptp_vclock_convert(vclock, ns, &frac);
 		mutex_unlock(&vclock->lock);
 		break;
 	}

-- 
2.53.0

Re: [PATCH net v2 1/2] ptp: drain virtual clock sysfs operations before unregistering children

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gives the wrong reason for the
  is_virtual_clock guard in ptp_vclock_remove_sysfs()…

--

ptp: drain virtual clock sysfs operations before unregistering children

This commit removes the n_vclocks sysfs attribute at the start of
ptp_clock_unregister(), before the virtual clock children are walked. Doing
so blocks new n_vclocks stores and drains any that are already running, so a
concurrent store can no longer unregister the same child a second time.
Virtual clocks skip the removal through an is_virtual_clock check.

> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index dc398c6b75284..53388b1231980 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -263,6 +263,12 @@ static ssize_t n_vclocks_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(n_vclocks);
>  
> +void ptp_vclock_remove_sysfs(struct ptp_clock *ptp)
> +{
> +	if (!ptp->is_virtual_clock)
> +		device_remove_file(&ptp->dev, &dev_attr_n_vclocks);
> +}

[Severity: Low]
This isn't a bug, but the commit message gives what looks like the wrong
reason for this guard:

  Virtual clocks have no such attribute and must not remove the parent
  attribute when being deleted by its active store.

When a vclock is deleted from the parent's active store, the path is:

n_vclocks_store()->unregister_vclock()->ptp_vclock_unregister()->
  ptp_clock_unregister(vclock->clock)->ptp_vclock_remove_sysfs()

Here ptp->dev is the virtual clock's own device, not the parent's.
device_remove_file() only acts on that device's directory. It cannot reach
the parent's n_vclocks attribute.

ptp_is_attribute_visible() also never creates the attribute for virtual
clocks:

	} else if (attr == &dev_attr_n_vclocks.attr ||
		   attr == &dev_attr_max_vclocks.attr) {
		if (ptp->is_virtual_clock)
			mode = 0;

Without the guard, device_remove_file() would just find nothing and
kernfs_remove_by_name_ns() would return -ENOENT. There is no self-drain of
the parent's attribute for the guard to prevent.

Could the commit message say instead that the guard only skips a no-op
removal on virtual clocks, which never have n_vclocks? As written, it
reads as though the guard is what stops the parent's active store from
draining itself.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-ptp-vclock-sampling-v2-0-8ed12d4d10af%40gmail.com

Re: [PATCH net v2 2/2] ptp: vclock: preserve time across failed physical clock samples

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new comment above ptp_vclock_convert() says "Physical
  clock samples already contain full-width nanoseconds", and the new…
- [Low] The patch adds a new way for vclock creation to fail:
  ptp_vclock_sample() calls getcycles64() before registering the clock,…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but:
  ptp_convert_timestamp() never treats a zero (absent) hardware…

--

ptp: vclock: preserve time across failed physical clock samples

This patch drops the cyclecounter/timecounter from the PTP virtual clock.
It now samples the parent PHC directly, returns read errors to the caller,
and tracks time with a full 64-bit nanosecond anchor plus fractional ns.
It also changes n_vclocks_store() to unregister clocks that were only
partly created when a request to create several vclocks fails.

> diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
> index 84cb527f59ccc..52201c6b4d1f7 100644
> --- a/drivers/ptp/ptp_vclock.c
> +++ b/drivers/ptp/ptp_vclock.c

[ ... ]

> @@ -42,21 +44,74 @@ static void ptp_vclock_hash_del(struct ptp_vclock *vclock)
>  	synchronize_srcu(&vclock_srcu);
>  }
>  
> +/*
> + * Physical clock samples already contain full-width nanoseconds. Do not
> + * truncate them to a 32-bit counter: failed reads can postpone a refresh
> + * beyond its wrap period. Use a wide multiply and retain fractional ns.
> + * Packet timestamps may precede the last sample; conversion must not move
> + * the clock's anchor in that case. The caller holds vclock->lock.
> + */
> +static u64 ptp_vclock_convert(struct ptp_vclock *vclock, u64 cycles, u64 *frac)
> +{
> +	u64 delta = cycles - vclock->cycles;
> +	bool backwards = delta > S64_MAX;

[Severity: Medium]
Is the "full-width nanoseconds" statement true for every getcycles64 or
getcyclesx64 provider? The kernel-doc in include/linux/ptp_clock_kernel.h
only says @getcycles64 "Reads the current free running cycle counter from
the hardware clock". It gives no unit or width.

For example, mlx5_ptp_getcyclesx() returns raw internal timer ticks:

drivers/net/ethernet/mellanox/mlx5/core/lib/clock.c:mlx5_ptp_getcyclesx() {
    ...
	cycles = mlx5_read_time(mdev, sts, false);
	*ts = ns_to_timespec64(cycles);
    ...
}

The same driver treats those ticks as frequency-scaled and masks them to
41 bits in mlx5_timecounter_init():

	timer->cycles.mult = clocksource_khz2mult(dev_freq,
						  timer->cycles.shift);
	timer->nominal_c_mult = timer->cycles.mult;
	timer->cycles.mask = CLOCKSOURCE_MASK(41);

Scaling raw mlx5 ticks 1:1 as ns is older than this patch. What this patch
changes is the wrap handling. The old CYCLECOUNTER_MASK(32) arithmetic
worked with any provider counter that was at least 32 bits wide. The new
code takes a full 64-bit delta and treats delta > S64_MAX as a backwards
step.

If a provider's counter wraps below 2^63, would ptp_vclock_convert() see
the wrap as a backward step and move the virtual clock back by the whole
wrap period? It isn't clear from the code whether the mlx5 hardware counter
really wraps at 41 bits. Even so, should the comment and the callback
contract be updated, or should this kind of wrap be handled?

> +	u64 nsec, rem;
> +
> +	if (backwards)
> +		delta = -delta;
> +	nsec = mul_u64_u32_shr(delta, vclock->mult, PTP_VCLOCK_CC_SHIFT);
> +	rem = (delta * vclock->mult) & PTP_VCLOCK_FRAC_MASK;
> +	if (backwards) {
> +		nsec = vclock->nsec - nsec - (rem > *frac);
> +		*frac = (*frac - rem) & PTP_VCLOCK_FRAC_MASK;
> +	} else {

[ ... ]

> @@ -214,6 +255,11 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
>  
>  	mutex_init(&vclock->lock);
>  
> +	if (ptp_vclock_sample(vclock, &vclock->cycles)) {
> +		kfree(vclock);
> +		return NULL;
> +	}
> +

[Severity: Low]
When getcycles64() fails here, the driver's error code is lost.
ptp_vclock_register() returns NULL, and n_vclocks_store() jumps to
err_register without touching err:

drivers/ptp/ptp_sysfs.c:n_vclocks_store() {
	int err = -EINVAL;
    ...
			vclock = ptp_vclock_register(ptp);
			if (!vclock)
				goto err_register;
    ...
}

So a transient -EIO, -EBUSY or -EAGAIN from the parent PHC, for example
during a NIC reset, reaches userspace as -EINVAL. That looks like an
invalid value was written to n_vclocks.

The commit message says failures from clock operations are now propagated.
Could this path pass the real error back as well, for example by returning
an ERR_PTR from ptp_vclock_register()? The ptp_clock_register() and
allocation failure paths already flattened their errors to -EINVAL before
this patch. The rollback in err_register looks fine; only the errno is
affected.

>  	vclock->clock = ptp_clock_register(&vclock->info, &pclock->dev);
>  	if (IS_ERR_OR_NULL(vclock->clock)) {
>  		kfree(vclock);

[ ... ]

> @@ -293,7 +338,8 @@ ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index)
>  
>  		if (mutex_lock_interruptible(&vclock->lock))
>  			break;
> -		vclock_ns = timecounter_cyc2time(&vclock->tc, ns);
> +		frac = vclock->frac;
> +		vclock_ns = ptp_vclock_convert(vclock, ns, &frac);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
ptp_convert_timestamp() never treats a zero hardware timestamp as "no
timestamp".

__sock_recv_timestamp() converts the value before it checks for zero:

net/socket.c:__sock_recv_timestamp() {
    ...
		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
			hwtstamp = ptp_convert_timestamp(&hwtstamp,
							 READ_ONCE(sk->sk_bind_phc));

		if (hwtstamp) {
			tss.ts[2] = hwtstamp;
    ...
}

Take a socket with SOF_TIMESTAMPING_BIND_PHC that also sets
SOF_TIMESTAMPING_RX_SOFTWARE or SOCK_RCVTSTAMP. It reaches this code even
for packets that have no hardware timestamp.

With ns == 0, delta = 0 - vclock->cycles is above S64_MAX, so the backwards
branch runs. ptp_vclock_convert() then returns vclock->nsec minus the
scaled anchor, which is non-zero. Is that value reported to userspace in
tss.ts[2] as a valid hardware timestamp?

The old timecounter_cyc2time() also returned a non-zero value here, within
about two seconds of the vclock time. This patch only changes how far off
the value is. Would it make sense to return 0 when *hwtstamp is 0?

>  		mutex_unlock(&vclock->lock);
>  		break;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-ptp-vclock-sampling-v2-0-8ed12d4d10af%40gmail.com