Re: [PATCH net-next 3/5] ptp: add functions to adjust system timestamps
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-10 15:10:58
Also in:
lkml
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 --
quoted hunk ↗ jump to hunk
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