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

[Jacob Keller <jacob.e.keller@intel.com>]

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.
> 
>>     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.