Re: [PATCH net v5 10/13] 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 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] ice_start_phy_timer_eth56g()
  (drivers/net/ethernet/intel/ice/ice_ptp_hw.c) now calls…
- [Medium] With this patch, the destructive E825 PHY soft-reset sequence
  runs at runtime from ice_start_phy_timer_eth56g(), mainly on E825…

--

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

This patch makes ice_start_phy_timer_eth56g() call
ice_ptp_phy_soft_reset_eth56g() before it programs the E825 PHY. The goal
is to clear any outstanding Tx timestamp memory and the PHY timestamp
counter. Only the timer block of the PHY is reset.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 5637925c0e68..2d000251b185 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]
Should the software Tx timestamp tracker be flushed before this reset
wipes the hardware timestamp memory?

On E825 link-up the path is ice_ptp_link_change() ->
ice_ptp_port_phy_restart() -> ice_start_phy_timer_eth56g(). The E825 case
in ice_ptp_port_phy_restart() only marks the outstanding entries stale:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
    ...
		ptp_port->tx.disabled = true;
		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);

		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

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

The in_use bits and tstamps[idx].skb stay in place. After that,
ice_ptp_phy_soft_reset_eth56g() calls
ice_ptp_clear_tx_memory_status_eth56g() and pulses the soft reset, which
clears every ready bit on the port.

Take an entry whose timestamp was captured but not read yet. Before this
patch it would have been read through the ready bit path and quietly
dropped as stale. Now its ready bit never comes back, so
ice_ptp_process_tx_tstamp() can only release it through the timeout:

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

			/* Count the number of Tx timestamps that timed out */
			pf->ptp.tx_hwtstamp_timeouts++;
		}

Does this hold the slot and the skb from ice_ptp_request_ts() for about
2 seconds? Does it also count a timestamp the driver threw away itself in
tx_hwtstamp_timeouts, which ethtool -S reports?

At this commit there is also no time bound on the hold. The periodic
fallback in ice_ptp_maybe_trigger_tx_interrupt() fires only when the
hardware ready bits are set, and the reset has just cleared them.

The later commit "ice: keep Tx timestamp slots tracked until completion
or timeout" changes that trigger to check the software in_use bitmap.
That bounds the reclaim to about 2.5 seconds. The miscounted timeouts and
the temporary slot and skb hold are still there at the end of the series.

ice_ptp_rebuild_owner() already flushes the software state before it
restarts the PHYs:

	/* Flush software tracking of any outstanding timestamps since we're
	 * about to flush the PHY timestamp block.
	 */
	ice_ptp_flush_all_tx_tracker(pf);

Would the link change restart path need a matching flush? The commit
message says only that the reset will "clear any outstanding Tx timestamp
memory". It doesn't mention what happens to the software tracker.

[Severity: Medium]
What keeps the Tx timestamp interrupt thread and in-flight hardware
captures away from this port while the soft reset sequence runs?

Before this patch, the only caller of ice_ptp_phy_soft_reset_eth56g() was
ice_ptp_init_phc_e825c(), at PHC init. Now it also runs on every E825
link-up restart. The sequence sweeps all slots and sleeps before the
reset is asserted:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_ptp_phy_soft_reset_eth56g() {
	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
    ...
	usleep_range(5000, 6000);

	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
    ...
	usleep_range(5000, 6000);
    ...
}

ice_stop_phy_timer_eth56g() leaves PHY_REG_TX_OFFSET_READY set, so the
PHY keeps capturing timestamps for descriptors that were posted before
tx.disabled was set. A capture can land on an index the sweep has already
passed, during the 5 ms gap before the reset is asserted.

The consumer path is ice_misc_intr_thread_fn() -> ice_ptp_process_ts() ->
ice_ptp_tx_tstamp_owner() -> ice_ptp_process_tx_tstamp(). It never takes
ps_lock. Its ice_is_reset_in_progress() check doesn't cover a link
triggered PHY restart. It snapshots the ready bitmap and then calls
ice_read_phy_tstamp() based on that snapshot.

Suppose the thread snapshots that new ready bit and then reads the slot
after the soft reset. It would read a slot whose valid bit is still set,
against a counter that was just reset to zero. The kdoc for
ice_ptp_clear_tx_memory_status_eth56g() says the soft reset does not clear
the timestamp memory.

The earlier commit "ice: E825: clear PHY_TSTAMP memory prior to soft
reset" says an unbalanced read "underflows the counter, and prevents new
timestamp interrupts from being triggered".

Could this stall Tx timestamp interrupts on the port until the next soft
reset? That is the failure this patch is meant to prevent. Whether it
happens depends on PHY behavior described only in this series and on a
narrow timing window. Even so, nothing in the code seems to prevent the
interleaving.

Does this path need to synchronize with the misc IRQ thread, or wait for
in-flight captures, before it runs the sweep and the reset?

>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
>  
>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);

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