Re: [PATCH net 00/15][pull request] ice: E82x: timestamp processing logic fixes

On 9/10/2026 5:34 PM, Tony Nguyen wrote:

Jake Keller says:

This series contains several related fixes for the ice driver PTP logic
relating to timestamp handling and device (re)initialization.

Of particular note is some changes around the handling of timestamps that
are requested near device state changes such as administrative up/down
cycles and link change events.

The E825 device logic in the PHY has an internal counter which is used as
part of the "threshold" logic which determines when the device will trigger
an interrupt signal from a given PHY port to the MAC. This internal counter
requires some precise handling to ensure that the internal device state
remains in sync with software expectations. Otherwise, the device can be
finagled into a state where the PHY stops producing new timestamp interrupt
notifications to the MAC indefinitely. This in turn degrades the timestamp
processing latency and results in application failures for common
timestamping applications such as ptp4l.

There are four major categories of problem resolved by this series:

 * Timestamp requests made while the PHY_REG_TX_OFFSET_READY bit is cleared
   will increment the internal counter, but leave their valid bit set to 0.
   Upon read, the counter is not decremented. This leads to a desync of the
   counter and blocks the PHY interrupt.

 * Software logic for tracking timestamps incorrectly cleared in-use bits
   without waiting for completion in certain cases. If the timestamp *does*
   later complete, it leaves an "orphaned" ready bit which is not
   tracked by software. This results in the internal counter becoming
   desynced if that index is re-used.

 * The PHY timestamp memory region lacks pull-down zero-initialization at
   power on, resulting in uninitialized random data in the memory region.
   If software reads these values, an entry with its valid bit set to 1 can
   trigger a counter decrement and cause an underflow which results in the
   counter becoming desynced.

 * Timestamp request which complete near the beginning of the PHY losing
   link can become "stuck" such that the hardware logic triggered by a
   timestamp read does not activate. The memory status bit and the valid
   bit in the timestamp index are not cleared. This window where this
   may occur begins *before* the firmware notifies the driver of link loss.
   When this occurs, the driver may accidentally re-use a stale timestamp,
   and the IRQ re-trigger logic triggers a repeated IRQ "storm" that can
   consume significant excess CPU time.

The series' primary focus is towards preventing driver flows that can
trigger the above sequences. It is based on work from Przemyslaw Korba which
was previously posted at [1]. During that series development, Petr from
RedHat reported the 3rd issue mentioned above. While attempting to root
cause that issue, several other issues were uncovered and those fixes have
also been included in this series.

First, the locking around the PTP ports list in the adapter structure is
converted to use RCU primitives and a spinlock, resolving a couple of
reports from Petr about places where the original list was accessed without
lock protection. Note that an older version of this fix used an xarray
instead of the list. The xarray has more overhead and results in an
increase of ~25 microseconds to the average latency for processing Tx
timestamps. The list is simpler and avoids this overhead.

Next, the PTP reset flow is fixed to stop tearing down the Tx tracker
during a CORE or GLOBAL reset. This avoids causing Tx timestamps to break
permanently after such a reset. This issue was found by Sashiko during
review of a previous version of this series.

Next, the ice_ptp_request_ts() function is updated to sequence the marking
of the in_use bitmap in order to work properly with the lockless reader in
the IRQ thread. This issue was reported by Sashiko during review of a
previous version of this series.

Next, come two fixes for E822 hardware that were originally posted as part
of Przemyslaw Korba's work [1]. The E822-only "vernier" offset validation
work task is properly canceled during device reset, and new timestamp
requests are kept disabled until the validation task completes.

Next, Arkadiusz modifies the driver to stop pretending that the link has
gone down during ice_down(). This removes "virtual" PTP link changes that
occurred on several flows including MTU change, Eswitch setup, and others.
Now, the driver only triggers a PTP PHY timer reinitialization when the
physical PHY link has changed instead of during many other actions.

Next, the driver is modified to stop clearing the PHY_REG_TX_OFFSET_READY
bit. This bits only purpose is to tell hardware to mark any captured
timestamps as invalid. Since this also disables the necessary side effects
on read it is problematic to have cleared. According to hardware engineers,
keeping it enabled should not have any other side effects.

Next, the driver is modified to clear the PHY_REG_TX_MEMORY_STATUS by
reading each index *prior* to the PHY soft reset. This ensures that any
stale or invalid data left in the memory array is cleared, followed by the
counter being reset via the PHY soft reset procedure.

Next, the E825 timer start procedure is modified to first include a soft
reset. This ensures that upon link up the device is reconfigured from a
known-good state with its internal counter reset and everything cleared.

Next, Petr modifies the ice_ptp_flush_tx_tracker() function to wait a little
bit for any outstanding timestamps before flushing.

