Re: [PATCH net v3 08/14] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The kernel-doc no longer matches how the code uses `struct
  ice_ptp_tx::calibrating` and `ice_ptp_mark_tx_tracker_stale()`.
- [Low] The new dev_err in `ice_ptp_port_phy_restart()`
  (ice_ptp.c:1347-1349) says "Timestamping remains disabled", which is…

--

ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY, because clearing it puts the hardware Tx timestamp
counter out of sync. Instead, the PHY stop and restart paths set
tx.calibrating to block new Tx timestamp requests, and mark outstanding
timestamps stale. The unused soft_reset parameter is removed and the
restart failure message is reworded.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 6297773806949..365cff9c10173 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -818,7 +818,8 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&tx->lock, flags);
> -	bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
> +	if (tx->init)
> +		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
>  	spin_unlock_irqrestore(&tx->lock, flags);
>  }
>  

[ ... ]

> @@ -1243,7 +1245,14 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
>  		err = ice_stop_phy_timer_e82x(hw, port, true);
>  		break;
>  	case ICE_MAC_GENERIC_3K_E825:
> -		err = ice_stop_phy_timer_eth56g(hw, port, true);
> +		/* Disable new Tx timestamp requests */
> +		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +		ptp_port->tx.calibrating = true;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

[Severity: Low]
This isn't a bug, but should the kernel-doc for the calibrating field be
updated to match how E825 now uses it? In ice_ptp.h, struct ice_ptp_tx
still says:

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

After this patch, E825 sets the bit in ice_ptp_port_phy_stop() on link
down, where no calibration runs. The commit message also says E825 "does
not perform vernier calibration".

ice_ptp_port_phy_restart() leaves the bit set on failure, so it acts as a
"timestamping disabled until link toggle" state. ice_ptp_release() also
leaves the bit set after teardown.

The kernel-doc for ice_ptp_mark_tx_tracker_stale() has the same problem. It
still says:

 * This should be called when the PTP clock is modified such as after a set
 * time request.

It doesn't mention the new PHY stop and restart callers. It also doesn't say
that the helper now does nothing when tx->init is clear. The teardown order
in ice_ptp_release() depends on that check, because the tracker bitmaps have
already been freed:

ice_ptp_release()
    ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
    ...
    ice_ptp_port_phy_stop(&pf->ptp.port);
        ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

Could both comments be updated to describe the new behavior?

> +
> +		err = ice_stop_phy_timer_eth56g(hw, port);
>  		break;

[ ... ]

> @@ -1309,15 +1318,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)

[ ... ]

>  		err = ice_start_phy_timer_eth56g(hw, port);
> +		if (err)
> +			break;
> +
> +		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +		ptp_port->tx.calibrating = false;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
>  		break;
>  	default:

[ ... ]

>  	if (err)
> -		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
> -			port, err);
> +		dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with err %pe; Timestamping remains disabled; A link-toggle may recover.\n",
> +			port, ERR_PTR(err));

[Severity: Low]
Is "Timestamping remains disabled" accurate for Rx on E825 in every
failure case?

Tx is now blocked only in software, through tx.calibrating. Rx is blocked
only in hardware, through PHY_REG_RX_OFFSET_READY.

Suppose the first register write in ice_start_phy_timer_eth56g() fails:

ice_start_phy_timer_eth56g()
    err = ice_stop_phy_timer_eth56g(hw, port);
    if (err)
        return err;

PHY_REG_RX_OFFSET_READY then keeps its previous value, for example 1 from
an earlier successful start. The hardware may keep marking Rx descriptor
timestamps valid, even though ice_sync_phy_timer_eth56g() never ran.

If any later step fails, RX_OFFSET_READY has already been cleared and the
message is accurate. The early return is not new; only the message's
absolute claim is.

Could the message be more precise here, for example by saying that Tx
timestamping remains disabled?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com