Re: [PATCH net v3 11/14] ice: wait for in-flight Tx timestamps before removing the tracker

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

On 10/19/2026 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] On E830, the new drain wait runs, but the flush that follows
>   cannot clear the ready entries it waited for.
> - [Medium] The tracker is released while the PF's Tx path is still live,
>   and the new drain wait gives up after 10 ms.
> 
> --
> 
> ice: wait for in-flight Tx timestamps before removing the tracker
> 
> This patch adds ice_ptp_wait_for_tracker_drain(). It polls the PHY Tx
> timestamp ready bitmap for up to 10 ms, until every in-use tracker index
> reports ready. ice_ptp_release_tx_tracker() now calls it before
> ice_ptp_flush_tx_tracker(), so in-flight captures get cleared instead of
> being left as orphaned ready bits, and E810 is skipped via
> tx->has_ready_bitmap.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 32be908373c30..ca581490f3edb 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -751,6 +751,68 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
> 
> [ ... ]
> 
>> +static void
>> +ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
>> +{
>> +	bool drained;
>> +	int err;
>> +
>> +	if (!tx->has_ready_bitmap)
>> +		return;
> 
> [Severity: Medium]
> Does this wait do anything useful on E830? ice_ptp_init_tx() sets:
> 

E830 should be doing something to clear timestamps, and its an oversight
and pre-existing issue that it doesn't. This should be an issue
addressed by a follow up patch and not this series.

> 	tx->has_ready_bitmap = pf->hw.mac_type != ICE_MAC_E810;
> 
> E830 passes this check. ice_ptp_is_tracker_drained() then polls the
> E830 valid bits through ice_get_phy_tx_tstamp_ready_e830() for up to
> 10 ms.
> 
> The flush that follows calls ice_clear_phy_tstamp() for each ready
> index, and that function has no E830 case:
> 
> drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_clear_phy_tstamp() {
> 	switch (hw->mac_type) {
> 	case ICE_MAC_E810:
> 		return ice_clear_phy_tstamp_e810(hw, block, idx);
> 	case ICE_MAC_GENERIC:
> 		return ice_clear_phy_tstamp_e82x(hw, block, idx);
> 	case ICE_MAC_GENERIC_3K_E825:
> 		return ice_clear_ptp_tstamp_eth56g(hw, block, idx);
> 	default:
> 		return -EOPNOTSUPP;
> 	}
> }
> 
> ice_ptp_flush_tx_tracker() ignores the return value, so the failure is
> silent.
> 
> The commit message says:
> 
>   Only later devices (E822, E825, E830) have the bitmap and potential
>   issues with internal tracking.
> 
> On E830, won't the valid bits this wait collected still be set after
> the flush? If so, the patch adds up to 10 ms of latency there without
> fixing the desync.
> 
> ice_read_phy_tstamp() already has an E830 case that reads
> E830_PRTTSYN_TXTIME_H/L, but the clear path never uses it. Should
> ice_clear_phy_tstamp() get an E830 case, or should E830 skip the wait?
> The E830 case is still missing at the end of the series.
> 
>> +
>> +	err = read_poll_timeout(ice_ptp_is_tracker_drained,
>> +				drained, drained, 500, 10 * USEC_PER_MSEC, false,
>> +				pf, tx);
>> +	if (err) {
>> +		dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
>> +			tx->block);
>> +	}
>> +}
> 
> [ ... ]
> 
>> @@ -868,6 +930,8 @@ ice_ptp_release_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>>  	/* wait for potentially outstanding interrupt to complete */
>>  	synchronize_irq(pf->oicr_irq.virq);
>>  
>> +	ice_ptp_wait_for_tracker_drain(pf, tx);
>> +
>>  	ice_ptp_flush_tx_tracker(pf, tx);
> 
> [Severity: Medium]
> Is the Tx path still live when this runs? On the remove and devlink
> reload paths the order is:
> 
> ice_unload()
>   ice_deinit_features()
>     ice_ptp_release()
>       ice_ptp_release_tx_tracker()
>         ice_ptp_wait_for_tracker_drain()
>         ice_ptp_flush_tx_tracker()
>   ...
>   ice_unregister_netdev()
> 
> Nothing quiesces the Tx queues before the tracker is released.
> ice_ptp_release() masks PFINT_OICR_TSYN_TX and turns off Rx
> timestamping. Clearing tx->init only stops ice_ptp_request_ts() from
> handing out new indexes.
> 
> Two cases seem to slip past the 10 ms wait:
> 
>   ice_tstamp() has already reserved an index through
>   ice_ptp_request_ts(), but the TSYN context descriptor has not been
>   posted yet.
> 
>   A TSYN descriptor stays in the hardware Tx ring for more than 10 ms,
>   for example at low link speed, under PFC/pause backpressure, or with
>   rate limiting.
> 
> In either case read_poll_timeout() times out with only a dev_dbg().
> ice_ptp_flush_tx_tracker() then sees the index as not ready, skips
> ice_clear_phy_tstamp(), and frees the entry. When the frame finally
> goes out, the PHY sets a ready bit (Q_REG_TX_MEMORY_STATUS on E82x)
> that no tracker owns.
> 
> On E82x that quad memory is shared with the other ports of the quad,
> possibly including ports owned by other PFs. ice_ptp_process_tx_tstamp()
> only walks in_use, so nothing ever reads the orphaned entry.
> 
> Isn't that the same "delivery for the whole quad degrades to the
> periodic work" situation the commit message describes? Would it be more
> robust to flush after Tx has been stopped, instead of relying on a fixed
> timeout? The ordering is the same at the end of the series.
> 

I'm not sure how we could safely re-arrange this ordering to do it after
stopping the queues. I think we ultimately fix any issues by performing
a soft reset on initializing PTP which would prevent any long term issue
here (especially as we are changing the logic to avoid flushing anywhere
else).

Maybe that means the waiting is pointless to begin with...

>>  
>>  	kfree(tx->tstamps);
>