Re: [PATCH net 05/15] ice: E822: cancel offset verification work during reset preparation

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: E822: cancel offset verification work during reset preparation

This adds a kthread_cancel_delayed_work_sync() of the E82x offset
verification work (ov_work) to ice_ptp_prepare_for_reset(), so that the
work does not run while the PTP reset sequence executes.  A few questions
about the changelog and about the placement of the new cancel.

First, on the described failure mechanism:

quoted

Without this, ice_ptp_wait_for_offsets() can execute during a reset,
find that ice_is_reset_in_progress() is true, and re-queue itself in a
tight loop. Meanwhile, the reset path in ice_ptp_rebuild_owner() calls
ice_ptp_port_phy_restart(), which starts a new ov_work. This results in
two ov_work instances running simultaneously, racing over the PHY offset
registers and the calibrating flag.

[Severity: Medium]
Can this sequence happen as described?  ov_work is a single
struct kthread_delayed_work per port (ice_ptp.h, struct ice_ptp_port) run
by one per-PF kworker, so the same work item cannot execute twice at once.

The re-queue in ice_ptp_wait_for_offsets() also does not look like a tight
loop, and it returns before touching the PHY offset registers or
port->tx.calibrating:

	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;
	}

And ice_ptp_port_phy_restart(), the only site that arms ov_work, already
cancel-syncs it under ps_lock before queueing again:

	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
	...
	kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0);

Could the changelog describe the actual window instead, for example a
worker instance that already passed the reset-in-progress check and is
mid-PHY-access when the reset starts, or one that gets re-armed after the
new cancel?

Hm. I'll update the commit message.

Second, on the Fixes tag:

quoted

Fixes: 4809671015a1 ("ice: Fix E810 PTP reset flow")

[Severity: Medium]
Is this the right commit to reference?  git grep ov_work at 4809671015a1
under drivers/net/ethernet/intel/ice/ returns no matches, so the E82x
offset verification work did not exist yet at that commit, and the code
being changed here is the ICE_MAC_GENERIC branch of the PTP reset flow
rather than the E810 reset flow.  Stable tooling will use this tag to pick
backport targets.

I'll check. I'm not sure how the original author landed on this commit.

Third, on the reference to an existing cancellation:

quoted

Cancel ov_work in ice_ptp_prepare_for_reset() alongside the existing
cancellation of the Tx tracker. The comment in the existing code already
documents that ov_work interference during reset is a concern but only
cancels it during ice_ptp_release().

[Severity: Medium]
Does ice_ptp_prepare_for_reset() still release the Tx tracker in this
tree?  The ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx) call that used
to be there was removed by the earlier patch in this series, "ice: fix
removal of PTP timestamp tracker during reset", so there is nothing left
in that function to cancel ov_work "alongside".

Which existing comment documents ov_work interference during reset?  At the
series baseline the only ov_work-adjacent comments are "Start the PHY timer
in Vernier mode" and "temporarily disable Tx timestamps while calibrating
PHY offset", and neither mentions reset.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index d018f02f700d7..9bf3bac552f11 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

@@ -2998,6 +2998,14 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (reset_type == ICE_RESET_PFR)
 		return;
 
+	/* Cancel the offset verification work for E82x before releasing the
+	 * Tx tracker. If ov_work is running during reset, it may issue
+	 * sideband queue commands that will fail or timeout, and may
+	 * reference state that is being torn down.
+	 */

[Severity: Medium]
Same point as above for the comment text itself: ice_ptp_prepare_for_reset()
does not release the Tx tracker anymore, so "before releasing the Tx
tracker" describes an ordering that does not exist here.  The only
remaining ice_ptp_release_tx_tracker() call is in ice_ptp_release(), and
there it runs before the ICE_MAC_GENERIC ov_work cancel reached via
ice_ptp_port_phy_stop(), i.e. the reverse order.  Would it be clearer to
drop that clause?

Yes.

The second sentence also mentions sideband queue commands, but
ice_ptp_wait_for_offsets() returns as soon as ice_is_reset_in_progress() is
true, before any sideband access.  Which state that is being torn down does
the work reference?

quoted

+	if (hw->mac_type == ICE_MAC_GENERIC)
+		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);

