Re: [PATCH net v5 11/13] ice: keep Tx timestamp slots tracked until completion or timeout

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

On 10/25/2026 4:44 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · 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] The commit message says link-down requests are marked stale,
>   but the patch never marks them stale on E810, E830 or E82x.
> 
> --
> 
> ice: keep Tx timestamp slots tracked until completion or timeout
> 
> This patch stops dropping outstanding Tx timestamp requests on link down
> and rejects new requests while the link is down. Slots that are not ready
> yet are now released by the existing 2 second timeout. It also makes the
> interrupt re-arm checks ignore stale slots, and lets
> ice_ptp_maybe_trigger_tx_interrupt() run for PFs that handle their own Tx
> timestamp interrupt.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index b7c539f98276..309fa2877cc3 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;
> 
> [Severity: Medium]
> The commit message says:
> 
>     Mark the outstanding requests stale so their completions are read
>     and discarded
> 
> Where does this stale marking happen for E810, E830 and E82x? The patch
> removes the link-down drop here, but it doesn't add any new calls to
> ice_ptp_mark_tx_tracker_stale().
> 

At present it doesn't. I think this was needed for E825 because of the
complexity of what the reset flow required. I am not sure its actually
necessary on the other devices. We can improve the commit message for
clarity.

> On link down, the only caller that can be reached is the
> ICE_MAC_GENERIC_3K_E825 branch of ice_ptp_port_phy_stop() (and the same
> branch of ice_ptp_port_phy_restart()):
> 
> ice_ptp_port_phy_stop() {
>     ...
> 	case ICE_MAC_E810:
> 	case ICE_MAC_E830:
> 		err = 0;
> 		break;
> 	case ICE_MAC_GENERIC:
> 		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
> 
> 		err = ice_stop_phy_timer_e82x(hw, port, true);
> 		break;
> 	case ICE_MAC_GENERIC_3K_E825:
> 		...
> 		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
>     ...
> }
> 
> Also, ice_ptp_link_change() skips E810 and E830 entirely:
> 
> 	case ICE_MAC_E810:
> 	case ICE_MAC_E830:
> 		/* Do not reconfigure E810 or E830 PHY */
> 		break;
> 
> The new in_irq check in ice_port_has_timestamps() only excludes stale
> slots:
> 
> 	if (in_irq)
> 		return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
> 
> Given that, can a link down with a request still outstanding on E810
> (non-LL), E830 or E82x cause a back-to-back interrupt loop? The sequence
> would be:
> 
>   link goes down while a not-ready slot is set in in_use
>   ice_ptp_maybe_trigger_tx_interrupt()
>     ice_ptp_tx_tstamps_pending(pf, false) -> true
>     wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
>   ice_ptp_process_tx_tstamp()
>     slot not ready and not timed out -> continue
>   ice_misc_intr_thread_fn() (E810/E82x) or ice_ptp_ts_irq() (E830)
>     ice_ptp_tx_tstamps_pending(pf, true) -> true, slot is not stale
>     wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
>   ... repeats until the 2 second timeout frees the slot
> 

This ultimately depends on whether a given index actually gets stuck or
completes, which may depend on each device type. We don't really have a
good mechanism to rate limit how often the IRQ re-tries, (and at this
point I am heavily considering just trying to switch to a polling
implementation given how much trouble we've had with the IRQ to begin
with...)

> On E830 the re-arm happens in hardirq context in ice_ptp_ts_irq(). On E82x
> every pass does sideband PHY reads. Before this patch, the first pass after
> link down dropped every slot, so this loop could not happen.
> 
> A later patch in this series, "ice: don't clear in_use until HW clears
> ready bitmap", adds an ice_ptp_is_tx_tracker_up() check to the in_irq
> branch. Combined with the link_up check added here, that seems to close
> the loop.
> 
> Even after that patch, though, E810, E830 and E82x still don't get any
> stale marking on link down. So this commit isn't bisect-safe on its own,
> and the commit message describes a mechanism the code doesn't implement.
> 
> Should the commit message be corrected, or should the stale marking be
> added for the other MAC types?
> 

I can certainly try to reword the commit message...