Re: [PATCH net-next 3/5] ptp: add functions to adjust system timestamps

[netdev-bot+sashiko]

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

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

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] ptp: direct addition of unscaled raw ns to slewed systime

--

> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -520,4 +520,52 @@ static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
>  		ktime_get_snapshot_id(sts->clockid, &sts->post_sts);
>  }
>  
> +static inline void __ptp_adjust_snapshot(struct system_time_snapshot *snap,
> +					 s64 ns)
> +{
> +	if (!snap->valid)
> +		return;
> +	snap->systime = ktime_add_ns(snap->systime, ns);
> +	snap->monoraw = ktime_add_ns(snap->monoraw, ns);
> +	/* No counter value corresponds to the adjusted times. */
> +	snap->cycles = 0;
> +	snap->cs_id = CSID_GENERIC;
> +	snap->hw_cycles = 0;
> +	snap->hw_csid = CSID_GENERIC;
> +}
> +
> +/**
> + * ptp_adjust_system_prets - Shift the lower system timestamp bound
> + * @sts: system timestamps, or NULL
> + * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
> + *
> + * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
> + * time of the lower bound, and clear its clocksource counter values.
> + * @ns is not scaled for the selected clock's frequency correction, so
> + * the bound is off by @ns times that correction.
> + */
> +static inline void ptp_adjust_system_prets(struct ptp_system_timestamp *sts,
> +					   s64 ns)
> +{
> +	if (sts)
> +		__ptp_adjust_snapshot(&sts->pre_sts, ns);
> +}
> +
> +/**
> + * ptp_adjust_system_postts - Shift the upper system timestamp bound
> + * @sts: system timestamps, or NULL
> + * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
> + *
> + * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
> + * time of the upper bound, and clear its clocksource counter values.
> + * @ns is not scaled for the selected clock's frequency correction, so
> + * the bound is off by @ns times that correction.
> + */
> +static inline void ptp_adjust_system_postts(struct ptp_system_timestamp *sts,
> +					    s64 ns)
> +{
> +	if (sts)
> +		__ptp_adjust_snapshot(&sts->post_sts, ns);
> +}
> +
>  #endif

[Severity: Medium]
__ptp_adjust_snapshot() adds ns straight onto snap->systime, and the
kerneldoc defines ns as a CLOCK_MONOTONIC_RAW duration. When
sts->clockid is CLOCK_MONOTONIC_RAW, systime is a copy of monoraw and
this is exact. For CLOCK_REALTIME, CLOCK_MONOTONIC, CLOCK_BOOTTIME or
an aux clock it is not: systime is in the selected clock's timescale,
which runs at the NTP/adjtimex corrected rate rather than the raw
rate. After the shift, systime and monoraw no longer describe the same
instant, and systime is off by ns times the current frequency
correction.

You already note this in the kerneldoc and below the cut line. For the
30 us MDIO write in this series the error stays at a few ns, even
during a large slew. My concern is the API scope.
ptp_adjust_system_prets() and ptp_adjust_system_postts() go into a
public header and accept any s64 ns. A later caller compensating for a
longer delay, such as a slow bus or a firmware round trip, would
silently move the bound by the wrong amount. The error is largest
exactly while the servo is slewing.

That matters most when pre_sts is moved forward. If it moves too far,
it can end up after the event it is supposed to precede. Userspace then
gets a pre/post window from PTP_SYS_OFFSET_EXTENDED that does not
actually bound the PHC read.

Could the ktime_scale_raw_ns() style helper you sketched be added to
timekeeping as part of this series, so the shift is done in the right
timescale? If that is out of scope, could the helpers document, or
enforce, a small upper bound on ns? Alternatively, should this stay
local to the MDIO driver until timekeeping can do the conversion?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com