Re: [PATCH net 14/15] ice: don't clear in_use until HW clears ready bitmap

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: don't clear in_use until HW clears ready bitmap

On E825 the PHY can leave the Tx timestamp ready bit and valid bit set
around a link down transition, so this change re-reads the ready bitmap
after each PHY timestamp read and marks the index stale if it is still
set. It also moves the hardware ready bitmap check out of
ice_ptp_tx_tstamps_pending() into the watchdog and stops re-arming the
interrupt while the tracker is calibrating or link is down.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index e060de5026621..832cc6ad2f90c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

@@ -620,6 +620,19 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 		if (err && !drop_ts)
 			continue;
 
+		/* verify ready bit cleared */
+		if (tx->has_ready_bitmap) {
+			err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
+			if (err || tstamp_ready & BIT_ULL(phy_idx)) {
+				spin_lock_irqsave(&tx->lock, flags);
+				if (!test_and_set_bit(idx, tx->stale))
+					dev_dbg(ice_pf_to_dev(pf), "PHY port %u failed to clear ready bit for idx %u\n",
+						ptp_port->port_num, phy_idx);
+				spin_unlock_irqrestore(&tx->lock, flags);
+				continue;
+			}
+		}
+

[Severity: High]
Should this new gate honour drop_ts the way the ready-bit gate just above
it does?

The earlier gate in ice_ptp_process_tx_tstamp() jumps to skip_ts_read when
the 2 second deadline has already passed:

	if (tx->has_ready_bitmap &&
	    !(tstamp_ready & BIT_ULL(phy_idx))) {
		if (drop_ts)
			goto skip_ts_read;

		continue;
	}

The new block instead does a plain continue, so for an index whose ready
bit stays set the skip_ts_read block is never reached:

skip_ts_read:
		spin_lock_irqsave(&tx->lock, flags);
		...
		clear_bit(idx, tx->in_use);
		skb = tx->tstamps[idx].skb;
		tx->tstamps[idx].skb = NULL;

That is the only place in this function that clears in_use, detaches the
SKB and later calls dev_kfree_skb_any() on the reference taken by
skb_get() in ice_ptp_request_ts(). Does this mean the 2 second timeout
reclaim no longer works for exactly the stuck-ready-bit case the patch
targets, holding up to INDEX_PER_PORT (64) SKBs per port, each with
SKBTX_IN_PROGRESS still set and each pinning its socket, for as long as
the condition lasts?

On this path ice_ptp_link_change() only calls
ice_ptp_mark_tx_tracker_stale() for ICE_MAC_GENERIC_3K_E825 and does not
flush, and ice_ptp_flush_tx_tracker() is only reached from
ice_ptp_release_tx_tracker() and ice_ptp_flush_all_tx_tracker(), so is
recovery dependent on a link-up event that may never arrive while a cable
stays unplugged?

There is a second effect from the same continue. Because the entry is
never released and tx->tstamps[idx].start is never refreshed, this block
in the same loop runs again on every pass:

		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
			drop_ts = true;

			/* Count the number of Tx timestamps that timed out */
			pf->ptp.tx_hwtstamp_timeouts++;
		}

With the watchdog seeing ice_check_phy_tx_tstamp_ready() > 0 and
re-triggering the IRQ every 500 ms, does tx_hwtstamp_timeouts grow by
roughly two per second per stuck index without any SKB actually being
discarded? That counter is user visible both as ethtool -S
tx_hwtstamp_timeouts in ice_gstrings_pf_stats and as the standard
ts_stats lost field filled in by ice_ptp_get_ts_stats(), and it is
documented as the number of Tx skbs discarded with no time stamp.

quoted

 		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
 
 		/* For PHYs which don't implement a proper timestamp ready

@@ -2768,10 +2781,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
 		if (!tx->init)
 			return false;
 
-		if (in_irq)
+		if (in_irq) {
+			if (!ice_ptp_is_tx_tracker_up(tx))
+				return false;
+
 			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
-		else
+		} else {
 			return !bitmap_empty(tx->in_use, tx->len);
+		}
 	}
 }
 

@@ -2794,41 +2811,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
 
 bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
-	struct ice_hw *hw = &pf->hw;
-	int ret;
-
-	/* Check software indicator */
 	switch (pf->ptp.tx_interrupt_mode) {
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		return false;
 	case ICE_PTP_TX_INTERRUPT_SELF:
-		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
-			return true;
-		break;
+		return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq);
 	case ICE_PTP_TX_INTERRUPT_ALL:
-		if (ice_any_port_has_timestamps(pf, in_irq))
-			return true;
-		break;
+		return ice_any_port_has_timestamps(pf, in_irq);
 	default:
 		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n",
 			  pf->ptp.tx_interrupt_mode);
-		break;
-	}
-
-	/* Check hardware indicator */
-	ret = ice_check_phy_tx_tstamp_ready(hw);
-	if (ret < 0) {
-		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
-			ret);
-		/* Stop triggering IRQs if we're unable to read PHY */
 		return false;
 	}
