Re: [PATCH] ptp: ocp: Fix PCI delay estimation

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 17/08/2025 23:29, Antoine Gagniere wrote:
> Since linux 6.12, a sign error causes the initial value of ts_window_adjust, (used in gettimex64) to be impossibly high, causing consumers like chrony to reject readings from PTP_SYS_OFFSET_EXTENDED.
> 
> This patch fixes ts_window_adjust's inital value and the sign-ness of various format flags
> 
> Context
> -------
> 
> The value stored in the read-write attribute ts_window_adjust is a number of nanoseconds subtracted to the post_ts timestamp of the reading in gettimex64, used notably in the ioctl PTP_SYS_OFFSET_EXTENDED, to compensate for PCI delay.
> Its initial value is set by estimating PCI delay.
> 
> Bug
> ---
> 
> The PCI delay estimation starts with the value U64_MAX and makes 3 measurements, taking the minimum value.
> However because the delay was stored in a s64, U64_MAX was interpreted as -1, which compared as smaller than any positive values measured.
> Then, that delay is divided by ~10 and placed in ts_window_adjust, which is a u32.
> So ts_window_adjust ends up with (u32)(((s64)U64_MAX >> 5) * 3) inside, which is 4294967293
> 
> Symptom
> -------
> 
> The consequence was that the post_ts of gettimex64, returned by PTP_SYS_OFFSET_EXTENDED, was substracted 4.29 seconds.
> As a consequence chrony rejected all readings from the PHC
> 
> Difficulty to diagnose
> ----------------------
> 
> Using cat to read the attribute value showed -3 because the format flags %d was used instead of %u, resulting in a re-interpret cast.
> 
> Fixes
> -----
> 
> 1. Using U32_MAX as initial value for PCI delays: no one is expecting an ioread to take more than 4 s
>     This will correctly compare as bigger that actual PCI delay measurements.
> 2. Fixing the sign of various format flags
> 
> Signed-off-by: Antoine Gagniere <antoine@gagniere.dev>
> ---
>   drivers/ptp/ptp_ocp.c | 32 +++++++++++++++++---------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index b651087f426f..153827722a63 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -1558,7 +1558,8 @@ ptp_ocp_watchdog(struct timer_list *t)
>   static void
>   ptp_ocp_estimate_pci_timing(struct ptp_ocp *bp)
>   {
> -	ktime_t start, end, delay = U64_MAX;
> +	ktime_t start, end;
> +	s64 delay_ns = U32_MAX; /* 4.29 seconds is high enough */
>   	u32 ctrl;
>   	int i;
>   
> @@ -1568,15 +1569,16 @@ ptp_ocp_estimate_pci_timing(struct ptp_ocp *bp)
>   
>   		iowrite32(ctrl, &bp->reg->ctrl);
>   
> -		start = ktime_get_raw_ns();
> +		start = ktime_get_raw();
>   
>   		ctrl = ioread32(&bp->reg->ctrl);
>   
> -		end = ktime_get_raw_ns();
> +		end = ktime_get_raw();
>   
> -		delay = min(delay, end - start);
> +		delay_ns = min(delay_ns, ktime_to_ns(end - start));
>   	}
> -	bp->ts_window_adjust = (delay >> 5) * 3;
> +	delay_ns = max(0, delay_ns);

I don't believe we can get a negative value from
ktime_to_ns(end - start), and that means that delay_ns will always be
positive and there is no need for the last max().

Apart from that the patch LGTM,
Thanks

> +	bp->ts_window_adjust = (delay_ns >> 5) * 3;
>   }
>