Re: [PATCH net v2 10/15] ice: E825: perform a soft reset when starting the PHY timer

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 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] The patch adds a call to ice_ptp_phy_soft_reset_eth56g() in
  ice_start_phy_timer_eth56g() (ice_ptp_hw.c:2155), so every runtime…
- [Medium] The patch runs the sweep plus soft reset on the live restart
  path (link-up, rebuild, and at the end of the series settime64) but…
- [Medium] The runtime sweep and soft reset added by this patch are not
  synchronized with the Tx timestamp consumer.
- [Low] ice_ptp_clear_tx_memory_status_eth56g() returns void.

--

ice: E825: perform a soft reset when starting the PHY timer

This commit makes ice_start_phy_timer_eth56g() call
ice_ptp_phy_soft_reset_eth56g() right after the PHY timer is stopped.
The E825 PHY timer block is now soft reset, and its Tx timestamp memory
swept, before the PHY is reprogrammed. ice_start_phy_timer_eth56g() is
used by the runtime restart paths, so the sweep and reset now also run on
live ports.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index e83ad2bb8d420..e5bfede904314 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  	if (err)
>  		return err;
>  
> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> +	if (err)
> +		return err;
> +

[Severity: Medium]
Does this discard Tx timestamps that the software tracker still treats as
outstanding?

ice_ptp_phy_soft_reset_eth56g() starts by calling
ice_ptp_clear_tx_memory_status_eth56g(). That function reads all
INDEX_PER_PORT entries, which clears every valid/ready bit on the port.

On the E825 branch of ice_ptp_port_phy_restart(), the tracker is only
marked stale before this runs:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
    ...
		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

		err = ice_start_phy_timer_eth56g(hw, port);
    ...
}

So outstanding entries keep their in_use bit and their skb reference.

Suppose hardware has captured a timestamp, but the IRQ thread has not
consumed it when the sweep reaches that index. Its ready bit is then gone
and will not be set again. ice_ptp_process_tx_tstamp() skips the entry
until the 2 second timeout:

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

		continue;
	}

When the timeout expires, it increments pf->ptp.tx_hwtstamp_timeouts.
Before this change the timestamp memory survived the restart, so the
stale entry would have been read and dropped quietly.

At this commit, ice_ptp_tx_tstamps_pending() also sees in_use as
non-empty. It keeps re-arming PFINT_OICR_TSYN_TX for those 2 seconds.

A later patch in the series stops counting stale entries in IRQ context.
The 2 second slot/skb hold and the timeout count are still there at the
end of the series, though.

The final patch, "ice: Recalibrate PHY after settime64 on E825-C", makes
ice_ptp_settime64() call ice_ptp_restart_all_phy() on live ports without
flushing, which makes this easier to hit. The rebuild path is not
affected, because ice_ptp_rebuild_owner() calls
ice_ptp_flush_all_tx_tracker() first.

Would it make sense to flush the tracker with ice_ptp_flush_tx_tracker()
before the soft reset, instead of only marking it stale? The commit
message also does not mention that in-flight timestamps are discarded on
every restart.

[Severity: Medium]
Is Tx timestamp capture still enabled while this sequence runs?

ice_stop_phy_timer_eth56g() only clears PHY_REG_RX_OFFSET_READY. Its
kernel-doc says PHY_REG_TX_OFFSET_READY is left set, and that "Tx
timestamp requests should be disabled by other means".

In ice_ptp_port_phy_restart(), the only other means are setting
tx.calibrating = true and calling ice_ptp_mark_tx_tracker_stale(). These
block new ice_ptp_request_ts() reservations. Descriptors already posted to
the Tx ring with a timestamp request are still outstanding in hardware.

Inside ice_ptp_phy_soft_reset_eth56g(), the memory sweep happens first.
Then come a PHY_REG_GLOBAL read/write and a sleep, and only after that is
the reset asserted:

	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
	...
	usleep_range(5000, 6000);

	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;

A timestamp might be captured after the sweep has passed its index but
before the assert. The memory entry would then stay valid while the reset
zeroes the internal outstanding-timestamp counter.

Wouldn't that cause the counter/memory mismatch that the comment on
ice_ptp_clear_tx_memory_status_eth56g() warns "may become out of sync and
prevent new timestamp interrupts"?

On the rebuild path, ice_ptp_flush_all_tx_tracker() has already freed the
software index. A new request that reuses the index could then read the
leftover ready entry.

Previously the soft reset only ran at PHC init, with no traffic. Later in
the series, "ice: wait for in-flight Tx timestamps before flushing the
tracker" adds drain logic, but only to ice_ptp_flush_tx_tracker() and not
to this restart path. The window is still there at the end of the series.

[Severity: Low]
What happens if one of the sweep reads fails here?

ice_ptp_clear_tx_memory_status_eth56g() returns void and only warns:

	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
		if (err)
			failed++;
	}

	if (failed)
		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
			 failed, port);

ice_ptp_phy_soft_reset_eth56g() then toggles the soft reset anyway. Next,
ice_start_phy_timer_eth56g() sets TX/RX_OFFSET_READY and returns success,
and ice_ptp_port_phy_restart() clears tx.calibrating.

The comment justifies this by saying software only reads indices it
already knows are ready. If the failed read was on a valid entry, though,
the reset zeroes the outstanding-timestamp counter while that entry stays
in memory. That is the same counter/memory desync the comment warns can
prevent new timestamp interrupts.

Before this patch the sweep and reset never ran at runtime, so a
transient sideband failure during a restart could not cause this.

The commit message says "This ensures that we clear any outstanding Tx
timestamp memory", which does not hold when a sweep read fails. Should
the sweep failure be propagated to the caller?

[Severity: Medium]
Does anything keep the Tx timestamp IRQ thread out while the sweep and
soft reset run?

ice_ptp_port_phy_restart() holds adapter->ps_lock. The consumer path does
not take ps_lock, or anything else that excludes the restart:

ice_misc_intr_thread_fn()
  ice_ptp_process_ts()
    ice_ptp_process_tx_tstamp() / ice_ptp_tx_tstamp_owner()

tx->lock only covers the in_use/skb bookkeeping.

ice_ptp_process_tx_tstamp() reads the ready bitmap once with
ice_get_phy_tx_tstamp_ready(). It then calls ice_read_phy_tstamp() for
each in_use index that bitmap marked ready. Those reads can interleave
with ice_ptp_clear_tx_memory_status_eth56g() and with the roughly 10ms
PHY_REG_GLOBAL toggle.

Suppose the bitmap snapshot is taken before the sweep, and the per-index
read happens after the reset. The IRQ thread then reads an entry that was
already cleared, against a counter that was just zeroed.

The comment on ice_ptp_clear_tx_memory_status_eth56g() says reads outside
the soft reset procedure can put the counter out of sync and prevent new
timestamp interrupts. The IRQ thread could also issue sideband reads to
the port's timestamp block while the reset is asserted.

ice_ptp_restart_all_phy() runs this sequence on every live port on the
adapter, including other PFs' ports. It is called on rebuild, and on
settime64 at the end of the series. Nothing in the series adds
synchronize_irq() or similar to keep the consumer out during this
sequence.

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