Re: [PATCH net 12/15] ice: remove unnecessary discarding of timestamps after clock adjust

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: remove unnecessary discarding of timestamps after clock adjust

This drops the ice_ptp_mark_tx_tracker_stale() call from
ice_ptp_reset_cached_phctime(), so outstanding Tx timestamps are no longer
discarded when the PHC is adjusted or set.  The stale bit itself is kept and
is now only used by the link-down path.  The kernel-doc of
ice_ptp_mark_tx_tracker_stale() is updated to describe the new meaning.

A couple of questions below.

quoted

    The original motivation for discarding timestamps was that extending an old
    timestamp using the new cached value of PHC was a problem, as it could
    produce incorrect results. The change did not describe what such "incorrect
    results" were.

    There are no such incorrect results. Extending the 32 bit timestamp with
    the new time value just means that the timestamp is reported in terms of
    the newly updated and adjusted system clock.

[Severity: High]
Does this hold for the .settime path too, or only for the atomic .adjtime
path?

ice_ptp_adjtime() only uses the atomic ice_ptp_write_adj() path for
|delta| <= S32_MAX:

	if (delta > S32_MAX || delta < S32_MIN) {
		dev_dbg(dev, "delta = %lld, adjtime non-atomic\n", delta);

Larger deltas, and every clock_settime() on /dev/ptpN, take
ice_ptp_adjtime_nonatomic() -> ice_ptp_settime64() -> ice_ptp_write_init()
-> ice_ptp_init_time(), which replaces the 64-bit source timer with an
arbitrary new value rather than shifting it by a bounded amount.

The PHY Tx timestamp memory keeps only the captured low 32 bits of nominal
nanoseconds, and ice_ptp_settime64() only restarts the PHY blocks on E82X:

	/* Recalibrate and re-enable timestamp blocks for E822/E823 */
	if (hw->mac_type == ICE_MAC_GENERIC)
		ice_ptp_restart_all_phy(pf);

So on ICE_MAC_E810 / ICE_MAC_E830 a capture taken just before the step is
still in the PHY bank with its ready bit set when
ice_ptp_process_tx_tstamp() runs after the cache was replaced.  With the
stale bit no longer set, that slot passes the drop checks and
ice_ptp_extend_40b_ts() extends the pre-step low 32 bits with the post-step
READ_ONCE(pf->ptp.cached_phc_time).

Isn't the value produced by ice_ptp_extend_32b_ts() then the alias of the
real event time nearest the new clock, i.e. off by (step mod 2^32), which is
up to about +/- 2.147 s and can even be dated after the new PHC value?  The
same function documents that this is outside its stated assumption:

 * 2) assume that the in_tstamp was captured 2^31 nanoseconds (~2.1
 *    seconds) before or after the PHC time was captured.

Separately, is the reverse branch safe when the clock is set to a small
absolute value (for example phc_ctl set 0)?

	if (delta > (U32_MAX / 2)) {
		/* reverse the delta calculation here */
		delta = (phc_time_lo - in_tstamp);
		ns = cached_phc_time - delta;

With cached_phc_time smaller than delta this u64 subtraction wraps, and
ns_to_ktime() then reports a negative hardware timestamp.  Previously the
stale bit hid this case; now it is reachable.

In both cases the result is counted in pf->ptp.tx_hwtstamp_good and handed
to skb_tstamp_tx() rather than dropped.  Would it make sense to keep
discarding outstanding captures for the settime and large-adjtime paths, or
alternatively to restrict the claim in the commit message to the atomic
adjustment case and note the trade-off (a wrong timestamp delivered instead
of no timestamp)?

Ugh. Yea, I think that analysis is right. We still need this for larger
adjustments and I think since we haven't had too many reports of missed
timestamps that it makes more sense to just drop this change from the
series.