Re: [PATCH net 07/15] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

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: E825: stop clearing PHY_REG_TX_OFFSET_READY

This drops the PHY_REG_TX_OFFSET_READY = 0 write from
ice_stop_phy_timer_eth56g() on E825 devices. Clearing that bit left hardware
capturing Tx timestamps without the valid bit, which desynchronized the
internal outstanding timestamp counter until a PHY soft reset. Only
PHY_REG_RX_OFFSET_READY is cleared now.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a41c711e751b..d48eb3c618231 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

@@ -2115,10 +2115,6 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
 {
 	int err;
 
-	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 0);
-	if (err)
-		return err;
-

[Severity: High]
With this write gone, is anything left that suppresses Tx timestamp
reporting while the E825 PHY is being re-calibrated?

ice_start_phy_timer_eth56g() begins by calling the stop helper and only
asserts Tx readiness at the very end:

	err = ice_stop_phy_timer_eth56g(hw, port, false);
	...
	err = ice_phy_cfg_parpcs_eth56g(hw, port);
	err = ice_phy_cfg_ptp_1step_eth56g(hw, port);
	err = ice_phy_cfg_mac_eth56g(hw, port);	/* PHY_REG_TOTAL_TX_OFFSET_L/U */
	...
	err = ice_write_40b_ptp_reg_eth56g(hw, port, PHY_REG_TIMETUS_L, incval);
	err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_INIT_INCVAL);
	...
	err = ice_sync_phy_timer_eth56g(hw, port);
	...
	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 1);

Across that whole window the Tx offset registers are being reprogrammed and
the port timer has not yet been re-synced to the PHC, but per the commit
message hardware now marks captures valid, so this check in
ice_ptp_process_tx_tstamp() no longer drops them:

		/* Discard any timestamp value without the valid bit set */
		if (!(raw_tstamp & ICE_PTP_TS_VALID))
			drop_ts = true;

The E82x branch of ice_ptp_port_phy_restart() gates this in software:

		spin_lock_irqsave(&ptp_port->tx.lock, flags);
		ptp_port->tx.calibrating = true;
		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
		...
		/* 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.
		 */

while the E825 branch does not:

	case ICE_MAC_GENERIC_3K_E825:
		err = ice_start_phy_timer_eth56g(hw, port);
		break;

Since ice_ptp_is_tx_tracker_up() only tests tx->init && !tx->calibrating,
new requests keep being accepted during the restart, and the restart path
holds only ps_lock while the reader takes tx->lock, so the two do not
exclude each other. Should the E825 case set tx.calibrating around
ice_start_phy_timer_eth56g() to replace the removed hardware gate?

The same exposure looks unbounded on the early-return paths of
ice_start_phy_timer_eth56g(), for example the ice_ptp_lock() failure:

	if (!ice_ptp_lock(hw)) {
		dev_err(ice_hw_to_dev(hw), "Failed to acquire PTP semaphore\n");
		return -EBUSY;
	}

because PHY_REG_TX_OFFSET_READY is only re-asserted on the success path.
The reachable trigger is a link up or link speed change
(ice_ptp_link_change() -> ice_ptp_port_phy_restart()) concurrent with any
SO_TIMESTAMPING application. Could the commit message also mention this
change in reporting behaviour during calibration?

I believe we accidentally get the correct behavior through other changes
in the series, but we should make this explicit. We cannot clear the bit
because the hardware behavior for "invalid" timestamps is problematic.
However, the software must gate this period. Later patches in the series
make it gate by link down, which effectively achieves a similar result.
However, a more explicit clearing is likely preferable. Will investigate
and fix.

quoted

 	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
 	if (err)
 		return err;

[Severity: Low]
The kernel-doc and the debug print of ice_stop_phy_timer_eth56g() now
describe more than the body does:

 * Stop the clock of a PHY port. This must be done as part of the flow to
 * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
 * initialized or when link speed changes.

	ice_debug(hw, ICE_DBG_PTP, "Disabled clock on PHY port %u\n", port);

After this change the body only clears PHY_REG_RX_OFFSET_READY, so no Tx
offset is invalidated and no clock enable bit is touched, while
ice_start_phy_timer_eth56g() still asserts both PHY_REG_TX_OFFSET_READY and
PHY_REG_RX_OFFSET_READY. ice_ptp_port_phy_stop() is likewise documented as
"Stop timestamping for a PHY port" while Tx timestamping stays enabled for
ICE_MAC_GENERIC_3K_E825. Should the comment and the log message be updated
to say only the Rx offset-ready bit is cleared?

I'll update this.

[Severity: Low]
This isn't a bug introduced by this patch, but with the body reduced to a
single register write the unused soft_reset parameter becomes quite visible:

 * @soft_reset: if true, hold the SOFT_RESET bit of PHY_REG_PS

ice_stop_phy_timer_eth56g() never reads soft_reset, and PHY_REG_PS appears
nowhere in drivers/net/ethernet/intel/ice/ outside this comment; the E82x
counterpart ice_stop_phy_timer_e82x() does honour the flag via P_REG_PS and
P_REG_PS_SFT_RESET_M. Both call sites are therefore equivalent:
ice_ptp_port_phy_stop() passes true for ICE_MAC_GENERIC_3K_E825 and
ice_start_phy_timer_eth56g() passes false. Would it make sense to drop the
parameter, or to document that it is ignored on ETH56G?

I'll drop this. The soft reset parameter was likely a copy from the E822
device and done when there was hope of merging the paths with a function
table. That never materialized, and is not worth doing at this point.