Re: [PATCH net v3 3/5] ptp: vclock: propagate physical clock sampling errors

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 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] After one failed PHC sample, the virtual clock's anchor
  (tc->cycle_last) goes stale for a full extra refresh period, so the…
- [Low] ptp_vclock_register() now takes the initial PHC sample and calls
  timecounter_init() (drivers/ptp/ptp_vclock.c:237-240) before…

--

ptp: vclock: propagate physical clock sampling errors

This patch samples the parent PHC before timecounter_read() or
timecounter_init(). The cyclecounter read callback now returns the cached
successful sample, so a failed read no longer feeds a zero into the
timecounter. Sampling errors are returned to the vclock callers, and
ptp_vclock_register() takes the initial sample before publishing the clock
and returns an ERR_PTR that n_vclocks_store() propagates.

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

[ ... ]

> @@ -75,15 +93,16 @@ static int ptp_vclock_gettime(struct ptp_clock_info *ptp,
>  			      struct timespec64 *ts)
>  {
>  	struct ptp_vclock *vclock = info_to_vclock(ptp);
> -	u64 ns;
> +	int err;
>  
>  	if (mutex_lock_interruptible(&vclock->lock))
>  		return -EINTR;
> -	ns = timecounter_read(&vclock->tc);
> +	err = ptp_vclock_sample(vclock);
> +	if (!err)
> +		*ts = ns_to_timespec64(timecounter_read(&vclock->tc));
>  	mutex_unlock(&vclock->lock);
> -	*ts = ns_to_timespec64(ns);
>  
> -	return 0;
> +	return err;
>  }

[Severity: Medium]
The commit message says "The refresh worker can retry later without
consuming an invalid sample". Does the retry come soon enough?

ptp_vclock_refresh() calls this function, ignores its return value, and
always reschedules after the full interval:

drivers/ptp/ptp_vclock.c:ptp_vclock_refresh() {
	...
	ptp_vclock_gettime(&vclock->info, &ts);

	return PTP_VCLOCK_REFRESH_INTERVAL;
}

PTP_VCLOCK_REFRESH_INTERVAL is HZ * 2. After one failed sample here,
tc->cycle_last is about 4s old by the time of the next retry.

The cyclecounter is CYCLECOUNTER_MASK(32) of nanoseconds, which is about
4.295s. timecounter_cyc2time() treats any delta above mask / 2 (about
2.147s) as a timestamp in the past:

include/linux/timecounter.h:timecounter_cyc2time() {
	...
	if (unlikely(delta > cc->mask / 2)) {
		delta = (tc->cycle_last - cycle_tstamp) & cc->mask;
		nsec -= cc_cyc2ns_backwards(cc, delta, frac);
	...
}

Between about 2.147s and 4s after the last good sample, would
ptp_convert_timestamp(), ptp_vclock_gettimex() and
ptp_vclock_getcrosststamp() all return times about 2^32 ns too early?

ptp_convert_timestamp() handles every hardware timestamp on a socket bound
to the vclock. ptp_vclock_gettimex() is the only gettime path when the
parent has getcyclesx64.

If the next refresh also fails, the next successful timecounter_read()
sees a delta above 2^32 ns, and that delta gets masked. Can virtual time
then permanently lose a full 2^32 ns wrap?

For that to happen, the parent only has to be unavailable at two
consecutive refresh points about 2s apart. That is shorter than "long
enough to miss a counter wrap", as the commit message describes it.

Could ptp_vclock_refresh() return a short delay after a failed sample?
ptp_aux_kworker() requeues the work with any non-negative delay that
do_aux_work returns.

[ ... ]

> @@ -214,20 +234,28 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
>  
>  	mutex_init(&vclock->lock);
>  
> +	err = ptp_vclock_sample(vclock);
> +	if (err)
> +		goto free_vclock;
> +	timecounter_init(&vclock->tc, &vclock->cc, 0);
> +
>  	vclock->clock = ptp_clock_register(&vclock->info, &pclock->dev);
> -	if (IS_ERR_OR_NULL(vclock->clock)) {
> -		kfree(vclock);
> -		return NULL;
> +	if (IS_ERR(vclock->clock)) {
> +		err = PTR_ERR(vclock->clock);
> +		goto free_vclock;
>  	}
>  
>  	ptp_vclock_set_subclass(vclock->clock);
>  
> -	timecounter_init(&vclock->tc, &vclock->cc, 0);
>  	ptp_schedule_worker(vclock->clock, PTP_VCLOCK_REFRESH_INTERVAL);

[Severity: Low]
The initial sample and timecounter_init() now run before
ptp_clock_register(), but the first refresh is still queued only after
registration returns. Does this make the gap between tc->cycle_last and
the first refresh longer by however long ptp_clock_register() takes?

ptp_clock_register() can sleep in GFP_KERNEL allocations, in
kthread_run_worker() and in posix_clock_register().

Suppose that time plus kworker latency is more than about 147ms. Then
conversions through ptp_convert_timestamp() or ptp_vclock_gettimex() just
before the first refresh would fall past the 2.147s half-window and come
out about 2^32 ns early. If it is more than about 2.29s, the first
timecounter_read() would lose a full wrap.

The reorder does fix the old race, where the posix clock was exposed
before timecounter_init() while tc.cc was still NULL. Either of these
would keep that fix without the longer gap:

  - re-sample and re-initialise the timecounter under vclock->lock once
    ptp_clock_register() succeeds
  - queue the first refresh with a short or zero delay

>  
>  	ptp_vclock_hash_add(vclock);
>  
>  	return vclock;
> +
> +free_vclock:
> +	kfree(vclock);
> +	return ERR_PTR(err);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-ptp-vclock-sampling-v3-0-d3ee4ea62fc6%40gmail.com