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

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)?

[Severity: Low]
This kernel-doc is updated, but two other comment blocks still describe the
removed behaviour.  Should they be updated in the same patch?

The kernel-doc of ice_ptp_reset_cached_phctime(), the function being changed
here, still says:

 * This function must be called when the cached PHC time is no longer valid,
 * such as after a time adjustment. It marks any currently outstanding Tx
 * timestamps as stale and updates the cached PHC time for both the PF and Rx
 * rings.

And the kernel-doc of ice_ptp_process_tx_tstamp() still says the opposite of
the reasoning in this commit message:

 * In cases where the PTP hardware clock was directly adjusted, some
 * timestamps may not be able to safely use the timestamp extension math. In
 * this case, software will set the stale bit for any outstanding Tx
 * timestamps when the clock is adjusted. Then this function will discard
 * those captured timestamps instead of sending them to the stack.

After this patch the only caller of ice_ptp_mark_tx_tracker_stale() is the
link-down branch in ice_ptp_link_change():

	if (!linkup)
		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

so nothing sets the stale bit on a clock adjustment any more.  Both comments
are still present unchanged at the end of the series.