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

[netdev-bot+sashiko@kernel.org]

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().

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

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com