Re: [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ice: 500ms offset before ice_phc_cyc2time() in ice_ptp_cfg_perout() wraps
- [High] ice: uncompensated timecounter offset for E830 TxTime in ice_tx_map()
- [Medium] ice: late PHC update check broken in ice_ptp_update_cached_phctime()
- [Medium] ice: missing timecounter conversion in ice_vc_get_phc_time()
- [Medium] ice: missing timecounter staleness and validity handling in ice_phc
- [Medium] ice: ice_ptp_prepare_for_reset() converts zero GLTSYN_TIME via timecounter
--

commit 348b48a49347fa2790d183338857035e4d8f1fa4
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: use struct timecounter for E825 and E830

This commit switches E825 and E830 PTP hardware clock management from direct
hardware timer adjustments to a software struct timecounter and cyclecounter
embedded in struct ice_phc. This avoids PCS timer synchronization stalls on
E825 caused by manual PHY timer adjustments and converts timestamp extension,
clock reads, cross-timestamping, and periodic output configuration to account
for the timecounter phase offset.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 4b4a00beadd2c..488db8298059a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -210,6 +210,38 @@ void ice_ptp_restore_timestamp_mode(struct ice_pf *pf)
>  	ice_flush(hw);
>  }
>  
> +u64 ice_ptp_read_cycles(struct cyclecounter *cc)
> +{
> +	struct ice_adapter *adapter;
> +	struct ice_phc *phc;
> +	struct ice_pf *pf;
> +	struct ice_hw *hw;
> +	u8 tmr_idx;
> +
> +	phc = container_of(cc, struct ice_phc, cc);
> +	adapter = container_of(phc, struct ice_adapter, phc);
> +
> +	scoped_guard(rcu) {
> +		pf = rcu_dereference(adapter->ctrl_pf);
> +		if (!pf)
> +			return 0;

[ ... ]

> @@ -995,19 +1046,17 @@ 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 value
>   * @pf: Board specific private structure
>   *
> - * Update the PTP hardware clock time values which are cached in the PHC
> - * structure.
> + * For devices that use a timecounter structure, update the timecounter.
> + * Ohterwise, update the cached PHC time used for timestamp extension.
>   *
>   * This function must be called periodically by the clock owner to ensure that
> - * the cached value is never more than 2 seconds old.
> + * the timecounter or cached value is never more than 2 seconds old.
>   */
>  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;
>  
>  	update_before = phc->update_jiffies + msecs_to_jiffies(2000);
>  	if (phc->cached_time && time_is_before_jiffies(update_before)) {
> @@ -1018,21 +1067,33 @@ static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
>  		pf->ptp.late_cached_phc_updates++;
>  	}
>  
> -	/* Read the current PHC time */
> -	err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
> -	if (err) {
> -		/* The source clock is gone, so the cache can no longer be
> -		 * refreshed. Publish zero instead of leaving the last good
> -		 * value behind: neither ice_ptp_get_rx_hwts() nor
> -		 * ice_ptp_extend_40b_ts() may extend against a value that no
> -		 * longer tracks the PHC, and both treat a zero cache as
> -		 * "no timestamp".
> -		 */
> -		WRITE_ONCE(phc->cached_time, 0);
> +	WRITE_ONCE(phc->update_jiffies, jiffies);
> +
> +	if (phc->use_timecounter) {
> +		unsigned long flags;
> +
> +		write_seqlock_irqsave(&phc->tc_lock, flags);
> +		timecounter_read(&phc->tc);
> +		write_sequnlock_irqrestore(&phc->tc_lock, flags);
>  	} else {
> -		/* Update the cached PHC time stored in the PHC structure */
> +		u64 systime;
> +		int err;
> +
> +		err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
> +		if (err) {
> +			/* The source clock is gone, so the cache can no
> +			 * longer be refreshed. Publish zero instead of
> +			 * leaving the last good value behind: neither
> +			 * ice_ptp_get_rx_hwts() nor ice_ptp_extend_40b_ts()
> +			 * may extend against a value that no longer tracks
> +			 * the PHC, and both treat a zero cache as "no
> +			 * timestamp".
> +			 */
> +			systime = 0;
> +		}
> +
> +		/* Update the cached PHC time stored in the PF structure */
>  		WRITE_ONCE(phc->cached_time, systime);
> -		WRITE_ONCE(phc->update_jiffies, jiffies);
>  	}
>  }

[Severity: Medium]
In ice_ptp_update_cached_phctime(), the check for late updates still tests
phc->cached_time:

	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));
		pf->ptp.late_cached_phc_updates++;
	}

When phc->use_timecounter is true on E825 and E830 devices, phc->cached_time
is initialized to 0 in ice_adapter_new() and is never written. Won't this
cause the condition to always evaluate to false in timecounter mode,
preventing late timecounter updates from ever emitting the warning or
incrementing pf->ptp.late_cached_phc_updates?

Also, by moving WRITE_ONCE(phc->update_jiffies, jiffies) above the
if (phc->use_timecounter) branch, phc->update_jiffies is now updated on
legacy devices even when ice_ptp_read_src_clk_reg() fails and systime is set
to 0. Should phc->update_jiffies only be updated when the clock read
succeeds, and should the late-update check also cover timecounter mode?

