> -----Original Message----- > From: Jacob Keller <[email protected]> > Sent: Saturday, August 22, 2026 2:13 AM > To: Intel Wired LAN <[email protected]> > Cc: [email protected]; Machnikowski, Maciej > <[email protected]>; Nguyen, Anthony L > <[email protected]>; Korba, Przemyslaw > <[email protected]>; Nitka, Grzegorz > <[email protected]>; Oros, Petr <[email protected]>; Nowlin, > Alexander <[email protected]>; Bross, Kevin > <[email protected]>; Cavatur, Ranjit <[email protected]>; > Keller, Jacob E <[email protected]> > Subject: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears > ready bitmap > > During a link down transition, the E825 PHY has a small window where > it does not properly respond to reading the PHY timestamp registers. > When this occurs, the PHY does not automatically clear the ready > bitmap or the valid bit for the timestamp. This begins happening > slightly before a link transition even before the firmware has > notified the driver of the state change. > > The driver happily completes the timestamp, releasing the in_use bit. > This allows another request to reuse the bit potentially reporting an > invalid stale timestamp. Additionally, with the ready bit still set > high the driver continues to re-trigger the IRQ and check for > timestamps in a tight loop, wasting CPU cycles. > > To fix this, re-read the PHY timestamp memory status after each read > of a PHY index. Double check if the hardware cleared the index > properly. If it hasn't, mark the timestamp index as stale and skip > processing it. > > Stale timestamps are already ignored by the > ice_any_port_has_timestamps() function. However, the > ice_ptp_tx_tstamps_pending() function also checks the ready bitmap. > Instead, modify it to only check the software tracker. > Additionally, stop re-triggering the interrupt from the IRQ if the > timestamp tracker is calibrating or has the link marked as down. > Continue to check the hardware ready bitmap from the watchdog to catch > cases of unexpected timestamps. > > With these changes, the timestamp processing no longer triggers a > repeated spamming of the IRQ during link down events where timestamps > get stuck as the PHY transitions to link down. Once link is restored, > the PHY will be reset and the stuck timestamps are cleared. > > Measuring CPU utilization of the miscellaneous IRQ thread function > during timestamp storms near a link reset shows that this prevents the > spikes caused by the "stuck" ready bit. Without this fix, the CPU > handling the IRQ becomes slammed due to the IRQ re-triggering logic. > > Measuring latency using the ice Tx timestamp traces does show that > this fix comes at a latency cost. Latency is measured using the ice Tx > timestamp traces for the request to completion time. I measured a > couple of different workloads both before and after this fix: > > * ptp4l using a profile with ~16 SYNC messages per second > > before: 159.40 microseconds mean, stdev 45.28 > after: 182.07 microseconds mean, stdev 43.43 > > * a C program generating 16 timestamp requests every 10 milliseconds > on > two different ports: > > before: 604.35 microseconds mean, stdev 345.32 > after: 990.13 microseconds mean, stdev 625.64 > > In the normal work flows this comes with about a 20 microsecond > penalty on the average, and the standard deviation remains > approximately the same. For heavy workloads with many more timestamps > than expected for typical applications this comes at a significant > cost. This is because we handle all timestamps in a single thread. If > there are many concurrent timestamps being requested at once, any > which use the later slots on ports later in the port list will take > much longer to be processed once the interrupt is fired. Since each > timestamp now requires an additional PHY register access, this cost is > much higher in the case where the device is under unusually heavy > load. > > However, *correctness* is more important than speed here. > Additionally, we still remain well below the default limit of 10 > milliseconds that ptp4l will wait before complaining about missing > timestamps. > > Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C > products") > Signed-off-by: Jacob Keller <[email protected]> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 54 +++++++++++++++-------- > --------- > 1 file changed, 26 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > index b337247c94e0..0e44ed6b0ae8 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -620,6 +620,22 @@ static void ice_ptp_process_tx_tstamp(struct > ice_ptp_tx *tx) > if (err && !drop_ts) > continue; > > + /* verify ready bit cleared */ > + if (tx->has_ready_bitmap) { > + err = ice_get_phy_tx_tstamp_ready(hw, tx->block, > &tstamp_ready); > + if (err) > + continue; > + > + if (tstamp_ready & BIT_ULL(phy_idx)) { > + spin_lock_irqsave(&tx->lock, flags); > + if (!test_and_set_bit(idx, tx->stale)) > + dev_dbg(ice_pf_to_dev(pf), "PHY port > %u failed to clear ready bit for idx %u\n", > + ptp_port->port_num, phy_idx); > + spin_unlock_irqrestore(&tx->lock, flags); > + continue; > + } > + } > + > ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx); > > /* For PHYs which don't implement a proper timestamp > ready @@ -2764,10 +2780,14 @@ static bool > ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq) > if (!tx->init) > return false; > > - if (in_irq) > + if (in_irq) { > + if (!ice_ptp_is_tx_tracker_up(tx)) > + return false; > + > return bitmap_andnot(tstamps, tx->in_use, tx- > >stale, tx->len); > - else > + } else { > return !bitmap_empty(tx->in_use, tx->len); > + } > } > } > > @@ -2790,41 +2810,18 @@ static bool ice_any_port_has_timestamps(struct > ice_pf *pf, bool in_irq) > > bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq) { > - struct ice_hw *hw = &pf->hw; > - int ret; > - > - /* Check software indicator */ > switch (pf->ptp.tx_interrupt_mode) { > case ICE_PTP_TX_INTERRUPT_NONE: > return false; > case ICE_PTP_TX_INTERRUPT_SELF: > - if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq)) > - return true; > - break; > + return ice_port_has_timestamps(&pf->ptp.port.tx, > in_irq); > case ICE_PTP_TX_INTERRUPT_ALL: > - if (ice_any_port_has_timestamps(pf, in_irq)) > - return true; > - break; > + return ice_any_port_has_timestamps(pf, in_irq); > default: > WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode > %u\n", > pf->ptp.tx_interrupt_mode); > - break; > - } > - > - /* Check hardware indicator */ > - ret = ice_check_phy_tx_tstamp_ready(hw); > - if (ret < 0) { > - dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx > timestamp ready bitmap, err %d\n", > - ret); > - /* Stop triggering IRQs if we're unable to read PHY */ > return false; > } > - > - /* ice_check_phy_tx_tstamp_ready() returns 1 if there are > timestamps > - * available, 0 if there are no waiting timestamps, and a > negative > - * value if there was an error (which we checked for above). > - */ > - return ret > 0; > } > > /** > @@ -2915,7 +2912,8 @@ static void > ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf) > if (!ice_pf_src_tmr_owned(pf)) > return; > > - if (ice_ptp_tx_tstamps_pending(pf, false)) { > + if (ice_ptp_tx_tstamps_pending(pf, false) || > + ice_check_phy_tx_tstamp_ready(hw)) { Not obvious conversion of returned -EOPNOTSUPP and other errors as bool.
> dev_dbg(dev, "PTP periodic task detected waiting > timestamps. Triggering Tx timestamp interrupt now.\n"); > > wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M); > > -- > 2.55.0.814.gc42f45431d0f
