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

[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] Possible requeue of ov_work after the new cancel (judged harmless
>   and by design).
> 
> --
> 
> ice: E822: cancel offset verification work during reset preparation
> 
> This patch cancels the E822 offset verification work (ov_work) in
> ice_ptp_prepare_for_reset(). Without the cancel, the work can keep running
> or rescheduling itself while a reset is in progress. The PHY gets
> reconfigured after the reset anyway, either by the clock owner or by
> ice_ptp_link_change() at the end of the rebuild.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 828d46a73f292..6297773806949 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -3006,6 +3006,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>>
>>  	kthread_cancel_delayed_work_sync(&ptp->port.work);
>>
>> +	if (hw->mac_type == ICE_MAC_GENERIC)
>> +		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);
>> +
> 
> [Severity: Low]
> This is a minor observation and it may well be intended. This cancel runs
> without adapter->ps_lock held, and nothing stops ov_work from being queued
> again afterwards. Can another PF queue a peer's ov_work again while that
> peer is still resetting?
> 
> On a multi-PF E822 adapter, the clock owner can reach this producer for
> every port that has link_up set:
> 
> ice_ptp_rebuild_owner() / ice_ptp_settime64()
>   ice_ptp_restart_all_phy()
>     ice_ptp_port_phy_restart(port)
>       kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0);
> 
> ice_ptp_restart_all_phy() checks only this before restarting the peer's PHY:
> 
> 	if (READ_ONCE(port->link_up))
> 		ice_ptp_port_phy_restart(port);
> 
> It does not look at the peer's ptp.state or its reset state.
> 
> The impact looks small. A requeued ice_ptp_wait_for_offsets() sees
> ice_is_reset_in_progress() and only reschedules, with no hardware access:
> 
> 	if (ice_is_reset_in_progress(pf->state)) {
> 		/* wait for device driver to complete reset */
> 		kthread_queue_delayed_work(pf->ptp.kworker,
> 					   &port->ov_work,
> 					   msecs_to_jiffies(100));
> 		return;
> 	}
> 
> Each PF's ice_rebuild() also calls ice_ptp_link_change(). That path goes
> through ice_ptp_port_phy_restart(), which cancels and requeues the work.
> 
> The commit message also says the PHY is reconfigured "as part of the clock
> owner resetting", so the requeue may be the intended recalibration path. If
> so, the only effect left is a few no-op 100 ms reschedules during a reset.
> 
> Is that the expected behaviour, or should the restart path skip ports whose
> PF is in reset?
> 

This is intended, in the sense that we don't really have a great method
of handling sequencing here. If the main PF resets it will be modifying
the clock so it must restart each PHY to account for that reset clock.
However, each port might be resetting too and will need to restart the
PHY in order to ensure the appropriate link state. We don't currently
have a good synchronization mechanism to ensure that we wait till
everything resets and then restart all PHY... This could use a better
design of the entire reset procedure but we don't have one since each PF
is (mostly) independent. At worst I believe this results in a duplicate
PHY reset which is annoying but overall ok as long as we reset after we
know the appropriate link state.

Cancelling the work is just done to avoid wasting unnecessary CPU doing
nothing useful.

>>  	if (reset_type == ICE_RESET_PFR)
>>  		return;
>>
>