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

[Jakub Kicinski <kuba@kernel.org>]

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

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 299de9d494233..277d9c77af1cb 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -826,12 +826,10 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>   * ice_ptp_mark_tx_tracker_stale - Mark unfinished timestamps as stale
>   * @tx: the tracker to mark
>   *
> - * Mark currently outstanding Tx timestamps as stale. This prevents sending
> - * their timestamp value to the stack. This is required to prevent extending
> - * the 40bit hardware timestamp incorrectly.
> - *
> - * This should be called when the PTP clock is modified such as after a set
> - * time request.
> + * Mark currently outstanding Tx timestamps as stale. This prevents the driver
> + * from reporting the timestamp to the stack. This is called to inform the
> + * driver that a timestamp is expected to fail if it was initiated as the link
> + * went down.
>   */
>  static void
>  ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)

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

>  static void
> @@ -1049,13 +1047,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
>  		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
>  					   msecs_to_jiffies(10));
>  	}
> -
> -	/* Mark any outstanding timestamps as stale, since they might have
> -	 * been captured in hardware before the time update. This could lead
> -	 * to us extending them with the wrong cached value resulting in
> -	 * incorrect timestamp values.
> -	 */
> -	ice_ptp_mark_tx_tracker_stale(&pf->ptp.port.tx);
>  }
>
>  /**