[Severity: High]
Should this cancel be taken under ptp->port.ps_lock?  Both existing
ov_work cancel sites hold it, and ice_ptp_port_phy_restart() holds it
across cancel, PHY start and re-queue:

	mutex_lock(&ptp_port->ps_lock);
	...
	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
	...
	err = ice_start_phy_timer_e82x(hw, port);
	...
	kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0);

Without ps_lock here, can ov_work be armed again right after the sync
cancel returns, so the reset sequence still runs with ov_work queued?

Two arming paths look reachable during the reset window.  One is the
PTP_CLOCK_SETTIME ioctl, which has no ptp->state or reset-in-progress
check:

ice_ptp_settime64()
	/* Recalibrate and re-enable timestamp blocks for E822/E823 */
	if (hw->mac_type == ICE_MAC_GENERIC)
		ice_ptp_restart_all_phy(pf);

The other is the clock-owner PF on a multi-PF E82x adapter, where
ice_ptp_rebuild_owner() -> ice_ptp_restart_all_phy() walks the adapter port
list and arms a peer PF's ov_work:

	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
		...
		if (port->link_up)
			ice_ptp_port_phy_restart(port);

while that peer PF's own service task is inside
ice_ptp_prepare_for_reset() on another CPU.  There is no cross-PF reset
serialization for E82x, since ice_ptp_prepare_rebuild_sec() is gated on
ICE_MAC_GENERIC_3K_E825 in the following hunk.

Would doing the cancel inside the ps_lock critical section, or simply
reusing ice_ptp_port_phy_stop(), close that window?  The ov_work callback
ice_ptp_wait_for_offsets() only takes port->tx.lock with
spin_lock_irqsave(), never ps_lock, so taking ps_lock around the sync
cancel should not deadlock.

Makes sense.

[Severity: Low]
Separately, who re-arms ov_work for a non-owner PF after this cancel?
ice_ptp_wait_for_offsets() is the only place that programs the vernier
offsets and clears the calibrating flag:

	port->tx.calibrating = false;

and ice_ptp_is_tx_tracker_up() gates every Tx timestamp on it:

	return tx->init && !tx->calibrating;

At this commit the only re-arm during reset is ice_ptp_restart_all_phy()
from ice_ptp_rebuild_owner(), which runs on the clock-owner PF only, for
non-PFR only, for link-up ports only, and can arm a peer PF's ov_work just
before that peer cancels it here.  Note this appears to be resolved by the
next patch in the series, "ice: call PTP link change only from link
events", which adds an unconditional per-PF ice_ptp_link_change() call at
the end of ice_rebuild(); with that in place every PF re-arms its own
ov_work after its own rebuild.  Worth mentioning in the changelog that this
patch depends on that follow-up?

Yes at a minimum we'll have to reference that, possibly re-ordering the
commits.

[Severity: Medium]
This isn't a bug introduced by this patch, but the state that
ice_ptp_prepare_for_reset() leaves behind interacts with an incomplete
teardown path.  ice_ptp_release() early-returns whenever
pf->ptp.state is neither ICE_PTP_UNINIT nor ICE_PTP_READY:

	if (pf->ptp.state != ICE_PTP_READY) {
		mutex_destroy(&pf->ptp.port.ps_lock);
		ice_ptp_cleanup_pf(pf);
		if (pf->ptp.clock) {
			ptp_clock_unregister(pf->ptp.clock);
			pf->ptp.clock = NULL;
		}
		return;
	}

ice_ptp_cleanup_pf() only removes the port from the adapter list and drops
the kref, so unloading the driver while the state is ICE_PTP_RESETTING
(set here) or ICE_PTP_ERROR (set on the ice_ptp_rebuild() failure path)
leaks the kthread worker created by kthread_run_worker() in
ice_ptp_init_work() and the tx->tstamps allocation from
ice_ptp_alloc_tx_tracker(), and does not sync-cancel ptp->work or
port.ov_work before the devm-allocated ice_pf embedding them goes away.
That branch also leaves state non-UNINIT, unlike the invariant the comment
above ice_ptp_release() describes.

I believe this will be addressed by other changes in the series.

quoted

 	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
 		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
 

[ ... ]