Re: [PATCH net v5 08/13] 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 commit message for "ice: E825: stop clearing
  PHY_REG_TX_OFFSET_READY" names a Tx tracker field that does not exist.

--

ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY. Clearing it left the hardware's outstanding Tx
timestamp counter out of sync. Instead, new Tx timestamp requests are
blocked in software while the PHY is stopped or restarting, and
ice_ptp_mark_tx_tracker_stale() now checks tx->init.

> Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
> requests could happen while the PHY is calibrating. To avoid this, set the
> tx.calibrating field of the Tx timestamp tracker when stopping the timer
> and clear it when finishing the restart.

[ ... ]

> Unlike E822 devices, set the calibrating flag when "stopping" the PHY and
> clear it immediately after the start procedure.

[ ... ]

> In the event that the PHY timer start procedure fails, the device is in an
> unknown state and timestamps will not behave properly. As such, and similar
> to E822 devices, the calibrating field is not cleared on failure.

[Severity: Low]
Should these references to tx.calibrating say tx.disabled instead?

struct ice_ptp_tx in ice_ptp.h has no calibrating member. Its bitfields
are:

	u8 init : 1;
	u8 disabled : 1;
	u8 has_ready_bitmap : 1;

The new E825 code in ice_ptp_port_phy_stop() and
ice_ptp_port_phy_restart() sets and clears ptp_port->tx.disabled:

	spin_lock_irqsave(&ptp_port->tx.lock, flags);
	ptp_port->tx.disabled = true;
	spin_unlock_irqrestore(&ptp_port->tx.lock, flags);

ice_ptp_wait_for_offsets() uses the same field for E82x, and the
kernel-doc update in this patch is for @disabled.

The behaviour the commit message describes matches the code. Only the
field name is wrong.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com