Re: [PATCH net v2 01/15] ice: fix removal of PTP timestamp tracker during reset

On 10/12/2026 4:15 PM, netdev-bot+sashiko@kernel.org wrote:

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 3 (4) · 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] The new non-READY (ERROR/RESETTING) branch of ice_ptp_release()
  destroys the worker too early.
- [Medium] The patch removes ice_ptp_release_tx_tracker() from
  ice_ptp_prepare_for_reset() and puts nothing in its place.
- [Medium] At this commit, when ice_ptp_init_work() fails, the code now
  jumps to err_clean_pf.
- [Medium] Now that the Tx tracker survives CORE/GLOBAL/EMP resets, its
  per-index tstamps[idx].cached_tstamp and last_ll_ts_idx_read are never…

Pre-existing issues:
- [Medium] The non-READY branch of ice_ptp_release() tears everything
  down: kworker, clock, list_del via ice_ptp_cleanup_pf(), the Tx…
- [Medium] At this commit, ice_ptp_flush_all_tx_tracker(),
  ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec() walk…
- [Medium] On E825, ice_ptp_setup_pf() sets this port's bit in the
  control PF's shared tx_refclks[phy][tx_clk] map.
- [Medium] Neither the ice_ptp_init() error unwind (after
  ice_ptp_reset_phy_timestamping() and ice_ptp_cfg_tx_interrupt()) nor…

--

ice: fix removal of PTP timestamp tracker during reset

This commit stops releasing the PTP Tx timestamp tracker in
ice_ptp_prepare_for_reset(), so timestamping can resume after CORE, GLOBAL
or EMP resets. It reorders ice_ptp_init() so the tracker is allocated
before the port is added to the port list, and the tracker is released on
error. ice_ptp_release() now cancels the kworker and releases the tracker
when PTP is not in the READY state.

Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=8
Closes: https://lore.kernel.org/netdev/20260916011213.1632286-1-kuba@kernel.org/ (local)

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3f..fe21cee4f9dee 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

@@ -2939,8 +2939,6 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
 		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
 
-	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
-

[Severity: Medium]

With this call removed, nothing clears tx->init or calls synchronize_irq()
before a CORE, GLOBAL or EMP reset. Is that intended?

Yes, this was intended.

The Tx timestamp paths check only tx->init and tx->calibrating. They never
check ptp->state. Suppose ice_ptp_rebuild() fails:

ice_ptp_rebuild()
    err:
        ptp->state = ICE_PTP_ERROR;

ice_rebuild() still calls ice_ptp_restore_timestamp_mode()
unconditionally, and that re-enables the Tx timestamp interrupt. After
that, ice_tstamp()->ice_ptp_request_ts() only checks:

    if (!ice_ptp_is_tx_tracker_up(tx)) {

So slots are still handed out (skb_get() plus SKBTX_IN_PROGRESS) against a
PHC that isn't running. In the ERROR state cached_phc_jiffies is never
refreshed, so ice_ptp_extend_40b_ts() would discard the result. Before
this patch, these requests were refused and counted in
tx_hwtstamp_skipped.

During the reset window itself, ice_ll_ts_intr() only checks tx->init:

    if (tx->init) {
        ice_ptp_complete_tx_single_tstamp(tx);

The E810 path in ice_ptp_ts_irq() also checks ice_pf_state_is_nominal().
Can the LL proxy registers now be accessed in the middle of a reset?

I am not certain any of this actually results in a real issue. We could
add a check to prevent new timestamp requests while the state is
non-ready I suppose.. But I don't know if anything here actually fails.
The messaging could be improved. In practice I am unsure if the driver
actually *can* timestamp here since it would have stopped the traffic
queues during queisce before reset.

The commit message doesn't mention this change in behaviour. Later patches
in the series only add link_up and calibrating gating, so tx->init still
stays set across the reset.

Yes, tx->init stays valid, because we need to keep the tracker and
removing it was problematic. Checking the reset state may make sense but
I am not convinced this is a real visible problem.

[Severity: Medium]

The tracker now survives the reset, so tstamps[idx].cached_tstamp and
last_ll_ts_idx_read are no longer reset. The E810 PHY timestamp memory is
still reset. Can the two get out of sync?

On rebuild, ice_ptp_rebuild_owner()->ice_ptp_flush_all_tx_tracker()->
ice_ptp_flush_tx_tracker() calls ice_clear_phy_tstamp() for every in_use
index, because tstamp_ready is all ones on E810.
ice_clear_phy_tstamp_e810() then writes 0 to both halves:

    err = ice_write_phy_reg_e810(hw, lo_addr, 0);
    ...
    err = ice_write_phy_reg_e810(hw, hi_addr, 0);

E810 has no ready bitmap. It detects a new capture by checking raw_tstamp
!= cached_tstamp. After the reset, a reused index that hasn't captured yet
reads 0, and 0 differs from the stale cached value.

In ice_ptp_process_tx_tstamp(), the missing VALID bit makes it drop the
timestamp and release the slot. Because raw is 0, cached_tstamp isn't
updated, so the later real capture could be credited to the next request
that uses that index.

This looks like a preexisting issue that we should clear the cached
value when writing the PHY registers for E810. This should be fixed, but
IMO should not block this series.

ice_ptp_complete_tx_single_tstamp() has no VALID check:

    if (raw_tstamp == tx->tstamps[idx].cached_tstamp)
        return;

    tx->tstamps[idx].cached_tstamp = raw_tstamp;
    clear_bit(idx, tx->in_use);

It would then pass ice_ptp_extend_40b_ts(pf, 0) to skb_tstamp_tx().

This also looks like a preexisting issue in the low latency path that
should have been checking valid bits.

Before 7a25fe5cd5fb the tracker was reallocated with zeroed cached values,
so software and hardware stayed in sync. This is still present at the end
of the series.

Before this series, timestamping stopped permanently after a device reset.

quoted

 	/* Disable periodic outputs */
 	ice_ptp_disable_all_perout(pf);
 

@@ -3347,13 +3345,13 @@ void ice_ptp_init(struct ice_pf *pf)
 		}
 	}
 