-
-	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
-	 * available, 0 if there are no waiting timestamps, and a negative
-	 * value if there was an error (which we checked for above).
-	 */
-	return ret > 0;
 }

[Severity: High]
After this change, which code path still consults the hardware ready
bitmap for an E830 PF that does not own the source timer?

ice_ptp_init_tx_interrupt_mode() assigns ICE_PTP_TX_INTERRUPT_SELF to
E810 and E830 PFs in the default arm of its switch, owner or not. The
hardware check has moved into ice_ptp_maybe_trigger_tx_interrupt(), which
returns before reaching it:

	if (!ice_pf_src_tmr_owned(pf))
		return;

	ret = ice_check_phy_tx_tstamp_ready(hw);

So for a non-owner E830 PF, both remaining re-arm sites,
ice_ptp_ts_irq() (case ICE_MAC_E830) and ice_misc_intr_thread_fn(), now
decide purely from the software tracker, and that decision was further
narrowed by the new ice_ptp_is_tx_tracker_up() early return and by
masking stale bits with bitmap_andnot().

The commit message says:

    Continue to check the hardware ready bitmap from the watchdog to catch
    cases of unexpected timestamps.

Does that hold only for clock-owner PFs? The kernel-doc of
ice_ptp_maybe_trigger_tx_interrupt() states the hardware will not
interrupt again until all current timestamp data is read, so does a valid
bit that software never reads block all further Tx timestamp interrupts on
that PF until a reset?

Two ways such an orphaned bit looks reachable on E830:

  - link down runs ice_ptp_mark_tx_tracker_stale(), which ORs stale over
    in_use and clears link_up, so the in_irq check returns false while the
    hardware bits for in-flight captures get set and are never read.

  - ice_ptp_flush_tx_tracker() clears in_use for an entry whose capture
    completes after the 10 ms drain wait, and ice_clear_phy_tstamp() has
    no E830 case (-EOPNOTSUPP), so the valid bit stays set with no
    software owner.

E810 looks unaffected since ice_check_phy_tx_tstamp_ready_e810() always
returns 0, and E82x uses the ALL/NONE modes where the owner still runs the
watchdog.

I think we need to change the flow to re-run a check from watchdog. We
really cannot continue to drop and release the index until we get a
proper clear from hardware, otherwise we could open up to that index
appearing valid for another timestamp. At least on E825 we have
confirmation that timestamps get "stuck" even before the software knows
link is down so we could accidentally re-use such an index.

I'd rather block the index indefinitely until we get a clear from
hardware. But that only works if we actually re-check from watchdog
which needs to be done properly for all devices.

I believe this will be addressed by one of the changes to address
comments from the other patches

quoted

 
 /**

@@ -2912,6 +2906,7 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
+	int ret;
 
 	if (!pf->ptp.port.tx.has_ready_bitmap)
 		return;

@@ -2919,7 +2914,15 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 	if (!ice_pf_src_tmr_owned(pf))
 		return;
 
-	if (ice_ptp_tx_tstamps_pending(pf, false)) {
+	ret = ice_check_phy_tx_tstamp_ready(hw);
+	if (ret < 0) {
+		dev_dbg(dev, "Unable to read PHY Tx timestamp ready bitmap, err %pe\n",
+			ERR_PTR(ret));
+		/* Don't trigger an IRQ if we are unable to access the PHY */
+		return;
+	}
+
+	if (ret > 0 || ice_ptp_tx_tstamps_pending(pf, false)) {
 		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
 
 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

[Severity: Medium]
One more question on a related patch in this series rather than on this
hunk. The patch "ice: fix removal of PTP timestamp tracker during reset"
dropped the ice_ptp_release_tx_tracker() call from
ice_ptp_prepare_for_reset(), so the tracker now survives a reset.

If ice_ptp_rebuild() then fails, it takes the err label:

err:
	ptp->state = ICE_PTP_ERROR;
	dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);

A later ice_ptp_rebuild() cannot recover, since the state is neither
READY nor RESETTING and it returns -EINVAL. On unload, ice_ptp_release()
takes the abbreviated branch:

	if (pf->ptp.state != ICE_PTP_READY) {
		mutex_destroy(&pf->ptp.port.ps_lock);
		ice_ptp_cleanup_pf(pf);
		if (pf->ptp.clock) {
			ptp_clock_unregister(pf->ptp.clock);
			pf->ptp.clock = NULL;
		}
		return;
	}

which never calls ice_ptp_release_tx_tracker(). Does this leak the
tstamps array and the two bitmaps allocated in
ice_ptp_alloc_tx_tracker(), along with any SKB references still held in
tx->tstamps[].skb, since ice_ptp_flush_tx_tracker() is not reached
either? Would releasing the tracker in the non-READY branch of
ice_ptp_release(), or on the ice_ptp_rebuild() error path, be
appropriate?

I am going to clean this up on the error path of ice_ptp_rebuild().