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? [ ... ] > @@ -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/[email protected]?part=2