Next, Petr modifies the ice_ptp_process_tx_tstamp() function to avoid
releasing any index from software unless either a) it is actually completed
by hardware or b) it is timed out waiting for a full two seconds. This
closes the final gap from the second issue mentioned above. Instead of
immediately releasing the index, the software now waits until hardware has
completed it or the driver has waited long enough to be sufficiently sure
that no such timestamp will be done.

Next, the driver is modified to no longer mark timestamps as "stale" during
a clock adjust event. This avoids marking timestamps as stale unnecessarily.

Next, the ice_ptp_process_tx_tstamp() function is modified to verify
that hardware actually cleared the ready bitmap. This ensures that we do
not report false timestamps near a link down event.

Finally, Maciek adds a needed PHY recalibration for E825-C after large system
time adjustments. Without recalibration, PHY timestamps do not properly
converge to the new time, resulting in inaccurate timestamp readings.

Link: [1] https://lore.kernel.org/intel-wired-lan/20260720120151.2675206-1-przemyslaw.korba@intel.com/ (local)
---

Sashiko had comments for this series, primarily around the RCU list
iteration. After some digging, it seems like the pattern I tried to use
won't work. I had tried a battery of LOCKDEP and spam rmmod but still
didn't manage to trigger any issue.

However, it seems like a simple solution will be to switch to sleepable
RCU so that the RCU read sections can safely sleep instead of trying to
be complicated.

I'm working on that change now to update to submit a v2.

The other feedback is as follows:

1. Regarding the choice of waiting only up to 15 seconds:

Can this timeout lead to a guaranteed use-after-free during PF teardown?
If any thread holds a reference to a port for longer than 15 seconds,
wait_var_event_timeout() returns and the memory teardown continues despite
the outstanding kref. When the delayed or stuck thread eventually resumes
and finishes using the reference, it will operate on freed memory.

We could change this to wait indefinitely. In the result of a driver bug
failing to release a reference results in the driver simply never
unloading. With this solution we wait up to 15 seconds and warn. I
suppose there could be some convoluted sequence that allows a reference
to be maintained for greater than 15 seconds and result in
use-after-free. But it does require creating this stuck condition, and
in either case its a driver bug. We felt that it was better to not block
indefinitely in the obvious and most likely case that some flow forgot
to kref_put vs the much more unlikely sequencing requiring somehow
delaying one of the iterations for an entire 15 seconds.

2. Regarding removal of the ice_ptp_release_tx_tracker()

Does removing this call introduce an SKB leak and tracker memory leak if the
PTP rebuild fails after reset?
Without this call, the tracker is not flushed and tx->init remains 1 during
the reset. If the PTP rebuild subsequently fails (e.g., due to a hardware
timeout), ptp->state becomes ICE_PTP_ERROR and the PTP interrupt remains
permanently disabled.
Because tx->init is still 1 and the link is up, ice_ptp_request_ts() passes
its tracker check and continues to accept new SKBs for timestamping:
drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_request_ts() {
    ...
    if (!ice_ptp_is_tx_tracker_up(tx)) {
        spin_unlock_irqrestore(&tx->lock, flags);
        return -1;
    }
    ...
}
Since the PTP interrupt is disabled, ice_ptp_process_tx_tstamp() never
executes, so these SKBs are never processed or evaluated for timeouts,
piling up indefinitely.
Furthermore, when the driver is eventually unloaded, ice_ptp_release() sees
the error state and takes an early return:
drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_release() {
    ...
    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;
    }
    ...
}
This early return completely skips ice_ptp_release_tx_tracker(), permanently
leaking the tracker arrays and all stranded SKBs.

Perhaps we should tear down if we fail when going into the error state.
I will need to investigate. This may best be served as a follow up fix.


3. Regarding the in_use bit cleanups with memory barriers

This is a pre-existing issue, but does the fix appear to be incomplete? While
this adds the necessary writer-side memory barrier, does the lockless reader
side also need a paired read memory barrier?
In ice_ptp_process_tx_tstamp(), the reader iterates over the in_use bits:
	for_each_set_bit(idx, tx->in_use, tx->len) {
		...
		/* Drop packets which have waited for more than 2 seconds */
		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
			drop_ts = true;
			...
		}
Without an smp_rmb() after discovering the set in_use bit, couldn't a weakly
ordered architecture (like ARM64) speculatively load tx->tstamps[idx].start
before verifying the in_use bit?
If the reader loads a stale start time and sees it as older than 2 seconds,
would it mark drop_ts = true and incorrectly free the newly queued SKB?
A similar lockless read of tx->tstamps[idx].skb happens shortly after:
		ice_trace(tx_tstamp_fw_req, tx->tstamps[idx].skb, idx);
Could this also lead to dereferencing a stale or NULL SKB pointer?

I think strictly we would an additional memory barrier when reading the
in_use bit. Will fix.


4. Regarding removal of ice_ptp_link_change

