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

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

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()):

	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?

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