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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/25/2026 4:44 PM, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 

I am getting very tired with this... I tried flushing in a previous
version and that got a ton of complaints.

> 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);
> 

I'm actually not entirely convinced that is safe either :/ I suspect
that if we did not have that and tried to add it now there would be
similar complaints to what I got with trying to use it during link change...

> 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.
> 

The current intention of the series that that those indexes remain
blocked until they time out. I could try to adjust the counting logic so
that stale packets don't count as a timeout. But the way the current
logic works is entirely built around a flow where the only thread that
clears in_use is the IRQ thread. Flushing the tracker breaks that and
caused a lot of complaints.

> [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.

Since the previous sweep cleared the bit, this read should do nothing
because the timestamp is not marked valid. A new timestamp shouldn't be
possible to land on this index because the index is still locked by
software. The existing timestamp that was on that index should be
cleared by the sweep and soft reset, so this should not trigger a faulty
flow.
> 
> 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".
> 

Yes, *if* the valid bit is set which the sweep before the soft reset is
supposed to clear.

> 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.

I don't believe this could stall. I am also not entirely certain what
the best mechanism to safely stop the IRQ from triggering is.
Theoretically I guess we could disable interrupts, do this work, then
re-enable interrupts? But at least currently soft reset can take a while
(we're under investigation as to whether those waits are necessary but
intended to change that as a followup).

> 
> 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?
> 

Waiting for in-flight captures is problematic as we saw with the older
series and its complaints. I suspect that synchronizing to stop the IRQ
is ok, but I am worried about something trying to disable the HW
register and getting in such a state that we actually never re-enable
interrupts...

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