[ ... ]

> @@ -1793,8 +1859,9 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan,
>  static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
>  			      int on)
>  {
> +	struct ice_phc *phc = &pf->adapter->phc;
> +	u64 clk, period, start, phase, cycles;
>  	unsigned int gpio_pin, prop_delay_ns;
> -	u64 clk, period, start, phase;
>  	struct ice_hw *hw = &pf->hw;
>  	int pin_desc_idx;
>  	int err;
> @@ -1840,12 +1907,21 @@ static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
>  	if (err)
>  		return err;
>  	clk += NSEC_PER_MSEC * 500;
> +	if (phc->use_timecounter) {
> +		cycles = clk;
> +		clk = ice_phc_cyc2time(phc, cycles);
> +	}
> +
>  	if (rq->flags & PTP_PEROUT_PHASE || start <= clk - prop_delay_ns)
>  		start = div64_u64(clk + period - 1, period) * period + phase;
>  
>  	/* Compensate for propagation delay from the generator to the pin. */
>  	start -= prop_delay_ns;
>  
> +	/* Compensate for timecounter phase offset, if necessary */
> +	if (phc->use_timecounter)
> +		start -= (clk - cycles);
> +
>  	return ice_ptp_write_perout(hw, rq->index, gpio_pin, start, period);
>  }

[Severity: High]
In ice_ptp_cfg_perout(), 500 ms (NSEC_PER_MSEC * 500) is added to the raw
hardware clock value before passing cycles to ice_phc_cyc2time():

	err = ice_ptp_read_src_clk_reg(pf, NULL, &clk);
	if (err)
		return err;
	clk += NSEC_PER_MSEC * 500;
	if (phc->use_timecounter) {
		cycles = clk;
		clk = ice_phc_cyc2time(phc, cycles);
	}

Since phc->cc uses CYCLECOUNTER_MASK(32), timecounter_cyc2time() treats any
cycle value where (cycles - tc->cycle_last) & 0xffffffff exceeds
cc->mask / 2 (~2.147 seconds) as having occurred in the past, subtracting the
delta from tc->nsec instead of adding it.

Because cycles is already 0.5 seconds ahead of the current hardware counter,
doesn't this shrink the forward window before wraparound from ~2.147 seconds
to ~1.647 seconds? If tc->cycle_last was updated between 1.648 and 2.0
seconds ago (within the normal periodic update window, or during
ice_ptp_adjtime_tc() -> ice_ptp_enable_all_perout() where tc->cycle_last is
not refreshed), ice_phc_cyc2time() will wrap backwards by 2^32 ns, making
(clk - cycles) and the resulting GLTSYN_TGT_L/H start time off by 2^32 ns
(and shifting the periodic output phase by 2^32 mod period).

Should ice_phc_cyc2time() be called on the raw clk value before adding
NSEC_PER_MSEC * 500?

[ ... ]

> @@ -2414,15 +2546,9 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
>  		return 0;
>  
>  	cached_time = READ_ONCE(pkt_ctx->phc->cached_time);
> -	if (!cached_time)
> +	if (!pkt_ctx->phc->use_timecounter && !cached_time)
>  		return 0;
>  
> -	/* Use ice_ptp_extend_32b_ts directly, using the ring-specific cached
> -	 * PHC value, rather than accessing the PF. This also allows us to
> -	 * simply pass the upper 32bits of nanoseconds directly. Calling
> -	 * ice_ptp_extend_40b_ts is unnecessary as it would just discard these
> -	 * bits itself.
> -	 */
>  	ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
>  	ts_ns = ice_ptp_extend_32b_ts(pkt_ctx->phc, ts_high);

[Severity: Medium]
When pkt_ctx->phc->use_timecounter is true, ice_ptp_get_rx_hwts() bypasses
the cached_time check and does not check phc->update_jiffies (unlike
ice_ptp_extend_40b_ts()) or whether the clock owner is active.

Because timecounter_cyc2time() with a 32-bit cyclecounter is only valid
within +/- 2.14 seconds of tc->cycle_last, what happens if the periodic work
is delayed past 2 seconds, or if the clock owner PF on a multi-PF adapter is
unbound in ice_ptp_release(), fails ice_ptp_rebuild(), or fails
ice_ptp_init_owner() after setting phc->use_timecounter = 1? Won't sibling
PFs continue extending Rx timestamps against a frozen or uninitialized
phc->tc instead of returning 0?

Relatedly, in ice_ptp_release(), ice_ptp_cleanup_adapter() clears
adapter->ctrl_pf before cancelling pf->ptp.work and unregistering the PTP
clock, which allows ice_ptp_read_cycles() to observe a NULL ctrl_pf and
return 0 as a valid cycle count into timecounter_read() or
timecounter_init(). Also, ice_ptp_init_owner(), ice_ptp_rebuild_owner(), and
ice_ptp_settime64_tc() call timecounter_init() without refreshing
phc->update_jiffies, which can leave phc->update_jiffies stale across a reset
until the periodic work runs and cause ice_ptp_extend_40b_ts() to discard
valid Tx timestamps right after ice_ptp_restore_timestamp_mode().

