[PATCH iwl-net] ice: fix E810 low latency timestamp interrupt handling

[PATCH iwl-net] ice: fix E810 low latency timestamp interrupt handling

[Jacob Keller]

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 <jacob.e.keller@intel.com>
---
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(&params->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(&params->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(&params->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(&params->atqbal_wq);
 
 	spin_unlock_irqrestore(&params->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(&params->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(&params->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(&params->atqbal_wq.lock);
 		return err;

---
base-commit: 2412591cfe66e681374c5265e691695cd913d099
change-id: 20260528-jk-fix-e810-ll-interface-function-5dad155217d8

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>