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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/23/2026 6:55 PM, sashiko-bot@kernel.org wrote:
> 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?
> 

Yea, this makes sense, I think I can re-order things to make that work.

> [ ... ]
> 
>> @@ -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?
> 

Hm. Ya that seems correct. Can fix.

> [ ... ]
> 
>> @@ -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?
> 

True. We weren't checking jiffies before because it wasn't accessible
and partially because it was considered expensive and not worth doing
for the per-packet Rx timestamps.

We can fix the

> 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().

Strictly, i don't think I had really intended the jiffies protection to
be applied to the timecounter logic. But I can fix that. Additionally,
ice_ptp_read_cycles() is actually only ever called from the periodic
work (all other callers should be using timecounter_cyc2time to avoid
modifying the timecounter unnecessarily and causing the seqlock contention).

I can try to rework this to fix that.

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

I am not certain this is actually possible, I would have to check if
those resets actually do clear these registers.

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

I'll try to fix the logic regardless.

> [ ... ]
> 
>> @@ -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?
> 

This may be somewhat tricky. We need to compute the inverse of the
timecounter phase like we do with periodic output.

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

We can use ice_phc_cyc2time to convert the time reported to the VF, but
then the raw timestamps captured won't convert properly with timestamp
extension (they lack access to the timecounter). I think we need to
disable Rx timestamp offload until we figure out how to resolve this.