-	err = ice_ptp_setup_pf(pf);
+	err = ice_ptp_init_port(pf, &ptp->port);
 	if (err)
-		goto err_exit;
+		goto err_destroy_ps_lock;
 
-	err = ice_ptp_init_port(pf, &ptp->port);
+	err = ice_ptp_setup_pf(pf);
 	if (err)
-		goto err_clean_pf;
+		goto err_release_tx_tracker;
 
 	/* Start the PHY timestamping block */
 	ice_ptp_reset_phy_timestamping(pf);

[Severity: Medium]

This isn't a bug introduced by this patch, but neither the ice_ptp_init()
error unwind nor the non-READY branch of ice_ptp_release() puts the
hardware back into a quiet state.

True. These are *error cleanup* flows which are just trying to do their
best to cleanup and avoid leaks.

ice_ptp_reset_phy_timestamping() and ice_ptp_cfg_tx_interrupt() run before
ice_ptp_init_work(). If ice_ptp_init_work() then fails, the unwind never
calls ice_ptp_disable_timestamp_mode(), ice_ptp_port_phy_stop() or the
perout/extts disables.

Can OICR TSYN_TX, PHY timestamping and the SDP outputs stay armed after the
software state has been torn down?

Presumably yes if you fail at exactly the right way. But the software
really has no way to know which parts would need to be torn down, and
whether it even has access. Should we balloon the error handling logic
to try and detect and cleanup every possible state here? I don't think
thats helpful.

[ ... ]

quoted

@@ -3365,14 +3363,17 @@ void ice_ptp_init(struct ice_pf *pf)
 
 	err = ice_ptp_init_work(pf, ptp);
 	if (err)
-		goto err_exit;
+		goto err_clean_pf;

[Severity: Medium]

When ice_ptp_init_work() fails, this path now frees the Tx tracker and
destroys ps_lock. However, ptp->state was set just above:

    ptp->state = ICE_PTP_READY;

    err = ice_ptp_init_work(pf, ptp);
    if (err)
        goto err_clean_pf;

The PHC is also still registered at this point. Could ice_ptp_link_change(),
ice_ptp_queue_work() or the PHC callbacks touch the freed tracker or the
destroyed mutex in this window? One example is
ice_ptp_reset_cached_phctime()->ice_ptp_mark_tx_tracker_stale().

The NULL kworker exposure while READY was already there before. Later in
the series, ice_ptp_init() creates the kworker earlier and sets
ICE_PTP_READY only after ice_ptp_init_work() succeeds, which appears to
close this. At this commit the window is still open.

This has been incredibly frustrating, as every time I change the order
here I end up with some other complaint about teardown order :\

We should be cleaning up the PTP clock first, but this is complicated by
the fact that each PF unloads independently and only the clock owner PF
actually has a clock. This outline does imply that the paths could
access the kworker while the port is still in the list, so ya I guess
this really should be afterwards.

The worker thread does appear to check the PTP state so I guess
reordering it would be safe.

quoted

 
 	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
 	return;
 
 err_clean_pf:
-	mutex_destroy(&ptp->port.ps_lock);
 	ice_ptp_cleanup_pf(pf);

[Severity: Medium]

This is a pre-existing issue, but on E825 ice_ptp_setup_pf() sets this
port's bit in the control PF's shared map:

    set_bit(port_num,
            &ctrl_ptp->tx_refclks[phy][pf->ptp.port.tx_clk]);

