* [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues
@ 2026-07-20 12:01 Przemyslaw Korba
2026-07-20 12:01 ` [PATCH iwl-net 1/4] ice: keep Tx timestamps disabled until offset calibration completes Przemyslaw Korba
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Przemyslaw Korba @ 2026-07-20 12:01 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov,
arkadiusz.kubalewski, konstantin.ilichev
This series fixes several PTP timestamp handling issues in the ice
driver that manifest during link state transitions on E82x devices.
Together, these issues can cause Tx timestamp timeouts, ptp4l service
disruptions, and on PREEMPT_RT kernels, a system livelock requiring a
hard reboot.
The root cause is a set of race conditions in the PTP link change path.
When taking the link down (ice_down) or bringing it up
(ice_up_complete), the PTP recalibration and cleanup were not properly
synchronized with actual physical link transitions. This allowed Tx
timestamp requests to be queued against uncalibrated or stopped PHY
hardware, producing orphaned timestamp ready bits in the PHY registers.
On E82x, the PHY will not generate a new timestamp interrupt until all
outstanding ready bits have been read. Orphaned bits — set for slots
with no matching software tracker entry — are never read, permanently
blocking new timestamp interrupts. The existing periodic work detects
this symptom and re-triggers the interrupt, but without clearing the
stale bits, this creates an infinite trigger loop. On PREEMPT_RT
kernels, the resulting interrupt storm causes RCU stalls and eventually
a full system livelock.
Arkadiusz Kubalewski (1):
ice: call PTP link change only from link events
Jacob Keller (1):
ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY
Karol Kolacinski (2):
ice: keep Tx timestamps disabled until offset calibration completes
ice: cancel E82x offset verification work during reset preparation
drivers/net/ethernet/intel/ice/ice_main.c | 3 +-
drivers/net/ethernet/intel/ice/ice_ptp.c | 155 +++++++++++++++++---
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++--
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 +
drivers/net/ethernet/intel/ice/ice_type.h | 7 +
5 files changed, 219 insertions(+), 36 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH iwl-net 1/4] ice: keep Tx timestamps disabled until offset calibration completes 2026-07-20 12:01 [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues Przemyslaw Korba @ 2026-07-20 12:01 ` Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY Przemyslaw Korba ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Przemyslaw Korba @ 2026-07-20 12:01 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, arkadiusz.kubalewski, konstantin.ilichev, Przemyslaw Korba From: Karol Kolacinski <karol.kolacinski@intel.com> Do not clear the tx.calibrating flag immediately after starting the PHY timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps disabled until the offset verification work (ice_ptp_wait_for_offsets) has confirmed that both Tx and Rx PHY offsets are properly configured. Previously, tx.calibrating was set to true, then immediately back to false right after ice_start_phy_timer_e82x() returned. This allowed Tx timestamp requests to be served during the window where offset verification was still pending. Timestamps produced during this window use uncalibrated PHY offsets and can produce incorrect values. When ptp4l receives incorrect timestamps, it may reject them and wait for the next sync interval (typically 1-2 seconds), compounding delays during link cycling. This contributes to the time transmitter port becoming unresponsive after repeated link down/up cycles. Move the tx.calibrating = false to ice_ptp_wait_for_offsets(), after both Tx and Rx offset configuration has completed successfully. This ensures that Tx timestamps are only reported with properly calibrated PHY offsets. If ice_start_phy_timer_e82x() fails, restore calibrating to false on the error path to prevent permanently disabling Tx timestamps when ov_work is never queued. Log a debug message while offset calibration is still pending, including the specific Tx/Rx error codes to aid debugging stalled calibration. This path is expected on every routine link-up: ov_work is first queued with no delay and the vernier offset cannot be computed until at least one packet has been transmitted, so the first several invocations normally land here. Use dev_dbg() rather than a rate-limited warning to avoid emitting KERN_WARNING on every link-up during normal operation. Log a debug message when calibration completes successfully. Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support") Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp.c | 36 ++++++++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 6984fe5bf033..1aa440b0639f 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1149,6 +1149,7 @@ static int ice_ptp_check_tx_fifo(struct ice_ptp_port *port) static void ice_ptp_wait_for_offsets(struct kthread_work *work) { struct ice_ptp_port *port; + unsigned long flags; struct ice_pf *pf; struct ice_hw *hw; int tx_err; @@ -1171,12 +1172,28 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work) tx_err = ice_phy_cfg_tx_offset_e82x(hw, port->port_num); rx_err = ice_phy_cfg_rx_offset_e82x(hw, port->port_num); if (tx_err || rx_err) { - /* Tx and/or Rx offset not yet configured, try again later */ + /* Tx and/or Rx offset not yet configured, try again later. + * This is expected during normal link-up: the vernier offset + * calibration cannot complete until at least one packet has + * been transmitted, so the first retries routinely land here. + */ + dev_dbg(ice_pf_to_dev(pf), + "PTP offset not yet valid for port %u (tx_err=%d rx_err=%d)\n", + port->port_num, tx_err, rx_err); kthread_queue_delayed_work(pf->ptp.kworker, &port->ov_work, msecs_to_jiffies(100)); return; } + + /* Tx and Rx offsets are now configured, enable Tx timestamps */ + spin_lock_irqsave(&port->tx.lock, flags); + port->tx.calibrating = false; + spin_unlock_irqrestore(&port->tx.lock, flags); + + dev_dbg(ice_pf_to_dev(pf), + "PTP offset valid for port %u, Tx timestamps enabled\n", + port->port_num); } /** @@ -1259,13 +1276,20 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port) /* Start the PHY timer in Vernier mode */ err = ice_start_phy_timer_e82x(hw, port); - if (err) + if (err) { + spin_lock_irqsave(&ptp_port->tx.lock, flags); + ptp_port->tx.calibrating = false; + spin_unlock_irqrestore(&ptp_port->tx.lock, flags); break; + } - /* Enable Tx timestamps right away */ - spin_lock_irqsave(&ptp_port->tx.lock, flags); - ptp_port->tx.calibrating = false; - spin_unlock_irqrestore(&ptp_port->tx.lock, flags); + /* Do not clear calibrating flag here. Tx timestamps remain + * disabled until ice_ptp_wait_for_offsets() has verified + * that the Tx and Rx offset calibration has completed. + * Clearing it here would allow Tx timestamps to be reported + * before the PHY offset registers are configured, leading + * to incorrect timestamp values. + */ kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0); -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY 2026-07-20 12:01 [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 1/4] ice: keep Tx timestamps disabled until offset calibration completes Przemyslaw Korba @ 2026-07-20 12:01 ` Przemyslaw Korba 2026-07-23 8:26 ` [Intel-wired-lan] " Petr Oros 2026-07-20 12:01 ` [PATCH iwl-net 3/4] ice: cancel E82x offset verification work during reset preparation Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 4/4] ice: call PTP link change only from link events Przemyslaw Korba 3 siblings, 1 reply; 9+ messages in thread From: Przemyslaw Korba @ 2026-07-20 12:01 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, arkadiusz.kubalewski, konstantin.ilichev, Grzegorz Nitka, Przemyslaw Korba From: Jacob Keller <jacob.e.keller@intel.com> Clear orphaned timestamp ready bits left in the PHY when a packet is timestamped just as the link goes down. The driver clears its software in_use bits during link-down cleanup, but the PHY has already latched the timestamp, so on the next link-up a stale ready bit from the previous link cycle remains set with no matching in_use entry. The PHY timestamp interrupt logic will not generate a new interrupt until ALL outstanding ready bits have been read. ice_ptp_process_tx_ tstamp() only iterates slots set in the software in_use bitmap, so it never reads these orphaned slots. The result is a permanent interrupt deadlock: 1. The PHY has ready bits set for slots with no in_use tracker entry 2. The driver never reads those slots because in_use is clear 3. The PHY refuses to generate new timestamp interrupts 4. All future Tx timestamps permanently fail 5. Only a power-on reset can recover the device Clear these stale ready bits on link-up, in ice_ptp_link_change(), before any new timestamp requests arrive, scoped to the affected port only so timestamps still pending on other ports are left untouched. For every affected MAC the clearing iterates the not-in_use slots with for_each_clear_bit(), so a pending software request is never discarded and tx->lock need not be held across the PHY access: - E810 is skipped (guarded by tx->has_ready_bitmap) because ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather than a real ready status. - E830 and E825/eth56g read the real Tx timestamp ready bitmap and clear only the slots the PHY actually latched, via ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the clear is skipped and a rate-limited warning is logged. On every MAC the ready bit is cleared by reading the slot's timestamp memory; the orphaned bits linger only because the normal processing path never reads those not-in_use slots. The link-up recovery clears them by reading the timestamp memory via ice_clear_phy_tstamp(): - E830: add ice_clear_phy_tstamp_e830(), which reads the PRTTSYN_TXTIME_H/L registers to clear the entry, and add an ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch. - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory location, which per the PHY spec is the operation that clears the entry's valid bit and its corresponding (read-only) ts_memory_status bit. The ts_memory_status registers cannot be written to clear a bit, so only reading the timestamp memory has any effect. The new for_each_clear_bit() loop runs from the service task via link events. During device removal, tx->in_use is freed before the service task is stopped. Fix by stopping the service task at the start of ice_unload(), before ice_ptp_release() frees tx->in_use. Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 1 + drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++-- drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++---- drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + drivers/net/ethernet/intel/ice/ice_type.h | 7 ++ 5 files changed, 149 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index e3d3810c791f..231d533309cb 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf) devl_assert_locked(priv_to_devlink(pf)); + ice_service_task_stop(pf); ice_unplug_aux_dev(pf); ice_deinit_rdma(pf); ice_deinit_features(pf); diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 1aa440b0639f..9d9d9958fe5c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup) switch (hw->mac_type) { case ICE_MAC_E810: case ICE_MAC_E830: - /* Do not reconfigure E810 or E830 PHY */ + case ICE_MAC_GENERIC_3K_E825: + /* Do not reconfigure E810 or E830 PHY, but on link-up clear + * any stale timestamp ready bits left over from a previous + * link cycle. The PHY may have latched timestamps for packets + * in flight when the link went down; these must be cleared + * before new timestamp requests arrive. + * + * E810 does not have a real ready bitmap + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so + * skip clearing on E810 to avoid unnecessary sideband queue + * operations for every not-in-use slot on each link-up. + */ + if (linkup && ptp_port->tx.has_ready_bitmap) { + struct ice_ptp_tx *tx = &ptp_port->tx; + u64 tstamp_ready; + int i; + + if (ice_get_phy_tx_tstamp_ready(hw, tx->block, + &tstamp_ready)) { + dev_warn_ratelimited(ice_pf_to_dev(pf), + "PTP failed to read Tx timestamp ready bitmap on link-up; stale PHY timestamps may remain and stall Tx timestamping\n"); + } else { + /* Only clear stale ready bits for slots that + * have no in-flight software request. Iterating + * the not-in-use slots skips any concurrent + * ice_ptp_request_ts() allocation without + * holding tx->lock across the PHY access. E830 + * and E825 reach this clear; E810 is filtered + * out above by has_ready_bitmap. + */ + for_each_clear_bit(i, tx->in_use, tx->len) { + u8 phy_idx = i + tx->offset; + + if (tstamp_ready & BIT_ULL(phy_idx)) + ice_clear_phy_tstamp(hw, + tx->block, + phy_idx); + } + } + } + + /* E810 and E830 need no further PHY reconfiguration */ + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825) + return; + + /* E825 recovers its Tx path by soft resetting the PHY + * timestamp block and restarting the port, but only on + * link-up. The reset is a three-step register toggle; if it + * fails partway through, the port can be left held in reset, + * and programming a PHY that is stuck in reset via + * ice_ptp_port_phy_restart() would leave Tx timestamping + * permanently broken. So warn and skip the restart on + * failure; the sequence is retried on the next link-up event. + */ + if (!linkup) + return; + + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num)) + dev_warn(ice_pf_to_dev(pf), + "PTP failed to soft reset PHY port %u on link-up; skipping restart, Tx timestamping may be stuck, try toggle a link to recover\n", + ptp_port->port_num); + else + ice_ptp_port_phy_restart(ptp_port); return; case ICE_MAC_GENERIC: ice_ptp_port_phy_restart(ptp_port); return; - case ICE_MAC_GENERIC_3K_E825: - if (linkup) - ice_ptp_port_phy_restart(ptp_port); - return; default: dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__); } @@ -3380,6 +3438,7 @@ void ice_ptp_init(struct ice_pf *pf) ptp->state = ICE_PTP_INITIALIZING; + mutex_init(&hw->ptp.tx_tstamp_lock); if (hw->lane_num < 0) { err = hw->lane_num; goto err_exit; @@ -3443,6 +3502,7 @@ void ice_ptp_init(struct ice_pf *pf) ice_ptp_cleanup_adapter(pf); err_exit: + mutex_destroy(&hw->ptp.tx_tstamp_lock); /* If we registered a PTP clock, release it */ if (pf->ptp.clock) { ptp_clock_unregister(ptp->clock); @@ -3469,6 +3529,7 @@ void ice_ptp_release(struct ice_pf *pf) if (pf->ptp.state != ICE_PTP_READY) { mutex_destroy(&pf->ptp.port.ps_lock); + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); ice_ptp_cleanup_pf(pf); ice_ptp_cleanup_adapter(pf); if (pf->ptp.clock) { @@ -3495,6 +3556,7 @@ void ice_ptp_release(struct ice_pf *pf) ice_ptp_port_phy_stop(&pf->ptp.port); mutex_destroy(&pf->ptp.port.ps_lock); + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); if (pf->ptp.kworker) { kthread_destroy_worker(pf->ptp.kworker); pf->ptp.kworker = NULL; diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index a1970a887c38..b23b68cd18ee 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -1160,11 +1160,15 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx, * * To directly clear the contents of the timestamp block entirely, discarding * all timestamp data at once, software should instead use - * ice_ptp_reset_ts_memory_quad_eth56g(). + * ice_ptp_reset_ts_memory_eth56g(). * * This function should only be called on an idx whose bit is set according to * ice_get_phy_tx_tstamp_ready(). * + * Serialized against ice_ptp_clear_tx_memory_status_eth56g() via + * tx_tstamp_lock so the two paths do not interleave their reads of the same + * port's Tx timestamp memory. + * * Return: * * %0 - success * * %other - failed to write to PHY @@ -1175,25 +1179,62 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx) u16 lo_addr; int err; - /* Read the timestamp register to ensure the timestamp status bit is - * cleared. + lo_addr = (u16)PHY_TSTAMP_L(idx); + + mutex_lock(&hw->ptp.tx_tstamp_lock); + + /* Per the PHY spec, reading the timestamp memory location is what + * clears the entry's valid bit and its corresponding (read-only) + * ts_memory_status bit. This clears only this index, leaving any + * other in-flight timestamps on the port untouched. */ err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); - if (err) { + if (err) ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n", port, idx, err); - } - - lo_addr = (u16)PHY_TSTAMP_L(idx); err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0); - if (err) { + if (err) ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n", port, idx, err); - return err; + + mutex_unlock(&hw->ptp.tx_tstamp_lock); + + return err; +} + +/** + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory + * @hw: pointer to the HW struct + * @port: port number to clear + * + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the + * only way to clear a timestamp valid bit (and its read-only ts_memory_status + * bit) is to read the timestamp memory location, so read every entry for the + * port (two 32-bit reads each). This discards all timestamp data on the port, + * so it must only be used for a full reset; callers that must preserve + * in-flight timestamps clear individual indices via ice_clear_phy_tstamp(). + * + * Holds tx_tstamp_lock so this full-port sweep does not interleave with the + * per-index reads in ice_clear_ptp_tstamp_eth56g() on the same port. + * + * Return: 0 on success, negative error code on failure to read the PHY. + */ +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port) +{ + u64 unused_tstamp; + int err = 0; + u8 idx; + + mutex_lock(&hw->ptp.tx_tstamp_lock); + for (idx = 0; idx < INDEX_PER_PORT; idx++) { + err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); + if (err) + break; } + mutex_unlock(&hw->ptp.tx_tstamp_lock); - return 0; + return err; } /** @@ -1204,12 +1245,8 @@ static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw) { unsigned int port; - for (port = 0; port < hw->ptp.num_lports; port++) { - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L, - 0); - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U, - 0); - } + for (port = 0; port < hw->ptp.num_lports; port++) + ice_ptp_clear_tx_memory_status_eth56g(hw, port); } /** @@ -5277,6 +5314,23 @@ static void ice_read_phy_tstamp_e830(const struct ice_hw *hw, u8 idx, FIELD_PREP(PHY_EXT_40B_LOW_M, lo); } +/** + * ice_clear_phy_tstamp_e830 - Clear a timestamp from the E830 PHY + * @hw: pointer to the HW struct + * @idx: the timestamp index to clear + * + * Clear the valid bit for the given timestamp index in the Tx memory (TS_MEM). + * On E830 devices the PRTMAC_TS_TX_MEM_VALID_L/H registers are read-only + * mirrors of the per-entry TX_VALID bits and cannot be written. The actual + * TS_MEM entry's TX_VALID bit is cleared by reading the corresponding + * PRTTSYN_TXTIME_L/H registers (read-to-clear). + */ +static void ice_clear_phy_tstamp_e830(const struct ice_hw *hw, u8 idx) +{ + rd32(hw, E830_PRTTSYN_TXTIME_H(idx)); + rd32(hw, E830_PRTTSYN_TXTIME_L(idx)); +} + /** * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register * @hw: pointer to the HW struct @@ -5772,6 +5826,9 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) switch (hw->mac_type) { case ICE_MAC_E810: return ice_clear_phy_tstamp_e810(hw, block, idx); + case ICE_MAC_E830: + ice_clear_phy_tstamp_e830(hw, idx); + return 0; case ICE_MAC_GENERIC: return ice_clear_phy_tstamp_e82x(hw, block, idx); case ICE_MAC_GENERIC_3K_E825: diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h index 16b1988e993d..b003e0aae8b1 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h @@ -304,6 +304,7 @@ int ice_ptp_clear_phy_offset_ready_e82x(struct ice_hw *hw); int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp); int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx); void ice_ptp_reset_ts_memory(struct ice_hw *hw); +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port); int ice_ptp_init_phc(struct ice_hw *hw); void ice_ptp_init_hw(struct ice_hw *hw); int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64 *tstamp_ready); diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index 710c519d670d..d8c73fcafc25 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -888,6 +888,13 @@ enum ice_global_link_topo { struct ice_ptp_hw { union ice_phy_params phy; + /* Serializes eth56g Tx timestamp-memory reads (the per-index + * timestamp entries, not the TX_MEMORY_STATUS registers). Reading an + * entry auto-clears its ts_memory_status bit as a side effect, so the + * per-index and full-port clear paths must not interleave on the same + * port or a bit could re-latch the interrupt. + */ + struct mutex tx_tstamp_lock; u8 num_lports; u8 ports_per_phy; }; -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY 2026-07-20 12:01 ` [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY Przemyslaw Korba @ 2026-07-23 8:26 ` Petr Oros 2026-07-23 13:15 ` Korba, Przemyslaw 0 siblings, 1 reply; 9+ messages in thread From: Petr Oros @ 2026-07-23 8:26 UTC (permalink / raw) To: Przemyslaw Korba, intel-wired-lan Cc: netdev, arkadiusz.kubalewski, aleksandr.loktionov, anthony.l.nguyen, przemyslaw.kitszel, konstantin.ilichev On 7/20/26 14:01, Przemyslaw Korba wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > Clear orphaned timestamp ready bits left in the PHY when a packet is > timestamped just as the link goes down. The driver clears its software > in_use bits during link-down cleanup, but the PHY has already latched > the timestamp, so on the next link-up a stale ready bit from the > previous link cycle remains set with no matching in_use entry. > > The PHY timestamp interrupt logic will not generate a new interrupt > until ALL outstanding ready bits have been read. ice_ptp_process_tx_ > tstamp() only iterates slots set in the software in_use bitmap, so it > never reads these orphaned slots. The result is a permanent interrupt > deadlock: > > 1. The PHY has ready bits set for slots with no in_use tracker entry > 2. The driver never reads those slots because in_use is clear > 3. The PHY refuses to generate new timestamp interrupts > 4. All future Tx timestamps permanently fail > 5. Only a power-on reset can recover the device > > Clear these stale ready bits on link-up, in ice_ptp_link_change(), > before any new timestamp requests arrive, scoped to the affected port > only so timestamps still pending on other ports are left untouched. For > every affected MAC the clearing iterates the not-in_use slots with > for_each_clear_bit(), so a pending software request is never discarded > and tx->lock need not be held across the PHY access: > > - E810 is skipped (guarded by tx->has_ready_bitmap) because > ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather > than a real ready status. > - E830 and E825/eth56g read the real Tx timestamp ready bitmap and > clear only the slots the PHY actually latched, via > ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the > clear is skipped and a rate-limited warning is logged. > > On every MAC the ready bit is cleared by reading the slot's timestamp > memory; the orphaned bits linger only because the normal processing path > never reads those not-in_use slots. The link-up recovery clears them by > reading the timestamp memory via ice_clear_phy_tstamp(): > > - E830: add ice_clear_phy_tstamp_e830(), which reads the > PRTTSYN_TXTIME_H/L registers to clear the entry, and add an > ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch. > - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory > location, which per the PHY spec is the operation that clears the > entry's valid bit and its corresponding (read-only) ts_memory_status > bit. The ts_memory_status registers cannot be written to clear a > bit, so only reading the timestamp memory has any effect. > > The new for_each_clear_bit() loop runs from the service task via link > events. During device removal, tx->in_use is freed before the service > task is stopped. Fix by stopping the service task at the start of > ice_unload(), before ice_ptp_release() frees tx->in_use. > > Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++-- > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++---- > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + > drivers/net/ethernet/intel/ice/ice_type.h | 7 ++ > 5 files changed, 149 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > index e3d3810c791f..231d533309cb 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf) > > devl_assert_locked(priv_to_devlink(pf)); > > + ice_service_task_stop(pf); > ice_unplug_aux_dev(pf); > ice_deinit_rdma(pf); > ice_deinit_features(pf); > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index 1aa440b0639f..9d9d9958fe5c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup) > switch (hw->mac_type) { > case ICE_MAC_E810: > case ICE_MAC_E830: > - /* Do not reconfigure E810 or E830 PHY */ > + case ICE_MAC_GENERIC_3K_E825: > + /* Do not reconfigure E810 or E830 PHY, but on link-up clear > + * any stale timestamp ready bits left over from a previous > + * link cycle. The PHY may have latched timestamps for packets > + * in flight when the link went down; these must be cleared > + * before new timestamp requests arrive. > + * > + * E810 does not have a real ready bitmap > + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so > + * skip clearing on E810 to avoid unnecessary sideband queue > + * operations for every not-in-use slot on each link-up. > + */ > + if (linkup && ptp_port->tx.has_ready_bitmap) { > + struct ice_ptp_tx *tx = &ptp_port->tx; > + u64 tstamp_ready; > + int i; > + > + if (ice_get_phy_tx_tstamp_ready(hw, tx->block, > + &tstamp_ready)) { > + dev_warn_ratelimited(ice_pf_to_dev(pf), > + "PTP failed to read Tx timestamp ready bitmap on link-up; stale PHY timestamps may remain and stall Tx timestamping\n"); > + } else { > + /* Only clear stale ready bits for slots that > + * have no in-flight software request. Iterating > + * the not-in-use slots skips any concurrent > + * ice_ptp_request_ts() allocation without > + * holding tx->lock across the PHY access. E830 > + * and E825 reach this clear; E810 is filtered > + * out above by has_ready_bitmap. > + */ > + for_each_clear_bit(i, tx->in_use, tx->len) { > + u8 phy_idx = i + tx->offset; > + > + if (tstamp_ready & BIT_ULL(phy_idx)) > + ice_clear_phy_tstamp(hw, > + tx->block, > + phy_idx); > + } > + } > + } > + > + /* E810 and E830 need no further PHY reconfiguration */ > + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825) > + return; > + > + /* E825 recovers its Tx path by soft resetting the PHY > + * timestamp block and restarting the port, but only on > + * link-up. The reset is a three-step register toggle; if it > + * fails partway through, the port can be left held in reset, > + * and programming a PHY that is stuck in reset via > + * ice_ptp_port_phy_restart() would leave Tx timestamping > + * permanently broken. So warn and skip the restart on > + * failure; the sequence is retried on the next link-up event. > + */ > + if (!linkup) > + return; > + > + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num)) > + dev_warn(ice_pf_to_dev(pf), > + "PTP failed to soft reset PHY port %u on link-up; skipping restart, Tx timestamping may be stuck, try toggle a link to recover\n", > + ptp_port->port_num); > + else > + ice_ptp_port_phy_restart(ptp_port); We hit a regression from this hunk while testing the series on an E825 T-BC system: after the soft reset the port no longer signals latched Tx timestamps. Nothing re-enables the port interrupt configuration (PHY_REG_TS_INT_CONFIG) afterwards, the only writers are the clock owner init and rebuild paths via ice_ptp_cfg_phy_interrupt(), and ice_ptp_port_phy_restart() does not touch it. So the first link-up that takes this path leaves the port without Tx timestamp interrupts until the next reset of the clock owner PF or a driver reload. Regards, Petr > return; > case ICE_MAC_GENERIC: > ice_ptp_port_phy_restart(ptp_port); > return; > - case ICE_MAC_GENERIC_3K_E825: > - if (linkup) > - ice_ptp_port_phy_restart(ptp_port); > - return; > default: > dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__); > } > @@ -3380,6 +3438,7 @@ void ice_ptp_init(struct ice_pf *pf) > > ptp->state = ICE_PTP_INITIALIZING; > > + mutex_init(&hw->ptp.tx_tstamp_lock); > if (hw->lane_num < 0) { > err = hw->lane_num; > goto err_exit; > @@ -3443,6 +3502,7 @@ void ice_ptp_init(struct ice_pf *pf) > > ice_ptp_cleanup_adapter(pf); > err_exit: > + mutex_destroy(&hw->ptp.tx_tstamp_lock); > /* If we registered a PTP clock, release it */ > if (pf->ptp.clock) { > ptp_clock_unregister(ptp->clock); > @@ -3469,6 +3529,7 @@ void ice_ptp_release(struct ice_pf *pf) > > if (pf->ptp.state != ICE_PTP_READY) { > mutex_destroy(&pf->ptp.port.ps_lock); > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > ice_ptp_cleanup_pf(pf); > ice_ptp_cleanup_adapter(pf); > if (pf->ptp.clock) { > @@ -3495,6 +3556,7 @@ void ice_ptp_release(struct ice_pf *pf) > > ice_ptp_port_phy_stop(&pf->ptp.port); > mutex_destroy(&pf->ptp.port.ps_lock); > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > if (pf->ptp.kworker) { > kthread_destroy_worker(pf->ptp.kworker); > pf->ptp.kworker = NULL; > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > index a1970a887c38..b23b68cd18ee 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > @@ -1160,11 +1160,15 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx, > * > * To directly clear the contents of the timestamp block entirely, discarding > * all timestamp data at once, software should instead use > - * ice_ptp_reset_ts_memory_quad_eth56g(). > + * ice_ptp_reset_ts_memory_eth56g(). > * > * This function should only be called on an idx whose bit is set according to > * ice_get_phy_tx_tstamp_ready(). > * > + * Serialized against ice_ptp_clear_tx_memory_status_eth56g() via > + * tx_tstamp_lock so the two paths do not interleave their reads of the same > + * port's Tx timestamp memory. > + * > * Return: > * * %0 - success > * * %other - failed to write to PHY > @@ -1175,25 +1179,62 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx) > u16 lo_addr; > int err; > > - /* Read the timestamp register to ensure the timestamp status bit is > - * cleared. > + lo_addr = (u16)PHY_TSTAMP_L(idx); > + > + mutex_lock(&hw->ptp.tx_tstamp_lock); > + > + /* Per the PHY spec, reading the timestamp memory location is what > + * clears the entry's valid bit and its corresponding (read-only) > + * ts_memory_status bit. This clears only this index, leaving any > + * other in-flight timestamps on the port untouched. > */ > err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > - if (err) { > + if (err) > ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n", > port, idx, err); > - } > - > - lo_addr = (u16)PHY_TSTAMP_L(idx); > > err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0); > - if (err) { > + if (err) > ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n", > port, idx, err); > - return err; > + > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > + > + return err; > +} > + > +/** > + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory > + * @hw: pointer to the HW struct > + * @port: port number to clear > + * > + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the > + * only way to clear a timestamp valid bit (and its read-only ts_memory_status > + * bit) is to read the timestamp memory location, so read every entry for the > + * port (two 32-bit reads each). This discards all timestamp data on the port, > + * so it must only be used for a full reset; callers that must preserve > + * in-flight timestamps clear individual indices via ice_clear_phy_tstamp(). > + * > + * Holds tx_tstamp_lock so this full-port sweep does not interleave with the > + * per-index reads in ice_clear_ptp_tstamp_eth56g() on the same port. > + * > + * Return: 0 on success, negative error code on failure to read the PHY. > + */ > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port) > +{ > + u64 unused_tstamp; > + int err = 0; > + u8 idx; > + > + mutex_lock(&hw->ptp.tx_tstamp_lock); > + for (idx = 0; idx < INDEX_PER_PORT; idx++) { > + err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > + if (err) > + break; > } > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > > - return 0; > + return err; > } > > /** > @@ -1204,12 +1245,8 @@ static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw) > { > unsigned int port; > > - for (port = 0; port < hw->ptp.num_lports; port++) { > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L, > - 0); > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U, > - 0); > - } > + for (port = 0; port < hw->ptp.num_lports; port++) > + ice_ptp_clear_tx_memory_status_eth56g(hw, port); > } > > /** > @@ -5277,6 +5314,23 @@ static void ice_read_phy_tstamp_e830(const struct ice_hw *hw, u8 idx, > FIELD_PREP(PHY_EXT_40B_LOW_M, lo); > } > > +/** > + * ice_clear_phy_tstamp_e830 - Clear a timestamp from the E830 PHY > + * @hw: pointer to the HW struct > + * @idx: the timestamp index to clear > + * > + * Clear the valid bit for the given timestamp index in the Tx memory (TS_MEM). > + * On E830 devices the PRTMAC_TS_TX_MEM_VALID_L/H registers are read-only > + * mirrors of the per-entry TX_VALID bits and cannot be written. The actual > + * TS_MEM entry's TX_VALID bit is cleared by reading the corresponding > + * PRTTSYN_TXTIME_L/H registers (read-to-clear). > + */ > +static void ice_clear_phy_tstamp_e830(const struct ice_hw *hw, u8 idx) > +{ > + rd32(hw, E830_PRTTSYN_TXTIME_H(idx)); > + rd32(hw, E830_PRTTSYN_TXTIME_L(idx)); > +} > + > /** > * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register > * @hw: pointer to the HW struct > @@ -5772,6 +5826,9 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) > switch (hw->mac_type) { > case ICE_MAC_E810: > return ice_clear_phy_tstamp_e810(hw, block, idx); > + case ICE_MAC_E830: > + ice_clear_phy_tstamp_e830(hw, idx); > + return 0; > case ICE_MAC_GENERIC: > return ice_clear_phy_tstamp_e82x(hw, block, idx); > case ICE_MAC_GENERIC_3K_E825: > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > index 16b1988e993d..b003e0aae8b1 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > @@ -304,6 +304,7 @@ int ice_ptp_clear_phy_offset_ready_e82x(struct ice_hw *hw); > int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp); > int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx); > void ice_ptp_reset_ts_memory(struct ice_hw *hw); > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port); > int ice_ptp_init_phc(struct ice_hw *hw); > void ice_ptp_init_hw(struct ice_hw *hw); > int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64 *tstamp_ready); > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > index 710c519d670d..d8c73fcafc25 100644 > --- a/drivers/net/ethernet/intel/ice/ice_type.h > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > @@ -888,6 +888,13 @@ enum ice_global_link_topo { > > struct ice_ptp_hw { > union ice_phy_params phy; > + /* Serializes eth56g Tx timestamp-memory reads (the per-index > + * timestamp entries, not the TX_MEMORY_STATUS registers). Reading an > + * entry auto-clears its ts_memory_status bit as a side effect, so the > + * per-index and full-port clear paths must not interleave on the same > + * port or a bit could re-latch the interrupt. > + */ > + struct mutex tx_tstamp_lock; > u8 num_lports; > u8 ports_per_phy; > }; ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY 2026-07-23 8:26 ` [Intel-wired-lan] " Petr Oros @ 2026-07-23 13:15 ` Korba, Przemyslaw 2026-07-27 8:21 ` Korba, Przemyslaw 0 siblings, 1 reply; 9+ messages in thread From: Korba, Przemyslaw @ 2026-07-23 13:15 UTC (permalink / raw) To: Oros, Petr, intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org, Kubalewski, Arkadiusz, Loktionov, Aleksandr, Nguyen, Anthony L, Kitszel, Przemyslaw, Ilichev, Konstantin > -----Original Message----- > From: Petr Oros <poros@redhat.com> > Sent: Thursday, July 23, 2026 10:27 AM > To: Korba, Przemyslaw <przemyslaw.korba@intel.com>; intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; > Ilichev, Konstantin <konstantin.ilichev@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY > > > On 7/20/26 14:01, Przemyslaw Korba wrote: > > From: Jacob Keller <jacob.e.keller@intel.com> > > > > Clear orphaned timestamp ready bits left in the PHY when a packet is > > timestamped just as the link goes down. The driver clears its software > > in_use bits during link-down cleanup, but the PHY has already latched > > the timestamp, so on the next link-up a stale ready bit from the > > previous link cycle remains set with no matching in_use entry. > > > > The PHY timestamp interrupt logic will not generate a new interrupt > > until ALL outstanding ready bits have been read. ice_ptp_process_tx_ > > tstamp() only iterates slots set in the software in_use bitmap, so it > > never reads these orphaned slots. The result is a permanent interrupt > > deadlock: > > > > 1. The PHY has ready bits set for slots with no in_use tracker entry > > 2. The driver never reads those slots because in_use is clear > > 3. The PHY refuses to generate new timestamp interrupts > > 4. All future Tx timestamps permanently fail > > 5. Only a power-on reset can recover the device > > > > Clear these stale ready bits on link-up, in ice_ptp_link_change(), > > before any new timestamp requests arrive, scoped to the affected port > > only so timestamps still pending on other ports are left untouched. For > > every affected MAC the clearing iterates the not-in_use slots with > > for_each_clear_bit(), so a pending software request is never discarded > > and tx->lock need not be held across the PHY access: > > > > - E810 is skipped (guarded by tx->has_ready_bitmap) because > > ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather > > than a real ready status. > > - E830 and E825/eth56g read the real Tx timestamp ready bitmap and > > clear only the slots the PHY actually latched, via > > ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the > > clear is skipped and a rate-limited warning is logged. > > > > On every MAC the ready bit is cleared by reading the slot's timestamp > > memory; the orphaned bits linger only because the normal processing path > > never reads those not-in_use slots. The link-up recovery clears them by > > reading the timestamp memory via ice_clear_phy_tstamp(): > > > > - E830: add ice_clear_phy_tstamp_e830(), which reads the > > PRTTSYN_TXTIME_H/L registers to clear the entry, and add an > > ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch. > > - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory > > location, which per the PHY spec is the operation that clears the > > entry's valid bit and its corresponding (read-only) ts_memory_status > > bit. The ts_memory_status registers cannot be written to clear a > > bit, so only reading the timestamp memory has any effect. > > > > The new for_each_clear_bit() loop runs from the service task via link > > events. During device removal, tx->in_use is freed before the service > > task is stopped. Fix by stopping the service task at the start of > > ice_unload(), before ice_ptp_release() frees tx->in_use. > > > > Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > > Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > > drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++-- > > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++---- > > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + > > drivers/net/ethernet/intel/ice/ice_type.h | 7 ++ > > 5 files changed, 149 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > > index e3d3810c791f..231d533309cb 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_main.c > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > > @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf) > > > > devl_assert_locked(priv_to_devlink(pf)); > > > > + ice_service_task_stop(pf); > > ice_unplug_aux_dev(pf); > > ice_deinit_rdma(pf); > > ice_deinit_features(pf); > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > > index 1aa440b0639f..9d9d9958fe5c 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup) > > switch (hw->mac_type) { > > case ICE_MAC_E810: > > case ICE_MAC_E830: > > - /* Do not reconfigure E810 or E830 PHY */ > > + case ICE_MAC_GENERIC_3K_E825: > > + /* Do not reconfigure E810 or E830 PHY, but on link-up clear > > + * any stale timestamp ready bits left over from a previous > > + * link cycle. The PHY may have latched timestamps for packets > > + * in flight when the link went down; these must be cleared > > + * before new timestamp requests arrive. > > + * > > + * E810 does not have a real ready bitmap > > + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so > > + * skip clearing on E810 to avoid unnecessary sideband queue > > + * operations for every not-in-use slot on each link-up. > > + */ > > + if (linkup && ptp_port->tx.has_ready_bitmap) { > > + struct ice_ptp_tx *tx = &ptp_port->tx; > > + u64 tstamp_ready; > > + int i; > > + > > + if (ice_get_phy_tx_tstamp_ready(hw, tx->block, > > + &tstamp_ready)) { > > + dev_warn_ratelimited(ice_pf_to_dev(pf), > > + "PTP failed to read Tx timestamp ready bitmap on link-up; stale PHY timestamps may > remain and stall Tx timestamping\n"); > > + } else { > > + /* Only clear stale ready bits for slots that > > + * have no in-flight software request. Iterating > > + * the not-in-use slots skips any concurrent > > + * ice_ptp_request_ts() allocation without > > + * holding tx->lock across the PHY access. E830 > > + * and E825 reach this clear; E810 is filtered > > + * out above by has_ready_bitmap. > > + */ > > + for_each_clear_bit(i, tx->in_use, tx->len) { > > + u8 phy_idx = i + tx->offset; > > + > > + if (tstamp_ready & BIT_ULL(phy_idx)) > > + ice_clear_phy_tstamp(hw, > > + tx->block, > > + phy_idx); > > + } > > + } > > + } > > + > > + /* E810 and E830 need no further PHY reconfiguration */ > > + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825) > > + return; > > + > > + /* E825 recovers its Tx path by soft resetting the PHY > > + * timestamp block and restarting the port, but only on > > + * link-up. The reset is a three-step register toggle; if it > > + * fails partway through, the port can be left held in reset, > > + * and programming a PHY that is stuck in reset via > > + * ice_ptp_port_phy_restart() would leave Tx timestamping > > + * permanently broken. So warn and skip the restart on > > + * failure; the sequence is retried on the next link-up event. > > + */ > > + if (!linkup) > > + return; > > + > > + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num)) > > + dev_warn(ice_pf_to_dev(pf), > > + "PTP failed to soft reset PHY port %u on link-up; skipping restart, Tx timestamping may be stuck, try toggle a > link to recover\n", > > + ptp_port->port_num); > > + else > > + ice_ptp_port_phy_restart(ptp_port); > > We hit a regression from this hunk while testing the series on an > E825 T-BC system: after the soft reset the port no longer signals > latched Tx timestamps. Nothing re-enables the port interrupt > configuration (PHY_REG_TS_INT_CONFIG) afterwards, the only writers > are the clock owner init and rebuild paths via > ice_ptp_cfg_phy_interrupt(), and ice_ptp_port_phy_restart() does not > touch it. So the first link-up that takes this path leaves the port > without Tx timestamp interrupts until the next reset of the clock > owner PF or a driver reload. > > Regards, > > Petr Hi, thanks for letting me know. I will look into it. > > > return; > > case ICE_MAC_GENERIC: > > ice_ptp_port_phy_restart(ptp_port); > > return; > > - case ICE_MAC_GENERIC_3K_E825: > > - if (linkup) > > - ice_ptp_port_phy_restart(ptp_port); > > - return; > > default: > > dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__); > > } > > @@ -3380,6 +3438,7 @@ void ice_ptp_init(struct ice_pf *pf) > > > > ptp->state = ICE_PTP_INITIALIZING; > > > > + mutex_init(&hw->ptp.tx_tstamp_lock); > > if (hw->lane_num < 0) { > > err = hw->lane_num; > > goto err_exit; > > @@ -3443,6 +3502,7 @@ void ice_ptp_init(struct ice_pf *pf) > > > > ice_ptp_cleanup_adapter(pf); > > err_exit: > > + mutex_destroy(&hw->ptp.tx_tstamp_lock); > > /* If we registered a PTP clock, release it */ > > if (pf->ptp.clock) { > > ptp_clock_unregister(ptp->clock); > > @@ -3469,6 +3529,7 @@ void ice_ptp_release(struct ice_pf *pf) > > > > if (pf->ptp.state != ICE_PTP_READY) { > > mutex_destroy(&pf->ptp.port.ps_lock); > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > > ice_ptp_cleanup_pf(pf); > > ice_ptp_cleanup_adapter(pf); > > if (pf->ptp.clock) { > > @@ -3495,6 +3556,7 @@ void ice_ptp_release(struct ice_pf *pf) > > > > ice_ptp_port_phy_stop(&pf->ptp.port); > > mutex_destroy(&pf->ptp.port.ps_lock); > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > > if (pf->ptp.kworker) { > > kthread_destroy_worker(pf->ptp.kworker); > > pf->ptp.kworker = NULL; > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > index a1970a887c38..b23b68cd18ee 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > @@ -1160,11 +1160,15 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx, > > * > > * To directly clear the contents of the timestamp block entirely, discarding > > * all timestamp data at once, software should instead use > > - * ice_ptp_reset_ts_memory_quad_eth56g(). > > + * ice_ptp_reset_ts_memory_eth56g(). > > * > > * This function should only be called on an idx whose bit is set according to > > * ice_get_phy_tx_tstamp_ready(). > > * > > + * Serialized against ice_ptp_clear_tx_memory_status_eth56g() via > > + * tx_tstamp_lock so the two paths do not interleave their reads of the same > > + * port's Tx timestamp memory. > > + * > > * Return: > > * * %0 - success > > * * %other - failed to write to PHY > > @@ -1175,25 +1179,62 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx) > > u16 lo_addr; > > int err; > > > > - /* Read the timestamp register to ensure the timestamp status bit is > > - * cleared. > > + lo_addr = (u16)PHY_TSTAMP_L(idx); > > + > > + mutex_lock(&hw->ptp.tx_tstamp_lock); > > + > > + /* Per the PHY spec, reading the timestamp memory location is what > > + * clears the entry's valid bit and its corresponding (read-only) > > + * ts_memory_status bit. This clears only this index, leaving any > > + * other in-flight timestamps on the port untouched. > > */ > > err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > > - if (err) { > > + if (err) > > ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n", > > port, idx, err); > > - } > > - > > - lo_addr = (u16)PHY_TSTAMP_L(idx); > > > > err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0); > > - if (err) { > > + if (err) > > ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n", > > port, idx, err); > > - return err; > > + > > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > > + > > + return err; > > +} > > + > > +/** > > + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory > > + * @hw: pointer to the HW struct > > + * @port: port number to clear > > + * > > + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the > > + * only way to clear a timestamp valid bit (and its read-only ts_memory_status > > + * bit) is to read the timestamp memory location, so read every entry for the > > + * port (two 32-bit reads each). This discards all timestamp data on the port, > > + * so it must only be used for a full reset; callers that must preserve > > + * in-flight timestamps clear individual indices via ice_clear_phy_tstamp(). > > + * > > + * Holds tx_tstamp_lock so this full-port sweep does not interleave with the > > + * per-index reads in ice_clear_ptp_tstamp_eth56g() on the same port. > > + * > > + * Return: 0 on success, negative error code on failure to read the PHY. > > + */ > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port) > > +{ > > + u64 unused_tstamp; > > + int err = 0; > > + u8 idx; > > + > > + mutex_lock(&hw->ptp.tx_tstamp_lock); > > + for (idx = 0; idx < INDEX_PER_PORT; idx++) { > > + err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > > + if (err) > > + break; > > } > > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > > > > - return 0; > > + return err; > > } > > > > /** > > @@ -1204,12 +1245,8 @@ static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw) > > { > > unsigned int port; > > > > - for (port = 0; port < hw->ptp.num_lports; port++) { > > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L, > > - 0); > > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U, > > - 0); > > - } > > + for (port = 0; port < hw->ptp.num_lports; port++) > > + ice_ptp_clear_tx_memory_status_eth56g(hw, port); > > } > > > > /** > > @@ -5277,6 +5314,23 @@ static void ice_read_phy_tstamp_e830(const struct ice_hw *hw, u8 idx, > > FIELD_PREP(PHY_EXT_40B_LOW_M, lo); > > } > > > > +/** > > + * ice_clear_phy_tstamp_e830 - Clear a timestamp from the E830 PHY > > + * @hw: pointer to the HW struct > > + * @idx: the timestamp index to clear > > + * > > + * Clear the valid bit for the given timestamp index in the Tx memory (TS_MEM). > > + * On E830 devices the PRTMAC_TS_TX_MEM_VALID_L/H registers are read-only > > + * mirrors of the per-entry TX_VALID bits and cannot be written. The actual > > + * TS_MEM entry's TX_VALID bit is cleared by reading the corresponding > > + * PRTTSYN_TXTIME_L/H registers (read-to-clear). > > + */ > > +static void ice_clear_phy_tstamp_e830(const struct ice_hw *hw, u8 idx) > > +{ > > + rd32(hw, E830_PRTTSYN_TXTIME_H(idx)); > > + rd32(hw, E830_PRTTSYN_TXTIME_L(idx)); > > +} > > + > > /** > > * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register > > * @hw: pointer to the HW struct > > @@ -5772,6 +5826,9 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) > > switch (hw->mac_type) { > > case ICE_MAC_E810: > > return ice_clear_phy_tstamp_e810(hw, block, idx); > > + case ICE_MAC_E830: > > + ice_clear_phy_tstamp_e830(hw, idx); > > + return 0; > > case ICE_MAC_GENERIC: > > return ice_clear_phy_tstamp_e82x(hw, block, idx); > > case ICE_MAC_GENERIC_3K_E825: > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > index 16b1988e993d..b003e0aae8b1 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > @@ -304,6 +304,7 @@ int ice_ptp_clear_phy_offset_ready_e82x(struct ice_hw *hw); > > int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp); > > int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx); > > void ice_ptp_reset_ts_memory(struct ice_hw *hw); > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port); > > int ice_ptp_init_phc(struct ice_hw *hw); > > void ice_ptp_init_hw(struct ice_hw *hw); > > int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64 *tstamp_ready); > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > > index 710c519d670d..d8c73fcafc25 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > @@ -888,6 +888,13 @@ enum ice_global_link_topo { > > > > struct ice_ptp_hw { > > union ice_phy_params phy; > > + /* Serializes eth56g Tx timestamp-memory reads (the per-index > > + * timestamp entries, not the TX_MEMORY_STATUS registers). Reading an > > + * entry auto-clears its ts_memory_status bit as a side effect, so the > > + * per-index and full-port clear paths must not interleave on the same > > + * port or a bit could re-latch the interrupt. > > + */ > > + struct mutex tx_tstamp_lock; > > u8 num_lports; > > u8 ports_per_phy; > > }; ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY 2026-07-23 13:15 ` Korba, Przemyslaw @ 2026-07-27 8:21 ` Korba, Przemyslaw 2026-07-27 22:46 ` Jacob Keller 0 siblings, 1 reply; 9+ messages in thread From: Korba, Przemyslaw @ 2026-07-27 8:21 UTC (permalink / raw) To: Oros, Petr, intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org, Kubalewski, Arkadiusz, Loktionov, Aleksandr, Nguyen, Anthony L, Kitszel, Przemyslaw, Ilichev, Konstantin > -----Original Message----- > From: Korba, Przemyslaw > Sent: Thursday, July 23, 2026 3:16 PM > To: 'Petr Oros' <poros@redhat.com>; intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; Kubalewski, Arkadiusz <Arkadiusz.Kubalewski@intel.com>; Loktionov, Aleksandr > <Aleksandr.Loktionov@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; > Ilichev, Konstantin <konstantin.ilichev@intel.com> > Subject: RE: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY > > > > > > -----Original Message----- > > From: Petr Oros <poros@redhat.com> > > Sent: Thursday, July 23, 2026 10:27 AM > > To: Korba, Przemyslaw <przemyslaw.korba@intel.com>; intel-wired-lan@lists.osuosl.org > > Cc: netdev@vger.kernel.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Loktionov, Aleksandr > > <aleksandr.loktionov@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; > > Ilichev, Konstantin <konstantin.ilichev@intel.com> > > Subject: Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY > > > > > > On 7/20/26 14:01, Przemyslaw Korba wrote: > > > From: Jacob Keller <jacob.e.keller@intel.com> > > > > > > Clear orphaned timestamp ready bits left in the PHY when a packet is > > > timestamped just as the link goes down. The driver clears its software > > > in_use bits during link-down cleanup, but the PHY has already latched > > > the timestamp, so on the next link-up a stale ready bit from the > > > previous link cycle remains set with no matching in_use entry. > > > > > > The PHY timestamp interrupt logic will not generate a new interrupt > > > until ALL outstanding ready bits have been read. ice_ptp_process_tx_ > > > tstamp() only iterates slots set in the software in_use bitmap, so it > > > never reads these orphaned slots. The result is a permanent interrupt > > > deadlock: > > > > > > 1. The PHY has ready bits set for slots with no in_use tracker entry > > > 2. The driver never reads those slots because in_use is clear > > > 3. The PHY refuses to generate new timestamp interrupts > > > 4. All future Tx timestamps permanently fail > > > 5. Only a power-on reset can recover the device > > > > > > Clear these stale ready bits on link-up, in ice_ptp_link_change(), > > > before any new timestamp requests arrive, scoped to the affected port > > > only so timestamps still pending on other ports are left untouched. For > > > every affected MAC the clearing iterates the not-in_use slots with > > > for_each_clear_bit(), so a pending software request is never discarded > > > and tx->lock need not be held across the PHY access: > > > > > > - E810 is skipped (guarded by tx->has_ready_bitmap) because > > > ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather > > > than a real ready status. > > > - E830 and E825/eth56g read the real Tx timestamp ready bitmap and > > > clear only the slots the PHY actually latched, via > > > ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the > > > clear is skipped and a rate-limited warning is logged. > > > > > > On every MAC the ready bit is cleared by reading the slot's timestamp > > > memory; the orphaned bits linger only because the normal processing path > > > never reads those not-in_use slots. The link-up recovery clears them by > > > reading the timestamp memory via ice_clear_phy_tstamp(): > > > > > > - E830: add ice_clear_phy_tstamp_e830(), which reads the > > > PRTTSYN_TXTIME_H/L registers to clear the entry, and add an > > > ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch. > > > - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory > > > location, which per the PHY spec is the operation that clears the > > > entry's valid bit and its corresponding (read-only) ts_memory_status > > > bit. The ts_memory_status registers cannot be written to clear a > > > bit, so only reading the timestamp memory has any effect. > > > > > > The new for_each_clear_bit() loop runs from the service task via link > > > events. During device removal, tx->in_use is freed before the service > > > task is stopped. Fix by stopping the service task at the start of > > > ice_unload(), before ice_ptp_release() frees tx->in_use. > > > > > > Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com> > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > > > Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> > > > --- > > > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > > > drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++-- > > > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++---- > > > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + > > > drivers/net/ethernet/intel/ice/ice_type.h | 7 ++ > > > 5 files changed, 149 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > > > index e3d3810c791f..231d533309cb 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_main.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > > > @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf) > > > > > > devl_assert_locked(priv_to_devlink(pf)); > > > > > > + ice_service_task_stop(pf); > > > ice_unplug_aux_dev(pf); > > > ice_deinit_rdma(pf); > > > ice_deinit_features(pf); > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > > > index 1aa440b0639f..9d9d9958fe5c 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > > @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup) > > > switch (hw->mac_type) { > > > case ICE_MAC_E810: > > > case ICE_MAC_E830: > > > - /* Do not reconfigure E810 or E830 PHY */ > > > + case ICE_MAC_GENERIC_3K_E825: > > > + /* Do not reconfigure E810 or E830 PHY, but on link-up clear > > > + * any stale timestamp ready bits left over from a previous > > > + * link cycle. The PHY may have latched timestamps for packets > > > + * in flight when the link went down; these must be cleared > > > + * before new timestamp requests arrive. > > > + * > > > + * E810 does not have a real ready bitmap > > > + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so > > > + * skip clearing on E810 to avoid unnecessary sideband queue > > > + * operations for every not-in-use slot on each link-up. > > > + */ > > > + if (linkup && ptp_port->tx.has_ready_bitmap) { > > > + struct ice_ptp_tx *tx = &ptp_port->tx; > > > + u64 tstamp_ready; > > > + int i; > > > + > > > + if (ice_get_phy_tx_tstamp_ready(hw, tx->block, > > > + &tstamp_ready)) { > > > + dev_warn_ratelimited(ice_pf_to_dev(pf), > > > + "PTP failed to read Tx timestamp ready bitmap on link-up; stale PHY timestamps may > > remain and stall Tx timestamping\n"); > > > + } else { > > > + /* Only clear stale ready bits for slots that > > > + * have no in-flight software request. Iterating > > > + * the not-in-use slots skips any concurrent > > > + * ice_ptp_request_ts() allocation without > > > + * holding tx->lock across the PHY access. E830 > > > + * and E825 reach this clear; E810 is filtered > > > + * out above by has_ready_bitmap. > > > + */ > > > + for_each_clear_bit(i, tx->in_use, tx->len) { > > > + u8 phy_idx = i + tx->offset; > > > + > > > + if (tstamp_ready & BIT_ULL(phy_idx)) > > > + ice_clear_phy_tstamp(hw, > > > + tx->block, > > > + phy_idx); > > > + } > > > + } > > > + } > > > + > > > + /* E810 and E830 need no further PHY reconfiguration */ > > > + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825) > > > + return; > > > + > > > + /* E825 recovers its Tx path by soft resetting the PHY > > > + * timestamp block and restarting the port, but only on > > > + * link-up. The reset is a three-step register toggle; if it > > > + * fails partway through, the port can be left held in reset, > > > + * and programming a PHY that is stuck in reset via > > > + * ice_ptp_port_phy_restart() would leave Tx timestamping > > > + * permanently broken. So warn and skip the restart on > > > + * failure; the sequence is retried on the next link-up event. > > > + */ > > > + if (!linkup) > > > + return; > > > + > > > + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num)) > > > + dev_warn(ice_pf_to_dev(pf), > > > + "PTP failed to soft reset PHY port %u on link-up; skipping restart, Tx timestamping may be stuck, try toggle a > > link to recover\n", > > > + ptp_port->port_num); > > > + else > > > + ice_ptp_port_phy_restart(ptp_port); > > > > We hit a regression from this hunk while testing the series on an > > E825 T-BC system: after the soft reset the port no longer signals > > latched Tx timestamps. Nothing re-enables the port interrupt > > configuration (PHY_REG_TS_INT_CONFIG) afterwards, the only writers > > are the clock owner init and rebuild paths via > > ice_ptp_cfg_phy_interrupt(), and ice_ptp_port_phy_restart() does not > > touch it. So the first link-up that takes this path leaves the port > > without Tx timestamp interrupts until the next reset of the clock > > owner PF or a driver reload. > > > > Regards, > > > > Petr > > Hi, thanks for letting me know. I will look into it. Hi, If I am not mistaken, Jake Keller sent you a patch fixing the issue with this patch regarding PHY reconfiguration. Did you have a chance to test this? If so, I will implement this fix in the new revision. > > > > > > return; > > > case ICE_MAC_GENERIC: > > > ice_ptp_port_phy_restart(ptp_port); > > > return; > > > - case ICE_MAC_GENERIC_3K_E825: > > > - if (linkup) > > > - ice_ptp_port_phy_restart(ptp_port); > > > - return; > > > default: > > > dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__); > > > } > > > @@ -3380,6 +3438,7 @@ void ice_ptp_init(struct ice_pf *pf) > > > > > > ptp->state = ICE_PTP_INITIALIZING; > > > > > > + mutex_init(&hw->ptp.tx_tstamp_lock); > > > if (hw->lane_num < 0) { > > > err = hw->lane_num; > > > goto err_exit; > > > @@ -3443,6 +3502,7 @@ void ice_ptp_init(struct ice_pf *pf) > > > > > > ice_ptp_cleanup_adapter(pf); > > > err_exit: > > > + mutex_destroy(&hw->ptp.tx_tstamp_lock); > > > /* If we registered a PTP clock, release it */ > > > if (pf->ptp.clock) { > > > ptp_clock_unregister(ptp->clock); > > > @@ -3469,6 +3529,7 @@ void ice_ptp_release(struct ice_pf *pf) > > > > > > if (pf->ptp.state != ICE_PTP_READY) { > > > mutex_destroy(&pf->ptp.port.ps_lock); > > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > > > ice_ptp_cleanup_pf(pf); > > > ice_ptp_cleanup_adapter(pf); > > > if (pf->ptp.clock) { > > > @@ -3495,6 +3556,7 @@ void ice_ptp_release(struct ice_pf *pf) > > > > > > ice_ptp_port_phy_stop(&pf->ptp.port); > > > mutex_destroy(&pf->ptp.port.ps_lock); > > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock); > > > if (pf->ptp.kworker) { > > > kthread_destroy_worker(pf->ptp.kworker); > > > pf->ptp.kworker = NULL; > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > index a1970a887c38..b23b68cd18ee 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > > > @@ -1160,11 +1160,15 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx, > > > * > > > * To directly clear the contents of the timestamp block entirely, discarding > > > * all timestamp data at once, software should instead use > > > - * ice_ptp_reset_ts_memory_quad_eth56g(). > > > + * ice_ptp_reset_ts_memory_eth56g(). > > > * > > > * This function should only be called on an idx whose bit is set according to > > > * ice_get_phy_tx_tstamp_ready(). > > > * > > > + * Serialized against ice_ptp_clear_tx_memory_status_eth56g() via > > > + * tx_tstamp_lock so the two paths do not interleave their reads of the same > > > + * port's Tx timestamp memory. > > > + * > > > * Return: > > > * * %0 - success > > > * * %other - failed to write to PHY > > > @@ -1175,25 +1179,62 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx) > > > u16 lo_addr; > > > int err; > > > > > > - /* Read the timestamp register to ensure the timestamp status bit is > > > - * cleared. > > > + lo_addr = (u16)PHY_TSTAMP_L(idx); > > > + > > > + mutex_lock(&hw->ptp.tx_tstamp_lock); > > > + > > > + /* Per the PHY spec, reading the timestamp memory location is what > > > + * clears the entry's valid bit and its corresponding (read-only) > > > + * ts_memory_status bit. This clears only this index, leaving any > > > + * other in-flight timestamps on the port untouched. > > > */ > > > err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > > > - if (err) { > > > + if (err) > > > ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n", > > > port, idx, err); > > > - } > > > - > > > - lo_addr = (u16)PHY_TSTAMP_L(idx); > > > > > > err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0); > > > - if (err) { > > > + if (err) > > > ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n", > > > port, idx, err); > > > - return err; > > > + > > > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > > > + > > > + return err; > > > +} > > > + > > > +/** > > > + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory > > > + * @hw: pointer to the HW struct > > > + * @port: port number to clear > > > + * > > > + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the > > > + * only way to clear a timestamp valid bit (and its read-only ts_memory_status > > > + * bit) is to read the timestamp memory location, so read every entry for the > > > + * port (two 32-bit reads each). This discards all timestamp data on the port, > > > + * so it must only be used for a full reset; callers that must preserve > > > + * in-flight timestamps clear individual indices via ice_clear_phy_tstamp(). > > > + * > > > + * Holds tx_tstamp_lock so this full-port sweep does not interleave with the > > > + * per-index reads in ice_clear_ptp_tstamp_eth56g() on the same port. > > > + * > > > + * Return: 0 on success, negative error code on failure to read the PHY. > > > + */ > > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port) > > > +{ > > > + u64 unused_tstamp; > > > + int err = 0; > > > + u8 idx; > > > + > > > + mutex_lock(&hw->ptp.tx_tstamp_lock); > > > + for (idx = 0; idx < INDEX_PER_PORT; idx++) { > > > + err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp); > > > + if (err) > > > + break; > > > } > > > + mutex_unlock(&hw->ptp.tx_tstamp_lock); > > > > > > - return 0; > > > + return err; > > > } > > > > > > /** > > > @@ -1204,12 +1245,8 @@ static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw) > > > { > > > unsigned int port; > > > > > > - for (port = 0; port < hw->ptp.num_lports; port++) { > > > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L, > > > - 0); > > > - ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U, > > > - 0); > > > - } > > > + for (port = 0; port < hw->ptp.num_lports; port++) > > > + ice_ptp_clear_tx_memory_status_eth56g(hw, port); > > > } > > > > > > /** > > > @@ -5277,6 +5314,23 @@ static void ice_read_phy_tstamp_e830(const struct ice_hw *hw, u8 idx, > > > FIELD_PREP(PHY_EXT_40B_LOW_M, lo); > > > } > > > > > > +/** > > > + * ice_clear_phy_tstamp_e830 - Clear a timestamp from the E830 PHY > > > + * @hw: pointer to the HW struct > > > + * @idx: the timestamp index to clear > > > + * > > > + * Clear the valid bit for the given timestamp index in the Tx memory (TS_MEM). > > > + * On E830 devices the PRTMAC_TS_TX_MEM_VALID_L/H registers are read-only > > > + * mirrors of the per-entry TX_VALID bits and cannot be written. The actual > > > + * TS_MEM entry's TX_VALID bit is cleared by reading the corresponding > > > + * PRTTSYN_TXTIME_L/H registers (read-to-clear). > > > + */ > > > +static void ice_clear_phy_tstamp_e830(const struct ice_hw *hw, u8 idx) > > > +{ > > > + rd32(hw, E830_PRTTSYN_TXTIME_H(idx)); > > > + rd32(hw, E830_PRTTSYN_TXTIME_L(idx)); > > > +} > > > + > > > /** > > > * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register > > > * @hw: pointer to the HW struct > > > @@ -5772,6 +5826,9 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) > > > switch (hw->mac_type) { > > > case ICE_MAC_E810: > > > return ice_clear_phy_tstamp_e810(hw, block, idx); > > > + case ICE_MAC_E830: > > > + ice_clear_phy_tstamp_e830(hw, idx); > > > + return 0; > > > case ICE_MAC_GENERIC: > > > return ice_clear_phy_tstamp_e82x(hw, block, idx); > > > case ICE_MAC_GENERIC_3K_E825: > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > > index 16b1988e993d..b003e0aae8b1 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > > @@ -304,6 +304,7 @@ int ice_ptp_clear_phy_offset_ready_e82x(struct ice_hw *hw); > > > int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp); > > > int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx); > > > void ice_ptp_reset_ts_memory(struct ice_hw *hw); > > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port); > > > int ice_ptp_init_phc(struct ice_hw *hw); > > > void ice_ptp_init_hw(struct ice_hw *hw); > > > int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64 *tstamp_ready); > > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h > > > index 710c519d670d..d8c73fcafc25 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_type.h > > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h > > > @@ -888,6 +888,13 @@ enum ice_global_link_topo { > > > > > > struct ice_ptp_hw { > > > union ice_phy_params phy; > > > + /* Serializes eth56g Tx timestamp-memory reads (the per-index > > > + * timestamp entries, not the TX_MEMORY_STATUS registers). Reading an > > > + * entry auto-clears its ts_memory_status bit as a side effect, so the > > > + * per-index and full-port clear paths must not interleave on the same > > > + * port or a bit could re-latch the interrupt. > > > + */ > > > + struct mutex tx_tstamp_lock; > > > u8 num_lports; > > > u8 ports_per_phy; > > > }; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY 2026-07-27 8:21 ` Korba, Przemyslaw @ 2026-07-27 22:46 ` Jacob Keller 0 siblings, 0 replies; 9+ messages in thread From: Jacob Keller @ 2026-07-27 22:46 UTC (permalink / raw) To: Korba, Przemyslaw, Oros, Petr, intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org, Kubalewski, Arkadiusz, Loktionov, Aleksandr, Nguyen, Anthony L, Kitszel, Przemyslaw, Ilichev, Konstantin On 7/27/2026 1:21 AM, Korba, Przemyslaw wrote: > > > >> -----Original Message----- >> From: Korba, Przemyslaw >> Sent: Thursday, July 23, 2026 3:16 PM >> To: 'Petr Oros' <poros@redhat.com>; intel-wired-lan@lists.osuosl.org >> Cc: netdev@vger.kernel.org; Kubalewski, Arkadiusz <Arkadiusz.Kubalewski@intel.com>; Loktionov, Aleksandr >> <Aleksandr.Loktionov@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; >> Ilichev, Konstantin <konstantin.ilichev@intel.com> >> Subject: RE: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY >> >> >> >> >>> -----Original Message----- >>> From: Petr Oros <poros@redhat.com> >>> Sent: Thursday, July 23, 2026 10:27 AM >>> To: Korba, Przemyslaw <przemyslaw.korba@intel.com>; intel-wired-lan@lists.osuosl.org >>> Cc: netdev@vger.kernel.org; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Loktionov, Aleksandr >>> <aleksandr.loktionov@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; >>> Ilichev, Konstantin <konstantin.ilichev@intel.com> >>> Subject: Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY >>> >>> >>> On 7/20/26 14:01, Przemyslaw Korba wrote: >>>> From: Jacob Keller <jacob.e.keller@intel.com> >>>> >>>> Clear orphaned timestamp ready bits left in the PHY when a packet is >>>> timestamped just as the link goes down. The driver clears its software >>>> in_use bits during link-down cleanup, but the PHY has already latched >>>> the timestamp, so on the next link-up a stale ready bit from the >>>> previous link cycle remains set with no matching in_use entry. >>>> >>>> The PHY timestamp interrupt logic will not generate a new interrupt >>>> until ALL outstanding ready bits have been read. ice_ptp_process_tx_ >>>> tstamp() only iterates slots set in the software in_use bitmap, so it >>>> never reads these orphaned slots. The result is a permanent interrupt >>>> deadlock: >>>> >>>> 1. The PHY has ready bits set for slots with no in_use tracker entry >>>> 2. The driver never reads those slots because in_use is clear >>>> 3. The PHY refuses to generate new timestamp interrupts >>>> 4. All future Tx timestamps permanently fail >>>> 5. Only a power-on reset can recover the device >>>> >>>> Clear these stale ready bits on link-up, in ice_ptp_link_change(), >>>> before any new timestamp requests arrive, scoped to the affected port >>>> only so timestamps still pending on other ports are left untouched. For >>>> every affected MAC the clearing iterates the not-in_use slots with >>>> for_each_clear_bit(), so a pending software request is never discarded >>>> and tx->lock need not be held across the PHY access: >>>> >>>> - E810 is skipped (guarded by tx->has_ready_bitmap) because >>>> ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather >>>> than a real ready status. >>>> - E830 and E825/eth56g read the real Tx timestamp ready bitmap and >>>> clear only the slots the PHY actually latched, via >>>> ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the >>>> clear is skipped and a rate-limited warning is logged. >>>> >>>> On every MAC the ready bit is cleared by reading the slot's timestamp >>>> memory; the orphaned bits linger only because the normal processing path >>>> never reads those not-in_use slots. The link-up recovery clears them by >>>> reading the timestamp memory via ice_clear_phy_tstamp(): >>>> >>>> - E830: add ice_clear_phy_tstamp_e830(), which reads the >>>> PRTTSYN_TXTIME_H/L registers to clear the entry, and add an >>>> ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch. >>>> - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory >>>> location, which per the PHY spec is the operation that clears the >>>> entry's valid bit and its corresponding (read-only) ts_memory_status >>>> bit. The ts_memory_status registers cannot be written to clear a >>>> bit, so only reading the timestamp memory has any effect. >>>> >>>> The new for_each_clear_bit() loop runs from the service task via link >>>> events. During device removal, tx->in_use is freed before the service >>>> task is stopped. Fix by stopping the service task at the start of >>>> ice_unload(), before ice_ptp_release() frees tx->in_use. >>>> >>>> Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com> >>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> >>>> --- >>>> drivers/net/ethernet/intel/ice/ice_main.c | 1 + >>>> drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++-- >>>> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++---- >>>> drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + >>>> drivers/net/ethernet/intel/ice/ice_type.h | 7 ++ >>>> 5 files changed, 149 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c >>>> index e3d3810c791f..231d533309cb 100644 >>>> --- a/drivers/net/ethernet/intel/ice/ice_main.c >>>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c >>>> @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf) >>>> >>>> devl_assert_locked(priv_to_devlink(pf)); >>>> >>>> + ice_service_task_stop(pf); >>>> ice_unplug_aux_dev(pf); >>>> ice_deinit_rdma(pf); >>>> ice_deinit_features(pf); >>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c >>>> index 1aa440b0639f..9d9d9958fe5c 100644 >>>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c >>>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c >>>> @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup) >>>> switch (hw->mac_type) { >>>> case ICE_MAC_E810: >>>> case ICE_MAC_E830: >>>> - /* Do not reconfigure E810 or E830 PHY */ >>>> + case ICE_MAC_GENERIC_3K_E825: >>>> + /* Do not reconfigure E810 or E830 PHY, but on link-up clear >>>> + * any stale timestamp ready bits left over from a previous >>>> + * link cycle. The PHY may have latched timestamps for packets >>>> + * in flight when the link went down; these must be cleared >>>> + * before new timestamp requests arrive. >>>> + * >>>> + * E810 does not have a real ready bitmap >>>> + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so >>>> + * skip clearing on E810 to avoid unnecessary sideband queue >>>> + * operations for every not-in-use slot on each link-up. >>>> + */ >>>> + if (linkup && ptp_port->tx.has_ready_bitmap) { >>>> + struct ice_ptp_tx *tx = &ptp_port->tx; >>>> + u64 tstamp_ready; >>>> + int i; >>>> + >>>> + if (ice_get_phy_tx_tstamp_ready(hw, tx->block, >>>> + &tstamp_ready)) { >>>> + dev_warn_ratelimited(ice_pf_to_dev(pf), >>>> + "PTP failed to read Tx timestamp ready bitmap on link-up; stale PHY timestamps may >>> remain and stall Tx timestamping\n"); >>>> + } else { >>>> + /* Only clear stale ready bits for slots that >>>> + * have no in-flight software request. Iterating >>>> + * the not-in-use slots skips any concurrent >>>> + * ice_ptp_request_ts() allocation without >>>> + * holding tx->lock across the PHY access. E830 >>>> + * and E825 reach this clear; E810 is filtered >>>> + * out above by has_ready_bitmap. >>>> + */ >>>> + for_each_clear_bit(i, tx->in_use, tx->len) { >>>> + u8 phy_idx = i + tx->offset; >>>> + >>>> + if (tstamp_ready & BIT_ULL(phy_idx)) >>>> + ice_clear_phy_tstamp(hw, >>>> + tx->block, >>>> + phy_idx); >>>> + } >>>> + } >>>> + } >>>> + >>>> + /* E810 and E830 need no further PHY reconfiguration */ >>>> + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825) >>>> + return; >>>> + >>>> + /* E825 recovers its Tx path by soft resetting the PHY >>>> + * timestamp block and restarting the port, but only on >>>> + * link-up. The reset is a three-step register toggle; if it >>>> + * fails partway through, the port can be left held in reset, >>>> + * and programming a PHY that is stuck in reset via >>>> + * ice_ptp_port_phy_restart() would leave Tx timestamping >>>> + * permanently broken. So warn and skip the restart on >>>> + * failure; the sequence is retried on the next link-up event. >>>> + */ >>>> + if (!linkup) >>>> + return; >>>> + >>>> + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num)) >>>> + dev_warn(ice_pf_to_dev(pf), >>>> + "PTP failed to soft reset PHY port %u on link-up; skipping restart, Tx timestamping may be stuck, try toggle a >>> link to recover\n", >>>> + ptp_port->port_num); >>>> + else >>>> + ice_ptp_port_phy_restart(ptp_port); >>> >>> We hit a regression from this hunk while testing the series on an >>> E825 T-BC system: after the soft reset the port no longer signals >>> latched Tx timestamps. Nothing re-enables the port interrupt >>> configuration (PHY_REG_TS_INT_CONFIG) afterwards, the only writers >>> are the clock owner init and rebuild paths via >>> ice_ptp_cfg_phy_interrupt(), and ice_ptp_port_phy_restart() does not >>> touch it. So the first link-up that takes this path leaves the port >>> without Tx timestamp interrupts until the next reset of the clock >>> owner PF or a driver reload. >>> >>> Regards, >>> >>> Petr >> >> Hi, thanks for letting me know. I will look into it. > > Hi, > If I am not mistaken, Jake Keller sent you a patch fixing the issue with this patch regarding PHY reconfiguration. > Did you have a chance to test this? If so, I will implement this fix in the new revision. > We have a patch undergoing testing, but I am not confident it will fix the issue. I believe we have a gap here where the ice_ptp_init and link state can race such that the PTP state shows the link is down, but we never follow up by calling ice_ptp_link_change on link going up. I'm still trying to investigate the full path, but I think the patch in this series which removes the call of ice_ptp_link_change from ice_down and ice_up is possibly the root cause here. I am not certain what a correct fix looks like yet, and Petr and I are still working on reproducing and confirming the situation. Thanks, Jake ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH iwl-net 3/4] ice: cancel E82x offset verification work during reset preparation 2026-07-20 12:01 [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 1/4] ice: keep Tx timestamps disabled until offset calibration completes Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY Przemyslaw Korba @ 2026-07-20 12:01 ` Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 4/4] ice: call PTP link change only from link events Przemyslaw Korba 3 siblings, 0 replies; 9+ messages in thread From: Przemyslaw Korba @ 2026-07-20 12:01 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, arkadiusz.kubalewski, konstantin.ilichev, Przemyslaw Korba From: Karol Kolacinski <karol.kolacinski@intel.com> Cancel the offset verification delayed work (ov_work) during PTP reset preparation to prevent it from running concurrently with the reset sequence. Without this, ice_ptp_wait_for_offsets() can execute during a reset, find that ice_is_reset_in_progress() is true, and re-queue itself in a tight loop. Meanwhile, the reset path in ice_ptp_rebuild_owner() calls ice_ptp_port_phy_restart(), which starts a new ov_work. This results in two ov_work instances running simultaneously, racing over the PHY offset registers and the calibrating flag. Cancel ov_work in ice_ptp_prepare_for_reset() alongside the existing cancellation of the Tx tracker. The comment in the existing code already documents that ov_work interference during reset is a concern but only cancels it during ice_ptp_release(). Fixes: 4809671015a1 ("ice: Fix E810 PTP reset flow") Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 9d9d9958fe5c..e0be88024ac3 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -3042,6 +3042,14 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type) if (reset_type == ICE_RESET_PFR) return; + /* Cancel the offset verification work for E82x before releasing the + * Tx tracker. If ov_work is running during reset, it may issue + * sideband queue commands that will fail or timeout, and may + * reference state that is being torn down. + */ + if (hw->mac_type == ICE_MAC_GENERIC) + kthread_cancel_delayed_work_sync(&ptp->port.ov_work); + if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825) ice_ptp_prepare_rebuild_sec(pf, false, reset_type); -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH iwl-net 4/4] ice: call PTP link change only from link events 2026-07-20 12:01 [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues Przemyslaw Korba ` (2 preceding siblings ...) 2026-07-20 12:01 ` [PATCH iwl-net 3/4] ice: cancel E82x offset verification work during reset preparation Przemyslaw Korba @ 2026-07-20 12:01 ` Przemyslaw Korba 3 siblings, 0 replies; 9+ messages in thread From: Przemyslaw Korba @ 2026-07-20 12:01 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, arkadiusz.kubalewski, konstantin.ilichev, Przemyslaw Korba From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Remove redundant ice_ptp_link_change() calls from ice_up_complete() and ice_down(). These duplicate the call already made from ice_handle_link_event(), creating three problems: 1. Double initialization on link-up: ice_handle_link_event() calls ice_ptp_link_change(true), then ice_up_complete() calls it again. The second call re-enters ice_ptp_port_phy_restart(), re-setting the calibrating flag and restarting the PHY timer while the first invocation's offset verification work (ov_work) may still be running. 2. Premature cleanup on administrative down: ice_down() calls ice_ptp_link_change(false) during ifconfig down or reset preparation, even when the physical link is still up. This clears timestamp state unnecessarily and can interfere with ongoing PTP operations. 3. Ordering dependency: ice_down()/ice_up_complete() are called during reset sequences where PTP may not be fully initialized, creating edge cases with partially configured state. The link event handler is the correct and sufficient place to drive PTP link state changes, as it reflects actual physical link transitions. Add the link_up to be set within ice_ptp_init(..), it is required to have actual state of link_up flag on driver load. Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void") Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 2 -- drivers/net/ethernet/intel/ice/ice_ptp.c | 39 +++++++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 231d533309cb..8c25c1604949 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -6782,7 +6782,6 @@ static int ice_up_complete(struct ice_vsi *vsi) ice_print_link_msg(vsi, true); netif_tx_start_all_queues(vsi->netdev); netif_carrier_on(vsi->netdev); - ice_ptp_link_change(pf, true); } /* Perform an initial read of the statistics registers now to @@ -7310,7 +7309,6 @@ int ice_down(struct ice_vsi *vsi) if (vsi->netdev) { vlan_err = ice_vsi_del_vlan_zero(vsi); - ice_ptp_link_change(vsi->back, false); netif_carrier_off(vsi->netdev); netif_tx_disable(vsi->netdev); } diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index e0be88024ac3..e1c8bc5fafc1 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -3343,9 +3343,13 @@ static int ice_ptp_init_owner(struct ice_pf *pf) } /** - * ice_ptp_init_work - Initialize PTP work threads + * ice_ptp_init_work - Initialize the PTP kworker * @pf: Board private structure * @ptp: PF PTP structure + * + * Allocate the kworker and initialize the periodic work function. The + * periodic work is not queued here; the caller starts it once the PTP + * state is ICE_PTP_READY. */ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp) { @@ -3364,9 +3368,6 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp) ptp->kworker = kworker; - /* Start periodic work going */ - kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0); - return 0; } @@ -3489,6 +3490,29 @@ void ice_ptp_init(struct ice_pf *pf) if (err) goto err_clean_pf; + /* Seed the cached PTP link state from the current PHY link status + * before starting the PHY timestamping block. On a fresh load the + * port structure is zeroed (link_up == false); if the link is + * already up at probe (for example after a PXE boot) no link-change + * edge will follow, and ice_ptp_link_change() is edge-driven, so the + * flag would stay stale. Seeding it here also lets + * ice_ptp_reset_phy_timestamping() make the correct start/stop + * decision instead of stopping the PHY timer on an already-up link. + */ + if (pf->hw.port_info) + ptp->port.link_up = + !!(pf->hw.port_info->phy.link_info.link_info & + ICE_AQ_LINK_UP); + + /* Create the PTP kworker before (re)starting the PHY, because the + * E82x restart path queues offset verification work on it, and + * before ICE_PTP_READY is set, so a concurrent link event cannot + * reach ice_ptp_port_phy_restart() while the kworker is still NULL. + */ + err = ice_ptp_init_work(pf, ptp); + if (err) + goto err_exit; + /* Start the PHY timestamping block */ ice_ptp_reset_phy_timestamping(pf); @@ -3497,9 +3521,10 @@ void ice_ptp_init(struct ice_pf *pf) ptp->state = ICE_PTP_READY; - err = ice_ptp_init_work(pf, ptp); - if (err) - goto err_exit; + /* Start periodic work only after the state is READY; the worker + * returns without rescheduling while the state is not READY. + */ + kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0); dev_info(ice_pf_to_dev(pf), "PTP init successful\n"); return; -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-07-27 22:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-20 12:01 [PATCH iwl-net 0/4] ice: fix PTP timestamp handling issues Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 1/4] ice: keep Tx timestamps disabled until offset calibration completes Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY Przemyslaw Korba 2026-07-23 8:26 ` [Intel-wired-lan] " Petr Oros 2026-07-23 13:15 ` Korba, Przemyslaw 2026-07-27 8:21 ` Korba, Przemyslaw 2026-07-27 22:46 ` Jacob Keller 2026-07-20 12:01 ` [PATCH iwl-net 3/4] ice: cancel E82x offset verification work during reset preparation Przemyslaw Korba 2026-07-20 12:01 ` [PATCH iwl-net 4/4] ice: call PTP link change only from link events Przemyslaw Korba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox