Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix E810 low latency timestamp interrupt handling

[Marcin Szycik <marcin.szycik@linux.intel.com>]

On 29/05/2026 22:22, Jacob Keller wrote:
> 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>

Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
+ some nits

> ---
> 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 */

Nit: no comma

> +};
>  
>  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",

Why the '?', are we not certain it's in progress?

> +			__func__);
> +		spin_unlock_irqrestore(&params->atqbal_wq.lock, flags);
> +		return;

Nit: could be an unroll.

> +	}
>  
>  	/* 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>
> 
>