[ ... ]

> @@ -3133,8 +3266,11 @@ 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;
> +	if (ice_ptp_read_src_clk_reg(pf, NULL, &ns))
> +		ns = 0;
> +	else if (pf->adapter->phc.use_timecounter)
> +		ns = ice_phc_cyc2time(&pf->adapter->phc, ns);
> +	ptp->phc_reset_time = ns;
>  }

[Severity: Medium]
When a hardware reset (such as CORER, GLOBR, or EMPR) has already cleared the
GLTSYN_TIME registers to 0 before ice_ptp_prepare_for_reset() runs,
ice_ptp_read_src_clk_reg() on the clock owner PF returns 0 with ns = 0.

Previously, ns = 0 resulted in ptp->phc_reset_time = 0, causing
ice_ptp_rebuild_owner() to fall back to ktime_get_real_ns():

	if (ptp->phc_reset_time) {
		time_diff = ktime_get_real_ns() - ptp->reset_time;
		ns = ptp->phc_reset_time + time_diff;
	} else {
		ns = ktime_get_real_ns();
	}

With phc.use_timecounter enabled, the else-if branch passes ns = 0 to
ice_phc_cyc2time(&pf->adapter->phc, 0), which computes a 32-bit delta between
cycle 0 and the pre-reset tc->cycle_last. Doesn't this turn a zeroed hardware
register into a non-zero ptp->phc_reset_time shifted by up to +/- 2.147
seconds, bypassing the ktime_get_real_ns() fallback in
ice_ptp_rebuild_owner()?

[ ... ]

> @@ -3375,10 +3522,21 @@ int ice_ptp_clock_index(struct ice_pf *pf)
>   */
>  static int ice_ptp_init_owner(struct ice_pf *pf)
>  {
> +	struct ice_phc *phc = &pf->adapter->phc;
>  	struct ice_hw *hw = &pf->hw;
> -	struct timespec64 ts;
>  	int err;
>  
> +	switch (hw->mac_type) {
> +	case ICE_MAC_GENERIC_3K_E825:
> +	case ICE_MAC_E830:
> +		/* Use struct timecounter for E825 and E830 devices */
> +		dev_dbg(ice_pf_to_dev(pf), "PTP 1588 enabling timecounter mode\n");
> +		phc->use_timecounter = 1;
> +		break;
> +	default:
> +		break;
> +	}

[Severity: High]
Enabling phc->use_timecounter on ICE_MAC_E830 means the hardware
E830_GLTSYN_TIME registers are no longer initialized in ice_ptp_init_owner()
or ice_ptp_rebuild_owner(), nor adjusted in ice_ptp_settime64_tc() and
ice_ptp_adjtime_tc().

However, E830 supports ICE_F_TXTIME hardware offload:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_init_feature_support() {
	if (pf->hw.mac_type == ICE_MAC_E830) {
		ice_set_feature_support(pf, ICE_F_MBX_LIMIT);
		ice_set_feature_support(pf, ICE_F_GCS);
		ice_set_feature_support(pf, ICE_F_TXTIME);
	}
}

And in ice_tx_map(), the TxTime descriptor timestamp is populated directly
from first->skb->tstamp without compensating for the timecounter phase
offset:

drivers/net/ethernet/intel/ice/ice_txrx.c:ice_tx_map() {
		ts = ktime_to_timespec64(first->skb->tstamp);
		tstamp = ts.tv_nsec >> ICE_TXTIME_CTX_RESOLUTION_128NS;

		ts_desc = ICE_TS_DESC(tstamp_ring, j);
		ts_desc->tx_desc_idx_tstamp = ice_build_tstamp_desc(i, tstamp);
}

How does ICE_F_TXTIME work on E830 when the PHC time is maintained in the
software timecounter while the hardware TxTime queue compares descriptor
timestamps against the unadjusted hardware clock?

[Severity: Medium]
Similarly, when an SR-IOV VF requests the PHC time via
VIRTCHNL_OP_1588_PTP_GET_TIME, ice_vc_get_phc_time() in
drivers/net/ethernet/intel/ice/virt/virtchnl.c still reads the raw hardware
register without converting through ice_phc_cyc2time():

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_phc_time() {
	if (ice_ptp_read_src_clk_reg(pf, NULL, &phc_time->time)) {
		v_ret = VIRTCHNL_STATUS_ERR_NOT_SUPPORTED;
		len = 0;
	}
}

Furthermore, in the VF driver, iavf_ptp_extend_32b_timestamp() in
drivers/net/ethernet/intel/iavf/iavf_ptp.c computes:

	u32 low = lower_32_bits(cached_phc_time);
	u32 delta = in_tstamp - low;

which assumes the lower 32 bits of the PHC time returned by the PF match the
raw 32-bit hardware timestamp domain in the Rx descriptor. Won't enabling
phc->use_timecounter on E825 and E830 break both VF PTP clock reads and VF Rx
hardware timestamp extension?

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