The driver has supported a low latency timestamp interface since commit 82e71b226e0e ("ice: Enable SW interrupt from FW for LL TS"). This interface is triggered by calling ice_ptp_req_tx_single_tstamp(), which is called by ice_ptp_ts_irq(), which in turn is called when the driver gets an interrupt from hardware indicating that a timestamp is available. This function doesn't check if a timestamp request is already in progress, and as a result could trash an existing outstanding requests when called. It turns out that this is unlikely in practice due to a number of circumstances that prevent most of the ways that could happen. 1. The ice_misc_intr_thread_fn() might trigger a software-generated interrupt with the PFINT_OICR_TSYN_TX flag. However, we don't enter the thread function since ice_ptp_ts_irq() always returns IRQ_HANDLED for E810 devices which support the low latency firmware interface. 2. The ice_ptp_maybe_trigger_tx_interrupt() function might trigger a software-generated interrupt if it detects waiting timetstamps. However it checks ptp.port.tx.has_ready_bitmap which is always false for E810, so never enters the code path. However, it is still possible that another Tx timestamp request could happen and complete and race with the firmware completing the outstanding low latency timestamp request. This doesn't happen often in practice because many applications only trigger a single outstanding Tx timestamp at once. However, if the user runs multiple copies of ptp4l or uses other userspace stack which does, they might miss timestamps or get corrupted timestamp data. To fix this, have the ice_ptp_req_tX_single_tstamp() function check and only begin the operation if the ATQBAL_FLAGS_INTR_IN_PROGRESS flag was not yet set. This prevents a new possible request from trashing an outstanding request. Note that on completion of a request, the ice_ll_ts_intr() function will initiate a request for the next outstanding timestamp, so no timestamps will be lost. Additionally, although the ice_ptp_tx_tstamps_pending() function doesn't currently get called for E810 devices, it should still not return true for devices which support the low latency interrupt. If for some reason code is refactored and the miscellaneous thread function does execute, it should not trigger a new software interrupt for devices using the low latency interrupt interface. Add an explicit check to make this function always return false when the device is operating in this mode. Finally, convert the atqbal_flags to DECLARE_BITMAP and use test/set bit functions. This helps in clarity as we can use test_and_set_bit and test_and_clear_bit. Fixes: 82e71b226e0e ("ice: Enable SW interrupt from FW for LL TS") Signed-off-by: Jacob Keller --- This was originally motivated by changes in our out-of-tree release where the issue of a software-generated interrupt was causing significantly more issues. After further investigation it seems that the upstream implementation is more robust, preventing the thread function from running for E810. However, there appears to be a small window where issues can crop up if multiple outstanding timestamps are requested near concurrently. This change is motivated at closing that gap and ensuring consistency of timestamps returned through the low latency interface. To trigger the issue you will need to issue multiple Tx timestamp requests near but not quite simultaneously, and it may be quite a rare race condition. --- drivers/net/ethernet/intel/ice/ice_type.h | 8 ++++++-- drivers/net/ethernet/intel/ice/ice_ptp.c | 24 +++++++++++++++++++----- drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 ++++++------ 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index 1e82f4c40b32..7035ea6c59db 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -859,12 +859,16 @@ struct ice_mbx_data { #define ICE_PORTS_PER_QUAD 4 #define ICE_GET_QUAD_NUM(port) ((port) / ICE_PORTS_PER_QUAD) -#define ATQBAL_FLAGS_INTR_IN_PROGRESS BIT(0) +enum ice_atqbal_flags { + ATQBAL_FLAGS_INTR_IN_PROGRESS, + + ATQBAL_FLAGS_NBITS, /* must be last */ +}; struct ice_e810_params { /* The wait queue lock also protects the low latency interface */ wait_queue_head_t atqbal_wq; - unsigned int atqbal_flags; + DECLARE_BITMAP(atqbal_flags, ATQBAL_FLAGS_NBITS); }; struct ice_eth56g_params { diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 36df742c326c..11059deb5d41 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -382,6 +382,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx) struct ice_ptp_port *ptp_port; unsigned long flags; struct sk_buff *skb; + struct device *dev; struct ice_pf *pf; if (!tx->init) @@ -389,6 +390,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx) ptp_port = container_of(tx, struct ice_ptp_port, tx); pf = ptp_port_to_pf(ptp_port); + dev = ice_pf_to_dev(pf); params = &pf->hw.ptp.phy.e810; /* Drop packets which have waited for more than 2 seconds */ @@ -408,7 +410,13 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx) spin_lock_irqsave(¶ms->atqbal_wq.lock, flags); - params->atqbal_flags |= ATQBAL_FLAGS_INTR_IN_PROGRESS; + if (test_and_set_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS, + params->atqbal_flags)) { + dev_dbg(dev, "%s: low latency interrupt request already in progress?\n", + __func__); + spin_unlock_irqrestore(¶ms->atqbal_wq.lock, flags); + return; + } /* Write TS index to read to the PF register so the FW can read it */ wr32(&pf->hw, REG_LL_PROXY_H, @@ -449,7 +457,8 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx) spin_lock_irqsave(¶ms->atqbal_wq.lock, flags); - if (!(params->atqbal_flags & ATQBAL_FLAGS_INTR_IN_PROGRESS)) + if (!test_and_clear_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS, + params->atqbal_flags)) dev_dbg(dev, "%s: low latency interrupt request not in progress?\n", __func__); @@ -459,8 +468,6 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx) reg_ll_high = rd32(&pf->hw, REG_LL_PROXY_H); /* Wake up threads waiting on low latency interface */ - params->atqbal_flags &= ~ATQBAL_FLAGS_INTR_IN_PROGRESS; - wake_up_locked(¶ms->atqbal_wq); spin_unlock_irqrestore(¶ms->atqbal_wq.lock, flags); @@ -2712,7 +2719,14 @@ bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf) struct ice_hw *hw = &pf->hw; int ret; - /* Check software indicator */ + /* E810 devices with support for the low latency timestamp interrupt + * have specialized handling for timestamps. They should not + * re-schedule the miscellaneous interrupt. + */ + if (hw->mac_type == ICE_MAC_E810 && + hw->dev_caps.ts_dev_info.ts_ll_int_read) + return false; + switch (pf->ptp.tx_interrupt_mode) { case ICE_PTP_TX_INTERRUPT_NONE: return false; diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index 2c18e16fe053..02d4cc942c8d 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -4521,8 +4521,8 @@ ice_read_phy_tstamp_ll_e810(struct ice_hw *hw, u8 idx, u8 *hi, u32 *lo) /* Wait for any pending in-progress low latency interrupt */ err = wait_event_interruptible_locked_irq(params->atqbal_wq, - !(params->atqbal_flags & - ATQBAL_FLAGS_INTR_IN_PROGRESS)); + !test_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS, + params->atqbal_flags)); if (err) { spin_unlock_irq(¶ms->atqbal_wq.lock); return err; @@ -4754,8 +4754,8 @@ static int ice_ptp_prep_phy_adj_ll_e810(struct ice_hw *hw, s32 adj) /* Wait for any pending in-progress low latency interrupt */ err = wait_event_interruptible_locked_irq(params->atqbal_wq, - !(params->atqbal_flags & - ATQBAL_FLAGS_INTR_IN_PROGRESS)); + !test_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS, + params->atqbal_flags)); if (err) { spin_unlock_irq(¶ms->atqbal_wq.lock); return err; @@ -4846,8 +4846,8 @@ static int ice_ptp_prep_phy_incval_ll_e810(struct ice_hw *hw, u64 incval) /* Wait for any pending in-progress low latency interrupt */ err = wait_event_interruptible_locked_irq(params->atqbal_wq, - !(params->atqbal_flags & - ATQBAL_FLAGS_INTR_IN_PROGRESS)); + !test_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS, + params->atqbal_flags)); if (err) { spin_unlock_irq(¶ms->atqbal_wq.lock); return err; --- base-commit: 2412591cfe66e681374c5265e691695cd913d099 change-id: 20260528-jk-fix-e810-ll-interface-function-5dad155217d8 Best regards, -- Jacob Keller