Re: [PATCH net v5 08/13] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/25/2026 4:44 PM, netdev-bot+sashiko@kernel.org wrote:
> 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.
> 

Yes, these doc changes should have been fixed but I missed them while
renaming the field.