Re: [PATCH iwl-next v2 3/4] ice: store cached PHC time in ice_adapter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ice: stale adapter->phc cache across reset in
  ice_ptp_rebuild_owner()
--

commit 3e2553cade32eeca4938b383704866bc53bd38ea
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: store cached PHC time in ice_adapter

This commit moves the cached PHC time and its update timestamp from the
per-PF ice_ptp and Rx ring structures into a shared struct ice_phc embedded
in ice_adapter, updated only by the clock owner PF.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index bf37c2a7ff863..a0451c1083a41 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
[ ... ]
> @@ -3099,24 +3056,20 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
>  {
>  	struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
>  	struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
> -	int err;
>  
>  	if (pf->ptp.state != ICE_PTP_READY)
>  		return;
>  
>  	ice_ptp_tspll_monitor(pf);
>  
> -	err = ice_ptp_update_cached_phctime(pf);
> +	if (ice_pf_src_tmr_owned(pf))
> +		ice_ptp_update_cached_phctime(pf);
>  
>  	ice_ptp_maybe_trigger_tx_interrupt(pf);
>  
> -	/* Run twice a second, or retry soon if another thread was busy
> -	 * updating the Rx rings. Other errors, such as a missing control PF,
> -	 * do not clear up on their own and must not turn the periodic worker
> -	 * into a busy loop.
> -	 */
> +	/* Run twice a second */
>  	kthread_queue_delayed_work(ptp->kworker, &ptp->work,
> -				   msecs_to_jiffies(err == -EAGAIN ? 10 : 500));
> +				   msecs_to_jiffies(500));
>  }
>  
>  /**
> @@ -3211,6 +3164,8 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	/* Acquire PHC and system timer to restore after reset */
>  	ptp->reset_time = ktime_get_real_ns();
> +	if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
> +		ptp->phc_reset_time = 0;
>  }
>  
>  /**
> @@ -3250,13 +3205,13 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
>  	if (err)
>  		goto err_unlock;
>  
> -	/* Write the initial Time value to PHY and LAN using the cached PHC
> -	 * time before the reset and time difference between stopping and
> -	 * starting the clock.
> +	/* Write the initial Time value to PHY and LAN using the PHC time
> +	 * before the reset and time difference between stopping and starting
> +	 * the clock.
>  	 */
> -	if (ptp->cached_phc_time) {
> +	if (ptp->phc_reset_time) {
>  		time_diff = ktime_get_real_ns() - ptp->reset_time;
> -		ts = ns_to_timespec64(ptp->cached_phc_time + time_diff);
> +		ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
>  	} else {
>  		ts = ktime_to_timespec64(ktime_get_real());
>  	}

[Severity: Medium]
Does ice_ptp_rebuild_owner() leave a stale cached_time in pf->adapter->phc
across a non-PFR reset?

When a reset such as EMPR zeroes GLTSYN_TIME before
ice_ptp_prepare_for_reset() reads it, ptp->phc_reset_time is 0 and
ice_ptp_rebuild_owner() reinitializes the hardware clock to
ktime_get_real() (UTC) via ice_ptp_write_init(), which can differ from a
pre-reset TAI PHC by 37 seconds.  Neither ice_ptp_prepare_for_reset() nor
ice_ptp_rebuild_owner() invalidates or refreshes pf->adapter->phc after
writing the new time, and non-owner PFs no longer update the cache in
ice_ptp_periodic_work().

If the reset and rebuild finish within 2000 ms of the last pre-reset
update, ice_ptp_extend_32b_ts() still sees a fresh phc->update_jiffies and
extends post-reset Rx and Tx hardware timestamps against the pre-reset
phc->cached_time until the owner PF's kworker runs
ice_ptp_periodic_work().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-jk-e825-e830-timecounter-v2-0-9c357a7ed349@intel.com?part=3