Re: [PATCH net 04/15] ice: E822: keep Tx timestamps disabled during offset calibration

[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: E822: keep Tx timestamps disabled during offset calibration

This moves the clearing of port->tx.calibrating out of
ice_ptp_port_phy_restart() and into ice_ptp_wait_for_offsets(), so that Tx
timestamps stay disabled until the Tx and Rx PHY offsets have been
configured.  It also adds two dev_dbg() messages covering the pending and
the completed calibration cases.

No Link: tags present in the commit header.

A few questions below.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 68537705e8399..d018f02f700d7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1156,6 +1156,7 @@ static int ice_ptp_check_tx_fifo(struct ice_ptp_port *port)
>  static void ice_ptp_wait_for_offsets(struct kthread_work *work)
>  {
>  	struct ice_ptp_port *port;
> +	unsigned long flags;
>  	struct ice_pf *pf;
>  	struct ice_hw *hw;
>  	int tx_err;
> @@ -1178,12 +1179,28 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work)
>  		tx_err = ice_phy_cfg_tx_offset_e82x(hw, port->port_num);
>  	rx_err = ice_phy_cfg_rx_offset_e82x(hw, port->port_num);
>  	if (tx_err || rx_err) {
> -		/* Tx and/or Rx offset not yet configured, try again later */
> +		/* Tx and/or Rx offset not yet configured, try again later.
> +		 * This is expected during normal link-up: the vernier offset
> +		 * calibration cannot complete until at least one packet has
> +		 * been transmitted, so the first retries routinely land here.
> +		 */
> +		dev_dbg(ice_pf_to_dev(pf),
> +			"PTP offset not yet valid for port %u (tx_err=%d rx_err=%d)\n",
> +			port->port_num, tx_err, rx_err);
>  		kthread_queue_delayed_work(pf->ptp.kworker,
>  					   &port->ov_work,
>  					   msecs_to_jiffies(100));
>  		return;
>  	}
> +
> +	/* Tx and Rx offsets are now configured, enable Tx timestamps */
> +	spin_lock_irqsave(&port->tx.lock, flags);
> +	port->tx.calibrating = false;
> +	spin_unlock_irqrestore(&port->tx.lock, flags);

[Severity: High]
Does placing the only clear of port->tx.calibrating after the combined
"if (tx_err || rx_err)" gate make the Tx timestamp gate depend on Rx
calibration?

The two directions have independent offset-ready registers.  On the Tx side,
ice_phy_cfg_tx_offset_e82x() programs the total Tx offset and sets:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_phy_cfg_tx_offset_e82x() {
	...
	err = ice_write_phy_reg_e82x(hw, port, P_REG_TX_OR, 1);
	...
}

and thereafter returns 0 immediately ("Nothing to do if we've already
programmed the offset").  Meanwhile the Rx side keeps failing until a packet
has actually been received:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_phy_cfg_rx_offset_e82x() {
	...
	if (!(reg & P_REG_RX_OV_STATUS_OV_M))
		return -EBUSY;
	...
}

So in the Tx-done/Rx-pending state, ice_ptp_wait_for_offsets() loops on the
100 ms retry branch with tx_err == 0 and rx_err == -EBUSY and never reaches
the new "port->tx.calibrating = false".

Since calibrating is written in only two places after this patch (set true
in ice_ptp_port_phy_restart(), cleared here), is there anything left that
can reopen the Tx gate?  ice_ptp_is_tx_tracker_up() stays false:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_is_tx_tracker_up() {
	return tx->init && !tx->calibrating;
}

and every request is refused, with ice_tstamp() only bumping the skipped
counter:

drivers/net/ethernet/intel/ice/ice_txrx.c:ice_tstamp() {
	idx = ice_ptp_request_ts(tx_ring->tx_tstamps, skb);
	if (idx < 0) {
		tx_ring->vsi->back->ptp.tx_hwtstamp_skipped++;
		return;
	}
	...
}

This is reachable on an ordinary link-up when the link partner is silent or
passive, or in one-way/tap topologies, and it persists until the next link
transition.  Before this patch the Tx gate did not depend on the Rx side.

Would gating the clear on tx_err == 0 only, and letting the Rx offset
configuration continue to retry independently, be the intended behaviour?

[Severity: Low]
Should the surrounding documentation be updated along with the new
semantics?  The kernel-doc directly above ice_ptp_wait_for_offsets() still
says:

 * Once the offset in either direction is measured, configure the associated
 * registers with the calibrated offset values and enable timestamping. The Tx
 * and Rx directions are configured independently as soon as their associated
 * offsets are known.

and the member doc in ice_ptp.h for struct ice_ptp_tx still says:

 * @calibrating: if true, the PHY is calibrating the Tx offset. During this
 *               window, timestamps are temporarily disabled.

After the patch the flag is cleared only when ice_ptp_check_tx_fifo(),
ice_phy_cfg_tx_offset_e82x() and ice_phy_cfg_rx_offset_e82x() all succeed,
so the Tx gate now also encodes Rx state and Tx FIFO state.  The window is
also no longer temporary in all cases: on the ice_start_phy_timer_e82x()
error path ice_ptp_port_phy_restart() breaks before queueing ov_work, and
ice_ptp_port_phy_stop() cancels ov_work without restoring the flag.

> +
> +	dev_dbg(ice_pf_to_dev(pf),
> +		"PTP offset valid for port %u, Tx timestamps enabled\n",
> +		port->port_num);
>  }
>  
>  /**
> @@ -1269,10 +1286,13 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
>  		if (err)
>  			break;
>  
> -		/* Enable Tx timestamps right away */
> -		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> -		ptp_port->tx.calibrating = false;
> -		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.
> +		 */

[Severity: Medium]
Is the "leading to incorrect timestamp values" description in this comment,
and the matching commit message text, accurate for this window?

The commit message says:

    Timestamps produced during this window use uncalibrated PHY offsets and
    can produce incorrect values.

    When ptp4l receives incorrect timestamps, it may reject them and wait
    for the next sync interval (typically 1-2 seconds), compounding delays
    during link cycling.

During that window P_REG_TX_OR is 0.  ice_start_phy_timer_e82x() begins by
calling ice_stop_phy_timer_e82x(), whose first action is:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_stop_phy_timer_e82x() {
	err = ice_write_phy_reg_e82x(hw, port, P_REG_TX_OR, 0);
	...
}

and TX_OR is only set back to 1 by ice_phy_cfg_tx_offset_e82x() once the
total Tx offset has been programmed.  The driver documents that write as
invalidating timestamps:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_ptp_clear_phy_offset_ready_e82x() {
 * Clear PHY TX_/RX_OFFSET_READY registers, effectively marking all transmitted
 * and received timestamps as invalid.
	...
}

And timestamps captured without the valid bit are dropped rather than
reported:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_process_tx_tstamp() {
	...
	/* Discard any timestamp value without the valid bit set */
	if (!(raw_tstamp & ICE_PTP_TS_VALID))
		drop_ts = true;
	...
}

Is the pre-patch symptom then a missing Tx timestamp (slot held until read
or timeout, counted in tx_hwtstamp_timeouts) rather than a wrong value
delivered to ptp4l?  The sibling patch in this series ("ice: E825: stop
clearing PHY_REG_TX_OFFSET_READY") states that with the offset-ready bit
clear "the hardware still captures Tx timestamps, but it no longer sets the
valid bit", which seems to point the same way.

Could the commit message and this new comment be reworded to describe the
actual failure mode, so that anyone matching user reports against the
Fixes: tag is not misled?

>  
>  		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
>  					   0);