Re: [PATCH net v3 11/14] ice: wait for in-flight Tx timestamps before removing the tracker

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

Critical: 0 · High: 0 · Medium: 2 · 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] On E830, the new drain wait runs, but the flush that follows
  cannot clear the ready entries it waited for.
- [Medium] The tracker is released while the PF's Tx path is still live,
  and the new drain wait gives up after 10 ms.

--

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

This patch adds ice_ptp_wait_for_tracker_drain(). It polls the PHY Tx
timestamp ready bitmap for up to 10 ms, until every in-use tracker index
reports ready. ice_ptp_release_tx_tracker() now calls it before
ice_ptp_flush_tx_tracker(), so in-flight captures get cleared instead of
being left as orphaned ready bits, and E810 is skipped via
tx->has_ready_bitmap.

[ ... ]

[Severity: Medium]
Does this wait do anything useful on E830? ice_ptp_init_tx() sets:

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

E830 passes this check. ice_ptp_is_tracker_drained() then polls the
E830 valid bits through ice_get_phy_tx_tstamp_ready_e830() for up to
10 ms.

The flush that follows calls ice_clear_phy_tstamp() for each ready
index, and that function has no E830 case:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c: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 the return value, so the failure is
silent.

The commit message says:

  Only later devices (E822, E825, E830) have the bitmap and potential
  issues with internal tracking.

On E830, won't the valid bits this wait collected still be set after
the flush? If so, the patch adds up to 10 ms of latency there without
fixing the desync.

ice_read_phy_tstamp() already has an E830 case that reads
E830_PRTTSYN_TXTIME_H/L, but the clear path never uses it. Should
ice_clear_phy_tstamp() get an E830 case, or should E830 skip the wait?
The E830 case is still missing at the end of the series.

[ ... ]

[Severity: Medium]
Is the Tx path still live when this runs? On the remove and devlink
reload paths the order is:

ice_unload()
  ice_deinit_features()
    ice_ptp_release()
      ice_ptp_release_tx_tracker()
        ice_ptp_wait_for_tracker_drain()
        ice_ptp_flush_tx_tracker()
  ...
  ice_unregister_netdev()

Nothing quiesces the Tx queues before the tracker is released.
ice_ptp_release() masks PFINT_OICR_TSYN_TX and turns off Rx
timestamping. Clearing tx->init only stops ice_ptp_request_ts() from
handing out new indexes.

Two cases seem to slip past the 10 ms wait:

  ice_tstamp() has already reserved an index through
  ice_ptp_request_ts(), but the TSYN context descriptor has not been
  posted yet.

  A TSYN descriptor stays in the hardware Tx ring for more than 10 ms,
  for example at low link speed, under PFC/pause backpressure, or with
  rate limiting.

In either case read_poll_timeout() times out with only a dev_dbg().
ice_ptp_flush_tx_tracker() then sees the index as not ready, skips
ice_clear_phy_tstamp(), and frees the entry. When the frame finally
goes out, the PHY sets a ready bit (Q_REG_TX_MEMORY_STATUS on E82x)
that no tracker owns.

On E82x that quad memory is shared with the other ports of the quad,
possibly including ports owned by other PFs. ice_ptp_process_tx_tstamp()
only walks in_use, so nothing ever reads the orphaned entry.

Isn't that the same "delivery for the whole quad degrades to the
periodic work" situation the commit message describes? Would it be more
robust to flush after Tx has been stopped, instead of relying on a fixed
timeout? The ordering is the same at the end of the series.

 
 	kfree(tx->tstamps);

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