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

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

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