Re: [PATCH iwl-net 11/12] ice: skip reading Tx ready bitmap on ports with no timestamps

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

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> On E82x devices, the interrupt for Tx timestamps are handled by the clock
> owner. When an interrupt with the Tx timestamp cause is fired, the clock
> owner PF iterates the list of ports and checks for timestamps across all
> ports.
> 
> The existing logic reads the PHY timestamp ready bitmap before iterating
> the list of in-use timestamp indexes, even for ports which have no
> timestamps waiting in the software timestamp tracker. This has a
> significant and measurable latency impact on reporting Tx timestamps.
> 
> Check the bitmap and exit early in the event that there are no timestamps
> waiting on a port. Observant reviewers may notice that the check is done
> without acquiring the lock. This is fine, as the only thread that can clear
> in_use bits is the miscellaneous interrupt handler. Whether the thread sees
> or fails to see a new outstanding timestamp does not affect correctness,
> only determining whether or not it should do extra work.
> 
> Using the ice Tx timestamp traces, with a simple ptp4l setup the average
> latency appears to be around 175 to 200 microseconds with a few outliers
> taking hundreds of microseconds to be reported. With the check to skip
> empty bitmaps (and thus skip reading the ready bitmap for inactive ports),
> the average latency drops ~50 microseconds.
> 
> Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 8c2dd83d8d6c..b337247c94e0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -573,7 +573,7 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  	pf = ptp_port_to_pf(ptp_port);
>  	hw = &pf->hw;
>  
> -	if (!tx->init)
> +	if (!tx->init || bitmap_empty(tx->in_use, tx->len))
>  		return;
>  
>  	/* Read the Tx ready status first */
> 

Sashiko says:

> This is a pre-existing issue, but could this lockless check of tx->in_use
> result in a use-after-free or NULL pointer dereference during a cross-PF
> teardown?
> In ice_ptp_release_tx_tracker(), a secondary PF undergoing reset sets
> tx->init = 0 and frees tx->in_use:
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_release_tx_tracker() {
>     tx->init = 0;
>     spin_unlock_irqrestore(&tx->lock, flags);
>     /* wait for potentially outstanding interrupt to complete */
>     synchronize_irq(pf->oicr_irq.virq);
>     ...
>     bitmap_free(tx->in_use);
>     tx->in_use = NULL;
> }
> Since synchronize_irq() only waits on the resetting PF's IRQ, it doesn't
> block the clock owner PF which runs on a different IRQ.
> Can the clock owner PF running ice_ptp_process_tx_tstamp() evaluate
> tx->init as true, get preempted, and then dereference tx->in_use after
> it has been freed by the resetting PF?


I believe this will be fixed by the patch which removes the call to stop
clearing the tracker except on load. I will investigate if we need any
further change as there may be a similar issue with teardown. We might
need to synchronize against the clock owner IRQ for ports using the
INTERRUPT_ALL mode.

> This is also a pre-existing issue, but does adding this early return prevent
> the driver from recovering if a hardware timestamp takes too long to arrive?
> When a timestamp request takes longer than 2 seconds, the software drops it
> and clears its index from tx->in_use:
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_process_tx_tstamp() {
>     ...
>     if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
>         drop_ts = true;
>     ...
> skip_ts_read:
>     ...
>     clear_bit(idx, tx->in_use);
> }
> If the hardware subsequently completes this dropped timestamp, the ready bit
> will assert. However, with tx->in_use now being empty, the early return
> prevents the driver from calling ice_read_phy_tstamp().
> As noted in the driver comments in this same function, failing to read valid
> PHY timestamps can cause the hardware interrupt generation logic to become
> permanently stuck on some devices. Should this path ensure orphaned timestamps
> are still read and cleared from PHY memory?

If hardware somehow holds onto a timestamp for longer than 2 seconds the
logic we have already fails, but we need some cut off. It *is* possible
that a timestamp never happens if it occurs near a link event. We have
no way to be informed by hardware that it won't complete a timestamp. If
we do nothing the more common case of a missed timestamp would lock the
index indefinitely.

The assumption being made here is that 2 seconds is sufficient time to
be certain the hardware will no longer complete the timestamp. I don't
think we can make the software robust in both ways, and have to make
some trade off here.