Re: [PATCH iwl-net 08/12] ice: wait for in-flight Tx timestamps before flushing the tracker

[Jacob Keller <jacob.e.keller@intel.com>]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> From: Petr Oros <poros@redhat.com>
> 
> ice_ptp_flush_tx_tracker() frees every tracked request, but a request
> whose timestamp is still being captured by the PHY at that moment is
> freed without touching the PHY entry. The ready bit published shortly
> after has no tracked owner, and the PHY does not raise another Tx
> timestamp interrupt until every outstanding ready bit is read, so
> delivery for the whole quad degrades to the periodic work.
> 
> Wait up to 10 ms for in-flight captures to publish their ready bits
> before flushing, so the flush clears them together with the rest.
> 
> Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
> Signed-off-by: Petr Oros <poros@redhat.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 8aa49dda90a2..a049dc7a2241 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -740,6 +740,37 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
>  	return 0;
>  }
>  
> +static void
> +ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(10);
> +	struct ice_hw *hw = &pf->hw;
> +	u64 tstamp_ready;
> +	bool pending;
> +	u8 idx;
> +
> +	if (hw->reset_ongoing)
> +		return;
> +
> +	do {
> +		if (ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready))
> +			return;
> +
> +		pending = false;
> +		for_each_set_bit(idx, tx->in_use, tx->len) {
> +			if (!(tstamp_ready & BIT_ULL(idx + tx->offset)))
> +				pending = true;


Sashiko says:

> 
> This isn't a bug introduced by this patch, but does this loop risk a NULL
> pointer dereference if tx->in_use is NULL?
> If ice_ptp_alloc_tx_tracker() fails due to memory pressure, it leaves
> tx->in_use as NULL while tx->len > 0.
> If a concurrent PF rebuild then calls ice_ptp_flush_all_tx_tracker(), it
> will iterate over the list, find this partially initialized port, and call
> ice_ptp_flush_tx_tracker(). This now calls ice_ptp_wait_for_tracker_drain()
> which would dereference tx->in_use here.


I am not sure about this one. I think if we fail to allocate the in_use
array we check and bail out and fail to initialize PTP, leaving the PTP
state in ICE_PTP_UNINIT.

ice_ptp_rebuild_owner does iterate the port list.. And if we fail to
initialize the PTP port it will get removed from the list. I guess it
may be that there is a small window where a concurrent rebuild could
cause a problem before it gets out of the teardown?

We could possibly delay inserting the port into the list until a later
step or have the loop check of PTP initialized before continuing here. Hmm.

I'll try to address this in v2, likely as part of the first patch with
the kref and RCU.

> +		}
> +		if (!pending)
> +			return;
> +
> +		usleep_range(500, 1000);
> +	} while (time_before(jiffies, deadline));
> +

Sashiko says:

> This is a pre-existing issue, but does adding this sleep here exacerbate an
> RCU use-after-free when iterating PTP ports during teardown?
> In ice_ptp_flush_all_tx_tracker(), the code drops rcu_read_lock(), calls
> ice_ptp_flush_tx_tracker() (which now sleeps here for up to 10ms), and calls
> kref_put() on the current port.
> If the last reference is dropped, a blocked teardown thread could wake up,
> complete synchronize_rcu(), and free the port.
> When ice_ptp_flush_all_tx_tracker() re-acquires the lock, it will evaluate
> port->list_node.next and dereference the freed port.

I think this will be fixed by correcting the locking and kref use of the
first patch, ensuring that we can't release the port in this way.


> +	dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
> +		tx->block);
> +}
> +
>  /**
>   * ice_ptp_flush_tx_tracker - Flush any remaining timestamps from the tracker
>   * @pf: Board private structure
> @@ -756,6 +787,8 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>  	int err;
>  	u8 idx;
>  
> +	ice_ptp_wait_for_tracker_drain(pf, tx);
> +
>  	err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
>  	if (err) {
>  		dev_dbg(ice_pf_to_dev(pf), "Failed to get the Tx tstamp ready bitmap for block %u, err %d\n",
> 

Finally, sashiko says:

> This is a pre-existing issue, but does Tx timestamping get permanently
> disabled after a non-PFR reset?
> When a non-PFR reset (like CORER or GLOBR) is triggered,
> ice_ptp_prepare_for_reset() calls ice_ptp_release_tx_tracker(). This frees
> tx->tstamps, tx->in_use, and tx->stale, setting tx->len = 0.
> However, the rebuild function ice_ptp_rebuild() doesn't seem to call
> ice_ptp_init_tx() to reallocate these resources, meaning subsequent Tx
> timestamp requests would silently fail.

This is a bug in commit 7a25fe5cd5fb ("ice: stop destroying and
reinitalizing Tx tracker during reset") which tried to remove the
destruction of the Tx tracker during reset. Unfortunately it looks like
that commit didn't actually remove the actual destruction of the tracker.

I'll include a fix for this in v2.