Re: [PATCH net v5 07/13] ice: E822: cancel offset verification work during reset preparation

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

On 10/25/2026 4:43 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] On E822 (ICE_MAC_GENERIC), a PF reset (PFR) during Vernier
>   calibration leaves Tx timestamping disabled on that port until the…
> 
> --
> 
> ice: E822: cancel offset verification work during reset preparation
> 
> This commit cancels the E822 offset verification work (ov_work) in
> ice_ptp_prepare_for_reset(). According to the commit message, the PHY is
> reconfigured after any reset, either by the clock owner rebuild or by
> ice_ptp_link_change() at the end of the rebuild. On that basis the work is
> not needed while the reset runs.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 57e2606655c4..6e1f60d7eef7 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -3004,6 +3004,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>>  
>>  	kthread_cancel_delayed_work_sync(&ptp->work);
>>  
>> +	if (hw->mac_type == ICE_MAC_GENERIC)
>> +		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);
>> +
>>  	if (reset_type == ICE_RESET_PFR)
>>  		return;
> 
> [Severity: High]
> Can a PF reset during vernier calibration leave Tx timestamps disabled on
> this port?
> 
> The new cancel sits before the ICE_RESET_PFR early return, so it also runs
> for PFR. ice_ptp_wait_for_offsets() is the only code that programs the
> offsets and sets P_REG_TX_OR/P_REG_RX_OR. It is also the only place that
> clears port->tx.disabled:
> 
> ice_ptp_wait_for_offsets() {
>     ...
> 		spin_lock_irqsave(&port->tx.lock, flags);
> 		if (port->tx.disabled) {
> 			port->tx.disabled = false;
>     ...
> }
> 
> After a PFR, nothing seems to queue ov_work again. For PFR,
> ice_ptp_rebuild() skips the owner rebuild, and ice_ptp_restart_all_phy()
> along with it:
> 
> ice_ptp_rebuild() {
>     ...
> 	if (ice_pf_src_tmr_owned(pf) && reset_type != ICE_RESET_PFR) {
> 		err = ice_ptp_rebuild_owner(pf);
>     ...
> }
> 
> If the link was up both before and after the reset, the
> ice_ptp_link_change() call at the end of ice_rebuild() returns early:
> 
> ice_ptp_link_change() {
>     ...
> 	if (READ_ONCE(ptp_port->link_up) == linkup)
> 		goto out_unlock;
>     ...
> }
> 

Yea. This was intended to work if ice_ptp_link_change was unconditional,
but the change to make it conditional makes this problematic. Will try
to fix...

> The only other path that queues ov_work is ice_ptp_port_phy_restart(). It
> is reached only from ice_ptp_init() at probe time and from
> ice_ptp_restart_all_phy(), via settime or the owner rebuild.
> 
> Calibration needs at least one Tx packet, so after link up ov_work can keep
> requeuing itself every 100ms. A PFR in that window would cancel it.
> Examples are an ethtool reset, Tx timeout recovery, a VSI rebuild failure
> that schedules ICE_RESET_PFR, or an IIDC function reset.
> 
> After that, TX_OR/RX_OR stay at the 0 that ice_stop_phy_timer_e82x()
> wrote, and tx.disabled stays true. ice_ptp_request_ts() then rejects every
> Tx timestamp request until the link flaps or the PHC is set.
> 
> Before this patch, the in-flight work saw ice_is_reset_in_progress(),
> requeued itself, and finished calibration after the reset. Later patches in
> the series don't appear to change any of these paths.
> 
> This also seems to contradict this part of the commit message:
> 
>     However, a device reset will result in the PHY being reconfigured either as
>     part of the clock owner resetting or due to the ice_ptp_link_change() call
>     at the end of the rebuild.
> 
> Neither of those paths runs on a PFR when the link state doesn't change.
> 
> Is there also a timing-dependent variant on CORER/GLOBR? The clock owner's
> ice_ptp_restart_all_phy() could restart a non-owner E822 peer's PHY and
> queue its ov_work before the peer's own ice_ptp_prepare_for_reset() runs.
> The peer's new cancel would then drop that work, and the peer's rebuild
> does not restart its PHY.
> 
> Would it work to cancel only for non-PFR resets where the PHY is known to
> be restarted? Another option is to requeue ov_work, or call
> ice_ptp_port_phy_restart(), during the E822 rebuild when the link is up.
>