Does removing this call expose a race condition during initialization?
In ice_ptp_init(), if a physical link-up interrupt fires after 
ice_ptp_reset_phy_timestamping() completes but before ptp->state is set 
to ICE_PTP_READY, the interrupt handler will call ice_ptp_link_change():
drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_link_change() {
    ...
    ptp_port->link_up = linkup;
    if (pf->ptp.state != ICE_PTP_READY)
        goto exit_kref_put;
    ...
}
Because ptp->state is not yet ICE_PTP_READY, it aborts early without
configuring the PHY. Previously, the call here in ice_up_complete() acted as a 
failsafe that would configure the PHY once the interface was brought up.
With it removed, if this race occurs, can the PTP hardware timestamping fail 
completely and silently until the next physical link toggle?

I don't believe this is correct. We now initialize the link state to the
current link status when initializing PTP. Additionally, the
ice_ptp_link_change call that triggers on an interrupt handler also sets
the link state before checking the PTP state, so we will then be correct.

5. Regarding the soft reset in ice_start_phy_timer_eth56g

Does this initiate a 10ms PHY soft reset without mutual exclusion against
concurrent Tx timestamp polling?
A link flap triggers ice_ptp_port_phy_restart(), which takes ps_lock and
initiates a PHY soft reset via ice_ptp_phy_soft_reset_eth56g().
Concurrently, the PTP Tx interrupt or periodic watchdog executes
ice_ptp_tx_tstamp_owner() -> ice_ptp_process_tx_tstamp(), which bypasses
ps_lock and issues PHY reads (e.g., ice_get_phy_tx_tstamp_ready()) via the SBQ.
Note that tx->calibrating is not set for E825 devices during this process to
temporarily halt polling.
Accessing PHY registers over the SBQ while the PHY is actively held in soft
reset (between the assert and clear of PHY_REG_GLOBAL_SOFT_RESET_M) causes I/O
timeouts. This can lead to SBQ timeouts, firmware lockups, and internal
timestamp counter corruption.

This is hallucinating behavior that doesn't exist.

Furthermore, ice_ptp_phy_soft_reset_eth56g() manually clears timestamp indices
without locking out the polling thread, creating a race on the read-to-clear HW
mechanism:
drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_ptp_phy_soft_reset_eth56g() {
    ...
	err = ice_ptp_clear_tx_memory_status_eth56g(hw, port);
	if (err) {
    ...
}
Does this PHY soft reset wipe the Tx timestamp interrupt configuration without
restoring it?
The soft reset restores PHY registers to their default state, which would clear
PHY_REG_TS_INT_CONFIG.

The soft reset doesn't reset PHY_REG_TS_INT_CONFIG.

While ice_start_phy_timer_eth56g() manually restores MAC, PARPCS, and 1-step
configurations, it omits a call to ice_phy_cfg_intr_eth56g() to restore the
interrupt enablement and threshold.
This can result in the loss of Tx timestamp interrupts on the affected port,
causing timestamp polling to fall back to the slow periodic watchdog or hang
entirely.

This entire block seems to be hallucinating PHY behavior, so I do not
believe it is relevant.


TL;DR; I am working updating this to use a sleepable RCU context which
should address the majority of the complaints from Sahisko, and I
believe the remaining complaints are not valid.

Thanks,
Jake

IWL: https://lore.kernel.org/intel-wired-lan/20260825-jk-e825c-minimized-fixes-v2-0-8223f95d26e3@intel.com/ (local)

The following are changes since commit 78445023439506ebd83b86d40b1e428a3b309d4a:
  Merge tag 'net-7.3-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Arkadiusz Kubalewski (1):
  ice: call PTP link change only from link events

Jacob Keller (9):
  ice: use reference counting and RCU for PTP port access
  ice: fix removal of PTP timestamp tracker during reset
  ice: set in_use only after preparing Tx timestamp index
  ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
  ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset
  ice: E825: perform a soft reset when starting the PHY timer
  ice: remove unnecessary discarding of timestamps after clock adjust
  ice: skip reading Tx ready bitmap on ports with no timestamps
  ice: don't clear in_use until HW clears ready bitmap

Karol Kolacinski (2):
  ice: E822: keep Tx timestamps disabled during offset calibration
  ice: E822: cancel offset verification work during reset preparation

Maciek Machnikowski (1):
  ice: Recalibrate PHY after settime64 on E825-C

Petr Oros (2):
  ice: wait for in-flight Tx timestamps before flushing the tracker
  ice: keep Tx timestamp slots tracked until completion or timeout

 drivers/net/ethernet/intel/ice/ice_adapter.c |   7 +-
 drivers/net/ethernet/intel/ice/ice_adapter.h |   6 +-
 drivers/net/ethernet/intel/ice/ice_main.c    |  12 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 358 +++++++++++++------
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  12 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 104 +++---
 6 files changed, 326 insertions(+), 173 deletions(-)