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 <[email protected]>
Reviewed-by: Marcin Szycik <[email protected]>
+ 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(¶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",
Why the '?', are we not certain it's in progress?
> + __func__);
> + spin_unlock_irqrestore(¶ms->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(¶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 <[email protected]>
>
>
