Re: [PATCH net 15/15] ice: Recalibrate PHY after settime64 on E825-C

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

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ice: Recalibrate PHY after settime64 on E825-C
> 
> The patch extends the post-settime PHY recalibration path in
> ice_ptp_settime64() so that it also runs for ICE_MAC_GENERIC_3K_E825,
> not only for ICE_MAC_GENERIC (E822/E823). The stated motivation is that
> the E825-C PHY needs recalibration after large system time jumps,
> otherwise PHY timestamps read back incorrectly.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 832cc6ad2f90c..a5efb0a95941e 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -2020,8 +2020,9 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
>>  	/* Reenable periodic outputs */
>>  	ice_ptp_enable_all_perout(pf);
>>  
>> -	/* Recalibrate and re-enable timestamp blocks for E822/E823 */
>> -	if (hw->mac_type == ICE_MAC_GENERIC)
>> +	/* Recalibrate and re-enable timestamp blocks for E822/E823/E825-C */
>> +	if (hw->mac_type == ICE_MAC_GENERIC ||
>> +	    hw->mac_type == ICE_MAC_GENERIC_3K_E825)
>>  		ice_ptp_restart_all_phy(pf);
> 
> [Severity: High]
> 
> The E82X sequence has two halves: timestamps are invalidated before the
> time is written, and the PHY is recalibrated afterwards.  Only the second
> half is being extended to E825-C here.  Earlier in ice_ptp_settime64() the
> invalidation is still E82X only:
> 
> 	if (hw->mac_type == ICE_MAC_GENERIC) {
> 		err = ice_ptp_clear_phy_offset_ready_e82x(hw);
> 		if (err)
> 			dev_warn(ice_pf_to_dev(pf), "Failed to mark timestamps as invalid before settime\n");
> 	}
> 
> Should something equivalent run for ICE_MAC_GENERIC_3K_E825?  There is no
> eth56g counterpart of ice_ptp_clear_phy_offset_ready_e82x() in the tree, and
> ice_stop_phy_timer_eth56g() clears only PHY_REG_RX_OFFSET_READY, so
> PHY_REG_TX_OFFSET_READY stays at 1 for the whole restart window while the
> PHY timer is being re-initialised.
> 

Even if we call ice_ptp_clear_phy_offset_ready_e825/eth56g or whatever,
we actually found that to be problematic. We can't clear the offset
ready bit without causing other issues.

> Related to that, the ICE_MAC_GENERIC_3K_E825 branch of
> ice_ptp_port_phy_restart() does not quiesce the Tx tracker at all:
> 
> 	case ICE_MAC_GENERIC_3K_E825:
> 		err = ice_start_phy_timer_eth56g(hw, port);
> 		break;
> 
> whereas the ICE_MAC_GENERIC branch does:
> 
> 		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> 		ptp_port->tx.calibrating = true;
> 		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> 
> tx.calibrating is only ever set in that E82X branch and only ever cleared in
> ice_ptp_wait_for_offsets(), so on E825-C ice_ptp_is_tx_tracker_up() keeps
> returning true (link_up is true, which is why the port was picked in
> ice_ptp_restart_all_phy()):

Right. I believe we do actually need to hold calibrating over this path,
and we should do the same for settime.

> 
> 	return tx->init && !tx->calibrating && ptp_port->link_up;
> 
> Does that mean Tx timestamp requests keep being armed and reported while
> ice_start_phy_timer_eth56g() is soft-resetting the PTP block, reprogramming
> PHY_REG_TIMETUS/INCVAL and re-running ice_sync_phy_timer_eth56g()?  The
> comment in the ICE_MAC_GENERIC branch describes that state as producing bad
> values:
> 
> 		/* Do not clear calibrating flag here. Tx timestamps remain
> 		 * disabled until ice_ptp_wait_for_offsets() has verified
> 		 * that the Tx and Rx offset calibration has completed.
> 		 * Clearing it here would allow Tx timestamps to be reported
> 		 * before the PHY offset registers are configured, leading
> 		 * to incorrect timestamp values.
> 		 */
> 
> One more effect of the same window: the call chain
> ice_start_phy_timer_eth56g() -> ice_ptp_phy_soft_reset_eth56g() ->
> ice_ptp_clear_tx_memory_status_eth56g() clears every hardware Tx timestamp
> entry:
> 
> 	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> 		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
> 
> but the software tracker keeps its in_use bits and skbs, since nothing calls
> ice_ptp_mark_tx_tracker_stale() on this path.  Can those already-outstanding
> requests then only complete via the 2 second timeout?
> 

Effectively, yes. I think we can address this.

> [Severity: Medium]
> 
> This is also the first E825-C path that can reach ice_ptp_port_phy_stop(),
> because ice_ptp_link_change() only calls ice_ptp_port_phy_restart() on a
> link-up transition for this MAC type:
> 
> 	case ICE_MAC_GENERIC_3K_E825:
> 		if (linkup)
> 			ice_ptp_port_phy_restart(ptp_port);
> 		goto exit_kref_put;
> 
> ice_ptp_port_phy_restart() tests link_up outside ps_lock:
> 
> 	if (!ptp_port->link_up)
> 		return ice_ptp_port_phy_stop(ptp_port);
> 
> 	mutex_lock(&ptp_port->ps_lock);
> 
> and ice_ptp_link_change() stores that field with no lock held:
> 
> 	/* Update cached link status for this port immediately */
> 	ptp_port->link_up = linkup;
> 
> Can a link bounce concurrent with a settime invert the decision here?
> 
> CPU0 (ice_ptp_settime64 -> ice_ptp_restart_all_phy)
> 	if (port->link_up)                /* true */
> 		ice_ptp_port_phy_restart(port);
> 	   -> !ptp_port->link_up          /* link just went down */
> 	   -> ice_ptp_port_phy_stop()     /* waits on ps_lock */
> 
> CPU1 (link comes back up)
> 	ice_ptp_link_change()
> 	   ptp_port->link_up = true;
> 	   ice_ptp_port_phy_restart()
> 	      -> ice_start_phy_timer_eth56g()   /* holds ps_lock, takes ms */
> 
> CPU0 then acquires ps_lock and issues ice_stop_phy_timer_eth56g(hw, port,
> true), which does:
> 
> 	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
> 
> Since the E825-C link handler only restarts on link-up, would this leave the
> port with the link up but Rx offsets marked not ready, with no recovery until
> the next link flap?
> 
> The unlocked check itself predates this change on the E82x path, but the
> E825-C path that can now hit it is new here.

Right. I think we need to address this sequencing by holding the
ps_lock. For simplicity, I think we need to reduce to a single lock for
all ports as opposed to one per-port. You can't actually concurrently
execute port restarts anyways since a port re-sync requires the source
timer command register (which requires the PTP semaphore), and that
would overall simplify the locking to allow us to take the lock, do the
settime and then reset all ports. We might even be able to amortize the
restart procedure by restarting all ports concurrently that way (as a
future improvement).

I'll look into this and see how complicated a cleanup will be.