Re: [PATCH net 11/13] ice: fix PTP hang for E825C devices

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

On 5/4/2026 10:14 PM, Jacob Keller wrote:
> From: Grzegorz Nitka <grzegorz.nitka@intel.com>
> 
> Change the order of PTP reconfiguration when port goes down or up
> (ice_down and ice_up calls) to be more graceful and consistent from
> timestamp interrupts processing perspective.
> 
> For both calls (ice_up and ice_down), accompanying ice_ptp_link_change
> is called which starts/stops PTP timer. This patch changes the order:
> - while link goes down: disable net device Tx first (netif_carrier_off,
>   netif_tx_disable), then call ice_ptp_link_change
> - while link goes up: ice_ptp_link_change called first, then re-enable
>   net device Tx (netif_tx_start_all_queues)
> 
> Otherwise, there is a narrow window in which PTP timestamp request has
> been triggered and timestamp processing occurs when PTP timer is not
> enabled yet (up case) or already disabled (down case). This may lead to
> undefined behavior and receiving invalid timestamps. This case was
> observed on E825C devices only.
> 

Sashiko has some concerns:

> Does this sequence still leave a window for early packets to trigger PTP
> processing before the timer is ready?
> Looking earlier in ice_up_complete(), the hardware Rx rings are started
> before we reach this point:
> 	err = ice_vsi_start_all_rx_rings(vsi);
> 	if (err)
> 		return err;
> Since ice_vsi_start_all_rx_rings() is called before ice_ptp_link_change(),
> could packets be received and timestamped before the PTP timer is actually
> enabled?

This doesn't matter. The issue is for Tx timestamps and the Rx path is
unrelated here.

Sashiko also says:

> Is the race condition fully closed by moving ice_ptp_link_change() here?
> While netif_tx_disable() stops the OS networking stack from enqueuing new
> packets, it does not flush or wait for the hardware DMA rings to drain. The
> hardware Tx rings are actually stopped later in the teardown process:
> 	ice_vsi_dis_irq(vsi);
> 	tx_err = ice_vsi_stop_lan_tx_rings(vsi, ICE_NO_RESET, 0);
> If an in-flight packet from the hardware Tx ring triggers a PTP timestamp
> request after ice_ptp_link_change() has set ptp_port->link_up = false,
> wouldn't the software worker ignore the timestamp?
> Leaving a valid timestamp unread in the hardware FIFO is noted as the cause
> of the PHY hang. Should ice_ptp_link_change() be called after the hardware
> rings are fully stopped to completely avoid this regression?
> Additionally, is there a missing VSI type check here in ice_down()?
> Unlike ice_up_complete(), which checks if the VSI is a PF or SF, this block
> executes for any VSI that has a netdev.
> Because the PTP timer is shared at the physical port level, could bringing
> down a secondary interface (like a sub-function) invoke ice_down() and
> unconditionally disable the PTP timer for vsi->back?
> This seems like it would break PTP functionality for the main PF interface
> and any other active sub-functions on that port.

This I am not sure. Gregorz, do you have any thoughts? I thought we had
spoken about this and were maybe investigating separating the PTP link
state from the admin up/down state entirely.