Re: [PATCH net v3 12/14] ice: keep Tx timestamp slots tracked until completion or timeout

[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 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 2 (1) · Low: 1
> 
> 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] The commit message says this patch adds the tracker-init
>   check: "Since we're adding a new call to ice_ptp_mark_tx_tracker,…
> - [Medium] ice_ptp_link_change() now calls
>   ice_ptp_mark_tx_tracker_stale() on every link-down, before the…
> - [Low] The concern says the kernel-doc of
>   ice_ptp_mark_tx_tracker_stale() ('should be called when the PTP clock…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but in the
>   ICE_PTP_READY branch ice_ptp_release() calls…
> - [Medium] The concern: the 2 s timeout in ice_ptp_process_tx_tstamp()
>   releases a not-ready index without reading it.
> 
> --
> 
> ice: keep Tx timestamp slots tracked until completion or timeout
> 
> This commit stops dropping outstanding Tx timestamp requests when the
> link goes down. Instead it marks them stale, rejects new requests while
> the link is down, and frees a slot that is not ready only after the 2
> second timeout. It also changes the pending-timestamp check used when
> re-arming the IRQ and in ice_ptp_maybe_trigger_tx_interrupt().
> 
>> Since we're adding a new call to ice_ptp_mark_tx_tracker, notice that the
>> function previously did not check the tracker initialization state. Fix
>> this by only accessing the bitmaps if the init field is set.
> 
> [Severity: Medium]
> Is this paragraph accurate for this patch? The diff does not modify
> ice_ptp_mark_tx_tracker_stale(). The if (tx->init) guard in it comes
> from the earlier commit "ice: E825: stop clearing
> PHY_REG_TX_OFFSET_READY".
> 
> The message also names the function ice_ptp_mark_tx_tracker instead of
> ice_ptp_mark_tx_tracker_stale.
> 
> This patch has Fixes: fcc2cef37fed, so it is likely to be picked for
> stable. A backport of this patch alone would add the new
> ice_ptp_link_change() caller without the guard. Then a link-down after a
> failed tracker allocation would call bitmap_or() on NULL in_use and
> stale bitmaps.
> 
> Could the message say that the guard is a prerequisite from the other
> commit, so the two get backported together?
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index ca581490f3edb..57141e94c5708 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -582,14 +584,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>>  			return;
>>  	}
>>  
>> -	/* Drop packets if the link went down */
>> -	link_up = READ_ONCE(ptp_port->link_up);
>> -
>>  	for_each_set_bit(idx, tx->in_use, tx->len) {
>>  		struct skb_shared_hwtstamps shhwtstamps = {};
>>  		u8 phy_idx = idx + tx->offset;
>>  		u64 raw_tstamp = 0, tstamp;
>> -		bool drop_ts = !link_up;
>> +		bool drop_ts = false;
>>  		struct sk_buff *skb;
>>  
>>  		/* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the 2 second timeout
> later in ice_ptp_process_tx_tstamp() still releases an index that is not
> ready, without reading it:
> 
> 		if (tx->has_ready_bitmap &&
> 		    !(tstamp_ready & BIT_ULL(phy_idx))) {
> 			if (drop_ts)
> 				goto skip_ts_read;
> 
> Suppose the hardware holds a timestamp-requested frame for more than 2
> seconds, for example during a long pause or PFC stall. Could the capture
> then land in an index that a new request has reused, or leave a ready
> bit with no in_use owner?
> 

We have to pick some cutoff. We believe that 2 seconds is a safe amount
of time to wait before we assume the timestamp will never happen.

> That seems to conflict with this statement in the commit message: "This
> way an index is never reused while the hardware can still write it".
>

I can try to rephrase the commit message, but ultimately we have to
decide at what point we assume the hardware will never complete an
index. There is no mechanism for the ring to tell that the index will
never be completed. I could increase this timeout even longer but then
we run the risk of many such stuck packets becoming locked for so long
that we can't handle timestamps. We *know* there are cases where the
index is never completed so we can't lock the index indefinitely either.


> For the link-down case itself, the device counts frames dropped while
> the link is down (GLPRT_TDOLD), so that case looks less likely to hit
> this.
> 
> [ ... ]
> 
>> @@ -1432,6 +1431,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>>  
>>  	WRITE_ONCE(ptp_port->link_up, linkup);
>>  
>> +	if (!linkup)
>> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
>> +
>>  	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
>>  		goto out_unlock;
>>  
> 
> [Severity: Medium]
> Can this take an uninitialized spinlock on link down?
> 
> ice_ptp_mark_tx_tracker_stale() takes tx->lock before it checks
> tx->init:
> 
> 	spin_lock_irqsave(&tx->lock, flags);
> 	if (tx->init)
> 		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
> 	spin_unlock_irqrestore(&tx->lock, flags);
> 
> The only spin_lock_init() of tx->lock is in ice_ptp_alloc_tx_tracker().
> That is reached only through ice_ptp_init() -> ice_ptp_init_port(), and
> ice_init_features() calls ice_ptp_init() only when
> ICE_FLAG_PTP_SUPPORTED is set and the driver is not in safe mode.
> 
> ice_link_event() calls ice_ptp_link_change() without checking
> ICE_FLAG_PTP_SUPPORTED, and the new call comes before the ICE_PTP_READY
> check. Consider a PF without 1588 support, a PF in safe mode, or a PF
> where ice_ptp_init() failed before reaching ice_ptp_init_port(). Every
> link-down there would do:
> 
> ice_link_event()
>   ice_ptp_link_change(pf, false)
>     ice_ptp_mark_tx_tracker_stale()
>       spin_lock_irqsave(&tx->lock, flags)
> 
> on a lock that was never initialized. With CONFIG_DEBUG_SPINLOCK this
> gives "spinlock bad magic". With lockdep it gives "trying to register
> non-static key" and lockdep turns itself off.
> 
> Also, ice_ptp_alloc_tx_tracker() sets tx->init = 1 before it calls
> spin_lock_init(&tx->lock).
> 
> Should this new call depend on PTP having been initialized, for example
> by moving it after the ICE_PTP_READY check?
> 

We can try adding a state check. Of course there are also complaints if
we try to check tx->init outside of lock too :\

> [Severity: Low]
> This isn't a bug, but the kernel-doc for ice_ptp_mark_tx_tracker_stale()
> still says:
> 
>  * This should be called when the PTP clock is modified such as after a set
>  * time request.
> 
> Now that ice_ptp_link_change() also calls it on link down, should the
> comment mention that use as well?
> 

Sure I can fix this.

> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch.
> 
> ice_ptp_release() destroys the PTP kworker before it unregisters the
> PHC:
> 
> 	if (pf->ptp.kworker) {
> 		kthread_destroy_worker(pf->ptp.kworker);
> 		pf->ptp.kworker = NULL;
> 	}
> 
> 	if (!pf->ptp.clock)
> 		return;
> 
> 	/* Disable periodic outputs */
> 	ice_ptp_disable_all_perout(pf);
> 
> 	ptp_clock_unregister(pf->ptp.clock);
> 
> Neither ice_ptp_settime64() nor ice_ptp_adjtime() checks ptp.state, and
> on success they call ice_ptp_reset_cached_phctime(). If
> ice_ptp_update_cached_phctime() returns -EAGAIN because ICE_CFG_BUSY is
> held, that function then does:
> 
> 		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
> 					   msecs_to_jiffies(10));
> 
> ICE_CFG_BUSY can be held by a concurrent settime or adjtime on the same
> clock, or by ethtool set_link_ksettings or set_ringparam.
> 
> Could a clock_settime() on /dev/ptpX that races with PF removal queue
> delayed work on a worker that kthread_destroy_worker() is about to free?
> Could it instead call kthread_queue_delayed_work() with a NULL kworker?
> 
> The ethtool paths can still run at this point, because ice_unload()
> tears down PTP before it unregisters the netdev:
> 
> ice_unload()
>   ice_deinit_features()
>     ice_ptp_release()
>       kthread_destroy_worker()
>       ...
>       ptp_clock_unregister()
>   ...
>   ice_unregister_netdev()
> 
> Would unregistering the clock before destroying the worker close this
> window?
> 

Yes, this is part of the entire issue that we register the PTP clock too
early and we unregister it too late. That should be fixed but its a
pre-existing issue that should be fixed in a follow up.

> [ ... ]
>