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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: keep Tx timestamp slots tracked until completion or timeout

Instead of dropping every outstanding Tx timestamp request when the link
goes down, the outstanding requests are marked stale, new requests are
rejected while the link is down, and a not yet ready slot is only released
by the two second timeout in ice_ptp_process_tx_tstamp().
ice_ptp_tx_tstamps_pending() gains an in_irq argument so the interrupt
re-arm path ignores stale slots while the periodic worker still sees them.

[Severity: Medium]
With link_up now part of the tracker-up test, is the E825 PHY restart path
still covered?

ice_ptp_link_change() sets ptp_port->link_up = true before it reaches
ice_ptp_port_phy_restart(), and for ICE_MAC_GENERIC_3K_E825 that function
calls ice_start_phy_timer_eth56g() without setting tx.calibrating:

	case ICE_MAC_GENERIC:
		...
		ptp_port->tx.calibrating = true;
		...
	case ICE_MAC_GENERIC_3K_E825:
		err = ice_start_phy_timer_eth56g(hw, port);
		break;

So during the E825 restart ice_ptp_is_tx_tracker_up() returns true
(tx->init && !tx->calibrating && ptp_port->link_up), and
ice_ptp_request_ts() keeps handing out indices while
ice_start_phy_timer_eth56g() walks the Tx timestamp memory and soft resets
the PHY.

The window is also reachable while the carrier stays up: ice_link_event()
continues past

	if (link_up == old_link && link_speed == old_link_speed)
		return 0;

on a speed-only change and calls ice_ptp_link_change(pf, link_up) while the
netdev is transmitting.

Are the captures requested in that window simply lost until the two second
timeout, and should tx.calibrating be set around the E825 restart as it is
for the E82X case?

[Severity: High]
Who runs the processing pass that applies the two second timeout on E810,
and on PFs that are not the source timer owner?

After this hunk the stale slots are only released from inside
ice_ptp_process_tx_tstamp(), by

		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
			drop_ts = true;

which requires something to schedule another pass. The candidates all look
closed on those configurations:

ice_misc_intr_thread_fn() re-arm now passes in_irq = true, and
ice_port_has_timestamps(tx, true) returns
bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len), which is 0 for an
all-stale tracker. The hardware fall-through in
ice_ptp_tx_tstamps_pending() does not help on E810 either, since
ice_check_phy_tx_tstamp_ready_e810() is:

	static int ice_check_phy_tx_tstamp_ready_e810(struct ice_hw *hw)
	{
		return 0;
	}

The replacement periodic sweep is not reached on those devices, see the
early returns in ice_ptp_maybe_trigger_tx_interrupt() quoted in the last
hunk below.

No hardware TSYN_TX interrupt can arrive either, because the packets whose
captures are outstanding were not transmitted, and
ice_ptp_is_tx_tracker_up() now rejects new requests while link_up is false.

Does this mean the index and the skb reference taken in
ice_ptp_request_ts() by

		tx->tstamps[idx].skb = skb_get(skb);
		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;

stay held for the entire link-down period?

There is a second effect worth checking. Requests are still accepted
between the physical link loss and the firmware link event, so the tracker
can fill up (tx->len is INDEX_PER_PORT, 64, on E810). If every index is
in_use and stale, does ice_ptp_request_ts() then return -1 for every packet
after link-up, so no timestamp is requested, no TSYN_TX interrupt is
generated, no processing pass ever runs, and Tx timestamping for that port
stays dead until a PF reset or driver reload? ice_ptp_flush_tx_tracker() is
only reached from ice_ptp_release_tx_tracker() and the clock-owner rebuild
path, and ice_ptp_link_change() returns early for E810 and E830 without
flushing.

For reference, the E810 low latency read sub-path does not appear affected:
ice_ll_ts_intr() -> ice_ptp_req_tx_single_tstamp() keeps walking in_use on
every LL_TS interrupt, so the timeout still fires there.

 	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
 	    test_bit(ICE_FLAG_DPLL, pf->flags)) {
 		int pin, err;

[ ... ]

[ ... ]

[Severity: Low]
This is a pre-existing issue and is not introduced by this patch, and on
closer reading it looks like a non-issue here, but it is worth noting for
completeness.

The commit message says the in_irq filtering avoids an IRQ storm, while
after the switch above ice_ptp_tx_tstamps_pending() still falls through to
the unconditional hardware check:

	/* Check hardware indicator */
	ret = ice_check_phy_tx_tstamp_ready(hw);

A PHY ready bit with no in_use owner would therefore still make
ice_misc_intr_thread_fn() and the ICE_MAC_E830 branch of ice_ptp_ts_irq()
re-arm PFINT_OICR(TSYN_TX) regardless of in_irq. Such an unowned ready bit
can be produced by the timeout path in ice_ptp_process_tx_tstamp(), which
clears in_use without reading or clearing the PHY slot:

		if (tx->has_ready_bitmap &&
		    !(tstamp_ready & BIT_ULL(phy_idx))) {
			if (drop_ts)
				goto skip_ts_read;

That release-without-read is unchanged from before this patch and is
described as intentional in the ice_ptp_process_tx_tstamp() kernel-doc. For
the case the commit message actually describes, a stale packet that is
never timestamped, no ready bit is ever set, so the hardware check returns
0 and the stated goal holds. The remaining hardware check is also removed
later in this series by "ice: don't clear in_use until HW clears ready
bitmap", which makes ice_ptp_tx_tstamps_pending() consult only the software
tracker.

[Severity: High]
Is the new in_irq = false check reachable on the devices that need it?

ice_ptp_init_tx() sets:

	tx->has_ready_bitmap = pf->hw.mac_type != ICE_MAC_E810;

so the first early return above skips every E810 PF, and the
ice_pf_src_tmr_owned() return skips PFs that are not the source timer
owner, including ICE_PTP_TX_INTERRUPT_SELF PFs on E830. Combined with the
suppressed re-arm in ice_misc_intr_thread_fn(), does anything on those
devices ever call ice_ptp_process_tx_tstamp() again so the two second
timeout can release the stale slots?

Would moving the ice_ptp_tx_tstamps_pending(pf, false) check ahead of the
has_ready_bitmap and ice_pf_src_tmr_owned() early returns give the
software tracker a guaranteed sweep on all configurations?