Re: [PATCH net v2 11/15] ice: wait for in-flight Tx timestamps before flushing the tracker

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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] On E830 the flush still leaves hardware-ready entries
  unacknowledged, so the drain does nothing for a device the commit…
- [Low] ice_ptp_rebuild_owner() calls ice_ptp_flush_all_tx_tracker(),
  which now runs ice_ptp_wait_for_tracker_drain() on every port of the…

Pre-existing issues:
- [Low] At this commit, ice_ptp_process_tx_tstamp() sets drop_ts =
  !link_up (line 592).

--

ice: wait for in-flight Tx timestamps before flushing the tracker

This patch adds a poll to ice_ptp_flush_tx_tracker(). For up to 10 ms per
port, it waits for the PHY to publish ready bits for all in-use Tx
timestamp slots, and only then flushes the tracker. E810 has no ready
bitmap, so it skips the wait.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 818e2e265a7e5..5220de274819a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -744,6 +744,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 the drain actually help on E830? E830 has has_ready_bitmap set, so it
enters this wait. The commit message also lists it among the devices that
benefit:

  "Only later devices (E822, E825, E830) have the bitmap ... Thus, skip
  the wait for E810"

However, ice_clear_phy_tstamp() in ice_ptp_hw.c has no E830 case:

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 that return value, then clears in_use
and frees the skb anyway:

	if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
		ice_clear_phy_tstamp(hw, tx->block, phy_idx);

ice_ptp_reset_ts_memory(), ice_ptp_port_phy_stop() and
ice_ptp_port_phy_restart() also do nothing for E830.

So on E830, wouldn't an entry that becomes ready during the wait stay set
in E830_PRTMAC_TS_TX_MEM_VALID with no owner? That is the orphaned ready
bit state this patch is trying to prevent.

On the rebuild path the tracker stays initialized. For E830,
ice_ptp_ts_irq() re-arms PFINT_OICR whenever ice_ptp_tx_tstamps_pending()
sees a hardware ready bit.

Could such an orphan keep re-triggering the interrupt, or be picked up as
a stale timestamp by a later request that reuses the index?

The actual effect depends on two hardware details: whether E830 valid bits
clear only when the timestamp is read, and whether CORER resets the PRTMAC
timestamp memory. The E830 case is still missing from
ice_clear_phy_tstamp() 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);

[Severity: Low]
Will this poll always run until its full 10 ms timeout on the post-reset
rebuild path?

This call chain reaches the poll for every port on the adapter, one port
after another:

ice_rebuild()->ice_ptp_rebuild()->ice_ptp_rebuild_owner()->
ice_ptp_flush_all_tx_tracker()->ice_ptp_flush_tx_tracker()

ice_reset_subtask() clears reset_ongoing before the rebuild:

		} else {
			/* done with reset. start rebuild */
			pf->hw.reset_ongoing = false;
			ice_rebuild(pf, reset_type);

So the reset_ongoing early return in ice_ptp_is_tracker_drained() does not
skip the poll.

A CORER/GLOBR/EMPR reset drops the Tx rings. A request whose packet was
dropped that way will never get a ready bit, so
ice_ptp_is_tracker_drained() keeps returning false. That costs about 20
rounds of SBQ ready-bitmap reads per port. On an 8-port adapter it adds
roughly 80 ms to reset recovery.

A Tx hang is a common reason for the reset, and it is also the case most
likely to leave such entries behind.

The wait also seems unnecessary there. Right after the flush,
ice_ptp_rebuild_owner() does:

	ice_ptp_flush_all_tx_tracker(pf);

	/* Enable quad interrupts */
	err = ice_ptp_cfg_phy_interrupt(pf, true, 1);

ice_ptp_cfg_phy_interrupt() calls ice_ptp_reset_ts_memory(), which resets
the E82x timestamp memory. On E825 ports with link up,
ice_ptp_restart_all_phy() then does a PHY soft reset.

Would it make sense to wait only on the release path? That is where new
requests are blocked and captures can still be in flight.

> +	if (err) {
> +		dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
> +			tx->block);
> +	}
> +}
> +

[ ... ]

> @@ -760,6 +822,8 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>  	int err;
>  	u8 idx;
>  
> +	ice_ptp_wait_for_tracker_drain(pf, tx);
> +

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The new
drain only covers ice_ptp_flush_tx_tracker(). At this commit,
ice_ptp_process_tx_tstamp() can still create the same orphan when the link
goes down:

	bool drop_ts = !link_up;
	...
			if (drop_ts)
				goto skip_ts_read;

When a slot is not ready yet, this path clears in_use and frees the skb
without touching the PHY entry. If the capture finishes later, its ready
bit has no owner. That is the case the commit message describes.

The next patch in the series, "ice: keep Tx timestamp slots tracked until
completion or timeout", removes the link-down drop. After that patch, only
the 2 second timeout frees a slot that is not ready. That timeout is
intentional and is documented both in the ice_ptp_process_tx_tstamp()
kernel-doc and in that patch's commit message. So by the end of the series
this no longer applies.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008215614.1987250-1-anthony.l.nguyen%40intel.com