ice_ptp_cleanup_pf() only does list_del() and never clears that bit. After
this error path, or after an unbind, would ice_txclk_any_port_uses() still
count this port as a user of the reference clock?

This looks like an issue for the developer of the refclk code to fix.

quoted

+err_release_tx_tracker:
+	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
+err_destroy_ps_lock:
+	mutex_destroy(&ptp->port.ps_lock);
 err_exit:

[ ... ]

quoted

@@ -3399,12 +3400,20 @@ void ice_ptp_release(struct ice_pf *pf)
 		return;
 
 	if (pf->ptp.state != ICE_PTP_READY) {
-		mutex_destroy(&pf->ptp.port.ps_lock);
-		ice_ptp_cleanup_pf(pf);
+		if (pf->ptp.kworker) {
+			kthread_cancel_delayed_work_sync(&pf->ptp.work);
+			if (pf->hw.mac_type == ICE_MAC_GENERIC)
+				kthread_cancel_delayed_work_sync(&pf->ptp.port.ov_work);
+			kthread_destroy_worker(pf->ptp.kworker);
+			pf->ptp.kworker = NULL;
+		}

[Severity: High]

The worker is destroyed here and pf->ptp.kworker is set to NULL. The PHC is
only unregistered further down, and the port stays on
adapter->ports.ports until ice_ptp_cleanup_pf() runs. Can anything still
reach this worker in between?

On ICE_MAC_GENERIC, ice_ptp_settime64() calls ice_ptp_restart_all_phy()
without checking ptp->state. ice_ptp_adjtime() does the same through its
non-atomic path. ice_ptp_restart_all_phy() walks the list without
ports.lock:

    list_for_each(entry, &pf->adapter->ports.ports) {
        ...
        if (port->link_up)
            ice_ptp_port_phy_restart(port);
    }

ice_ptp_port_phy_restart() then does:

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

Here pf is the releasing PF. That covers the releasing PF's own port, and
also its port when it is a peer and the owner PF is the one calling
settime.

ice_ptp_reset_cached_phctime() also queues on pf->ptp.kworker when
ICE_CFG_BUSY is set, with no NULL or state check.

Before this patch the non-READY branch leaked the worker rather than
freeing it, so this NULL dereference or use-after-free of the
kthread_worker looks new. It can be hit if a PF in ERROR or RESETTING
state is unbound while userspace calls clock_settime() on the owner PHC.

Could the clock be unregistered and the port removed from the list before
the worker is destroyed? The commit message already describes that
ordering for ps_lock. The ordering here is unchanged at the end of the
series.

Yea, I guess we need to do the cleanup first and then teardown the
kworkers.. This has been quite frustrating because I believe I got other
complaints about order when trying to do that way before :\ Hopefully we
can find something that works...

quoted

 		if (pf->ptp.clock) {
 			ptp_clock_unregister(pf->ptp.clock);
 			pf->ptp.clock = NULL;
 		}
+		ice_ptp_cleanup_pf(pf);
+		ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);

[Severity: Medium]

This is a pre-existing issue, but three functions walk
adapter->ports.ports without ports.lock: ice_ptp_flush_all_tx_tracker(),
ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec(). For example:

    list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
        ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);

Meanwhile ice_ptp_cleanup_pf() unlinks the port under the lock, and
ice_ptp_release_tx_tracker() then frees the tracker. Trackers now stay
allocated across CORE/GLOBAL resets, so the owner's rebuild-time flush
works on live peer trackers. Can one of these walkers race with this
teardown?

A later patch in this series, "ice: use reference counting and SRCU for
PTP port access", converts these walkers to SRCU with kref and appears to
close this race.

Yes. I tried to have that patch first but it got far too many complaints
about *this* issue, so I re-ordered the series.

quoted

+		mutex_destroy(&pf->ptp.port.ps_lock);
 		return;
 	}

[Severity: Medium]

This isn't a bug introduced by this patch, but this branch tears
everything down and returns without setting pf->ptp.state to
ICE_PTP_UNINIT. If ice_ptp_release() runs again before ice_ptp_init()
resets the state, would ice_ptp_cleanup_pf() call list_del() on a node
that already holds LIST_POISON pointers?

Another on the pile of possible pre-existing issues... :D

One possible sequence:

1. devlink reinit_down runs while PTP is in ERROR or RESETTING, which goes
   through ice_unload()->ice_ptp_release().
2. reinit_up fails before ice_load().
3. ice_remove()->ice_unload()->ice_deinit_features()->ice_ptp_release()
   runs again.

The old branch had the same list_del() without a state change, and the end
of the series still doesn't set ICE_PTP_UNINIT here.

Problem is that I am not sure if setting to UNINIT is totally correct
here :\ Of course the original design/logic here was that this is part
of driver remove and not something that would be re-triggered such as
via devlink.