On 10/23/2026 6:55 PM, [email protected] 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 <[email protected]>
>
> 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.
