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

[Jakub Kicinski <kuba@kernel.org>]

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?

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

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