Re: [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ice: stale shared phc.cached_time when clock-owning PF stops or unbinds
--

commit 866055ac7173935e690af3f73268e559a0698a8d
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: store cached PHC time in ice_adapter

This commit moves the cached PHC time from per-PF and per-Rx-ring fields
into a shared struct ice_phc embedded in struct ice_adapter, and updates
only the clock-owning PF to refresh that shared value in its periodic
work.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index ec24c09280785..4b4a00beadd2c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -987,35 +992,26 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
>  }
>  
>  /**
> - * ice_ptp_update_cached_phctime - Update the cached PHC time values
> + * ice_ptp_update_cached_phctime - Update the cached PHC time value
>   * @pf: Board specific private structure
>   *
> - * This function updates the system time values which are cached in the PF
> - * structure and the Rx rings.
> - *
> - * This function must be called periodically to ensure that the cached value
> - * is never more than 2 seconds old.
> + * Update the PTP hardware clock time values which are cached in the PHC
> + * structure.
>   *
> - * Note that the cached copy in the PF PTP structure is always updated, even
> - * if we can't update the copy in the Rx rings.
> - *
> - * Return:
> - * * 0 - OK, successfully updated
> - * * -EAGAIN - PF was busy, need to reschedule the update
> - * * -ENODEV - no control PF owns the source clock register space
> - * * -EOPNOTSUPP - PTP support is not compiled in
> + * This function must be called periodically by the clock owner to ensure that
> + * the cached value is never more than 2 seconds old.
>   */
> -static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
> +static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
>  {
> +	struct ice_phc *phc = &pf->adapter->phc;
>  	struct device *dev = ice_pf_to_dev(pf);
>  	unsigned long update_before;
>  	u64 systime;
> -	int err, i;
> +	int err;
>  
> -	update_before = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
> -	if (pf->ptp.cached_phc_time &&
> -	    time_is_before_jiffies(update_before)) {
> -		unsigned long time_taken = jiffies - pf->ptp.cached_phc_jiffies;
> +	update_before = phc->update_jiffies + msecs_to_jiffies(2000);
> +	if (phc->cached_time && time_is_before_jiffies(update_before)) {
> +		unsigned long time_taken = jiffies - phc->update_jiffies;
>  
>  		dev_warn(dev, "%u msecs passed between update to cached PHC time\n",
>  			 jiffies_to_msecs(time_taken));
> @@ -1032,36 +1028,12 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
>  		 * longer tracks the PHC, and both treat a zero cache as
>  		 * "no timestamp".
>  		 */
> -		systime = 0;
> -		WRITE_ONCE(pf->ptp.cached_phc_time, 0);
> +		WRITE_ONCE(phc->cached_time, 0);
>  	} else {
> -		/* Update the cached PHC time stored in the PF structure */
> -		WRITE_ONCE(pf->ptp.cached_phc_time, systime);
> -		WRITE_ONCE(pf->ptp.cached_phc_jiffies, jiffies);
> +		/* Update the cached PHC time stored in the PHC structure */
> +		WRITE_ONCE(phc->cached_time, systime);
> +		WRITE_ONCE(phc->update_jiffies, jiffies);
>  	}

[ ... ]

> @@ -3078,24 +3025,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);

[Severity: Medium]
What happens to pf->adapter->phc.cached_time on remaining PFs if the
clock-owning PF is unbound, reset, or fails PTP rebuild?

Previously, commit dcc2c9108e65 ("ice: Reject PTP access without a control
PF") handled control PF removal by having each PF run
ice_ptp_update_cached_phctime() from ice_ptp_periodic_work(). When the
control PF went away, ice_ptp_read_src_clk_reg() returned -ENODEV on the
secondary PFs, which cleared cached_phc_time and the Rx ring cached_phctime
copies to 0.

With this change, ice_ptp_periodic_work() in
drivers/net/ethernet/intel/ice/ice_ptp.c only calls
ice_ptp_update_cached_phctime() when ice_pf_src_tmr_owned(pf) is true
(and the only other callers are ice_ptp_settime64() and ice_ptp_adjtime()
via ice_ptp_reset_cached_phctime(), which also only run on the clock
owner). Because ice_pf_src_tmr_owned(pf) guarantees ice_is_primary(&pf->hw)
is true, ice_ptp_read_src_clk_reg() never returns -ENODEV here, making the
error branch in ice_ptp_update_cached_phctime() that zeroes
phc->cached_time unreachable.

Meanwhile, when the clock-owning PF stops its periodic worker in
ice_ptp_prepare_for_reset() or is unbound via
ice_ptp_release() -> ice_ptp_cleanup_adapter():

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_cleanup_adapter() {
    ...
	if (ice_get_ctrl_pf(pf) == pf) {
		rcu_assign_pointer(pf->adapter->ctrl_pf, NULL);
		synchronize = true;
	}
    ...
}

nothing zeroes pf->adapter->phc.cached_time.

Because the Rx path in ice_ptp_get_rx_hwts() -> ice_ptp_extend_32b_ts()
only checks whether phc->cached_time is non-zero and does not check
phc->update_jiffies, won't active sibling or secondary PFs continue
extending 32-bit hardware Rx timestamps against the frozen
phc->cached_time indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-jk-e825-e830-timecounter-v1-0-e932602f0d16@intel.com?part=1