Re: [PATCH net v3 06/14] ice: E822: keep Tx timestamps disabled during offset calibration

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

On 10/19/2026 1:14 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 kernel-doc for ice_ptp_tx::calibrating (ice_ptp.h:109-110)
>   still says "if true, the PHY is calibrating the Tx offset".
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   ice_ptp_settime64() leaves the E82x PHY offset-ready bits cleared with…
> 
> --
> 
> ice: E822: keep Tx timestamps disabled during offset calibration
> 
> ice_ptp_port_phy_restart() no longer clears tx.calibrating right after
> ice_start_phy_timer_e82x() succeeds. The flag is now cleared in
> ice_ptp_wait_for_offsets(), once ice_phy_cfg_tx_offset_e82x() has
> succeeded. The patch also adds debug messages for pending and completed
> offset calibration.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 776f211290eca..828d46a73f292 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -1283,11 +1301,10 @@ 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 timestamp requests
>> +		 * remain disabled until ice_ptp_wait_for_offsets() has
>> +		 * verified that the Tx offset calibration has completed.
>> +		 */
>>  		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
>>  					   0);
>>  		break;
> 
> [Severity: Medium]
> This is a pre-existing issue, and this patch did not introduce it. ov_work
> is the only thing that re-arms the E82x PHY offsets. Can
> ice_ptp_settime64() leave Tx and Rx timestamps invalid on every port if
> ice_ptp_lock() fails?
> 
> ice_ptp_settime64() clears the offset-ready bits first. It can then bail
> out before the PHYs are restarted:
> 
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_settime64() {
>     ...
> 	if (hw->mac_type == ICE_MAC_GENERIC) {
> 		err = ice_ptp_clear_phy_offset_ready_e82x(hw);
> 		...
> 	}
> 
> 	if (!ice_ptp_lock(hw)) {
> 		err = -EBUSY;
> 		goto exit;
> 	}
>     ...
> 	if (hw->mac_type == ICE_MAC_GENERIC)
> 		ice_ptp_restart_all_phy(pf);
> exit:
>     ...
> }
> 
> ice_ptp_clear_phy_offset_ready_e82x() writes 0 to P_REG_TX_OR and
> P_REG_RX_OR on every port. ice_ptp_lock() polls the shared PFTSYN_SEM up
> to MAX_TRIES times, with usleep_range(5000, 6000) between polls, and then
> gives up. So this path can be hit if another function holds the semaphore
> for that long.
> 
> The goto skips ice_ptp_restart_all_phy(). That is the only route to
> ice_ptp_port_phy_restart(), and so the only way ov_work gets queued. If
> ov_work had already finished, nothing calls
> ice_phy_cfg_tx_offset_e82x() or ice_phy_cfg_rx_offset_e82x() again.
> 
> Would every Tx and Rx timestamp then be marked invalid and dropped by the
> ICE_PTP_TS_VALID checks? That would last until a link flap, a reset, or a
> later successful settime. tx.calibrating stays false the whole time, so
> Tx timestamp requests are still accepted.
> 
> Could the offset-ready clear move after the lock is taken, or could the
> error path still restart the PHYs? This still looks present at the end of
> the series.
> 

This is a pre-existing issue that I think should be addressed in a
different series, specifically we have some planned work around the
settime/adjtime flows that this would make sense to handle in.

>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
>> index 27ea502b75768..029ee4612d765 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
>> @@ -107,7 +107,7 @@ enum ice_tx_tstamp_work {
>>   * @len: length of the tstamps and in_use fields.
>>   * @init: if true, the tracker is initialized;
>>   * @calibrating: if true, the PHY is calibrating the Tx offset. During this
>> - *               window, timestamps are temporarily disabled.
>> + *               window, timestamp requests are disabled.
> 
> [Severity: Low]
> This isn't a bug, but is the first half of this kernel-doc still accurate
> after this patch?
> 
> On E822, ice_ptp_wait_for_offsets() is now the only place that clears the
> flag, so it can stay true while nothing is calibrating. Say link goes down
> before ice_phy_cfg_tx_offset_e82x() has succeeded. ice_ptp_port_phy_stop()
> then cancels ov_work and stops the PHY timer without touching the flag:
> 
> 	case ICE_MAC_GENERIC:
> 		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
> 
> 		err = ice_stop_phy_timer_e82x(hw, port, true);
> 		break;
> 
> Before this patch that could not happen, because the flag was cleared
> right after a successful start. The same is true when
> ice_start_phy_timer_e82x() fails: ice_ptp_port_phy_restart() breaks out
> with calibrating still set and no ov_work queued.
> 
> Later in the series, the E825 ice_ptp_port_phy_stop() path also sets
> calibrating to true on link-down. That moves the meaning further from
> this description.
> 
> This has no functional effect. The next ice_ptp_port_phy_restart() sets
> the flag again and requeues ov_work.
> 
> The flag now reads more like "Tx timestamp requests are blocked until the
> Tx offset has been configured" than "the PHY is calibrating". The
> "temporarily disable Tx timestamps while calibrating PHY offset" comment
> in ice_ptp_port_phy_restart() has the same mismatch.
> 
> Could both comments be updated to match the new behaviour?
> 

I can fix this if we need a respin.

>>   * @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
>>   *                    bitmap register. If false, fall back to verifying new
>>   *                    timestamp values against previously cached copy.
>