[PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[Jacob Keller]

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 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. The fix also ensures that all teardown
paths properly release the tracker and kworker in a safe order. This issue
was found by Sashiko during review of a previous version of this series. 

Next, 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 PHY restart locking is fixed to avoid an issue with a concurrent
execution of ice_ptp_restart_all_phy() and a link change on a given port.
Additionally, the PHY port locks are merged into a single per-adapter lock
to reduce locking complexity.

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, Arkadiusz and Jake modify the driver to stop pretending that the link
has changed at administrative ice_down() and ice_up(). 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, 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, 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. Instead, the
tx.calibrating field is used to disable new timestamps from software in a
similar manner to the older E822 devices.

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.

Finally, 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.

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

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
For those interested, here is a summary of the latency numbers for Tx
timestamps on my setup for comparison throughout the series. In all cases,
the ptp4l test used a profile with a sync rate of 16/second and the
"torture" test additionally operated a second thread on another port
operating a burst of 32 concurrent timestamp requests every 10
milliseconds, all operating on E825 hardware, with a 3 minute capture time
for each test.

1. Before this series

  ptp4l only
  Mean: 298.67 microseconds, stddev: 28.60

  ptp4l + torture
  Mean: 637.02 microseconds, stddev: 351.59

2. After SRCU rework

  ptp4l only
  Mean: 314.10 microseconds, stddev: 32.99

  ptp4l + torture
  Mean: 622.68 microseconds, stddev: 343.57

3. Everything up to keeping Tx timestamps tracked until completion

  ptp4l only
  Mean: 317.43 microseconds, stddev: 34.51

  ptp4l + torture
  Mean: 622.31 microseconds, stddev: 339.26

5. After the full series (including rechecking the ready bitmap)

  ptp4l only
  Mean: 195.92 microseconds, stddev: 25.28

  ptp4l + torture
  Mean: 717.77 microseconds, stddev: 321.58

The run-to-run variance here is somewhat high, but its clear that
timestamping across multiple ports under heavy load has a significant
latency cost. This is to be expected due to the nature of serializing the
timestamps to a single IRQ. In the usual cases with a lower timestamp load
and especially if ports do not have active timestamp requests, the series
has a decent reduction in timestamp latency. The use of Sleepable RCU does
seem to have a minor latency cost but it is overshadowed by the improvement
to elide checking when there are no requests in software.
---
Changes since v4:
- Resend with the appropriate subject
Changes since v3:
- Add <linux/srcu.h> to ice_adapter.h
- Squash the READ_ONCE/WRITE_ONCE and init re-ordering from later patches
  into the first patch about PHY restart serialization.
- Call ice_ptp_cfg_tx_interrupt() before setting the state to
  ICE_PTP_READY.
- Update the link state in ice_update_pf_netdev_link by setting
  get_link_info to true, ensuring that we have up-to-date link settings.
- Remove a stale comment about the ordering of ice_ptp_rebuild().
- Always call ice_ptp_link_change instead of only when the link state
  doesn't match the cached value. Instead, check the ptp_port->link_up
  field, to ensure that PTP is properly restored to the expected state.
  This should hopefully finally remove the last few races that Sashiko has
  pointed out...
- Always call ice_ptp_port_phy_restart for E825 instead of only on link up.
  This will stop the PHY (set tx.disabled to true and clear the Rx offset
  ready flag). This matches the behavior of a restart triggered from
  another flow besides link-up and better guarantees that we disable
  timestamp requests immediately.
- Rename tx.calibrating to tx.disabled and update comments for clarity.
- Update the dev_err message on a failed port restart to clarify that
  timestamping is not enabled. Hopefully this will be clear to the AI about
  the intended meaning.
- Update the ice_ptp_reset_ts_memory to more clearly indicate that it only
  really does something on older E82X hardware. Update the kdoc comment and
  switch statement.
- Revert back to using ice_ptp_mark_tx_tracker_stale instead of trying to
  flush the tracker. This better reflects the more widely tested code and
  avoids many issues reported in v3. This does come with the potential to
  leave indexes locked for 2 seconds while waiting for the timestamps to
  complete, but this is a minor issue compared to the potential failures
  reported by Sashiko. This is also what has been widely tested on several
  systems at Intel and RedHat.
- Drop the patch to wait for in-flight timestamps during tracker flush.
  This was already relegated to only during remove, and after more comments
  from Sashiko I do not think it will be helpful. It may have aided us in
  debugging, but I think it really can't be correct. If it is required, we
  can't really time out since that could leave a window where we end up
  with a stale counter. But we already assume that might happen and soft
  reset on re-init and after rebuild anyway. Since we're going to reset
  there isn't much benefit to trying to wait.
- Remove the timeout logic from ice_ptp_flush_tx_tracker since that was
  only added because we called it from link change. This reverts back to
  the v2 behavior.
- Change the logic in ice_ptp_maybe_trigger_tx_interrupt to match the
  original logic moved from the ice_ptp_tx_tstamps_pending() call. Now the
  IRQ will still be retriggered in the event that there are software
  timestamps but the ice_check_phy_tx_tstamp_ready() has a transient read
  failure.
Changes since v2:
- Allocate kworker before inserting the PTP port into the adapter port
  list.
- Fix cleanup logic to avoid deleting the kworker until the port has been
  removed from the port list.
- Mark the pf->ptp.state as ICE_PTP_UNINIT in ice_remove() after performing
  cleanup for the ICE_PTP_ERROR state.
- Add the necessary smp_mb__after_atomic() memory barrier before calling
  wake_up_var().
- Re-write the description for the commit that removes
  ice_ptp_link_change() from ice_up_complete() and ice_down() for clarity
  and to focus on the main issues.
- Move the seeding of PTP link status into ice_init_link prior to the
  service task intialization.
- Have ice_ptp_link_change update the link_up variable regardless of
  whether the PTP state is ready, ensuring no edges are missed.
- Set the PTP state to ICE_PTP_READY before performing the final port
  restart, ensuring that we can't have the port set to down by
  ice_ptp_init() and then have ice_ptp_link_change try to set the state to
  up but skip the restart by seeing a stale PTP state value.
- Cancel E822 offset verification for PF reset as well as CORE and GLOBAL
  reset. Update the commit description to match.
- Move the tx->init check in ice_ptp_mark_tx_trakcer_stale to an earlier
  commit in the series.
- Remove the erroneous "on link-up" text from a dev_err message.
- Fix the commit message mentioning clearing the PHY_REG_TX_MEMORY_STATUS
  to correctly reflect it is clearing the PHY_TSTAMP memory region not the
  PHY_REG_TX_MEMORY_STATUS ready bitmap.
- Update several kdoc comments to better reflect the newly updated code.
- Call ice_ptp_flush_tx_tracker() instead of
  ice_ptp_mark_tx_tracker_stale() when starting the E825 PHY. This
  immediately drops the packets instead of blocking the index for 2
  seconds, and is safe because the PHY will be soft reset as part of the
  restart procedure.
- Only wait for in-flight Tx timestamps before tearing down the tracker,
  rather than for all invocations of ice_ptp_flush_tx_tracker(). The other
  callers all reset the timestamp memory region and restart the PHY so E825
  will soft reset anyways.
- Immediately free the SKB when marking a timestamp as stale due to a stuck
  ready bit. This avoids potentially keeping the associated socket alive
  indefinitely. Theoretically we could do the same for
  ice_ptp_mark_tx_tracker_stale() as well, but those flows don't lock an
  index indefinitely and should always eventually time the index out after
  2 seconds. Such a cleanup has been left as an exercise for the future.
- Drop the E825 recalibration fix. Sashiko pointed out that the restart
  interacts poorly with the HW semaphore and could result in commands
  overwriting other actions. Additionally, this fix is incomplete as we
  discovered some flaws with how the E825 device manages the PHY timer, and
  a more complete fix is being developed separately.
- Count timeouts from ice_ptp_flush_tx_tracker() in the case where a
  timestamp is flushed while being >2 seconds old.
- Fix the comment about smp_rmb() in ice_ptp_process_tx_tstamp() to better
  reflect what it orders.
Changes since v1:
- Drop the timeout on waiting for kref drain. The synchronize_srcu() makes
  it moot, and its only purpose is for a case that won't happen without
  programming errors like a reference leak.
- Check for errors on init_srcu_struct() when initializing.
- Hold the ps_lock in ice_ptp_link_change() earlier, as it is safe to
  acquire around the dplls.lock, which simplifies the reasoning for the
  given patch as well as following patches in the series.
- Re-order ice_ptp_init_port() in ice_ptp_init() to happen prior to
  ice_ptp_setup_pf() so that the Tx timestamp tracker is already
  initialized before the PTP port is inserted into the list. Ensure that we
  also tear the tracker down only after the port has been removed from the
  list.
- Re-order the patch that adresses PTP link changes to be before E822
  fixes, since AI pointed out that one of the issues was addressed by the
  link change re-ordering.
- Revert back to kthread_cancel_delayed_work_sync() instead of
  kthread_flush_work() since the latter doesn't safely handle a delayed
  work task.
- Fix the ordering of the parameters in the warning in
  ice_clear_ptp_tstamp_eth56g().
- Add a note about the E810 hardware skipping the
  ice_ptp_maybe_trigger_tx_interrupt(). The hardware doesn't have the same
  ready bitmap issues as E822 and E825. There is a possible gap because
  nothing will re-arm and sweep the tracker if all 64 slots are filled.
  However, there is a pre-existing issue on E810 where triggering a new
  timestamp interrupt could cause issues with outstanding low latency
  requests. I can't fix that in this series, and plan to address it as a
  followup.
- Check tx->init in ice_ptp_mark_tx_tracker_stale.
- Call ice_ptp_mark_tx_tracker_stale when stopping the PHY timer for E825,
  preventing existing outstanding timestamps from being reported with a bad
  offset.
- Update the dev_err on failure in ice_ptp_port_phy_restart() to better
  reflect the magnitude of the problem, and indicate a link toggle may help
  recover.
- Update some comments and kernel doc messages for clarity.
- Skip maybe_trigger_tx_interrupt for the E810 low latency interrupt path
  only, not for the older legacy paths.
- Re-order patches so that the Tx tracker fix comes first before the PTP
  port SRCU changes. This should avoid some spurious reports from Sashiko
  about ordering guarantees which are fixed by that change.
- Convert to sleepable RCU instead of the convoluted and likely broken
  dropping of RCU readlock critical sections.
- Update the messaging to be clear this does not solve the ctrl_pf access
  issues.
- Update the comments regarding the teardown and 15 second timeout to
  better reflect the intention.
- Drop the patch that reverted marking timestamps as stale during an
  adjustment. It may be safe for some smaller atomic adjustments, however
  possible issues were reported for larger adjustments by multiple models.
  Since we do not have many reports of missing timestamps, just keep the
  logic as-is.
- Add a new patch to correct serialization of the PHY port restarts to
  avoid a potential re-ordering of the link_up status causing the port to
  be left in a disabled state if a race occurs near a link up transition.
- Fix the PTP teardown during a failed reset to avoid leaking the Tx
  timestamp tracker and other PTP state.
- Add smp_rmb() to the lockless in_use reads in
  ice_ptp_process_tx_tstamp() ensuring that weak ordered architectures do
  not re-order the start time in the event of a race with a new timestamp
  request.
- Update the patch which drops the E825 clearing of PHY_REG_TX_OFFSET_READY
  with a software gate of new timestamp requests via the tx.cablibrating
  field, so that new requests will be rejected until the PHY has been
  initialized.
- Update the fixes tag for one of the E822 fixes to better reflect the
  actual kernel that introduced the problem.
- Update several commit messages and comments for clarity and accuracy as
  suggested by Sashiko during review.
- Since the E822 offset validation work already checks and reschedules when
  the driver is resetting, replace the kthread_cancel_delayed_work_sync
  with kthread_flush_work() to ensure that it sees an updated state. This
  avoids some complex changes that would otherwise be required to be
  certain that a reset with a concurrent link change wouldn't result in the
  offset task being canceled indefinitely.
- Use READ_ONCE/WRITE_ONCE when checking the link_up field of the PTP
  state.
- Treat failure to read a timestamp during the E825 sweep before a soft
  reset as non-fatal with a warning. In practice, if a read is skipped we
  still should not have a problem unless other code incorrectly reads the
  index without checking the PHY ready bitmap first. To avoid log spam if
  the device is truly inaccessible, the total read failure count is
  summarized in one message per port.
- Remove the unused soft_reset parameter from one of the E825 functions.
- Fix the watchdog re-trigger of the timestamp interrupt to work for
  devices which manage their own interrupt, instead of only on the clock
  owner.
- Fix the accounting for timed out timestamps in the unlikely case that a
  timestamp will timeout but suddenly have its ready bitmap bit stuck high.
  Now, the timeout counter is only incremented once the timestamp is
  actually dropped.
- Update commit message and comments to better reflect current
  understanding of the PHY soft reset behavior including its (non)impact on
  configuration registers.
- Switched to read_poll_timeout for the tracker drain logic to get better
  timing behavior.
- Skip the tracker drain on E810 with a has_ready_bitmap flag check.

v1: https://lore.kernel.org/netdev/20260911003430.3386340-1-anthony.l.nguyen@intel.com/
v2: https://lore.kernel.org/netdev/20261008215614.1987250-1-anthony.l.nguyen@intel.com/
v3: https://lore.kernel.org/netdev/20261015191102.1174111-1-anthony.l.nguyen@intel.com/
v4: https://lore.kernel.org/netdev/20261020-jk-e825c-timestamp-processing-logic-fixes-srcu-v4-0-3e30fc9adc73@intel.com/

Possible outstanding issues not addressed:
- ctrl_pf serialization and access is not handled by this series. Another
  developer has been investigating this and is still undergoing feedback.
  Perhaps the solution could piggyback on the port reference count, but we
  do not yet have a complete solution.
- Recent reports of missing PTP semaphore locking on certain flows are
  being investigated by another engineer and will be handled as a follow
  up.
- The low latency timestamp interface for E810 appears to possibly have
  some gaps and potential to override an in-flight timestamp request. This
  will be investigated as part of a separate follow-up series.

AI reports that may still remain:
- Sashiko pointed out some ideas about flushing the tracker when we restart
  the PHY port for E825, which I have not opted to implement in this
  series. It still needs investigation and I am currently thinking that it
  is best if we always keep timestamp indexes locked by software until we
  wait that 2 seconds. There are just so many ways this can race and go
  wrong :\ Relatedly, Sashiko also pointed out some potential races with
  flushing the trackers, which I will investigate but do not think should
  hold this series up.
- Sashiko pointed out that the behavior of restarting the PHY ports
  post-reset may be somewhat problematic. This needs investigation and I do
  not yet have a solution. In particular, we can't just let each port do
  its own restart because we need to be sure that the clock owner has
  finished setting the time, but we also cannot necessarily just let the
  clock owner handle it either because the clock owner cannot reliably know
  the link state of the other ports. This is being investigated as well,
  but I would prefer to not hold this already large series up for this.
- Sashiko has complained about potential for missing link event triggers
  since we no longer call ice_ptp_link_change() at ice_up_complete(). This
  seems to be a theoretical problem which I don't have a good solution for.
  The attempts to fix this in previous versions has consistently still led
  to complaints from Sashiko. We need to investigate what mechanism would
  robustly ensure that any link transition or failure to program the PHY is
  not missed and can be restored. However, I would prefer not to delay this
  series while we try to figure that out.
- Sashiko suggested clearing the PHY soft reset device state if the
  function exits early. I opted not to change this flow as it was the
  suggested flow from hardware engineers, and the chance of failure in this
  flow is low and likely already implies a more catastrophic failure (i.e.
  failure to access the sideband queue).
- Some models love to point out places where we bail out on failure to
  access the PHY and leave various flags or state in a disabled way, such
  as not clearing the calibrating flag or leaving the PHY with its soft
  reset bit set. I did not make an attempt to fix these. It is very
  unexpected to be unable to access the PHY, and we're more or less
  treating such failures as catastrophic.
- Sashiko complains about possible ways that ice_ptp_process_tx_tstamps()
  could fail and exit early that would cause problems, either keeping an
  SKB held in memory and essentially leaked for longer than expected, or if
  we fail to clear the in_use bits and could keep re-triggering the IRQ.
  These are highly unexpected errors which are pre-existing issues that may
  be tricky to address, and I haven't attempted to resolve them here.
- The PTP clock is registered early when it should be registered near the
  last step of PTP initialization. It is also removed late, leaving
  potential windows for use-after-free. This should be fixed but I think it
  is pre-existing and should not hold this series up.
- E810 does not clear its cached timestamp array on flushing the tracker.
  This was reported but I think it works better as a separate fix in a
  follow-up series.
- ice_ptp_link_change has serialization issues with ice_ptp_release(), but
  these again appear to be pre-existing and should not block this series.
- The ice_adapter is released early prior to the device
  deinitiatialization. It really should be one of the last steps of the
  driver, but again this looks to be a pre-existing issue that should be
  resolved as a follow-up. I think all the reported potential
  use-after-free pathways are handled in this version anyways.
- certain transient error flows could cause settime64 to lock timestamps
  indefinitely. I think this is a pre-existing issue and not necessarily
  something we consider likely in practice. We also have some forthcoming
  patches that may address this due to other related changes to settime and
  adjtime for E825.

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

Jacob Keller (9):
      ice: fix removal of PTP timestamp tracker during reset
      ice: use reference counting and SRCU for PTP port access
      ice: fix PHY port restart serialization
      ice: set in_use only after preparing Tx timestamp index
      ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
      ice: E825: clear PHY_TSTAMP memory prior to soft reset
      ice: E825: perform a soft reset when starting the PHY timer
      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

Petr Oros (1):
      ice: keep Tx timestamp slots tracked until completion or timeout

 drivers/net/ethernet/intel/ice/ice_adapter.h |  21 +-
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  21 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h  |   2 +-
 drivers/net/ethernet/intel/ice/ice_adapter.c |  20 +-
 drivers/net/ethernet/intel/ice/ice_main.c    |  40 ++-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 471 +++++++++++++++++----------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 151 +++++----
 7 files changed, 452 insertions(+), 274 deletions(-)
---
base-commit: abeacca61eea4f6803df4a595b656ea45c2d3249
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git main
change-id: 20260917-jk-e825c-timestamp-processing-logic-fixes-srcu-fb0b50d33e10

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>

[PATCH net v5 01/13] ice: fix removal of PTP timestamp tracker during reset

[Jacob Keller]

Commit 7a25fe5cd5fb ("ice: stop destroying and reinitalizing Tx tracker
during reset") intended to modify the PTP reset flow of the driver so that
it stopped calling ice_ptp_reset_tx_tracker() during teardown and stopped
calling ice_ptp_init_tx_*() during rebuild.

Unfortunately, the commit only removed the calls to ice_ptp_init_tx_*().
This fixed a memory leak in PF reset. However, now a CORE, GLOBAL, or EMP
reset will leave the device unable to initiate Tx timestamp requests
indefinitely.

In practice the CORE and GLOBAL resets rarely happen in production
environments, while EMP resets happen after a firmware update that is often
followed by a platform reboot. This explains why this has not been caught
until now. However, it is trivial to verify by triggering the reset from
userspace via ethtool. For ice the following command will trigger a GLOBAL
reset:

  $ ethtool --reset eno8303np0 irq-shared dma-shared filter-shared \
                               offload-shared ram-shared           \
                               mac-shared phy-shared

Remove the call of ice_ptp_release_tx_tracker() from
ice_ptp_prepare_for_reset(), to keep the tracker memory in place so that
timestamping can resume after a reset.

During review of a previous version of this change, sashiko pointed out
that the teardown flows for ice_ptp_init() and ice_ptp_release() could
potentially leak the PTP timestamp tracker.

Fix ice_ptp_init() so that it allocates the tracker before adding the port
to the port list, and properly calls ice_ptp_release_tx_tracker() as part
of its cleanup on error. Ensure the ps_lock mutex isn't destroyed until the
PF has been cleared from the port list. Also fix the initialization order
of the kworker to happen before inserting the port into the list.

Fix ice_ptp_release() so that it handles the cleanup if PTP is in the
error state by cancelling the kworker items and releasing the Tx tracker as
appropriate.

This was found by Sashiko review during feedback for an unrelated change,
and iterated based on further feedback from Sashiko after the initial fix
to remove the call to ice_ptp_release_tx_tracker(); It was again iterated
after further sequencing issues were found with initialization and
teardown.

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/
Fixes: 7a25fe5cd5fb ("ice: stop destroying and reinitalizing Tx tracker during reset")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 46 +++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3..e1c66cd0f94f 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);
-
 	/* Disable periodic outputs */
 	ice_ptp_disable_all_perout(pf);
 
@@ -3210,9 +3208,13 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
 }
 
 /**
- * ice_ptp_init_work - Initialize PTP work threads
+ * ice_ptp_init_work - Initialize the PTP kworker
  * @pf: Board private structure
  * @ptp: PF PTP structure
+ *
+ * Allocate the kworker and initialize the periodic work function. The
+ * periodic work is not queued here; Instead, it will be scheduled once the
+ * PTP state is set to ICE_PTP_READY.
  */
 static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 {
@@ -3231,9 +3233,6 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 
 	ptp->kworker = kworker;
 
-	/* Start periodic work going */
-	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
-
 	return 0;
 }
 
@@ -3347,13 +3346,17 @@ void ice_ptp_init(struct ice_pf *pf)
 		}
 	}
 
-	err = ice_ptp_setup_pf(pf);
+	err = ice_ptp_init_work(pf, ptp);
 	if (err)
 		goto err_exit;
 
 	err = ice_ptp_init_port(pf, &ptp->port);
 	if (err)
-		goto err_clean_pf;
+		goto err_release_kworker;
+
+	err = ice_ptp_setup_pf(pf);
+	if (err)
+		goto err_release_tx_tracker;
 
 	/* Start the PHY timestamping block */
 	ice_ptp_reset_phy_timestamping(pf);
@@ -3363,16 +3366,19 @@ void ice_ptp_init(struct ice_pf *pf)
 
 	ptp->state = ICE_PTP_READY;
 
-	err = ice_ptp_init_work(pf, ptp);
-	if (err)
-		goto err_exit;
+	/* Start periodic work now that the state is READY */
+	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
 	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
 	return;
 
-err_clean_pf:
+err_release_tx_tracker:
+	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
+err_release_kworker:
+	kthread_cancel_delayed_work_sync(&ptp->work);
+	kthread_destroy_worker(ptp->kworker);
+	ptp->kworker = NULL;
 	mutex_destroy(&ptp->port.ps_lock);
-	ice_ptp_cleanup_pf(pf);
 err_exit:
 	/* If we registered a PTP clock, release it */
 	if (pf->ptp.clock) {
@@ -3399,12 +3405,22 @@ 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.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);
+		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;
+		}
+		mutex_destroy(&pf->ptp.port.ps_lock);
+
+		pf->ptp.state = ICE_PTP_UNINIT;
 		return;
 	}
 

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 02/13] ice: use reference counting and SRCU for PTP port access

[Jacob Keller]

The ice adapter structure maintains a list of ports associated with the
adapter. This is used for supporting PTP, where the clock owner must handle
many operations that require access to the PTP port structures of the
associated PFs.

This is implemented using a linked list and a mutex. This sort of works,
but a few places within the code do not acquire the mutex when iterating
the list. This includes ice_ptp_flush_all_tx_tracker(),
ice_ptp_restart_all_phy(), and ice_ptp_prepare_rebuild_sec().

Fixing this is tricky, especially since it is not clear if we can simply
acquire the lock around the complete iterations.

The pattern of use for the port list is read-mostly with modifications only
happening during PF initialization when elements are inserted. This
typically only happens during early boot, though a PF could in principle be
removed or loaded at arbitrary times via bind and unbind operations.

The use of a mutex does mean the driver can sleep while holding it, but it
still creates complicates with lock ordering and prevents iterating the
list in any code path that *can't* sleep.

Instead, use SRCU primitives for the port linked list, along with a
reference count on the port. The kref reference counter ensures that a PF
will have a valid lifetime and not be removed until the reference is
released. Note that this change focuses solely on the port list and does
not make an effort to resolve access to ctrl_pf, which is currently being
investigated by another developer.

Use of sleepable RCU is required because we often iterate the PTP port list
and perform operations that might sleep. Attempts at implementing regular
RCU have thus far not proven to be acceptable.

The remove path first removes the port from the list, and we use
kref_get_unless_zero to ensure that such ports are skipped when iterating
the list. This ensures that once a port starts removing we will drop
references and no longer be able to acquire new ones. This avoids loop
iterations chaining together to indefinitely block removal.

The ice_ptp_release_port_srcu() function is used as the release function
for the kref_put() call. This uses wake_up_var() to wake the removing
thread. The waiting thread will block until the final reference has been
removed, then it will use synchronize_srcu() to ensure any outstanding SRCU
critical sections have had the necessary grace period.

This flow ensures that accesses to ports via the adapter port list will
remain valid until both the SRCU critical sections have ended and all the
references to the ports have been dropped. Strictly speaking, SRCU alone
might be sufficient for existing code paths, but the reference count allows
the option for passing a pointer to the port on to other functions if
necessary in the future.

Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.h |  17 ++--
 drivers/net/ethernet/intel/ice/ice_ptp.h     |   4 +
 drivers/net/ethernet/intel/ice/ice_adapter.c |  17 +++-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 125 ++++++++++++++++++++-------
 4 files changed, 121 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index 4f695f32da3d..c61bd645e460 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/srcu.h>
 #include <linux/spinlock_types.h>
 #include <linux/refcount_types.h>
 
@@ -17,15 +18,19 @@ struct ice_pf;
 /**
  * struct ice_port_list - data used to store the list of adapter ports
  *
- * This structure contains data used to maintain a list of adapter ports
+ * This structure contains data used to maintain a list of adapter ports.
+ * Writers modifying the list *must* acquire the lock, and use SRCU safe list
+ * operations. Readers should use srcu_read_lock() on the provided domain.
  *
- * @ports: list of ports
- * @lock: protect access to the ports list
+ * @list: list of ports
+ * @lock: protect write access to the list
+ * @srcu: Sleepable RCU domain for this adapter
  */
 struct ice_port_list {
-	struct list_head ports;
-	/* To synchronize the ports list operations */
-	struct mutex lock;
+	struct list_head list;
+	/* To synchronize write operations on the port list */
+	spinlock_t lock;
+	struct srcu_struct srcu;
 };
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index c4b0da7ce20e..da2003ba3bb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -5,6 +5,8 @@
 #define _ICE_PTP_H_
 
 #include <linux/ptp_clock_kernel.h>
+#include <linux/rculist.h>
+#include <linux/kref.h>
 #include <linux/kthread.h>
 
 #include "ice_ptp_hw.h"
@@ -138,6 +140,7 @@ struct ice_ptp_tx {
  * and determine when the port's PHY offset is valid.
  *
  * @list_node: list member structure
+ * @ref: reference counter for use with adapter ports list
  * @tx: Tx timestamp tracking for this port
  * @ov_work: delayed work task for tracking when PHY offset is valid
  * @ps_lock: mutex used to protect the overall PTP PHY start procedure
@@ -149,6 +152,7 @@ struct ice_ptp_tx {
  */
 struct ice_ptp_port {
 	struct list_head list_node;
+	struct kref ref;
 	struct ice_ptp_tx tx;
 	struct kthread_delayed_work ov_work;
 	struct mutex ps_lock; /* protects overall PTP PHY start procedure */
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 2dc3629d6d0f..536923b6ae97 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -6,6 +6,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/srcu.h>
 #include <linux/xarray.h>
 #include "ice_adapter.h"
 #include "ice.h"
@@ -54,11 +55,18 @@ static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
 static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 {
 	struct ice_adapter *adapter;
+	int err;
 
 	adapter = kzalloc_obj(*adapter);
 	if (!adapter)
 		return NULL;
 
+	err = init_srcu_struct(&adapter->ports.srcu);
+	if (err) {
+		kfree(adapter);
+		return NULL;
+	}
+
 	adapter->index = ice_adapter_index(pdev);
 	spin_lock_init(&adapter->ptp_gltsyn_time_lock);
 	spin_lock_init(&adapter->txq_ctx_lock);
@@ -66,18 +74,19 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 		mutex_init(&adapter->cpi_phy_lock[i]);
 	refcount_set(&adapter->refcount, 1);
 
-	mutex_init(&adapter->ports.lock);
-	INIT_LIST_HEAD(&adapter->ports.ports);
+	spin_lock_init(&adapter->ports.lock);
+	INIT_LIST_HEAD(&adapter->ports.list);
 
 	return adapter;
 }
 
 static void ice_adapter_free(struct ice_adapter *adapter)
 {
-	WARN_ON(!list_empty(&adapter->ports.ports));
+	WARN_ON(!list_empty(&adapter->ports.list));
 	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
 		mutex_destroy(&adapter->cpi_phy_lock[i]);
-	mutex_destroy(&adapter->ports.lock);
+
+	cleanup_srcu_struct(&adapter->ports.srcu);
 
 	kfree(adapter);
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index e1c66cd0f94f..55241802d17a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) 2021, Intel Corporation. */
 
+#include <linux/rculist.h>
+#include <linux/srcu.h>
+#include <linux/wait_bit.h>
 #include "ice.h"
 #include "ice_lib.h"
 #include "ice_trace.h"
@@ -673,20 +676,37 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	pf->ptp.tx_hwtstamp_good += tstamp_good;
 }
 
+static void ice_ptp_release_port_srcu(struct kref *ref)
+{
+	/* wake_up_var() requires full ordering but kref_put only provides
+	 * release ordering.
+	 */
+	smp_mb__after_atomic();
+	wake_up_var(ref);
+}
+
 static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
 {
+	struct ice_port_list *ports = &pf->adapter->ports;
 	struct ice_ptp_port *port;
+	int srcu_idx;
 
-	mutex_lock(&pf->adapter->ports.lock);
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node) {
+	srcu_idx = srcu_read_lock(&ports->srcu);
+	list_for_each_entry_srcu(port, &ports->list, list_node,
+				 srcu_read_lock_held(&ports->srcu)) {
 		struct ice_ptp_tx *tx = &port->tx;
 
-		if (!tx || !tx->init)
+		if (!tx->init)
+			continue;
+
+		if (!kref_get_unless_zero(&port->ref))
 			continue;
 
 		ice_ptp_process_tx_tstamp(tx);
+
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
 	}
-	mutex_unlock(&pf->adapter->ports.lock);
+	srcu_read_unlock(&ports->srcu, srcu_idx);
 }
 
 /**
@@ -806,10 +826,19 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
 static void
 ice_ptp_flush_all_tx_tracker(struct ice_pf *pf)
 {
+	struct ice_port_list *ports = &pf->adapter->ports;
 	struct ice_ptp_port *port;
+	int srcu_idx;
 
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
+	srcu_idx = srcu_read_lock(&ports->srcu);
+	list_for_each_entry_srcu(port, &ports->list, list_node,
+				 srcu_read_lock_held(&ports->srcu)) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
+	}
+	srcu_read_unlock(&ports->srcu, srcu_idx);
 }
 
 /**
@@ -1424,16 +1453,22 @@ static void ice_ptp_reset_phy_timestamping(struct ice_pf *pf)
  */
 static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 {
-	struct list_head *entry;
+	struct ice_port_list *ports = &pf->adapter->ports;
+	struct ice_ptp_port *port;
+	int srcu_idx;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	srcu_idx = srcu_read_lock(&ports->srcu);
+	list_for_each_entry_srcu(port, &ports->list, list_node,
+				 srcu_read_lock_held(&ports->srcu)) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
 
 		if (port->link_up)
 			ice_ptp_port_phy_restart(port);
+
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
 	}
+	srcu_read_unlock(&ports->srcu, srcu_idx);
 }
 
 /**
@@ -2694,19 +2729,28 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx)
 
 static bool ice_any_port_has_timestamps(struct ice_pf *pf)
 {
+	struct ice_port_list *ports = &pf->adapter->ports;
+	bool have_tstamps = false;
 	struct ice_ptp_port *port;
+	int srcu_idx;
 
-	scoped_guard(mutex, &pf->adapter->ports.lock) {
-		list_for_each_entry(port, &pf->adapter->ports.ports,
-				    list_node) {
-			struct ice_ptp_tx *tx = &port->tx;
+	srcu_idx = srcu_read_lock(&ports->srcu);
+	list_for_each_entry_srcu(port, &ports->list, list_node,
+				 srcu_read_lock_held(&ports->srcu)) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
 
-			if (ice_port_has_timestamps(tx))
-				return true;
-		}
+		if (ice_port_has_timestamps(&port->tx))
+			have_tstamps = true;
+
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
+
+		if (have_tstamps)
+			break;
 	}
+	srcu_read_unlock(&ports->srcu, srcu_idx);
 
-	return false;
+	return have_tstamps;
 }
 
 bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
@@ -2890,14 +2934,18 @@ void ice_ptp_queue_work(struct ice_pf *pf)
 static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
 					enum ice_reset_req reset_type)
 {
-	struct list_head *entry;
+	struct ice_port_list *ports = &pf->adapter->ports;
+	struct ice_ptp_port *port;
+	int srcu_idx;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	srcu_idx = srcu_read_lock(&ports->srcu);
+	list_for_each_entry_srcu(port, &ports->list, list_node,
+				 srcu_read_lock_held(&ports->srcu)) {
 		struct ice_pf *peer_pf = ptp_port_to_pf(port);
 
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+
 		if (!ice_is_primary(&peer_pf->hw)) {
 			if (rebuild) {
 				/* TODO: When implementing rebuild=true:
@@ -2909,7 +2957,10 @@ static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
 				ice_ptp_prepare_for_reset(peer_pf, reset_type);
 			}
 		}
+
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
 	}
+	srcu_read_unlock(&ports->srcu, srcu_idx);
 }
 
 /**
@@ -3084,11 +3135,11 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&ptp->port.list_node);
-	mutex_lock(&pf->adapter->ports.lock);
+	kref_init(&ptp->port.ref);
 
-	list_add(&ptp->port.list_node,
-		 &pf->adapter->ports.ports);
-	mutex_unlock(&pf->adapter->ports.lock);
+	spin_lock(&pf->adapter->ports.lock);
+	list_add_rcu(&ptp->port.list_node, &pf->adapter->ports.list);
+	spin_unlock(&pf->adapter->ports.lock);
 
 	/* Seed the per-PHY Tx reference clock usage map for this port.
 	 * Only meaningful on E825 (other MAC types don't expose tx-clk
@@ -3110,13 +3161,23 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 
 static void ice_ptp_cleanup_pf(struct ice_pf *pf)
 {
+	struct ice_port_list *ports = &pf->adapter->ports;
 	struct ice_ptp *ptp = &pf->ptp;
+	struct kref *ref;
 
-	if (pf->hw.mac_type != ICE_MAC_UNKNOWN) {
-		mutex_lock(&pf->adapter->ports.lock);
-		list_del(&ptp->port.list_node);
-		mutex_unlock(&pf->adapter->ports.lock);
-	}
+	if (pf->hw.mac_type == ICE_MAC_UNKNOWN)
+		return;
+
+	spin_lock(&ports->lock);
+	list_del_rcu(&ptp->port.list_node);
+	spin_unlock(&ports->lock);
+
+	ref = &ptp->port.ref;
+	kref_put(ref, ice_ptp_release_port_srcu);
+
+	wait_var_event(ref, !kref_read(ref));
+
+	synchronize_srcu(&ports->srcu);
 }
 
 /**

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 03/13] ice: fix PHY port restart serialization

[Jacob Keller]

The ice_port_phy_restart() function is responsible for restarting a PHY
port, primarily after a link transition. PHY ports must also all be
restarted after a reset, and the E822 devices also restart the PHY after
the .settime64 operation.

If a PHY restart occurs concurrently with a link transition, it is possible
that the link state read could be re-ordered such that the link transition
would start the PHY but a concurrent restart (such as from .settime64)
could see the old link state and decide it must stop the PHY.

This occurs because the restart procedure depends on the link state value
but setting that value is not serialized with the per-port ps_lock. A
following fix for E825 is going to modify the .setttime64 to also correctly
restart the PHY timers, opening the E825 device to a race between
.settime64 and a concurrent link change.

To avoid this, we need to ensure that the port link_up field is set within
the same critical section as the restart procedure. This way we ensure that
the end result is the PHY programmed to the correct state.

Additionally note that the PHY restart procedure already cannot run
concurrently across two ports. The procedure requires executing timer
commands which in turn require the PTP hardware semaphore.

Thus, to simplify the locking behavior, convert the per-port ps_lock to a
single mutex in the ice_adapter. Acquire this in ice_ptp_restart_all_phy(),
and hold it while calling ice_ptp_port_phy_stop() and
ice_ptp_port_phy_restart().

The ice_ptp_link_change() function now acquires the port lock for the
entire change. This ensures that the link state is set under lock. Note the
function does also acquire the dplls.lock for E825 devices. This ordering
is safe because the other callers of dplls.lock do not acquire the
ps_lock.

The switch to a single lock for the adapter instead of one per port is
easier to reason about. It also could be a first step in a plan to replace
the PTP hardware semaphore completely, which is under investigation for the
future.

To avoid losing a link edge, modify the ice_ptp_link_change function so
that it always updates the current link status even if the state is not
ready. Modify the ice_ptp_init function to set the PTP state to ready
before performing its final PTP state initialization. This way, a link edge
that occurs during PTP initialization will not be lost. One of two things
will happen:

a) the link change happens before the link state is set to ready, in which
case the driver will set the current PTP link state before the
ice_ptp_port_phy_restart() intiiates. Since this is under ps_lock it will
be reflected as the current state when programming the PHY from
ice_ptp_init()

b) the link change happens after the ice_ptp_init restarts. In this case,
the ice_ptp_init will set the PHY to the old state, then
ice_ptp_link_change() will initiate a new start that sets the PHY to the
proper state.

Finally, update the link_up field to use READ_ONCE/WRITE_ONCE since the
value is read from the IRQ without a lock.

This fixes one of the issues reported by Sashiko during a previous review
of this series, linked here as the Closes tag.

Closes: https://lore.kernel.org/netdev/20260916011228.1632848-1-kuba@kernel.org/
Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  2 -
 drivers/net/ethernet/intel/ice/ice_adapter.c |  3 ++
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 73 +++++++++++++---------------
 4 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index c61bd645e460..5cefcd7af8b3 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -41,6 +41,7 @@ struct ice_port_list {
  * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
  * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
  *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
+ * @ps_lock: Mutex to serialize PHY port start/stop across adapter.
  * @ctrl_pf: Control PF of the adapter
  * @ports: Ports list
  * @index: 64-bit index cached for collision detection on 32bit systems
@@ -54,6 +55,9 @@ struct ice_adapter {
 	/* Serialize CPI REQ/ACK transactions per PHY (E825C only) */
 	struct mutex cpi_phy_lock[ICE_E825_MAX_PHYS];
 
+	/* For serializing PHY port start/stop sequences */
+	struct mutex ps_lock;
+
 	struct ice_pf *ctrl_pf;
 	struct ice_port_list ports;
 	u64 index;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index da2003ba3bb0..27ea502b7576 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -143,7 +143,6 @@ struct ice_ptp_tx {
  * @ref: reference counter for use with adapter ports list
  * @tx: Tx timestamp tracking for this port
  * @ov_work: delayed work task for tracking when PHY offset is valid
- * @ps_lock: mutex used to protect the overall PTP PHY start procedure
  * @link_up: indicates whether the link is up
  * @tx_fifo_busy_cnt: number of times the Tx FIFO was busy
  * @port_num: the port number this structure represents
@@ -155,7 +154,6 @@ struct ice_ptp_port {
 	struct kref ref;
 	struct ice_ptp_tx tx;
 	struct kthread_delayed_work ov_work;
-	struct mutex ps_lock; /* protects overall PTP PHY start procedure */
 	bool link_up;
 	u8 tx_fifo_busy_cnt;
 	u8 port_num;
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 536923b6ae97..572862fcd247 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -77,6 +77,8 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 	spin_lock_init(&adapter->ports.lock);
 	INIT_LIST_HEAD(&adapter->ports.list);
 
+	mutex_init(&adapter->ps_lock);
+
 	return adapter;
 }
 
@@ -87,6 +89,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
 		mutex_destroy(&adapter->cpi_phy_lock[i]);
 
 	cleanup_srcu_struct(&adapter->ports.srcu);
+	mutex_destroy(&adapter->ps_lock);
 
 	kfree(adapter);
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 55241802d17a..7a1deb3ea212 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -583,7 +583,7 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	}
 
 	/* Drop packets if the link went down */
-	link_up = ptp_port->link_up;
+	link_up = READ_ONCE(ptp_port->link_up);
 
 	for_each_set_bit(idx, tx->in_use, tx->len) {
 		struct skb_shared_hwtstamps shhwtstamps = {};
@@ -1195,6 +1195,8 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work)
 /**
  * ice_ptp_port_phy_stop - Stop timestamping for a PHY port
  * @ptp_port: PTP port to stop
+ *
+ * Context: must hold the adapter ps_lock.
  */
 static int
 ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
@@ -1204,7 +1206,7 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 	struct ice_hw *hw = &pf->hw;
 	int err;
 
-	mutex_lock(&ptp_port->ps_lock);
+	lockdep_assert_held(&pf->adapter->ps_lock);
 
 	switch (hw->mac_type) {
 	case ICE_MAC_E810:
@@ -1226,8 +1228,6 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d down, err %d\n",
 			port, err);
 
-	mutex_unlock(&ptp_port->ps_lock);
-
 	return err;
 }
 
@@ -1238,6 +1238,8 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
  * Start the PHY timestamping block, and initiate Vernier timestamping
  * calibration. If timestamping cannot be calibrated (such as if link is down)
  * then disable the timestamping block instead.
+ *
+ * Context: must hold the adapter ps_lock.
  */
 static int
 ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
@@ -1248,10 +1250,10 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 	unsigned long flags;
 	int err;
 
-	if (!ptp_port->link_up)
-		return ice_ptp_port_phy_stop(ptp_port);
+	lockdep_assert_held(&pf->adapter->ps_lock);
 
-	mutex_lock(&ptp_port->ps_lock);
+	if (!READ_ONCE(ptp_port->link_up))
+		return ice_ptp_port_phy_stop(ptp_port);
 
 	switch (hw->mac_type) {
 	case ICE_MAC_E810:
@@ -1294,8 +1296,6 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
 			port, err);
 
-	mutex_unlock(&ptp_port->ps_lock);
-
 	return err;
 }
 
@@ -1309,17 +1309,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 	struct ice_ptp_port *ptp_port;
 	struct ice_hw *hw = &pf->hw;
 
-	if (pf->ptp.state != ICE_PTP_READY)
-		return;
-
 	ptp_port = &pf->ptp.port;
 
-	/* Update cached link status for this port immediately */
-	ptp_port->link_up = linkup;
+	mutex_lock(&pf->adapter->ps_lock);
 
-	/* Skip HW writes if reset is in progress */
-	if (pf->hw.reset_ongoing)
-		return;
+	WRITE_ONCE(ptp_port->link_up, linkup);
+
+	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
+		goto out_unlock;
 
 	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
 	    test_bit(ICE_FLAG_DPLL, pf->flags)) {
@@ -1362,17 +1359,20 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 	case ICE_MAC_E810:
 	case ICE_MAC_E830:
 		/* Do not reconfigure E810 or E830 PHY */
-		return;
+		break;
 	case ICE_MAC_GENERIC:
 		ice_ptp_port_phy_restart(ptp_port);
-		return;
+		break;
 	case ICE_MAC_GENERIC_3K_E825:
 		if (linkup)
 			ice_ptp_port_phy_restart(ptp_port);
-		return;
+		break;
 	default:
 		dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__);
 	}
+
+out_unlock:
+	mutex_unlock(&pf->adapter->ps_lock);
 }
 
 /**
@@ -1438,18 +1438,11 @@ static int ice_ptp_cfg_phy_interrupt(struct ice_pf *pf, bool ena, u32 threshold)
 	}
 }
 
-/**
- * ice_ptp_reset_phy_timestamping - Reset PHY timestamping block
- * @pf: Board private structure
- */
-static void ice_ptp_reset_phy_timestamping(struct ice_pf *pf)
-{
-	ice_ptp_port_phy_restart(&pf->ptp.port);
-}
-
 /**
  * ice_ptp_restart_all_phy - Restart all PHYs to recalibrate timestamping
  * @pf: Board private structure
+ *
+ * Context: acquires the adapter ps_lock
  */
 static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 {
@@ -1457,18 +1450,22 @@ static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 	struct ice_ptp_port *port;
 	int srcu_idx;
 
+	mutex_lock(&pf->adapter->ps_lock);
+
 	srcu_idx = srcu_read_lock(&ports->srcu);
 	list_for_each_entry_srcu(port, &ports->list, list_node,
 				 srcu_read_lock_held(&ports->srcu)) {
 		if (!kref_get_unless_zero(&port->ref))
 			continue;
 
-		if (port->link_up)
+		if (READ_ONCE(port->link_up))
 			ice_ptp_port_phy_restart(port);
 
 		kref_put(&port->ref, ice_ptp_release_port_srcu);
 	}
 	srcu_read_unlock(&ports->srcu, srcu_idx);
+
+	mutex_unlock(&pf->adapter->ps_lock);
 }
 
 /**
@@ -3308,8 +3305,6 @@ static int ice_ptp_init_port(struct ice_pf *pf, struct ice_ptp_port *ptp_port)
 {
 	struct ice_hw *hw = &pf->hw;
 
-	mutex_init(&ptp_port->ps_lock);
-
 	switch (hw->mac_type) {
 	case ICE_MAC_E810:
 	case ICE_MAC_E830:
@@ -3419,14 +3414,16 @@ void ice_ptp_init(struct ice_pf *pf)
 	if (err)
 		goto err_release_tx_tracker;
 
-	/* Start the PHY timestamping block */
-	ice_ptp_reset_phy_timestamping(pf);
-
 	/* Configure initial Tx interrupt settings */
 	ice_ptp_cfg_tx_interrupt(pf);
 
 	ptp->state = ICE_PTP_READY;
 
+	/* (Re)start the PHY timestamping block */
+	mutex_lock(&pf->adapter->ps_lock);
+	ice_ptp_port_phy_restart(&ptp->port);
+	mutex_unlock(&pf->adapter->ps_lock);
+
 	/* Start periodic work now that the state is READY */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
@@ -3439,7 +3436,6 @@ void ice_ptp_init(struct ice_pf *pf)
 	kthread_cancel_delayed_work_sync(&ptp->work);
 	kthread_destroy_worker(ptp->kworker);
 	ptp->kworker = NULL;
-	mutex_destroy(&ptp->port.ps_lock);
 err_exit:
 	/* If we registered a PTP clock, release it */
 	if (pf->ptp.clock) {
@@ -3479,7 +3475,6 @@ void ice_ptp_release(struct ice_pf *pf)
 			kthread_destroy_worker(pf->ptp.kworker);
 			pf->ptp.kworker = NULL;
 		}
-		mutex_destroy(&pf->ptp.port.ps_lock);
 
 		pf->ptp.state = ICE_PTP_UNINIT;
 		return;
@@ -3498,8 +3493,10 @@ void ice_ptp_release(struct ice_pf *pf)
 
 	kthread_cancel_delayed_work_sync(&pf->ptp.work);
 
+	mutex_lock(&pf->adapter->ps_lock);
 	ice_ptp_port_phy_stop(&pf->ptp.port);
-	mutex_destroy(&pf->ptp.port.ps_lock);
+	mutex_unlock(&pf->adapter->ps_lock);
+
 	if (pf->ptp.kworker) {
 		kthread_destroy_worker(pf->ptp.kworker);
 		pf->ptp.kworker = NULL;

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 04/13] ice: set in_use only after preparing Tx timestamp index

[Jacob Keller]

The ice_ptp_request_ts() function is used to request a timestamp index for
use with a packet. When reserving an index, it sets the start time and
saves a pointer to the skb into the appropriate index. The function marks
the in_use bit first before doing any of these steps. The IRQ handler which
clears the timestamps reads the in_use bits uses a lockless flow for
reading the in_use bits to determine which ones are in-use. This is
necessary as actually processing a complete timestamp must be able to sleep
so we cannot hold the timestamp tracker lock over the entire sequence.
Additionally, blocking the Tx hotpath with such a lock indefinitely would
be problematic.

However, the existing flow now has a very narrow window where the IRQ
handler could see a timestamp as in-use but read a stale value for its
"start" time.

Fix this by ordering the sequence to mark the in_use bit last, and add a
memory barrier to prevent re-ordering of the previous writes to setup the
index.

This was found and reported by Sashiko while reviewing an unrelated change.

Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=7
Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 7a1deb3ea212..1e6ba6bbf6c2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -592,6 +592,12 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 		bool drop_ts = !link_up;
 		struct sk_buff *skb;
 
+		/* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()
+		 * to ensure ordering the in_use bitmap and the indexed fields
+		 * in the tstamps array.
+		 */
+		smp_rmb();
+
 		/* Drop packets which have waited for more than 2 seconds */
 		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
 			drop_ts = true;
@@ -2670,11 +2676,13 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
 		 * a reference to the skb and the start time to allow discarding old
 		 * requests.
 		 */
-		set_bit(idx, tx->in_use);
-		clear_bit(idx, tx->stale);
 		tx->tstamps[idx].start = jiffies;
 		tx->tstamps[idx].skb = skb_get(skb);
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		clear_bit(idx, tx->stale);
+		/* Ensure index is setup before marking it as used */
+		smp_mb__before_atomic();
+		set_bit(idx, tx->in_use);
 		ice_trace(tx_tstamp_request, skb, idx);
 	}
 

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 05/13] ice: call PTP link change only from link events

[Jacob Keller]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

The ice_ptp_link_change() function is currently called from
ice_up_complete() and ice_down(). The up and down functions are called from
many places including a variety of administrative tasks such as MTU change.
When called, the ice_ptp_link_change() function forces the PHY to restart,
which can be destructive to ongoing timestamps. We must restart the
timestamping block if the link has changed, but we do not need to restart
it for most of the administrative tasks. The calls also cause unnecessary
re-initialization at every ice_open(), and trigger premature cleanup at
ice_down() even if the actual link hasn't dropped.

Ideally we only want to toggle a PHY timestamping block restart if the
physical link has changed or we are fully reconfiguring the clock. To that
end, stop calling ice_ptp_link_change() during ice_up_complete() and
ice_down().

Initialize the PTP link_up field at probe as part of ice_init_link(),
before the service task and link event handling are enabled. Change
ice_link_event() to call ice_ptp_link_change first before checking if the
link value is the same. Update ice_ptp_link_change to check the current PTP
link status and avoid triggering a restart when the link states match.

For the ice_rebuild() path, call ice_ptp_link_change() to ensure that the
PTP reflects the current link status if it changed while down. Make sure
the link state is current by having ice_update_pf_netdev_link() setting the
get_link_info field of the PF port info, ensuring the state is accurate.

These changes ensure that the link status will reflect the current state of
the physical link without needing to start and stop on every internal
ice_down_up() sequence.

This sequencing should ensure that any link transition will properly update
the PTP state to reflect the new mode, without causing unnecessary
duplicate starts and stops. By removing the calls from ice_up_complete()
and ice_down() the driver no longer unnecessarily thrashes the PTP state in
the hardware.

Finally, for E825, call ice_ptp_port_phy_restart() even when link is down.
This will ensure that the PHY is properly disabled immediately instead of
only calling the stop implementation after some other flow triggers a
restart.

Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 38 ++++++++++++++++++++-----------
 drivers/net/ethernet/intel/ice/ice_ptp.c  |  8 +++----
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..910ebdcdde19 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1144,6 +1144,8 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 		ice_set_link(vsi, false);
 	}
 
+	ice_ptp_link_change(pf, link_up);
+
 	/* if the old link up/down and speed is the same as the new */
 	if (link_up == old_link && link_speed == old_link_speed)
 		return 0;
@@ -1151,8 +1153,6 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 	if (!link_up && old_link)
 		pf->link_down_events++;
 
-	ice_ptp_link_change(pf, link_up);
-
 	if (ice_is_dcb_active(pf)) {
 		if (test_bit(ICE_FLAG_DCB_ENA, pf->flags))
 			ice_dcb_rebuild(pf);
@@ -4810,6 +4810,14 @@ static void ice_init_link(struct ice_pf *pf)
 
 	ice_init_link_dflt_override(pf->hw.port_info);
 
+	/* Seed PTP link status from current PHY status, since link may
+	 * already be up (e.g. after PXE boot) without a link-change edge to
+	 * catch it later.
+	 */
+	WRITE_ONCE(pf->ptp.port.link_up,
+		   !!(pf->hw.port_info->phy.link_info.link_info &
+		      ICE_AQ_LINK_UP));
+
 	ice_check_link_cfg_err(pf,
 			       pf->hw.port_info->phy.link_info.link_cfg_err);
 
@@ -6745,7 +6753,6 @@ static int ice_up_complete(struct ice_vsi *vsi)
 		ice_print_link_msg(vsi, true);
 		netif_tx_start_all_queues(vsi->netdev);
 		netif_carrier_on(vsi->netdev);
-		ice_ptp_link_change(pf, true);
 	}
 
 	/* Perform an initial read of the statistics registers now to
@@ -7273,7 +7280,6 @@ int ice_down(struct ice_vsi *vsi)
 
 	if (vsi->netdev) {
 		vlan_err = ice_vsi_del_vlan_zero(vsi);
-		ice_ptp_link_change(vsi->back, false);
 		netif_carrier_off(vsi->netdev);
 		netif_tx_disable(vsi->netdev);
 	}
@@ -7623,13 +7629,15 @@ static void ice_update_pf_netdev_link(struct ice_pf *pf)
 		if (!vsi || vsi->type != ICE_VSI_PF)
 			return;
 
-		ice_get_link_status(pf->vsi[i]->port_info, &link_up);
+		/* Make sure to get current link info */
+		vsi->port_info->phy.get_link_info = true;
+		ice_get_link_status(vsi->port_info, &link_up);
 		if (link_up) {
-			netif_carrier_on(pf->vsi[i]->netdev);
-			netif_tx_wake_all_queues(pf->vsi[i]->netdev);
+			netif_carrier_on(vsi->netdev);
+			netif_tx_wake_all_queues(vsi->netdev);
 		} else {
-			netif_carrier_off(pf->vsi[i]->netdev);
-			netif_tx_stop_all_queues(pf->vsi[i]->netdev);
+			netif_carrier_off(vsi->netdev);
+			netif_tx_stop_all_queues(vsi->netdev);
 		}
 	}
 }
@@ -7745,10 +7753,6 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (test_bit(ICE_FLAG_DCB_ENA, pf->flags))
 		ice_dcb_rebuild(pf);
 
-	/* If the PF previously had enabled PTP, PTP init needs to happen before
-	 * the VSI rebuild. If not, this causes the PTP link status events to
-	 * fail.
-	 */
 	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
 		ice_ptp_rebuild(pf, reset_type);
 
@@ -7794,6 +7798,14 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	ice_update_pf_netdev_link(pf);
 
+	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
+		bool link_up;
+
+		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
+			     ICE_AQ_LINK_UP);
+		ice_ptp_link_change(pf, link_up);
+	}
+
 	/* tell the firmware we are up */
 	err = ice_send_version(pf);
 	if (err) {
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 1e6ba6bbf6c2..f43918f334e8 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1319,6 +1319,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 
 	mutex_lock(&pf->adapter->ps_lock);
 
+	if (READ_ONCE(ptp_port->link_up) == linkup)
+		goto out_unlock;
+
 	WRITE_ONCE(ptp_port->link_up, linkup);
 
 	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
@@ -1367,11 +1370,8 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 		/* Do not reconfigure E810 or E830 PHY */
 		break;
 	case ICE_MAC_GENERIC:
-		ice_ptp_port_phy_restart(ptp_port);
-		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		if (linkup)
-			ice_ptp_port_phy_restart(ptp_port);
+		ice_ptp_port_phy_restart(ptp_port);
 		break;
 	default:
 		dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n", __func__);

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 06/13] ice: E822: keep Tx timestamps disabled during offset calibration

[Jacob Keller]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Do not clear the tx.calibrating flag immediately after starting the PHY
timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps
disabled until the offset verification work (ice_ptp_wait_for_offsets)
has confirmed that the Tx PHY offset is properly configured.

Previously, tx.calibrating was set to true, then immediately back to
false right after ice_start_phy_timer_e82x() returned. This allowed Tx
timestamp requests to be served during the window where offset
verification was still pending. Timestamps produced during this window
should have their valid bit cleared, but it is better to prevent requests
until the driver has confirmed calibration is complete.

Rename the tx.calibrating field to tx.disabled, and change where it is
cleared. Only clear it in ice_ptp_wait_for_offsets(), and only after Tx
offset configuration has completed successfully. This ensures that new Tx
timestamp requests are only accepted after the PHY has properly calibrated
the Tx offset.

If ice_start_phy_timer_e82x() fails, do not restore disabled to false.
The device is in a state where timestamps cannot succeed properly anyways.
A dev_err message is already logged on failure to start the timer at the
end of the function.

Renaming the field improves clarity and will better reflect the nature of
the flag in a future change that also applies disabling Tx timestamps in
software in a following commit.

Log a debug message while offset calibration is still pending, including
the specific Tx/Rx error codes to aid debugging stalled calibration.
This path is expected on every routine link-up: ov_work is first queued
with no delay and the vernier offset cannot be computed until at least
one packet has been transmitted, so the first several invocations
normally land here. Use dev_dbg() rather than a rate-limited warning to
avoid emitting KERN_WARNING on every link-up during normal operation.
Log a debug message when calibration completes successfully.

Note that sashiko review has previously complained that leaving the
disabled flag set "permanently" disables Tx timestamps if vernier
calibration never completes. This is true, but its important to realize
that timestamps would still fail regardless of whether the flag is set.
Until vernier calibration completes the device will not report valid
timestamps regardless. Thus, keeping the calibrating flag set simply
prevents new timestamp requests from software while it is known that the
hardware will not complete them.

Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.h |  6 +++---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 37 ++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 27ea502b7576..d70f022c850b 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -106,8 +106,8 @@ enum ice_tx_tstamp_work {
  * @offset: offset into timestamp block to get the real index
  * @len: length of the tstamps and in_use fields.
  * @init: if true, the tracker is initialized;
- * @calibrating: if true, the PHY is calibrating the Tx offset. During this
- *               window, timestamps are temporarily disabled.
+ * @disabled: set to true when new Tx timestamp requests are disabled, such as
+ *            while the PHY Tx offset configuration has not yet completed.
  * @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
  *                    bitmap register. If false, fall back to verifying new
  *                    timestamp values against previously cached copy.
@@ -122,7 +122,7 @@ struct ice_ptp_tx {
 	u8 offset;
 	u8 len;
 	u8 init : 1;
-	u8 calibrating : 1;
+	u8 disabled : 1;
 	u8 has_ready_bitmap : 1;
 	s8 last_ll_ts_idx_read;
 };
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index f43918f334e8..57e2606655c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -367,7 +367,7 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
 {
 	lockdep_assert_held(&tx->lock);
 
-	return tx->init && !tx->calibrating;
+	return tx->init && !tx->disabled;
 }
 
 /**
@@ -1168,6 +1168,7 @@ static int ice_ptp_check_tx_fifo(struct ice_ptp_port *port)
 static void ice_ptp_wait_for_offsets(struct kthread_work *work)
 {
 	struct ice_ptp_port *port;
+	unsigned long flags;
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	int tx_err;
@@ -1188,9 +1189,26 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work)
 	tx_err = ice_ptp_check_tx_fifo(port);
 	if (!tx_err)
 		tx_err = ice_phy_cfg_tx_offset_e82x(hw, port->port_num);
+	if (!tx_err) {
+		/* Tx offset has been configured, re-enable Tx timestamps */
+		spin_lock_irqsave(&port->tx.lock, flags);
+		if (port->tx.disabled) {
+			port->tx.disabled = false;
+			dev_dbg(ice_pf_to_dev(pf), "PTP Tx offset valid for port %u, Tx timestamps enabled\n",
+				port->port_num);
+		}
+		spin_unlock_irqrestore(&port->tx.lock, flags);
+	}
+
 	rx_err = ice_phy_cfg_rx_offset_e82x(hw, port->port_num);
 	if (tx_err || rx_err) {
-		/* Tx and/or Rx offset not yet configured, try again later */
+		/* Tx and/or Rx offset not yet configured, try again later.
+		 * This is expected during normal link-up: the vernier offset
+		 * calibration cannot complete until at least one packet has
+		 * been transmitted, so the first retries routinely land here.
+		 */
+		dev_dbg(ice_pf_to_dev(pf), "PTP offset not yet valid for port %u (tx_err=%d rx_err=%d)\n",
+			port->port_num, tx_err, rx_err);
 		kthread_queue_delayed_work(pf->ptp.kworker,
 					   &port->ov_work,
 					   msecs_to_jiffies(100));
@@ -1270,11 +1288,9 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 		/* Start the PHY timer in Vernier mode */
 		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
 
-		/* temporarily disable Tx timestamps while calibrating
-		 * PHY offset
-		 */
+		/* Disable Tx timestamps until PHY calibration completes */
 		spin_lock_irqsave(&ptp_port->tx.lock, flags);
-		ptp_port->tx.calibrating = true;
+		ptp_port->tx.disabled = true;
 		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
 		ptp_port->tx_fifo_busy_cnt = 0;
 
@@ -1283,11 +1299,10 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 		if (err)
 			break;
 
-		/* Enable Tx timestamps right away */
-		spin_lock_irqsave(&ptp_port->tx.lock, flags);
-		ptp_port->tx.calibrating = false;
-		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
-
+		/* Do not clear disabled flag here. Tx timestamp requests
+		 * remain disabled until ice_ptp_wait_for_offsets() has
+		 * verified that the Tx offset calibration has completed.
+		 */
 		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
 					   0);
 		break;

-- 
2.56.0.212.g129ede20d986

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

[Jacob Keller]

From: Karol Kolacinski <karol.kolacinski@intel.com>

The offset verification work task for E822 is executed after a PHY restart
to complete vernier calibration. In the event that either Tx or Rx
calibration hasn't finished, it may still be executing when a reset occurs.

The ice_ptp_wait_for_offsets() function does check if the driver is
resetting, and will reschedule itself. However, it is possible that a given
execution is already past this check. The function may then fail to access
the sideband queue. It treats failures as a reason to re-schedule and check
again later. This re-schedule will then see the updated reset state via
ice_is_reset_in_progress() and will keep re-scheduling until the reset
finishes.

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. Preemptively cancel the task to avoid doing
unnecessary checks while resetting.

Fixes: 95af1f1c4c9f ("ice: reschedule ice_ptp_wait_for_offset_valid during reset")
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 3 +++
 1 file changed, 3 insertions(+)

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;
 

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 08/13] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Jacob Keller]

The ice_stop_phy_timer_eth56g() function is called by the driver for E825
devices to ensure that the PHY timer has been stopped. The equivalent
function for older E822 devices performed many steps. However, on E825 it
only clears the PHY_REG_TX_OFFSET_READY and PHY_REG_RX_OFFSET_READY bits to
indicate to HW that it should no longer treat the PHY offset as valid.

When PHY_REG_TX_OFFSET_READY is cleared, the hardware still captures Tx
timestamps, but it no longer sets the valid bit for these timestamps. This
sounds reasonable at first glance. However, this results in the internal
outstanding timestamp counter becoming out of sync.

When capturing a timestamp, hardware increments its internal counter and
sets the associated "ready" bit in the timestamp memory status. Then it
compares the timestamp count to the threshold to determine if it should
trigger an interrupt to the MAC.

Upon reading the timestamp hardware is supposed to decrement the counter,
clear the valid bit, and clear the associated bit from the memory status
register. However, it only performs these steps *if* the valid bit is set.

Since the valid bit is not set while PHY_REG_TX_OFFSET_READY is clear, the
timestamp counter is not decremented and the memory status is not cleared.
This leaves the counter out-of-sync until a PHY soft reset.

According to the hardware engineers, the PHY_REG_TX_OFFSET_READY bit has no
other effects. It only controls whether hardware captures timestamps with
the valid bit set or not. Since capturing timestamps with the valid bit
clear is problematic, they recommend simply not clearing
PHY_REG_TX_OFFSET_READY.

Note that the PHY_REG_RX_OFFSET_READY performs a similar task. However,
clearing it is fine as there is no associated timestamp counter on the Rx
side. Receive timestamps are simply inserted into the descriptor. Clearing
this register clears the valid bit for timestamps until we complete
calibration and re-enable the register.

Notice that the soft_reset parameter of ice_stop_phy_timer_eth56g() is
totally unused. It is a relic from a copy-paste of
ice_stop_phy_timer_e82x() that is unnecessary, so remove it.

Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
requests could happen while the PHY is calibrating. To avoid this, set the
tx.calibrating field of the Tx timestamp tracker when stopping the timer
and clear it when finishing the restart. This ensures that any new requests
will be rejected until the PHY timer calibration has completed. Also call
ice_ptp_mark_tx_tracker_stale() to prevent reporting any previous
outstanding timestamps to the stack. Fix ice_ptp_mark_tx_tracker_stale() to
properly check if the tracker is initialized, to prevent a possible
use-after-free on the bitmap.

Unlike E822 devices, set the calibrating flag when "stopping" the PHY and
clear it immediately after the start procedure. The E825 device does not
perform vernier calibration and thus does not need to wait for hardware to
mark the offsets as valid.

In the event that the PHY timer start procedure fails, the device is in an
unknown state and timestamps will not behave properly. As such, and similar
to E822 devices, the calibrating field is not cleared on failure. This
leaves timestamp requests disabled until the next link restart.

Failure in the start flow is unexpected and it is unclear precisely what
state the hardware is left in. Attempting to add a complex retry mechanism
for a rare event is not worthwhile. The port restart procedure can already
be re-initiated by triggering a link reset (i.e. via ethtool).

Instead update the dev_err message at the end of ice_ptp_port_phy_restart.
Clearly indicate that timestamping is not functional, and add a note that a
link toggle might recover the device. For the invalid MAC type path
returning -ENODEV, skip this message and log a dev_dbg that we failed with
an unknown MAC type instead.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Suggested-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.h    |  5 ++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 42 +++++++++++++++++++++++------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 25 ++++++++---------
 4 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index d70f022c850b..43a02759a7ea 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -106,8 +106,9 @@ enum ice_tx_tstamp_work {
  * @offset: offset into timestamp block to get the real index
  * @len: length of the tstamps and in_use fields.
  * @init: if true, the tracker is initialized;
- * @disabled: set to true when new Tx timestamp requests are disabled, such as
- *            while the PHY Tx offset configuration has not yet completed.
+ * @disabled: set to true when new Tx timestamp requests are disabled. This
+ *            includes during E82X Vernier calibration, and for as long as the
+ *            PHY is considered stopped on E825.
  * @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
  *                    bitmap register. If false, fall back to verifying new
  *                    timestamp values against previously cached copy.
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 16b1988e993d..17000df77ce9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -378,7 +378,7 @@ int ice_cgu_get_output_pin_state_caps(struct ice_hw *hw, u8 pin_id,
 
 /* ETH56G family functions */
 int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status);
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset);
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port);
 int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port);
 int ice_phy_cfg_intr_eth56g(struct ice_hw *hw, u8 port, bool ena, u8 threshold);
 int ice_phy_cfg_ptp_1step_eth56g(struct ice_hw *hw, u8 port);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 6e1f60d7eef7..b7c539f98276 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -808,9 +808,6 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
  * Mark currently outstanding Tx timestamps as stale. This prevents sending
  * their timestamp value to the stack. This is required to prevent extending
  * the 40bit hardware timestamp incorrectly.
- *
- * This should be called when the PTP clock is modified such as after a set
- * time request.
  */
 static void
 ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
@@ -818,7 +815,8 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
 	unsigned long flags;
 
 	spin_lock_irqsave(&tx->lock, flags);
-	bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
+	if (tx->init)
+		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
 	spin_unlock_irqrestore(&tx->lock, flags);
 }
 
@@ -1228,6 +1226,7 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 	struct ice_pf *pf = ptp_port_to_pf(ptp_port);
 	u8 port = ptp_port->port_num;
 	struct ice_hw *hw = &pf->hw;
+	unsigned long flags;
 	int err;
 
 	lockdep_assert_held(&pf->adapter->ps_lock);
@@ -1243,7 +1242,14 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 		err = ice_stop_phy_timer_e82x(hw, port, true);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		err = ice_stop_phy_timer_eth56g(hw, port, true);
+		/* Disable new Tx timestamp requests */
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = true;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
+		err = ice_stop_phy_timer_eth56g(hw, port);
 		break;
 	default:
 		err = -ENODEV;
@@ -1307,15 +1313,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 					   0);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
+		/* ice_ptp_port_phy_stop() may have already disabled
+		 * timestamps, but some restarts occur without first stopping
+		 * the timer, so we ensure that new requests are disabled
+		 * here.
+		 */
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = true;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
 		err = ice_start_phy_timer_eth56g(hw, port);
+		if (err)
+			break;
+
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = false;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
 		break;
 	default:
-		err = -ENODEV;
+		dev_dbg(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with unknown MAC type %d\n",
+			port, hw->mac_type);
+		return -ENODEV;
 	}
 
 	if (err)
-		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
-			port, err);
+		dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with err %pe; Timestamping not enabled; A link-toggle may recover.\n",
+			port, ERR_PTR(err));
 
 	return err;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a41c711e751..07b55fbb88dd 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2098,32 +2098,33 @@ static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 }
 
 /**
- * ice_stop_phy_timer_eth56g - Stop the PHY clock timer
+ * ice_stop_phy_timer_eth56g - Clear PHY Rx offset ready flag
  * @hw: pointer to the HW struct
  * @port: the PHY port to stop
- * @soft_reset: if true, hold the SOFT_RESET bit of PHY_REG_PS
  *
- * Stop the clock of a PHY port. This must be done as part of the flow to
- * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
- * initialized or when link speed changes.
+ * Disable Rx timestamping by clearing the PHY_REG_RX_OFFSET_READY. This
+ * causes Rx timestamps to be captured with their valid bit clear, ensuring we
+ * discard any timestamp captured while the PHY is being recalibrated.
+ *
+ * Note this does *not* clear PHY_REG_TX_OFFSET_READY. Clearing it would
+ * cause the Tx timestamps to be captured with their valid bit clear.
+ * Unfortunately the captured timestamps still increment the internal counter
+ * and result in off-by-one accounting. Instead, Tx timestamp requests should
+ * be disabled by other means.
  *
  * Return:
  * * %0     - success
  * * %other - failed to write to PHY
  */
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 {
 	int err;
 
-	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 0);
-	if (err)
-		return err;
-
 	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
 	if (err)
 		return err;
 
-	ice_debug(hw, ICE_DBG_PTP, "Disabled clock on PHY port %u\n", port);
+	ice_debug(hw, ICE_DBG_PTP, "Disabled Rx timestamps on PHY port %u\n", port);
 
 	return 0;
 }
@@ -2151,7 +2152,7 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 
 	tmr_idx = ice_get_ptp_src_clock_index(hw);
 
-	err = ice_stop_phy_timer_eth56g(hw, port, false);
+	err = ice_stop_phy_timer_eth56g(hw, port);
 	if (err)
 		return err;
 

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 09/13] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[Jacob Keller]

The current implementation of ice_ptp_reset_ts_memory_eth56g() is flawed.
It tries to clear the timestamp memory by writing to the
PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it does
not trigger appropriate PHY actions.

To clear outstanding timestamp memory, the driver must read the timestamps.
However, naively doing this as part of ice_ptp_reset_ts_memory() is
problematic. When reading the timestamp index, hardware kicks off a chain
of actions including clearing the ready bitmap index, and decrementing an
internal counter if the timestamp index was marked as valid.

This can potentially leave the internal hardware counter out of sync with
the actual number of timestamps. This occurs because the
PHY_TSTAMP memory region containing the timestamp data is not
zero-initialized when the devices boots up. Instead, it is filled with
garbage. On a cold power on, attempts to read the stale data result in the
hardware triggering a counter decrement for a timestamp that never
happened. This underflows the counter, and prevents new timestamp
interrupts from being triggered for real timestamp requests.

In normal operation this is "fine" because the driver should never read
from the PHY_TSTAMP region without first checking if the
PHY_REG_TX_MEMORY_STATUS bit for a given index is set. However, driver bugs
or programmer error could result in accidentally reading this data.

Attempting to write to the PHY_TSTAMP region is also ineffective as the
registers are read only. Instead, reading a register triggers PHY action to
clear its valid bit. However, the PHY also performs other actions such as
clearing the bit in the PHY_REG_TX_MEMORY_STATUS and decrementing the
internal counter.

We must read the PHY_TSTAMP memory in order to clear stale timestamps. But
doing so may cause a desync with the counter. To prevent issues, perform
this clearing always and only right before initiating a PHY soft reset.

The soft reset will clear and reset the internal counter and the
PHY_REG_TX_MEMORY_STATUS ready bitmap. The reads to PHY_TSTAMP reagion will
reset the region valid bits ensuring that no stale data is left behind.
This combination ensures that we always have a clean slate with no stale
data and with the counter properly reset to zero.

If a read fails (i.e. due to a transient sideband queue failure) it is not
treated as a fatal error. There is already a dynamic debug message for each
read failure. Keep track of the total number of timestamp registers that
fail for a given port and log a warning message indicating the total number
of failures. Continuing is acceptable as clearing the memory is a
precaution against faulty software. Correct software access won't read the
index twice and won't read it at all except when it has already confirmed
that the index is valid by reading ice_get_phy_tx_tstamp_ready(), at which
point the hardware will have overwritten any stale data.

Since ice_ptp_reset_ts_memory now only works for E82X hardware, update its
kdoc comment to clarify what hardware is skipped and note specifically why
E825 is skipped.

Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at initialization")
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 110 ++++++++++++++--------------
 1 file changed, 57 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 07b55fbb88dd..5637925c0e68 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
 	return ice_read_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
 }
 
-/**
- * ice_write_port_mem_eth56g - Write a PHY port memory location
- * @hw: pointer to the HW struct
- * @port: Port number to be read
- * @offset: Offset from PHY port register base
- * @val: Pointer to the value to read (out param)
- *
- * Return:
- * * %0      - success
- * * %EINVAL - invalid port number or resource type
- * * %other  - failed to write to PHY
- */
-static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
-				     u32 val)
-{
-	return ice_write_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
-}
-
 /**
  * ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
  * @hw: pointer to the HW struct
@@ -1129,47 +1111,35 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
 }
 
 /**
- * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from the quad block
+ * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from PHY memory
  * @hw: pointer to the HW struct
- * @port: the quad to read from
+ * @port: the port to read from
  * @idx: the timestamp index to reset
  *
- * Read and then forcibly clear the timestamp index to ensure the valid bit is
- * cleared and the timestamp status bit is reset in the PHY port memory of
- * internal PHYs of the 56G devices.
+ * Read the timestamp index to ensure that the valid bit is cleared and the
+ * timestamp status bit is reset in the PHY port memory.
  *
- * To directly clear the contents of the timestamp block entirely, discarding
- * all timestamp data at once, software should instead use
- * ice_ptp_reset_ts_memory_quad_eth56g().
- *
- * This function should only be called on an idx whose bit is set according to
- * ice_get_phy_tx_tstamp_ready().
+ * This function should only be called on an index whose bit is set according
+ * to ice_get_phy_tx_tstamp_ready(), or as part of a full sweep when paired
+ * with a PHY soft reset via ice_ptp_phy_soft_reset_eth56g().
  *
  * Return:
  * * %0     - success
- * * %other - failed to write to PHY
+ * * %other - failed to read from PHY
  */
 static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
 {
 	u64 unused_tstamp;
-	u16 lo_addr;
 	int err;
 
-	/* Read the timestamp register to ensure the timestamp status bit is
-	 * cleared.
+	/* Per the PHY spec, reading the timestamp memory location is what
+	 * clears the entry's valid bit and its corresponding (read-only)
+	 * ts_memory_status bit.
 	 */
 	err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n",
 			  port, idx, err);
-	}
-
-	lo_addr = (u16)PHY_TSTAMP_L(idx);
-
-	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
-	if (err) {
-		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n",
-			  port, idx, err);
 		return err;
 	}
 
@@ -1177,19 +1147,43 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
 }
 
 /**
- * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the port block
+ * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory
  * @hw: pointer to the HW struct
+ * @port: port number to clear
+ *
+ * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the
+ * only way to clear a timestamp valid bit (and its read-only ts_memory_status
+ * bit) is to read the timestamp memory location, so read every entry for the
+ * port (two 32-bit reads each). This discards all timestamp data on the port,
+ * so it must only be used for a full reset; callers that must preserve
+ * in-flight timestamps clear individual indices via ice_clear_phy_tstamp().
+ *
+ * Due to interactions with an internal HW counter for the number of
+ * outstanding Tx timestamps, this *must* only be called as part of the
+ * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
+ * may become out of sync and prevent new timestamp interrupts.
+ *
+ * Failure to read a given index (i.e. due to a transient sideband queue
+ * failure) is not considered fatal as the PHY port is about to be soft reset.
+ * While the soft reset does not clear the timestamp memory, software
+ * shouldn't be reading the timestamp memory without already knowing it is
+ * valid via ice_get_phy_tx_tstamp_ready(), and this sweep is just
+ * a precaution to ensure the memory is in a known state.
  */
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
+static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
 {
-	unsigned int port;
+	int err, failed = 0;
+	u8 idx;
 
-	for (port = 0; port < hw->ptp.num_lports; port++) {
-		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L,
-					 0);
-		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U,
-					 0);
+	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
+		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
+		if (err)
+			failed++;
 	}
+
+	if (failed)
+		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
+			 failed, port);
 }
 
 /**
@@ -2295,6 +2289,7 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
  *
  * Trigger a soft reset of the ETH56G PHY by toggling the soft reset
  * bit in the PHY global register. The reset sequence consists of:
+ *   0. Reading every timestamp memory register to clear its valid bit
  *   1. Clearing the soft reset bit
  *   2. Asserting the soft reset bit
  *   3. Clearing the soft reset bit again
@@ -2303,6 +2298,12 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
  * to settle. This provides a controlled way to reinitialize the PHY
  * without requiring a full device reset.
  *
+ * To ensure that the internal counter matches the contents of the
+ * PHY_TSTAMP memory region, read every timestamp index prior to performing
+ * the soft reset. The read side effects ensure that the timestamp memory is
+ * cleared while the soft reset ensures that the PHY_REG_TX_MEMORY_STATUS and
+ * internal counter are reset to zero.
+ *
  * Return: 0 on success, or a negative error code on failure when
  *         reading or writing the PHY register.
  */
@@ -2311,6 +2312,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
 	u32 global_val;
 	int err;
 
+	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
+
 	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
@@ -5794,6 +5797,11 @@ static int ice_get_pf_c827_idx(struct ice_hw *hw, u8 *idx)
 /**
  * ice_ptp_reset_ts_memory - Reset timestamp memory for all blocks
  * @hw: pointer to the HW struct
+ *
+ * Only E82X hardware can directly reset memory for all blocks. E825 hardware
+ * can only safely reset timestamp memory as part of the soft reset, so is not
+ * reset here. E810 and E830 also do not implement direct reset of the
+ * timestamp memory region.
  */
 void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 {
@@ -5801,10 +5809,6 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 	case ICE_MAC_GENERIC:
 		ice_ptp_reset_ts_memory_e82x(hw);
 		break;
-	case ICE_MAC_GENERIC_3K_E825:
-		ice_ptp_reset_ts_memory_eth56g(hw);
-		break;
-	case ICE_MAC_E810:
 	default:
 		return;
 	}

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 10/13] ice: E825: perform a soft reset when starting the PHY timer

[Jacob Keller]

To ensure that the E825 PHY timer begins in a clean state, initiate a PHY
soft reset prior to programming the PHY. This ensures that we clear any
outstanding Tx timestamp memory, and ensures that the PHY internal state
including its timestamp counter has been reset.

Note that despite the name, this PHY soft reset only impacts the timer
block of the PHY, and not its regular Tx/Rx functionality. Additionally,
many configuration registers such as the PHY_REG_TS_INT_CONFIG and
PHY_REG_TX_OFFSET_READY are not reset.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 5637925c0e68..2d000251b185 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2128,9 +2128,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port)
  * @hw: pointer to the HW struct
  * @port: the PHY port to start
  *
- * Start the clock of a PHY port. This must be done as part of the flow to
- * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
- * initialized or when link speed changes.
+ * Perform a PHY soft reset and then start the clock for the PHY port.
+ *
+ * This must be done as part of the flow to re-calibrate Tx and Rx
+ * timestamping offsets whenever the clock time is initialized or when link
+ * speed changes.
  *
  * Return:
  * * %0     - success
@@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	if (err)
 		return err;
 
+	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
+	if (err)
+		return err;
+
 	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
 
 	err = ice_phy_cfg_parpcs_eth56g(hw, port);

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 11/13] ice: keep Tx timestamp slots tracked until completion or timeout

[Jacob Keller]

From: Petr Oros <poros@redhat.com>

When the link goes down the processing loop drops every outstanding
request, and a request whose timestamp is not ready yet is freed
without reading the PHY slot. The hardware completes the capture a
moment later, the orphaned ready bit blocks the port interrupt until
the next link-up sweep, and the freed index can meanwhile be reused
by a new request whose slot the hardware then overwrites. Captured on
a reproducer as ready bits with no in_use owner right after a link
bounce.

Stop dropping on link down. Mark the outstanding requests stale so
their completions are read and discarded, reject new requests while
the link is down, and free a not yet ready slot only after the two
second timeout. This way an index is never reused while the hardware
can still write it and never left untracked while a completion can
still arrive.

To avoid continuously re-triggering the IRQ at the end of a processing loop
when we have a stale packet that is not timestamped, modify
ice_ptp_tx_tstamps_pending() to ignore stale timestamps when checking for
whether to re-arm the IRQ from the miscellaneous thread function. Instead,
only count stale packets as part of the check in the auxiliary work thread.
This way we do not keep spamming the IRQ when the timestamp won't be
reported anyways.

To ensure that forward progress is made on clearing stale timestamps, fix
the check for ice_ptp_maybe_trigger_tx_interrupt to properly apply for
devices that manage their own interrupt, instead of only checking on the
clock owner. In the case of a timestamp which never completes, this does
result in one extra IRQ every 500 msec until the timeout. In some sense
this is extra work, but we need to recheck to ensure that timestamp slots
do not remain locked forever, and the software can't reliably know whether
a given index will or won't complete.

Note that E810 does not have the ready bitmap in its hardware. It is
currently excluded from the ice_ptp_maybe_trigger_tx_interrupt() by a check
against the has_ready_bitmap field. This could lead to an interrupt stall
if every single Tx timestamp index slot becomes stale. However, just
removing the check against has_ready_bitmap would be problematic for the
low latency path which currently is not protected against another IRQ
happening while a request is outstanding. That is a pre-existing issue, but
we should avoid making it worse until the path can be addressed by another
fix. The low latency path also completely bypasses the
ice_ptp_process_tx_tstamp() flow. For now, skip the
ice_ptp_maybe_trigger_tx_interrupt() when ts_ll_int_read is set and the low
latency path is in use. The path still has preexisting issues but this
avoids making it worse.

This effectively reverts commit fcc2cef37fed ("ice/ptp: fix the PTP worker
retrying indefinitely if the link went down"), which tried to release an
index before this 2 second wait period.

Fixes: fcc2cef37fed ("ice/ptp: fix the PTP worker retrying indefinitely if the link went down")
Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.h    |  8 +++--
 drivers/net/ethernet/intel/ice/ice_main.c   |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 53 ++++++++++++++---------------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c |  4 +--
 4 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 43a02759a7ea..a9c577279023 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -133,6 +133,9 @@ struct ice_ptp_tx {
 #define INDEX_PER_PORT_E82X		16
 #define INDEX_PER_PORT			64
 
+/* Maximum number of timestamp indexes across all devices */
+#define INDEX_PER_PORT_MAX              INDEX_PER_PORT
+
 /**
  * struct ice_ptp_port - data used to initialize an external port for PTP
  *
@@ -315,7 +318,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx);
 void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx);
 void ice_ptp_process_ts(struct ice_pf *pf);
 irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf);
-bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf);
+bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq);
 u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 			     struct ptp_system_timestamp *sts);
 
@@ -363,7 +366,8 @@ static inline irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 	return IRQ_HANDLED;
 }
 
-static inline bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
+static inline bool
+ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
 	return false;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 910ebdcdde19..125a0b9b260f 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3248,7 +3248,7 @@ static irqreturn_t ice_misc_intr_thread_fn(int __always_unused irq, void *data)
 	ice_irq_dynamic_ena(hw, NULL, NULL);
 	ice_flush(hw);
 
-	if (ice_ptp_tx_tstamps_pending(pf)) {
+	if (ice_ptp_tx_tstamps_pending(pf, true)) {
 		/* If any new Tx timestamps happened while in interrupt,
 		 * re-arm the interrupt to trigger it again.
 		 */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index b7c539f98276..309fa2877cc3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -365,9 +365,12 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
 static bool
 ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
 {
+	struct ice_ptp_port *ptp_port =
+		container_of(tx, struct ice_ptp_port, tx);
+
 	lockdep_assert_held(&tx->lock);
 
-	return tx->init && !tx->disabled;
+	return tx->init && !tx->disabled && READ_ONCE(ptp_port->link_up);
 }
 
 /**
@@ -564,7 +567,6 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	u64 tstamp_ready;
-	bool link_up;
 	int err;
 	u8 idx;
 
@@ -582,14 +584,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 			return;
 	}
 
-	/* Drop packets if the link went down */
-	link_up = READ_ONCE(ptp_port->link_up);
-
 	for_each_set_bit(idx, tx->in_use, tx->len) {
 		struct skb_shared_hwtstamps shhwtstamps = {};
 		u8 phy_idx = idx + tx->offset;
 		u64 raw_tstamp = 0, tstamp;
-		bool drop_ts = !link_up;
+		bool drop_ts = false;
 		struct sk_buff *skb;
 
 		/* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()
@@ -2759,21 +2758,22 @@ void ice_ptp_process_ts(struct ice_pf *pf)
 	}
 }
 
-static bool ice_port_has_timestamps(struct ice_ptp_tx *tx)
+static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
 {
-	bool more_timestamps;
+	DECLARE_BITMAP(tstamps, INDEX_PER_PORT_MAX) = {};
 
 	scoped_guard(spinlock_irqsave, &tx->lock) {
 		if (!tx->init)
 			return false;
 
-		more_timestamps = !bitmap_empty(tx->in_use, tx->len);
+		if (in_irq)
+			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
+		else
+			return !bitmap_empty(tx->in_use, tx->len);
 	}
-
-	return more_timestamps;
 }
 
-static bool ice_any_port_has_timestamps(struct ice_pf *pf)
+static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
 {
 	struct ice_port_list *ports = &pf->adapter->ports;
 	bool have_tstamps = false;
@@ -2786,7 +2786,7 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf)
 		if (!kref_get_unless_zero(&port->ref))
 			continue;
 
-		if (ice_port_has_timestamps(&port->tx))
+		if (ice_port_has_timestamps(&port->tx, in_irq))
 			have_tstamps = true;
 
 		kref_put(&port->ref, ice_ptp_release_port_srcu);
@@ -2799,7 +2799,7 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf)
 	return have_tstamps;
 }
 
-bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
+bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
 	struct ice_hw *hw = &pf->hw;
 	int ret;
@@ -2809,11 +2809,11 @@ bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		return false;
 	case ICE_PTP_TX_INTERRUPT_SELF:
-		if (ice_port_has_timestamps(&pf->ptp.port.tx))
+		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
 			return true;
 		break;
 	case ICE_PTP_TX_INTERRUPT_ALL:
-		if (ice_any_port_has_timestamps(pf))
+		if (ice_any_port_has_timestamps(pf, in_irq))
 			return true;
 		break;
 	default:
@@ -2889,7 +2889,7 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 		/* E830 can read timestamps in the top half using rd32() */
 		ice_ptp_process_ts(pf);
 
-		if (ice_ptp_tx_tstamps_pending(pf)) {
+		if (ice_ptp_tx_tstamps_pending(pf, true)) {
 			/* Process outstanding Tx timestamps. If there
 			 * is more work, re-arm the interrupt to trigger again.
 			 */
@@ -2911,27 +2911,24 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
  * current timestamp data is read. In rare circumstances, it is possible that
  * the driver fails to read all outstanding data.
  *
- * To avoid getting permanently stuck, periodically check if the PHY has
- * outstanding timestamp data. If so, trigger an interrupt from software to
- * process this data.
+ * To avoid getting permanently stuck, periodically check if there is any
+ * outstanding timestamps or if the PHY has marked any index as ready. If so,
+ * trigger an interrupt from software to process this data.
  */
 static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
-	int ret;
 
-	if (!pf->ptp.port.tx.has_ready_bitmap)
+	/* Avoid re-triggering OICR on E810 with low latency interrupt path */
+	if (hw->dev_caps.ts_dev_info.ts_ll_int_read)
 		return;
 
-	if (!ice_pf_src_tmr_owned(pf))
+	if (pf->ptp.tx_interrupt_mode != ICE_PTP_TX_INTERRUPT_SELF &&
+	    !ice_pf_src_tmr_owned(pf))
 		return;
 
-	ret = ice_check_phy_tx_tstamp_ready(hw);
-	if (ret < 0) {
-		dev_dbg(dev, "PTP periodic task unable to read PHY timestamp ready bitmap, err %d\n",
-			ret);
-	} else if (ret) {
+	if (ice_ptp_tx_tstamps_pending(pf, false)) {
 		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
 
 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 2d000251b185..b0390f49a5a0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -5001,8 +5001,8 @@ ice_get_phy_tx_tstamp_ready_e810(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
  * The E810 devices do not have a Tx memory status register. Note this is
  * intentionally different behavior from ice_get_phy_tx_tstamp_ready_e810
  * which always says that all bits are ready. This function is called in cases
- * where code will trigger interrupts if timestamps are waiting, and should
- * not be called for E810 hardware.
+ * where code will trigger interrupts if timestamps are waiting, and which
+ * should not trigger for E810.
  *
  * Return: 0.
  */

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 12/13] ice: skip reading Tx ready bitmap on ports with no timestamps

[Jacob Keller]

On E82x devices, the interrupt for Tx timestamps are handled by the clock
owner. When an interrupt with the Tx timestamp cause is fired, the clock
owner PF iterates the list of ports and checks for timestamps across all
ports.

The existing logic reads the PHY timestamp ready bitmap before iterating
the list of in-use timestamp indexes, even for ports which have no
timestamps waiting in the software timestamp tracker. This has a
significant and measurable latency impact on reporting Tx timestamps.

Check the bitmap and exit early in the event that there are no timestamps
waiting on a port. Observant reviewers may notice that the check is done
without acquiring the lock. This is fine, as whether the thread sees or
fails to see a new outstanding timestamp does not affect correctness, only
determining whether or not it should do extra work.

Skipping the check has the highest impact on E822 an E825 devices which
iterate all ports in a single thread, but it is applied universally to all
device types since the extra read is unnecessary regardless.

The average latency of a Tx timestamp is impacted by several factors
including system load, the number of timestamp requests, and some random
factors that are difficult to control. However for comparison on my system
with these changes, while operating ptp4l on a single port with a sync rate
of 16/second:

  Before: mean 317.43 microseconds, standard deviation 34.51
  After: mean 189.71 microseconds, standard deviation 43.24

Of course this level of improvement may not be indicative of a production
setup as one might expect timestamps to be operating out of multiple ports
on the device. However, even in that case an improvement is still likely
depending on the actual timestamp rates.

Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 309fa2877cc3..b0ea01cfcc4f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -574,7 +574,7 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	pf = ptp_port_to_pf(ptp_port);
 	hw = &pf->hw;
 
-	if (!tx->init)
+	if (!tx->init || bitmap_empty(tx->in_use, tx->len))
 		return;
 
 	/* Read the Tx ready status first */

-- 
2.56.0.212.g129ede20d986

[PATCH net v5 13/13] ice: don't clear in_use until HW clears ready bitmap

[Jacob Keller]

During a link down transition, there is a small window where hardware
does not properly respond to reading the PHY timestamp registers. When this
occurs, the PHY does not automatically clear the ready bitmap or the valid
bit for the timestamp. This begins happening slightly before a link
transition even before the firmware has notified the driver of the state
change.

The driver happily completes the timestamp, releasing the in_use bit. This
allows another request to reuse the bit potentially reporting an invalid
stale timestamp. Additionally, with the ready bit still set high the driver
continues to re-trigger the IRQ and check for timestamps in a tight loop,
wasting CPU cycles.

To fix this, re-read the PHY timestamp memory status after each read of a
PHY index. Double check if the hardware cleared the index properly. If it
hasn't, mark the timestamp index as stale and keep the index locked.
The index remains locked in software, and will be re-checked the next
iteration through ice_ptp_process_tx_tstamp(). The driver will still see
the ready bit set, and will re-read the timestamp. Once the hardware link
down phase has completed, the read should clear the ready bitmap. Note that
it is expected that a read to the timestamp will either clear the ready bit
or be blocked and leave the ready bit stuck.

Marking the packet as stale makes sense since we know this hardware
behavior begins near when link is going down.

If the ice_get_phy_tx_tstamp_ready() fails, treat it the same as if the
hardware hadn't cleared the bitmap. Continuing here is safe since the
ice_get_phy_tx_tstamp_ready() device implementations do not update the
output parameter except on success. Note that such "transient" errors to
access the PHY may result in locking na index for two seconds, but this is
expected to be extremely rare and unlikely to trigger in most production
environments. This is the simplest approach and avoids risking a potential
reuse of the stuck ready bit which could lead to reporting garbage data.

The extra reads of the timestamp ready bitmap are saved in the same
tstamp_ready field. This effectively updates the ready bitmap for all
timestamps remaining in the loop. This is safe, and actually increases the
chance that a given timestamp will be processed by the loop in the event
that a timestamp completed after the initial read. Update the comments to
remove some text that might imply otherwise.

This flow has been observed on E825, but allowing the driver to free an
index which is not cleared would be incorrect regardless of which device
type it occurs on. Thus, this re-read is applied to all device types.

In the unlikely event that a timestamp has timed out the 2 second wait
*and* somehow suddenly has its ready bit set but unable to clear on read,
this could accidentally increment the timeout counter. It is intentional
that we do *not* release the index even in a timed out case, as we must not
allow reuse of that index until we can be certain it has cleared. Instead,
refactor so that the timeout counter is only incremented once the index is
released, (after the skip_tx_read label). This ensures that we don't double
count timeouts or count them prematurely. The "stuck" ready indexes remain
locked *indefinitely* until the hardware reaches a state where the clear
works.

Stale timestamps are already ignored by the ice_any_port_has_timestamps()
function. However, the ice_ptp_tx_tstamps_pending() function also checks
the ready bitmap. Instead, modify it to only check the software tracker.
Additionally, stop re-triggering the interrupt from the IRQ if the
timestamp tracker is calibrating or has the link marked as down. Continue
to check the hardware ready bitmap from the watchdog to catch cases of
unexpected timestamps.

With these changes, the timestamp processing no longer triggers a repeated
spamming of the IRQ during link down events where timestamps get stuck as
the PHY transitions to link down. Once link is restored, the PHY will be
reset and the stuck timestamps are cleared.

Measuring CPU utilization of the miscellaneous IRQ thread function during
timestamp storms near a link reset shows that this prevents the spikes
caused by the "stuck" ready bit. Without this fix, the CPU handling the IRQ
becomes slammed due to the IRQ re-triggering logic.

Measuring latency using the ice Tx timestamp traces does show that this fix
comes at a latency cost. Latency is measured using the ice Tx timestamp
traces for the request to completion time. I measured a couple of different
workloads both before and after this fix:

 * ptp4l using a profile with ~16 SYNC messages per second

    before: 189.71 microseconds mean, stdev 43.24
     after: 195.92 microseconds mean, stdev 25.28

 * a C program generating 16 timestamp requests every 10 milliseconds on
   two different ports:

    before: 457.11 microseconds mean, stdev 180.63
     after: 717.77 microseconds mean, stdev 321.58

In the normal work flows this comes with about a 10-20 microsecond penalty
on the average, and the standard deviation remains similar (with some
variance between run to run comparison).

For heavy workloads with many more timestamps than expected for typical
applications this comes at a significant cost. This is because we handle
all timestamps in a single thread. If there are many concurrent timestamps
being requested at once, any which use the later slots on ports later in
the port list will take much longer to be processed once the interrupt is
fired. Since each timestamp now requires an additional PHY register access,
this cost is much higher in the case where the device is under unusually
heavy load. The high standard deviation indicates a very high variance in
timestamp latency, with many timestamps completing in the usual time but
some taking significantly longer when multiple timestamps are outstanding
in a single IRQ.

Ultimately, *correctness* is more important than speed here. Additionally,
we still remain well below the default limit of 10 milliseconds that ptp4l
will wait before complaining about missing timestamps.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 88 +++++++++++++++++---------------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index b0ea01cfcc4f..c0d056896825 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -557,7 +557,9 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx)
  * extremely unlikely that a packet will ever take this long to timestamp. If
  * we detect a Tx timestamp request that has waited for this long we assume
  * the packet will never be sent by hardware and discard it without reading
- * the timestamp register.
+ * the timestamp register. Note that in the unusual case where the PHY
+ * continuously fails to clear the ready bitmap index, the slot may remain
+ * locked for more than two seconds.
  */
 static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 {
@@ -586,9 +588,9 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 
 	for_each_set_bit(idx, tx->in_use, tx->len) {
 		struct skb_shared_hwtstamps shhwtstamps = {};
+		bool drop_ts = false, timeout = false;
 		u8 phy_idx = idx + tx->offset;
 		u64 raw_tstamp = 0, tstamp;
-		bool drop_ts = false;
 		struct sk_buff *skb;
 
 		/* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()
@@ -600,18 +602,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 		/* Drop packets which have waited for more than 2 seconds */
 		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
 			drop_ts = true;
-
-			/* Count the number of Tx timestamps that timed out */
-			pf->ptp.tx_hwtstamp_timeouts++;
+			timeout = true;
 		}
 
-		/* Only read a timestamp from the PHY if its marked as ready
-		 * by the tstamp_ready register. This avoids unnecessary
-		 * reading of timestamps which are not yet valid. This is
-		 * important as we must read all timestamps which are valid
-		 * and only timestamps which are valid during each interrupt.
-		 * If we do not, the hardware logic for generating a new
-		 * interrupt can get stuck on some devices.
+		/* Only read a timestamp from the PHY if it is marked as ready
+		 * by the timestamp_ready register.
 		 */
 		if (tx->has_ready_bitmap &&
 		    !(tstamp_ready & BIT_ULL(phy_idx))) {
@@ -627,6 +622,27 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 		if (err && !drop_ts)
 			continue;
 
+		/* verify ready bit cleared */
+		if (tx->has_ready_bitmap) {
+			err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
+			if (err || tstamp_ready & BIT_ULL(phy_idx)) {
+				spin_lock_irqsave(&tx->lock, flags);
+				if (test_bit(idx, tx->in_use) &&
+				    !test_and_set_bit(idx, tx->stale))
+					dev_dbg(ice_pf_to_dev(pf), "PHY port %u failed to clear ready bit for idx %u\n",
+						ptp_port->port_num, phy_idx);
+				skb = tx->tstamps[idx].skb;
+				tx->tstamps[idx].skb = NULL;
+				spin_unlock_irqrestore(&tx->lock, flags);
+				/* Free the SKB immediately since we're never
+				 * going to report a timestamp for the stale
+				 * packet.
+				 */
+				dev_kfree_skb_any(skb);
+				continue;
+			}
+		}
+
 		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
 
 		/* For PHYs which don't implement a proper timestamp ready
@@ -643,6 +659,9 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 			drop_ts = true;
 
 skip_ts_read:
+		if (timeout)
+			pf->ptp.tx_hwtstamp_timeouts++;
+
 		spin_lock_irqsave(&tx->lock, flags);
 		if (!tx->has_ready_bitmap && raw_tstamp)
 			tx->tstamps[idx].cached_tstamp = raw_tstamp;
@@ -653,8 +672,9 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 			drop_ts = true;
 		spin_unlock_irqrestore(&tx->lock, flags);
 
-		/* It is unlikely but possible that the SKB will have been
-		 * flushed at this point due to link change or teardown.
+		/* The SKB may have already been freed such as due to failure
+		 * to clear the ready bit, or being flushed due to link change
+		 * or teardown.
 		 */
 		if (!skb)
 			continue;
@@ -786,6 +806,7 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
 			ice_clear_phy_tstamp(hw, tx->block, phy_idx);
 
 		spin_lock_irqsave(&tx->lock, flags);
+
 		skb = tx->tstamps[idx].skb;
 		tx->tstamps[idx].skb = NULL;
 		clear_bit(idx, tx->in_use);
@@ -2766,10 +2787,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
 		if (!tx->init)
 			return false;
 
-		if (in_irq)
+		if (in_irq) {
+			if (!ice_ptp_is_tx_tracker_up(tx))
+				return false;
+
 			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
-		else
+		} else {
 			return !bitmap_empty(tx->in_use, tx->len);
+		}
 	}
 }
 
@@ -2801,41 +2826,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
 
 bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
-	struct ice_hw *hw = &pf->hw;
-	int ret;
-
-	/* Check software indicator */
 	switch (pf->ptp.tx_interrupt_mode) {
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		return false;
 	case ICE_PTP_TX_INTERRUPT_SELF:
-		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
-			return true;
-		break;
+		return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq);
 	case ICE_PTP_TX_INTERRUPT_ALL:
-		if (ice_any_port_has_timestamps(pf, in_irq))
-			return true;
-		break;
+		return ice_any_port_has_timestamps(pf, in_irq);
 	default:
 		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n",
 			  pf->ptp.tx_interrupt_mode);
-		break;
-	}
-
-	/* Check hardware indicator */
-	ret = ice_check_phy_tx_tstamp_ready(hw);
-	if (ret < 0) {
-		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
-			ret);
-		/* Stop triggering IRQs if we're unable to read PHY */
 		return false;
 	}
-
-	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
-	 * available, 0 if there are no waiting timestamps, and a negative
-	 * value if there was an error (which we checked for above).
-	 */
-	return ret > 0;
 }
 
 /**
@@ -2919,6 +2921,7 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
+	int ret;
 
 	/* Avoid re-triggering OICR on E810 with low latency interrupt path */
 	if (hw->dev_caps.ts_dev_info.ts_ll_int_read)
@@ -2928,7 +2931,8 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 	    !ice_pf_src_tmr_owned(pf))
 		return;
 
-	if (ice_ptp_tx_tstamps_pending(pf, false)) {
+	ret = ice_check_phy_tx_tstamp_ready(hw);
+	if (ret > 0 || ice_ptp_tx_tstamps_pending(pf, false)) {
 		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
 
 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

-- 
2.56.0.212.g129ede20d986

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · 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).

- [Medium] The new error unwind in ice_ptp_init() destroys the PTP
  kworker while the owner's PTP clock is still registered.
- [Medium] On E810 (has_ready_bitmap == 0), the Tx tracker now survives
  CORE/GLOBAL/EMP resets.

Pre-existing issues:
- [Medium] Concern: ice_ptp_flush_all_tx_tracker(),
  ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec() walk…

--

ice: fix removal of PTP timestamp tracker during reset

This patch stops ice_ptp_prepare_for_reset() from releasing the Tx
timestamp tracker, so timestamping works again after CORE, GLOBAL or EMP
resets. It also reorders ice_ptp_init() so the kworker and tracker are set
up before the port is added to the port list, with a matching error
unwind. The non-READY branch of ice_ptp_release() now frees the tracker
and kworker.

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/

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index eaec36ab6ae3..e1c66cd0f94f 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);
> -
>  	/* Disable periodic outputs */
>  	ice_ptp_disable_all_perout(pf);

[Severity: Medium]
The tracker now survives CORE/GLOBAL/EMP resets. On E810, can the
cached_tstamp values then drift from the PHY state after the post-reset
flush?

The rebuild path runs after hw->reset_ongoing has already been set back
to false:

ice_rebuild()->ice_ptp_rebuild()->ice_ptp_rebuild_owner()
  ->ice_ptp_flush_all_tx_tracker()->ice_ptp_flush_tx_tracker()

On E810, ice_get_phy_tx_tstamp_ready_e810() reports every slot as ready:

	*tstamp_ready = 0xFFFFFFFFFFFFFFFF;

So ice_ptp_flush_tx_tracker() clears every in_use slot in hardware:

	if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
		ice_clear_phy_tstamp(hw, tx->block, phy_idx);

That writes 0 to the PHY slot. However, tx->tstamps[idx].cached_tstamp
keeps its old non-zero value.

E810 has has_ready_bitmap == 0, so ice_ptp_process_tx_tstamp() treats any
raw value that differs from the cache as captured:

	if (!drop_ts && !tx->has_ready_bitmap &&
	    raw_tstamp == tx->tstamps[idx].cached_tstamp)
		continue;

	/* Discard any timestamp value without the valid bit set */
	if (!(raw_tstamp & ICE_PTP_TS_VALID))
		drop_ts = true;

The next request on that slot might be processed before hardware captures
it, for example because another slot raised the TSYN_TX interrupt. In that
case raw is 0, which does not match the stale cache. The skb then
completes without a timestamp and in_use is cleared. The cache is not
updated, because raw is 0.

The real capture for that packet then lands in the slot that was just
freed. Could the next request that reuses the slot read this leftover
value and report it as its own timestamp?

Would resetting cached_tstamp when the flush clears the PHY slot keep the
software and hardware state in sync?

[ ... ]

> @@ -3363,16 +3366,19 @@ void ice_ptp_init(struct ice_pf *pf)

[ ... ]

>  	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
>  	return;
>  
> -err_clean_pf:
> +err_release_tx_tracker:
> +	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> +err_release_kworker:
> +	kthread_cancel_delayed_work_sync(&ptp->work);
> +	kthread_destroy_worker(ptp->kworker);
> +	ptp->kworker = NULL;
>  	mutex_destroy(&ptp->port.ps_lock);
> -	ice_ptp_cleanup_pf(pf);
>  err_exit:
>  	/* If we registered a PTP clock, release it */
>  	if (pf->ptp.clock) {

[Severity: Medium]
Is the PTP clock still registered while the kworker is torn down here?

On the clock owner, ice_ptp_init_owner() has already registered the
clock. /dev/ptpN stays live until err_exit calls ptp_clock_unregister().

The settime64 and adjtime ops do not check ptp->state, and they call
ice_ptp_reset_cached_phctime(). Suppose ice_ptp_update_cached_phctime()
returns -EAGAIN because ICE_CFG_BUSY is held, for example by a concurrent
adjtime on the same PHC. That function then does:

	kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
				   msecs_to_jiffies(10));

If this runs after kthread_cancel_delayed_work_sync() but before or during
kthread_destroy_worker(), a timer is left pending. Could
kthread_delayed_work_timer_fn() later touch the freed worker?

If it runs after ptp->kworker = NULL, would it dereference NULL?

One way to reach this window: ice_ptp_init_port() fails with -ENOMEM from
ice_ptp_alloc_tx_tracker() while a process with /dev/ptpN open calls
clock_settime() or clock_adjtime().

In this same patch, the non-READY branch of ice_ptp_release() unregisters
the clock before tearing down the worker. Should the init error path use
the same order? This ordering is still present at the end of the series.

> @@ -3399,12 +3405,22 @@ 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.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 and was not introduced by this patch.
ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
ice_ptp_prepare_rebuild_sec() walk the adapter port list without holding
ports.lock:

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

At the same time, a peer PF's ice_ptp_release() can remove its port from
the list and free the tracker, kworker and ps_lock. Since trackers are now
kept across resets, the owner's flush walks memory that belongs to live
peers.

The next patch in the series, "ice: use reference counting and SRCU for
PTP port access", fixes this. It converts these walkers to
srcu_read_lock() with list_for_each_entry_srcu() and
kref_get_unless_zero(). It also makes ice_ptp_cleanup_pf() wait for the
references and SRCU readers to finish before the tracker and worker are
freed. The race is gone by the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 03/13] ice: fix PHY port restart serialization

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 4

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).

- [Medium] ice_ptp_link_change() now calls
  mutex_lock(&pf->adapter->ps_lock) on every invocation, before any PTP-…
- [Low] The commit message describes the .settime64 race the wrong way
  round.
- [Low] adapter->ps_lock now also guards writes to ice_ptp_port::link_up
  and the whole of ice_ptp_link_change(), including the SyncE/DPLL…
- [Low] The patch's link-edge handling depends on ice_ptp_link_change()
  reading pf->ptp.state inside the adapter ps_lock critical section.
- [Low] To justify replacing the per-port ps_lock with a single adapter-
  wide lock, the commit message says 'the PHY restart procedure already…

--

ice: fix PHY port restart serialization

This commit replaces the per-port ps_lock with a single adapter-wide
ps_lock. The lock is taken in ice_ptp_link_change(),
ice_ptp_restart_all_phy(), ice_ptp_init() and ice_ptp_release(), so link_up
is updated in the same critical section as the PHY start/stop. It also
moves the ICE_PTP_READY transition in ice_ptp_init() ahead of the initial
PHY restart, so link edges during init are not lost.

Closes: https://lore.kernel.org/netdev/20260916011228.1632848-1-kuba@kernel.org/

> If a PHY restart occurs concurrently with a link transition, it is possible
> that the link state read could be re-ordered such that the link transition
> would start the PHY but a concurrent restart (such as from .settime64)
> could see the old link state and decide it must stop the PHY.

[Severity: Low]
Does the race go in this direction? Before this patch,
ice_ptp_restart_all_phy() only called ice_ptp_port_phy_restart() after
checking link_up:

    if (port->link_up)
        ice_ptp_port_phy_restart(port);

A stale "down" value there only skips the port. It never issues a stop.

The inner !ptp_port->link_up check in ice_ptp_port_phy_restart() reads the
same variable later in program order. While the link is going up, it
can't see "down" after the outer check saw "up".

On the old code, the race that does look possible is the opposite one,
on link down:

CPU0 (.settime64)                 CPU1 (link down)
ice_ptp_restart_all_phy()
  port->link_up == true
  ice_ptp_port_phy_restart()
    link_up check passes
                                  ice_ptp_link_change()
                                    ptp_port->link_up = false
                                    ice_ptp_port_phy_restart()
                                      ice_ptp_port_phy_stop()
                                        lock/unlock ps_lock
    mutex_lock(&ptp_port->ps_lock)
    ice_start_phy_timer_e82x()

This leaves timestamping running on a down link.

The new adapter-wide ps_lock closes both directions, so the code change
itself looks fine. Could the commit message describe the scenario that is
actually being fixed?

> Additionally note that the PHY restart procedure already cannot run
> concurrently across two ports. The procedure requires executing timer
> commands which in turn require the PTP hardware semaphore.

[Severity: Low]
Is this accurate for E822/E823? ice_start_phy_timer_e82x() issues its
timer commands without holding ice_ptp_lock():

ice_start_phy_timer_e82x() {
    ...
    err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_INIT_INCVAL);
    ...
    ice_ptp_src_cmd(hw, ICE_PTP_NOP);

    ice_ptp_exec_tmr_cmd(hw);
    ...
    err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_INIT_INCVAL);
    ...
    ice_ptp_exec_tmr_cmd(hw);
    ...
    ice_ptp_exec_tmr_cmd(hw);

    err = ice_sync_phy_timer_e82x(hw, port);
    ...
}

Only the final ice_sync_phy_timer_e82x() step takes the semaphore. On
E825, ice_start_phy_timer_eth56g() does hold it around its port command.

The single adapter lock serializes more than the old per-port lock did,
so the patch doesn't depend on this claim. Could the justification in the
commit message be corrected, though?

A related point: ice_ptp_one_port_cmd() writes P_REG_TMR_CMD on every
port (NOP on the non-target ports). The clock owner's ice_ptp_tmr_cmd()
paths (adjfine/adjtime/settime) also program every port's TMR_CMD, and
they hold only the semaphore, not ps_lock. This patch did not introduce
that possible interleaving, and the old per-port lock didn't cover it
either. Whether it matters depends on how the hardware handles the timer
command sync across PFs.

> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index c61bd645e460..5cefcd7af8b3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -41,6 +41,7 @@ struct ice_port_list {
>   * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
>   * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
>   *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
> + * @ps_lock: Mutex to serialize PHY port start/stop across adapter.

[Severity: Low]
This isn't a bug, but should this description cover everything the lock
now protects? After this patch, ps_lock also guards writes to
ice_ptp_port::link_up and the whole of ice_ptp_link_change(). That
includes the SyncE/DPLL reconfiguration and ice_txclk_update_and_notify().

The @link_up kernel-doc in ice_ptp.h still just says "indicates whether
the link is up". It doesn't say that writers hold adapter->ps_lock, or
that lockless readers such as ice_ptp_process_tx_tstamp() use READ_ONCE().

Also, ice_ptp_port_phy_stop(), ice_ptp_port_phy_restart() and
ice_ptp_restart_all_phy() all gained a Context: line. ice_ptp_link_change()
now takes ps_lock unconditionally, but its kernel-doc has no "Context:
acquires the adapter ps_lock" line.

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 55241802d17a..7a1deb3ea212 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -1309,17 +1309,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  	struct ice_ptp_port *ptp_port;
>  	struct ice_hw *hw = &pf->hw;
>  
> -	if (pf->ptp.state != ICE_PTP_READY)
> -		return;
> -
>  	ptp_port = &pf->ptp.port;
>  
> -	/* Update cached link status for this port immediately */
> -	ptp_port->link_up = linkup;
> +	mutex_lock(&pf->adapter->ps_lock);

[Severity: Medium]
Can this dereference a freed ice_adapter during device removal? Before
this patch, ice_ptp_link_change() returned early once ice_ptp_release()
had moved the state away from ICE_PTP_READY, and it never touched
pf->adapter. Now the mutex is taken before any state check.

In ice_remove(), the adapter reference is dropped before the service task
is synchronously stopped:

ice_remove()
    ice_unload(pf);           <- PTP torn down via ice_ptp_release()
    ice_deinit(pf);           <- only sets ICE_SERVICE_DIS
    ice_vsi_release_all(pf);
    ...
    ice_adapter_put(pdev);    <- may kfree() the adapter
    ice_deinit_hw(&pf->hw);
    ice_deinit_dev(pf);       <- ice_service_task_stop()->cancel_work_sync()

The ice_probe() unroll path uses the same order:

unroll_adapter:
	ice_adapter_put(pdev);
unroll_hw_init:
	ice_deinit_hw(hw);
	if (need_dev_deinit)
		ice_deinit_dev(pf);

Suppose a service task is already in ice_link_event() and has passed the
ice_get_main_vsi() check when removal runs through ice_adapter_put(), and
this PF held the last adapter reference. Wouldn't the task then reach:

ice_service_task()
  ice_clean_adminq_subtask()
    ...
      ice_link_event()
        ice_ptp_link_change()
          mutex_lock(&pf->adapter->ps_lock);

on an adapter that ice_adapter_free() has already passed to mutex_destroy()
and kfree()?

The teardown ordering predates this patch, but the patch adds a new
unconditional adapter access on this path.

[ ... ]

> @@ -3419,14 +3414,16 @@ void ice_ptp_init(struct ice_pf *pf)
>  	if (err)
>  		goto err_release_tx_tracker;
>  
> -	/* Start the PHY timestamping block */
> -	ice_ptp_reset_phy_timestamping(pf);
> -
>  	/* Configure initial Tx interrupt settings */
>  	ice_ptp_cfg_tx_interrupt(pf);
>  
>  	ptp->state = ICE_PTP_READY;
>  
> +	/* (Re)start the PHY timestamping block */
> +	mutex_lock(&pf->adapter->ps_lock);
> +	ice_ptp_port_phy_restart(&ptp->port);
> +	mutex_unlock(&pf->adapter->ps_lock);

[Severity: Low]
Should the ICE_PTP_READY store happen under ps_lock? The link-edge
reasoning in the commit message relies on ice_ptp_link_change() reading
pf->ptp.state inside the ps_lock critical section. Here, though, the
store is a plain write made before mutex_lock().

If a link event already holds ps_lock when this store happens,
ice_ptp_link_change() can see READY through a plain load:

    if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
        goto out_unlock;

Nothing pairs that load (no release/acquire) with the earlier
initialization writes (pf->ptp.kworker, ov_work, the Tx tracker). It then
goes on into ice_ptp_port_phy_restart(), which on E822 calls
kthread_queue_delayed_work(pf->ptp.kworker, ...).

In practice the window is very small, and both outcomes described in the
commit message are handled correctly. It is still a plain-access data
race that KCSAN could report.

Would it make the ordering explicit to set the state to READY inside the
ps_lock section just before the initial restart, or to use
smp_store_release()/smp_load_acquire()?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 05/13] ice: call PTP link change only from link events

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 1

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).

- [Medium] A link speed change while the link stays up no longer re-
  calibrates the PHY timestamp block, and on E825 it no longer…
- [Medium] After a CORER/GLOBR/EMPR reset, non-owner ports' PHY timestamp
  blocks now depend on a single unordered restart from the clock owner,…
- [Medium] The post-rebuild resync in ice_rebuild()
  (ice_main.c:7801-7807) relies on ice_ptp_link_change() to bring the…
- [Medium] After a devlink reload, pf->ptp.port.link_up is stale and can
  leave the PHY timestamp block in the wrong state indefinitely.
- [Low] If ice_ptp_port_phy_restart() fails, an administrative down/up no
  longer retries it.

--

ice: call PTP link change only from link events

This patch stops calling ice_ptp_link_change() from ice_up_complete() and
ice_down(). It seeds the PTP link_up field in ice_init_link(), adds an
early return in ice_ptp_link_change() for when the cached link state
already matches, and adds a resync call at the end of ice_rebuild(). It
also makes E825 call ice_ptp_port_phy_restart() on link down.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..910ebdcdde19 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -1144,6 +1144,8 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>  		ice_set_link(vsi, false);
>  	}
>  
> +	ice_ptp_link_change(pf, link_up);
> +
>  	/* if the old link up/down and speed is the same as the new */
>  	if (link_up == old_link && link_speed == old_link_speed)
>  		return 0;
> @@ -1151,8 +1153,6 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>  	if (!link_up && old_link)
>  		pf->link_down_events++;
>  
> -	ice_ptp_link_change(pf, link_up);
> -

[Severity: Medium]
If the link speed changes while the link stays up, does the PHY timestamp
block still get restarted?

Before this patch, an up->up event with a new link_speed got past the
"link_speed == old_link_speed" check. It then called
ice_ptp_link_change(pf, true), which always ran ice_ptp_port_phy_restart().

On E82x, that restart reprograms the speed-dependent lane, UIX and PAR-PCS
settings and requeues the Vernier ov_work. On E825 with DPLL enabled, the
call also ran ice_tspll_cfg_synce_ethdiv_e825c(), which computes the CGU_R10
divider from phy.link_info.link_speed.

The new early return in ice_ptp_link_change() only compares up/down:

	if (READ_ONCE(ptp_port->link_up) == linkup)
		goto out_unlock;

ice_link_event() also forces link_up to true whenever the refreshed link
info reports the link as up:

	if (phy_info->link_info.link_info & ICE_AQ_LINK_UP)
		link_up = true;

Say a down LSE is processed after the link has already retrained at a new
speed (ethtool speed change, AN restart). That event is treated as up->up,
and so is the up LSE that follows.

Would the PHY timer keep the calibration for the old speed, and the E825
SyncE output keep the old divider, until the driver next sees a link
toggle? The kernel-doc for ice_start_phy_timer_e82x() and
ice_start_phy_timer_eth56g() says this must be done "whenever the clock
time is initialized or when link speed changes."

This also seems to conflict with the commit message:

  "This sequencing should ensure that any link transition will properly
  update the PTP state to reflect the new mode"

This still appears to be present at the end of the series.

[ ... ]

> @@ -4810,6 +4810,14 @@ static void ice_init_link(struct ice_pf *pf)
>  
>  	ice_init_link_dflt_override(pf->hw.port_info);
>  
> +	/* Seed PTP link status from current PHY status, since link may
> +	 * already be up (e.g. after PXE boot) without a link-change edge to
> +	 * catch it later.
> +	 */
> +	WRITE_ONCE(pf->ptp.port.link_up,
> +		   !!(pf->hw.port_info->phy.link_info.link_info &
> +		      ICE_AQ_LINK_UP));
> +

[Severity: Medium]
Can pf->ptp.port.link_up go stale across a devlink reload? This seeding
runs only from ice_init() at probe time.

ice_devlink_reinit_up() calls ice_init_hw(), ice_init_dev(), ice_init_pf()
and ice_load(), but never ice_init_link().

Before this patch, reload-down went through
ice_unload() -> ice_unregister_netdev() -> ndo_stop -> ice_down(), which
wrote link_up = false. The next ice_open() then wrote link_up = true via
ice_up_complete(). Neither write happens now.

ice_load() -> ice_init_features() -> ice_ptp_init() then calls
ice_ptp_port_phy_restart() using the link_up value from before the reload.

Suppose the link was up before the reload and is down after it. The PHY
timer is started on a down link. When the link comes back, the up LSE hits
the "link_up == linkup" early return in ice_ptp_link_change(), so the PHY
is never restarted for the negotiated link.

Now suppose the link was down before the reload and came up while LSEs were
not yet re-enabled. ice_ptp_init() stops the PHY, and no LSE edge arrives
to start it again.

In both cases, would PTP timestamping stay wrong or disabled until a
physical link toggle? This still appears to be present at the end of the
series.

>  	ice_check_link_cfg_err(pf,
>  			       pf->hw.port_info->phy.link_info.link_cfg_err);
>  
> @@ -6745,7 +6753,6 @@ static int ice_up_complete(struct ice_vsi *vsi)
>  		ice_print_link_msg(vsi, true);
>  		netif_tx_start_all_queues(vsi->netdev);
>  		netif_carrier_on(vsi->netdev);
> -		ice_ptp_link_change(pf, true);
>  	}

[Severity: Low]
Every administrative up used to retry the PHY restart from here, and this
removes that retry.

If ice_ptp_port_phy_restart() fails because ice_start_phy_timer_e82x() or
ice_start_phy_timer_eth56g() returns an error, Tx timestamps stay disabled
and ov_work is not queued.

Before, an ifdown/ifup, MTU change or ring change called
ice_ptp_link_change(pf, true) again and retried the restart. That call is
gone now. Any later call hits the "link_up == linkup" early return while
the physical link stays up.

A later commit in the series, "ice: E825: stop clearing
PHY_REG_TX_OFFSET_READY", says a link reset (e.g. via ethtool) is the
intended way to recover. The dev_err() in ice_ptp_port_phy_restart() also
says "A link-toggle may recover". So this looks deliberate.

Could the commit message mention that an administrative down/up no longer
retries a failed PHY restart?

[ ... ]

> @@ -7794,6 +7798,14 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	ice_update_pf_netdev_link(pf);
>  
> +	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
> +		bool link_up;
> +
> +		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
> +			     ICE_AQ_LINK_UP);
> +		ice_ptp_link_change(pf, link_up);
> +	}
> +

[Severity: Medium]
After a CORER, GLOBR or EMPR reset, does this call restart the PHY on
non-owner ports?

Before this patch, each PF's VSI rebuild called
ice_up_complete() -> ice_ptp_link_change(pf, true). That restarted the
PF's PHY after its own control and sideband queues had been reinitialized.

Also, ice_down() no longer clears ptp_port->link_up during
ice_prepare_for_reset(), so link_up stays true across the reset.

The only restart left comes from the owner PF's rebuild:

ice_rebuild() (owner PF)
  ice_ptp_rebuild()
    ice_ptp_rebuild_owner()
      ice_ptp_restart_all_phy()
        ice_ptp_port_phy_restart(port)

This path uses ptp_port_to_pf(port)->hw, which is the non-owner PF's own
sideband queue. Each PF handles the reset on its own from its service task
(ice_prepare_for_reset() shuts its control queues down and ice_rebuild()
brings them back up), and nothing orders one PF against another.

If the owner gets to ice_ptp_restart_all_phy() before a non-owner has run
ice_init_all_ctrlq(), the PHY access fails. On E82x, tx.disabled has
already been set by then and ov_work is not queued.

The non-owner's own call here then returns at the "link_up == linkup"
check, because the link was up both before and after the reset. Would
timestamping on that port stay disabled until a physical link toggle?

This still appears to be present at the end of the series.

>  	/* tell the firmware we are up */
>  	err = ice_send_version(pf);
>  	if (err) {
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1e6ba6bbf6c2..f43918f334e8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1319,6 +1319,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  
>  	mutex_lock(&pf->adapter->ps_lock);
>  
> +	if (READ_ONCE(ptp_port->link_up) == linkup)
> +		goto out_unlock;
> +
>  	WRITE_ONCE(ptp_port->link_up, linkup);
>  
>  	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)

[Severity: Medium]
Can the cached link_up drift away from what was actually applied to the
hardware?

link_up is written before the READY/reset_ongoing check, so an event that
skips the PHY action still updates the cache. Nothing on the reset path
(ice_ptp_prepare_for_reset() or ice_ptp_rebuild()) invalidates it.

The first case is an LSE handled while hw->reset_ongoing is set (it is set
without ps_lock in ice_misc_intr()) or while PTP is in ICE_PTP_RESETTING.
link_up is updated but the PHY is not touched.

If the owner's ice_ptp_restart_all_phy() has already run or skipped this
port, the ice_ptp_link_change() call at the end of ice_rebuild() returns
early. The PHY then stays stopped while the link is up.

The second case is PFR. ice_ptp_rebuild() skips the owner restart for
ICE_RESET_PFR:

	if (ice_pf_src_tmr_owned(pf) && reset_type != ICE_RESET_PFR) {
		err = ice_ptp_rebuild_owner(pf);

So if the link does not change, nothing restarts the PHY after a PF reset.

Later in the series, "ice: E822: cancel offset verification work during
reset preparation" adds an unconditional
kthread_cancel_delayed_work_sync(&ptp->port.ov_work) to
ice_ptp_prepare_for_reset() for ICE_MAC_GENERIC, before the PFR early
return. Its justification is "the ice_ptp_link_change() call at the end of
the rebuild".

Also, "ice: E822: keep Tx timestamps disabled during offset calibration"
makes ov_work the only place that clears tx.disabled.

With both of those applied, a PFR while Vernier calibration is still
pending cancels ov_work. The rebuild call then returns at this check
without requeuing it. Would Tx timestamps and the Rx offset stay disabled
until a physical link toggle?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

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

[netdev-bot+sashiko]

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

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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 08/13] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[netdev-bot+sashiko]

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] The commit message for "ice: E825: stop clearing
  PHY_REG_TX_OFFSET_READY" names a Tx tracker field that does not exist.

--

ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY. Clearing it left the hardware's outstanding Tx
timestamp counter out of sync. Instead, new Tx timestamp requests are
blocked in software while the PHY is stopped or restarting, and
ice_ptp_mark_tx_tracker_stale() now checks tx->init.

> Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
> requests could happen while the PHY is calibrating. To avoid this, set the
> tx.calibrating field of the Tx timestamp tracker when stopping the timer
> and clear it when finishing the restart.

[ ... ]

> Unlike E822 devices, set the calibrating flag when "stopping" the PHY and
> clear it immediately after the start procedure.

[ ... ]

> In the event that the PHY timer start procedure fails, the device is in an
> unknown state and timestamps will not behave properly. As such, and similar
> to E822 devices, the calibrating field is not cleared on failure.

[Severity: Low]
Should these references to tx.calibrating say tx.disabled instead?

struct ice_ptp_tx in ice_ptp.h has no calibrating member. Its bitfields
are:

	u8 init : 1;
	u8 disabled : 1;
	u8 has_ready_bitmap : 1;

The new E825 code in ice_ptp_port_phy_stop() and
ice_ptp_port_phy_restart() sets and clears ptp_port->tx.disabled:

	spin_lock_irqsave(&ptp_port->tx.lock, flags);
	ptp_port->tx.disabled = true;
	spin_unlock_irqrestore(&ptp_port->tx.lock, flags);

ice_ptp_wait_for_offsets() uses the same field for E82x, and the
kernel-doc update in this patch is for @disabled.

The behaviour the commit message describes matches the code. Only the
field name is wrong.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 09/13] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[netdev-bot+sashiko]

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] In ice_ptp_phy_soft_reset_eth56g()
  (drivers/net/ethernet/intel/ice/ice_ptp_hw.c), the read sweep…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This commit removes the write-based ice_ptp_reset_ts_memory_eth56g(). In
its place, a new ice_ptp_clear_tx_memory_status_eth56g() reads every
PHY_TSTAMP index for a port. That sweep is now called at the start of
ice_ptp_phy_soft_reset_eth56g(), so the soft reset clears the internal Tx
timestamp counter after the reads. E825 is also dropped from
ice_ptp_reset_ts_memory().

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dd..5637925c0e68 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -2311,6 +2312,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>  	u32 global_val;
>  	int err;
>  
> +	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> +
>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
>  	if (err) {
>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",

[Severity: Low]
The sweep has no return value and always runs before the PHY_REG_GLOBAL
read and the three PHY_REG_GLOBAL writes. Any of those sideband accesses
can fail and make ice_ptp_phy_soft_reset_eth56g() return early. If that
happens, the port has been swept but its internal counter was never
reset.

The new kdoc for ice_ptp_clear_tx_memory_status_eth56g() says:

  Due to interactions with an internal HW counter for the number of
  outstanding Tx timestamps, this *must* only be called as part of the
  ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
  may become out of sync and prevent new timestamp interrupts.

Doesn't this error path leave the port in the state that comment warns
about?

I could not find any visible effect from this today:

- ice_ptp_init_phc_e825c() stops at the first port that fails.
- The error reaches ice_ptp_init_owner() or ice_ptp_rebuild_owner(), and
  both treat it as fatal for PTP.
- Recovery paths, such as a link toggle or a driver reload, run the sweep
  and the soft reset again.

So this looks like a robustness and documentation mismatch rather than a
functional regression.

Could PHY_REG_GLOBAL be read before the sweep, so that a failure at that
first step happens before the counter is touched? Alternatively, the kdoc
could be updated to cover the error path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 10/13] ice: E825: perform a soft reset when starting the PHY timer

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] ice_start_phy_timer_eth56g()
  (drivers/net/ethernet/intel/ice/ice_ptp_hw.c) now calls…
- [Medium] With this patch, the destructive E825 PHY soft-reset sequence
  runs at runtime from ice_start_phy_timer_eth56g(), mainly on E825…

--

ice: E825: perform a soft reset when starting the PHY timer

This patch makes ice_start_phy_timer_eth56g() call
ice_ptp_phy_soft_reset_eth56g() before it programs the E825 PHY. The goal
is to clear any outstanding Tx timestamp memory and the PHY timestamp
counter. Only the timer block of the PHY is reset.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 5637925c0e68..2d000251b185 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  	if (err)
>  		return err;
>  
> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> +	if (err)
> +		return err;
> +

[Severity: Medium]
Should the software Tx timestamp tracker be flushed before this reset
wipes the hardware timestamp memory?

On E825 link-up the path is ice_ptp_link_change() ->
ice_ptp_port_phy_restart() -> ice_start_phy_timer_eth56g(). The E825 case
in ice_ptp_port_phy_restart() only marks the outstanding entries stale:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
    ...
		ptp_port->tx.disabled = true;
		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);

		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

		err = ice_start_phy_timer_eth56g(hw, port);
    ...
}

The in_use bits and tstamps[idx].skb stay in place. After that,
ice_ptp_phy_soft_reset_eth56g() calls
ice_ptp_clear_tx_memory_status_eth56g() and pulses the soft reset, which
clears every ready bit on the port.

Take an entry whose timestamp was captured but not read yet. Before this
patch it would have been read through the ready bit path and quietly
dropped as stale. Now its ready bit never comes back, so
ice_ptp_process_tx_tstamp() can only release it through the timeout:

		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
			drop_ts = true;

			/* Count the number of Tx timestamps that timed out */
			pf->ptp.tx_hwtstamp_timeouts++;
		}

Does this hold the slot and the skb from ice_ptp_request_ts() for about
2 seconds? Does it also count a timestamp the driver threw away itself in
tx_hwtstamp_timeouts, which ethtool -S reports?

At this commit there is also no time bound on the hold. The periodic
fallback in ice_ptp_maybe_trigger_tx_interrupt() fires only when the
hardware ready bits are set, and the reset has just cleared them.

The later commit "ice: keep Tx timestamp slots tracked until completion
or timeout" changes that trigger to check the software in_use bitmap.
That bounds the reclaim to about 2.5 seconds. The miscounted timeouts and
the temporary slot and skb hold are still there at the end of the series.

ice_ptp_rebuild_owner() already flushes the software state before it
restarts the PHYs:

	/* Flush software tracking of any outstanding timestamps since we're
	 * about to flush the PHY timestamp block.
	 */
	ice_ptp_flush_all_tx_tracker(pf);

Would the link change restart path need a matching flush? The commit
message says only that the reset will "clear any outstanding Tx timestamp
memory". It doesn't mention what happens to the software tracker.

[Severity: Medium]
What keeps the Tx timestamp interrupt thread and in-flight hardware
captures away from this port while the soft reset sequence runs?

Before this patch, the only caller of ice_ptp_phy_soft_reset_eth56g() was
ice_ptp_init_phc_e825c(), at PHC init. Now it also runs on every E825
link-up restart. The sequence sweeps all slots and sleeps before the
reset is asserted:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_ptp_phy_soft_reset_eth56g() {
	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
    ...
	usleep_range(5000, 6000);

	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
    ...
	usleep_range(5000, 6000);
    ...
}

ice_stop_phy_timer_eth56g() leaves PHY_REG_TX_OFFSET_READY set, so the
PHY keeps capturing timestamps for descriptors that were posted before
tx.disabled was set. A capture can land on an index the sweep has already
passed, during the 5 ms gap before the reset is asserted.

The consumer path is ice_misc_intr_thread_fn() -> ice_ptp_process_ts() ->
ice_ptp_tx_tstamp_owner() -> ice_ptp_process_tx_tstamp(). It never takes
ps_lock. Its ice_is_reset_in_progress() check doesn't cover a link
triggered PHY restart. It snapshots the ready bitmap and then calls
ice_read_phy_tstamp() based on that snapshot.

Suppose the thread snapshots that new ready bit and then reads the slot
after the soft reset. It would read a slot whose valid bit is still set,
against a counter that was just reset to zero. The kdoc for
ice_ptp_clear_tx_memory_status_eth56g() says the soft reset does not clear
the timestamp memory.

The earlier commit "ice: E825: clear PHY_TSTAMP memory prior to soft
reset" says an unbalanced read "underflows the counter, and prevents new
timestamp interrupts from being triggered".

Could this stall Tx timestamp interrupts on the port until the next soft
reset? That is the failure this patch is meant to prevent. Whether it
happens depends on PHY behavior described only in this series and on a
narrow timing window. Even so, nothing in the code seems to prevent the
interleaving.

Does this path need to synchronize with the misc IRQ thread, or wait for
in-flight captures, before it runs the sweep and the reset?

>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
>  
>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 11/13] ice: keep Tx timestamp slots tracked until completion or timeout

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message says link-down requests are marked stale,
  but the patch never marks them stale on E810, E830 or E82x.

--

ice: keep Tx timestamp slots tracked until completion or timeout

This patch stops dropping outstanding Tx timestamp requests on link down
and rejects new requests while the link is down. Slots that are not ready
yet are now released by the existing 2 second timeout. It also makes the
interrupt re-arm checks ignore stale slots, and lets
ice_ptp_maybe_trigger_tx_interrupt() run for PFs that handle their own Tx
timestamp interrupt.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b7c539f98276..309fa2877cc3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -582,14 +584,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  			return;
>  	}
>  
> -	/* Drop packets if the link went down */
> -	link_up = READ_ONCE(ptp_port->link_up);
> -
>  	for_each_set_bit(idx, tx->in_use, tx->len) {
>  		struct skb_shared_hwtstamps shhwtstamps = {};
>  		u8 phy_idx = idx + tx->offset;
>  		u64 raw_tstamp = 0, tstamp;
> -		bool drop_ts = !link_up;
> +		bool drop_ts = false;
>  		struct sk_buff *skb;

[Severity: Medium]
The commit message says:

    Mark the outstanding requests stale so their completions are read
    and discarded

Where does this stale marking happen for E810, E830 and E82x? The patch
removes the link-down drop here, but it doesn't add any new calls to
ice_ptp_mark_tx_tracker_stale().

On link down, the only caller that can be reached is the
ICE_MAC_GENERIC_3K_E825 branch of ice_ptp_port_phy_stop() (and the same
branch of ice_ptp_port_phy_restart()):

ice_ptp_port_phy_stop() {
    ...
	case ICE_MAC_E810:
	case ICE_MAC_E830:
		err = 0;
		break;
	case ICE_MAC_GENERIC:
		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);

		err = ice_stop_phy_timer_e82x(hw, port, true);
		break;
	case ICE_MAC_GENERIC_3K_E825:
		...
		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
    ...
}

Also, ice_ptp_link_change() skips E810 and E830 entirely:

	case ICE_MAC_E810:
	case ICE_MAC_E830:
		/* Do not reconfigure E810 or E830 PHY */
		break;

The new in_irq check in ice_port_has_timestamps() only excludes stale
slots:

	if (in_irq)
		return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);

Given that, can a link down with a request still outstanding on E810
(non-LL), E830 or E82x cause a back-to-back interrupt loop? The sequence
would be:

  link goes down while a not-ready slot is set in in_use
  ice_ptp_maybe_trigger_tx_interrupt()
    ice_ptp_tx_tstamps_pending(pf, false) -> true
    wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
  ice_ptp_process_tx_tstamp()
    slot not ready and not timed out -> continue
  ice_misc_intr_thread_fn() (E810/E82x) or ice_ptp_ts_irq() (E830)
    ice_ptp_tx_tstamps_pending(pf, true) -> true, slot is not stale
    wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
  ... repeats until the 2 second timeout frees the slot

On E830 the re-arm happens in hardirq context in ice_ptp_ts_irq(). On E82x
every pass does sideband PHY reads. Before this patch, the first pass after
link down dropped every slot, so this loop could not happen.

A later patch in this series, "ice: don't clear in_use until HW clears
ready bitmap", adds an ice_ptp_is_tx_tracker_up() check to the in_irq
branch. Combined with the link_up check added here, that seems to close
the loop.

Even after that patch, though, E810, E830 and E82x still don't get any
stale marking on link down. So this commit isn't bisect-safe on its own,
and the commit message describes a mechanism the code doesn't implement.

Should the commit message be corrected, or should the stale marking be
added for the other MAC types?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

Re: [PATCH net v5 13/13] ice: don't clear in_use until HW clears ready bitmap

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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).

- [Medium] The patch removes the hardware ready-bitmap check
  (ice_check_phy_tx_tstamp_ready()) from ice_ptp_tx_tstamps_pending().
- [Low] The kernel-doc of ice_ptp_process_tx_tstamp() (ice_ptp.c:514-562)
  no longer matches the code.

--

ice: don't clear in_use until HW clears ready bitmap

After each PHY timestamp read, this patch re-reads the Tx timestamp ready
bitmap. If the index's ready bit has not cleared, the index is marked
stale and kept locked, and its SKB is freed. The patch also drops the
hardware ready bitmap check from the in-IRQ re-arm decision. That check
now runs only from the periodic watchdog.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b0ea01cfcc4f..c0d056896825 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -557,7 +557,9 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx)
>   * extremely unlikely that a packet will ever take this long to timestamp. If
>   * we detect a Tx timestamp request that has waited for this long we assume
>   * the packet will never be sent by hardware and discard it without reading
> - * the timestamp register.
> + * the timestamp register. Note that in the unusual case where the PHY
> + * continuously fails to clear the ready bitmap index, the slot may remain
> + * locked for more than two seconds.
>   */
>  static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  {

[Severity: Low]
Is the rest of this kernel-doc still accurate after this change? The
numbered list earlier in the same comment still says:

 * 3) read and copy the timestamp out of the PHY register
 * 4) unlock the index by clearing the associated in_use bit
 * 5) check if the timestamp is stale, and discard if so

Look at the new "verify ready bit cleared" block in
ice_ptp_process_tx_tstamp(). If the ready bit is still set after the read,
or the re-read of the ready bitmap fails, the index is marked stale and
the SKB is freed. in_use stays set, so step 4 never happens.

The opening line, "loop over each index with a waiting skb", is also no
longer true. in_use slots with a NULL skb are now visited on every pass
until the hardware clears the bit.

This paragraph also still says timed-out requests are discarded "without
reading the timestamp register". A timed-out index whose ready bit is set
is still read through ice_read_phy_tstamp(), and with this patch it can
stay locked.

Could the kernel-doc be updated to describe the new flow?

[ ... ]

> @@ -2766,10 +2787,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
>  		if (!tx->init)
>  			return false;
>  
> -		if (in_irq)
> +		if (in_irq) {
> +			if (!ice_ptp_is_tx_tracker_up(tx))
> +				return false;
> +
>  			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
> -		else
> +		} else {
>  			return !bitmap_empty(tx->in_use, tx->len);
> +		}
>  	}
>  }
>  
> @@ -2801,41 +2826,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
>  
>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
>  {
> -	struct ice_hw *hw = &pf->hw;
> -	int ret;
> -
> -	/* Check software indicator */
>  	switch (pf->ptp.tx_interrupt_mode) {
>  	case ICE_PTP_TX_INTERRUPT_NONE:
>  		return false;
>  	case ICE_PTP_TX_INTERRUPT_SELF:
> -		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
> -			return true;
> -		break;
> +		return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq);
>  	case ICE_PTP_TX_INTERRUPT_ALL:
> -		if (ice_any_port_has_timestamps(pf, in_irq))
> -			return true;
> -		break;
> +		return ice_any_port_has_timestamps(pf, in_irq);
>  	default:
>  		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n",
>  			  pf->ptp.tx_interrupt_mode);
> -		break;
> -	}
> -
> -	/* Check hardware indicator */
> -	ret = ice_check_phy_tx_tstamp_ready(hw);
> -	if (ret < 0) {
> -		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
> -			ret);
> -		/* Stop triggering IRQs if we're unable to read PHY */
>  		return false;
>  	}
> -
> -	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
> -	 * available, 0 if there are no waiting timestamps, and a negative
> -	 * value if there was an error (which we checked for above).
> -	 */
> -	return ret > 0;
>  }

[Severity: Medium]
Can removing this hardware fallback leave the PHY Tx timestamp interrupt
blocked until the watchdog runs?

ice_misc_intr_thread_fn() and the E830 path in ice_ptp_ts_irq() call
ice_ptp_tx_tstamps_pending(pf, true) to decide whether to re-arm:

drivers/net/ethernet/intel/ice/ice_main.c:ice_misc_intr_thread_fn() {
    ...
	if (ice_ptp_tx_tstamps_pending(pf, true)) {
		/* If any new Tx timestamps happened while in interrupt,
		 * re-arm the interrupt to trigger it again.
		 */
		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
		ice_flush(hw);
	}
    ...
}

This change, combined with the new ice_ptp_is_tx_tracker_up() check in
ice_port_has_timestamps(), means the re-arm only looks at non-stale
in_use slots on trackers that are up.

The kernel-doc of ice_ptp_maybe_trigger_tx_interrupt() says the PHY "will
not interrupt again until all current timestamp data is read". So if a
ready entry is still unread at the end of an IRQ pass, nothing
retriggers the interrupt. It stays blocked until the 500 ms watchdog,
ice_ptp_periodic_work()->ice_ptp_maybe_trigger_tx_interrupt(), runs.

A few ways such an entry can be left behind:

 - a stale request whose timestamp lands mid-pass, after the ready
   bitmap was read
 - a late completion on a port that is link down, or (on E82x) in
   Vernier calibration with tx.disabled set until
   ice_ptp_wait_for_offsets() succeeds
 - an index this patch keeps locked on purpose because its ready bit
   did not clear

Before this patch, the in-IRQ ice_check_phy_tx_tstamp_ready() call
retriggered the interrupt right away and drained these entries.

On E82x, timestamp memory and the interrupt are per quad (see
ice_phy_cfg_intr_e82x() and the quad split in ice_ptp_init_tx_e82x()).
In ICE_PTP_TX_INTERRUPT_ALL mode the clock owner services every port.

Could one leftover entry on a port that is going down or calibrating
then delay Tx timestamps for healthy link-up ports in the same quad by
up to about 500 ms? That is far beyond the 10 ms ptp4l default the
commit message cites. The race window looks narrow, but link flaps and
E82x recalibration happen in normal operation.

The commit "ice: keep Tx timestamp slots tracked until completion or
timeout" in this series also says an orphaned ready bit "blocks the
port interrupt".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com

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

[Jacob Keller]

On 10/25/2026 4:43 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (1) · 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).
> 
> - [Medium] The new error unwind in ice_ptp_init() destroys the PTP
>   kworker while the owner's PTP clock is still registered.
> - [Medium] On E810 (has_ready_bitmap == 0), the Tx tracker now survives
>   CORE/GLOBAL/EMP resets.
> 
> Pre-existing issues:
> - [Medium] Concern: ice_ptp_flush_all_tx_tracker(),
>   ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec() walk…
> 
> --
> 
> ice: fix removal of PTP timestamp tracker during reset
> 
> This patch stops ice_ptp_prepare_for_reset() from releasing the Tx
> timestamp tracker, so timestamping works again after CORE, GLOBAL or EMP
> resets. It also reorders ice_ptp_init() so the kworker and tracker are set
> up before the port is added to the port list, with a matching error
> unwind. The non-READY branch of ice_ptp_release() now frees the tracker
> and kworker.
> 
> 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/
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index eaec36ab6ae3..e1c66cd0f94f 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);
>> -
>>  	/* Disable periodic outputs */
>>  	ice_ptp_disable_all_perout(pf);
> 
> [Severity: Medium]
> The tracker now survives CORE/GLOBAL/EMP resets. On E810, can the
> cached_tstamp values then drift from the PHY state after the post-reset
> flush?
> 

This is a pre-existing issue that should be resolved separately, as
discussed on previous versions of this series.

> [Severity: Medium]
> Is the PTP clock still registered while the kworker is torn down here?
> 
> On the clock owner, ice_ptp_init_owner() has already registered the
> clock. /dev/ptpN stays live until err_exit calls ptp_clock_unregister().
> 
> The settime64 and adjtime ops do not check ptp->state, and they call
> ice_ptp_reset_cached_phctime(). Suppose ice_ptp_update_cached_phctime()
> returns -EAGAIN because ICE_CFG_BUSY is held, for example by a concurrent
> adjtime on the same PHC. That function then does:
> 
> 	kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
> 				   msecs_to_jiffies(10));
> 
> If this runs after kthread_cancel_delayed_work_sync() but before or during
> kthread_destroy_worker(), a timer is left pending. Could
> kthread_delayed_work_timer_fn() later touch the freed worker?
> 
> If it runs after ptp->kworker = NULL, would it dereference NULL?
> 
> One way to reach this window: ice_ptp_init_port() fails with -ENOMEM from
> ice_ptp_alloc_tx_tracker() while a process with /dev/ptpN open calls
> clock_settime() or clock_adjtime().
> 
> In this same patch, the non-READY branch of ice_ptp_release() unregisters
> the clock before tearing down the worker. Should the init error path use
> the same order? This ordering is still present at the end of the series.
> 

This is actually a problem that I think needs to be addressed
separately, and work from other developers around the lifetime of
adapter->ctrl_pf. I've tried multiple different ordering schemes while
trying to get this series to pass Sashiko review, and every time I get
something else as a problem.

The current behavior is a problem, and the ways I've tried to fix it are
less bad problems, (but still problems). I am getting extremely
frustrated by this entire process.

>> @@ -3399,12 +3405,22 @@ 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.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 and was not introduced by this patch.
> ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
> ice_ptp_prepare_rebuild_sec() walk the adapter port list without holding
> ports.lock:
> 
> 	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
> 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
> 
> At the same time, a peer PF's ice_ptp_release() can remove its port from
> the list and free the tracker, kworker and ps_lock. Since trackers are now
> kept across resets, the owner's flush walks memory that belongs to live
> peers.
> 
> The next patch in the series, "ice: use reference counting and SRCU for
> PTP port access", fixes this. It converts these walkers to
> srcu_read_lock() with list_for_each_entry_srcu() and
> kref_get_unless_zero(). It also makes ice_ptp_cleanup_pf() wait for the
> references and SRCU readers to finish before the tracker and worker are
> freed. The race is gone by the end of the series.
> 

Yes. I tried different orderings of the series, but putting the RCU
fixes first  seemed to have it complain about problems fixed by this
series. etc. etc.

Re: [PATCH net v5 03/13] ice: fix PHY port restart serialization

[Jacob Keller]

On 10/25/2026 4:43 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 4
> 
> 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).
> 
> - [Medium] ice_ptp_link_change() now calls
>   mutex_lock(&pf->adapter->ps_lock) on every invocation, before any PTP-…
> - [Low] The commit message describes the .settime64 race the wrong way
>   round.
> - [Low] adapter->ps_lock now also guards writes to ice_ptp_port::link_up
>   and the whole of ice_ptp_link_change(), including the SyncE/DPLL…
> - [Low] The patch's link-edge handling depends on ice_ptp_link_change()
>   reading pf->ptp.state inside the adapter ps_lock critical section.
> - [Low] To justify replacing the per-port ps_lock with a single adapter-
>   wide lock, the commit message says 'the PHY restart procedure already…
> 
> --
> 
> ice: fix PHY port restart serialization
> 
> This commit replaces the per-port ps_lock with a single adapter-wide
> ps_lock. The lock is taken in ice_ptp_link_change(),
> ice_ptp_restart_all_phy(), ice_ptp_init() and ice_ptp_release(), so link_up
> is updated in the same critical section as the PHY start/stop. It also
> moves the ICE_PTP_READY transition in ice_ptp_init() ahead of the initial
> PHY restart, so link edges during init are not lost.
> 
> Closes: https://lore.kernel.org/netdev/20260916011228.1632848-1-kuba@kernel.org/
> 
>> If a PHY restart occurs concurrently with a link transition, it is possible
>> that the link state read could be re-ordered such that the link transition
>> would start the PHY but a concurrent restart (such as from .settime64)
>> could see the old link state and decide it must stop the PHY.
> 
> [Severity: Low]
> Does the race go in this direction? Before this patch,
> ice_ptp_restart_all_phy() only called ice_ptp_port_phy_restart() after
> checking link_up:
> 
>     if (port->link_up)
>         ice_ptp_port_phy_restart(port);
> 
> A stale "down" value there only skips the port. It never issues a stop.
> 
> The inner !ptp_port->link_up check in ice_ptp_port_phy_restart() reads the
> same variable later in program order. While the link is going up, it
> can't see "down" after the outer check saw "up".
> 
> On the old code, the race that does look possible is the opposite one,
> on link down:
> 
> CPU0 (.settime64)                 CPU1 (link down)
> ice_ptp_restart_all_phy()
>   port->link_up == true
>   ice_ptp_port_phy_restart()
>     link_up check passes
>                                   ice_ptp_link_change()
>                                     ptp_port->link_up = false
>                                     ice_ptp_port_phy_restart()
>                                       ice_ptp_port_phy_stop()
>                                         lock/unlock ps_lock
>     mutex_lock(&ptp_port->ps_lock)
>     ice_start_phy_timer_e82x()
> 
> This leaves timestamping running on a down link.
> 
> The new adapter-wide ps_lock closes both directions, so the code change
> itself looks fine. Could the commit message describe the scenario that is
> actually being fixed?
> 

I can try to fix the commit message if we need a respin.

>> Additionally note that the PHY restart procedure already cannot run
>> concurrently across two ports. The procedure requires executing timer
>> commands which in turn require the PTP hardware semaphore.
> 
> [Severity: Low]
> Is this accurate for E822/E823? ice_start_phy_timer_e82x() issues its
> timer commands without holding ice_ptp_lock():
> 
> ice_start_phy_timer_e82x() {
>     ...
>     err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_INIT_INCVAL);
>     ...
>     ice_ptp_src_cmd(hw, ICE_PTP_NOP);
> 
>     ice_ptp_exec_tmr_cmd(hw);
>     ...
>     err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_INIT_INCVAL);
>     ...
>     ice_ptp_exec_tmr_cmd(hw);
>     ...
>     ice_ptp_exec_tmr_cmd(hw);
> 
>     err = ice_sync_phy_timer_e82x(hw, port);
>     ...
> }
> 

This is a pre-existing issue that I think are fixed by some patches we
have on Intel Wired LAN. The E822/E823 shouldn't be issuing timer
commands without the global semaphore.

> Only the final ice_sync_phy_timer_e82x() step takes the semaphore. On
> E825, ice_start_phy_timer_eth56g() does hold it around its port command.
> 
> The single adapter lock serializes more than the old per-port lock did,
> so the patch doesn't depend on this claim. Could the justification in the
> commit message be corrected, though?
> 

I can try to clean that up...

> A related point: ice_ptp_one_port_cmd() writes P_REG_TMR_CMD on every
> port (NOP on the non-target ports). The clock owner's ice_ptp_tmr_cmd()
> paths (adjfine/adjtime/settime) also program every port's TMR_CMD, and
> they hold only the semaphore, not ps_lock. This patch did not introduce
> that possible interleaving, and the old per-port lock didn't cover it
> either. Whether it matters depends on how the hardware handles the timer
> command sync across PFs.

The semaphore is cross-PF and is supposed to block other PFs.

> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> index c61bd645e460..5cefcd7af8b3 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> @@ -41,6 +41,7 @@ struct ice_port_list {
>>   * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
>>   * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
>>   *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
>> + * @ps_lock: Mutex to serialize PHY port start/stop across adapter.
> 
> [Severity: Low]
> This isn't a bug, but should this description cover everything the lock
> now protects? After this patch, ps_lock also guards writes to
> ice_ptp_port::link_up and the whole of ice_ptp_link_change(). That
> includes the SyncE/DPLL reconfiguration and ice_txclk_update_and_notify().
> 
> The @link_up kernel-doc in ice_ptp.h still just says "indicates whether
> the link is up". It doesn't say that writers hold adapter->ps_lock, or
> that lockless readers such as ice_ptp_process_tx_tstamp() use READ_ONCE().
> 
> Also, ice_ptp_port_phy_stop(), ice_ptp_port_phy_restart() and
> ice_ptp_restart_all_phy() all gained a Context: line. ice_ptp_link_change()
> now takes ps_lock unconditionally, but its kernel-doc has no "Context:
> acquires the adapter ps_lock" line.
> 

I can try to clean those up...

> [ ... ]
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 55241802d17a..7a1deb3ea212 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -1309,17 +1309,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>>  	struct ice_ptp_port *ptp_port;
>>  	struct ice_hw *hw = &pf->hw;
>>  
>> -	if (pf->ptp.state != ICE_PTP_READY)
>> -		return;
>> -
>>  	ptp_port = &pf->ptp.port;
>>  
>> -	/* Update cached link status for this port immediately */
>> -	ptp_port->link_up = linkup;
>> +	mutex_lock(&pf->adapter->ps_lock);
> 
> [Severity: Medium]
> Can this dereference a freed ice_adapter during device removal? Before
> this patch, ice_ptp_link_change() returned early once ice_ptp_release()
> had moved the state away from ICE_PTP_READY, and it never touched
> pf->adapter. Now the mutex is taken before any state check.
> 
> In ice_remove(), the adapter reference is dropped before the service task
> is synchronously stopped:
> 
> ice_remove()
>     ice_unload(pf);           <- PTP torn down via ice_ptp_release()
>     ice_deinit(pf);           <- only sets ICE_SERVICE_DIS
>     ice_vsi_release_all(pf);
>     ...
>     ice_adapter_put(pdev);    <- may kfree() the adapter
>     ice_deinit_hw(&pf->hw);
>     ice_deinit_dev(pf);       <- ice_service_task_stop()->cancel_work_sync()
> 
> The ice_probe() unroll path uses the same order:
> 
> unroll_adapter:
> 	ice_adapter_put(pdev);
> unroll_hw_init:
> 	ice_deinit_hw(hw);
> 	if (need_dev_deinit)
> 		ice_deinit_dev(pf);
> 
> Suppose a service task is already in ice_link_event() and has passed the
> ice_get_main_vsi() check when removal runs through ice_adapter_put(), and
> this PF held the last adapter reference. Wouldn't the task then reach:
> 
> ice_service_task()
>   ice_clean_adminq_subtask()
>     ...
>       ice_link_event()
>         ice_ptp_link_change()
>           mutex_lock(&pf->adapter->ps_lock);
> 
> on an adapter that ice_adapter_free() has already passed to mutex_destroy()
> and kfree()?
> 
> The teardown ordering predates this patch, but the patch adds a new
> unconditional adapter access on this path.
> 

This really should be considered a pre-existing issue that we don't have
a synchronize with the service task during teardown.

> [ ... ]
> 
>> @@ -3419,14 +3414,16 @@ void ice_ptp_init(struct ice_pf *pf)
>>  	if (err)
>>  		goto err_release_tx_tracker;
>>  
>> -	/* Start the PHY timestamping block */
>> -	ice_ptp_reset_phy_timestamping(pf);
>> -
>>  	/* Configure initial Tx interrupt settings */
>>  	ice_ptp_cfg_tx_interrupt(pf);
>>  
>>  	ptp->state = ICE_PTP_READY;
>>  
>> +	/* (Re)start the PHY timestamping block */
>> +	mutex_lock(&pf->adapter->ps_lock);
>> +	ice_ptp_port_phy_restart(&ptp->port);
>> +	mutex_unlock(&pf->adapter->ps_lock);
> 
> [Severity: Low]
> Should the ICE_PTP_READY store happen under ps_lock? The link-edge
> reasoning in the commit message relies on ice_ptp_link_change() reading
> pf->ptp.state inside the ps_lock critical section. Here, though, the
> store is a plain write made before mutex_lock().
> 
> If a link event already holds ps_lock when this store happens,
> ice_ptp_link_change() can see READY through a plain load:
> 
>     if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
>         goto out_unlock;
> 
> Nothing pairs that load (no release/acquire) with the earlier
> initialization writes (pf->ptp.kworker, ov_work, the Tx tracker). It then
> goes on into ice_ptp_port_phy_restart(), which on E822 calls
> kthread_queue_delayed_work(pf->ptp.kworker, ...).
> 
> In practice the window is very small, and both outcomes described in the
> commit message are handled correctly. It is still a plain-access data
> race that KCSAN could report.
> 
> Would it make the ordering explicit to set the state to READY inside the
> ps_lock section just before the initial restart, or to use
> smp_store_release()/smp_load_acquire()?
> 

I think this needs to be something like this:

1: set the PTP state
2: synchronize with the service task

But that likely requires more work to make sure everything is right and
I would rather not overload this already loaded series :\

> [ ... ]
> 

Re: [PATCH net v5 05/13] ice: call PTP link change only from link events

[Jacob Keller]

On 10/25/2026 4:43 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 4 · Low: 1
> 
> 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).
> 
> - [Medium] A link speed change while the link stays up no longer re-
>   calibrates the PHY timestamp block, and on E825 it no longer…
> - [Medium] After a CORER/GLOBR/EMPR reset, non-owner ports' PHY timestamp
>   blocks now depend on a single unordered restart from the clock owner,…
> - [Medium] The post-rebuild resync in ice_rebuild()
>   (ice_main.c:7801-7807) relies on ice_ptp_link_change() to bring the…
> - [Medium] After a devlink reload, pf->ptp.port.link_up is stale and can
>   leave the PHY timestamp block in the wrong state indefinitely.
> - [Low] If ice_ptp_port_phy_restart() fails, an administrative down/up no
>   longer retries it.
> 
> --
> 
> ice: call PTP link change only from link events
> 
> This patch stops calling ice_ptp_link_change() from ice_up_complete() and
> ice_down(). It seeds the PTP link_up field in ice_init_link(), adds an
> early return in ice_ptp_link_change() for when the cached link state
> already matches, and adds a resync call at the end of ice_rebuild(). It
> also makes E825 call ice_ptp_port_phy_restart() on link down.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index d88835482d3a..910ebdcdde19 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -1144,6 +1144,8 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>>  		ice_set_link(vsi, false);
>>  	}
>>  
>> +	ice_ptp_link_change(pf, link_up);
>> +
>>  	/* if the old link up/down and speed is the same as the new */
>>  	if (link_up == old_link && link_speed == old_link_speed)
>>  		return 0;
>> @@ -1151,8 +1153,6 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>>  	if (!link_up && old_link)
>>  		pf->link_down_events++;
>>  
>> -	ice_ptp_link_change(pf, link_up);
>> -
> 
> [Severity: Medium]
> If the link speed changes while the link stays up, does the PHY timestamp
> block still get restarted?
> 
> Before this patch, an up->up event with a new link_speed got past the
> "link_speed == old_link_speed" check. It then called
> ice_ptp_link_change(pf, true), which always ran ice_ptp_port_phy_restart().
> 
> On E82x, that restart reprograms the speed-dependent lane, UIX and PAR-PCS
> settings and requeues the Vernier ov_work. On E825 with DPLL enabled, the
> call also ran ice_tspll_cfg_synce_ethdiv_e825c(), which computes the CGU_R10
> divider from phy.link_info.link_speed.
> 
> The new early return in ice_ptp_link_change() only compares up/down:
> 
> 	if (READ_ONCE(ptp_port->link_up) == linkup)
> 		goto out_unlock;
> 
> ice_link_event() also forces link_up to true whenever the refreshed link
> info reports the link as up:
> 
> 	if (phy_info->link_info.link_info & ICE_AQ_LINK_UP)
> 		link_up = true;
> 
> Say a down LSE is processed after the link has already retrained at a new
> speed (ethtool speed change, AN restart). That event is treated as up->up,
> and so is the up LSE that follows.
> 
> Would the PHY timer keep the calibration for the old speed, and the E825
> SyncE output keep the old divider, until the driver next sees a link
> toggle? The kernel-doc for ice_start_phy_timer_e82x() and
> ice_start_phy_timer_eth56g() says this must be done "whenever the clock
> time is initialized or when link speed changes."
> 
> This also seems to conflict with the commit message:
> 
>   "This sequencing should ensure that any link transition will properly
>   update the PTP state to reflect the new mode"
> 
> This still appears to be present at the end of the series.


This entire flow is frustrating and I have no idea how to properly fix
this garbage to actually successfully get everything lined up without
also causing the issues we had with other changes. If I don't move this
call, then we end up missing link events. If I move it, we end up
missing other events.

I've now had to respin this series so many times and each time I get a
complaint that something is wrong and I can't reproduce or test any of
the paths reliably, so I have no idea which way is right.

> 
> [ ... ]
> 
>> @@ -4810,6 +4810,14 @@ static void ice_init_link(struct ice_pf *pf)
>>  
>>  	ice_init_link_dflt_override(pf->hw.port_info);
>>  
>> +	/* Seed PTP link status from current PHY status, since link may
>> +	 * already be up (e.g. after PXE boot) without a link-change edge to
>> +	 * catch it later.
>> +	 */
>> +	WRITE_ONCE(pf->ptp.port.link_up,
>> +		   !!(pf->hw.port_info->phy.link_info.link_info &
>> +		      ICE_AQ_LINK_UP));
>> +
> 
> [Severity: Medium]
> Can pf->ptp.port.link_up go stale across a devlink reload? This seeding
> runs only from ice_init() at probe time.
> 
> ice_devlink_reinit_up() calls ice_init_hw(), ice_init_dev(), ice_init_pf()
> and ice_load(), but never ice_init_link().
> 
> Before this patch, reload-down went through
> ice_unload() -> ice_unregister_netdev() -> ndo_stop -> ice_down(), which
> wrote link_up = false. The next ice_open() then wrote link_up = true via
> ice_up_complete(). Neither write happens now.
> 
> ice_load() -> ice_init_features() -> ice_ptp_init() then calls
> ice_ptp_port_phy_restart() using the link_up value from before the reload.
> 
> Suppose the link was up before the reload and is down after it. The PHY
> timer is started on a down link. When the link comes back, the up LSE hits
> the "link_up == linkup" early return in ice_ptp_link_change(), so the PHY
> is never restarted for the negotiated link.
> 
> Now suppose the link was down before the reload and came up while LSEs were
> not yet re-enabled. ice_ptp_init() stops the PHY, and no LSE edge arrives
> to start it again.
> 
> In both cases, would PTP timestamping stay wrong or disabled until a
> physical link toggle? This still appears to be present at the end of the
> series.
> >>  	ice_check_link_cfg_err(pf,
>>  			       pf->hw.port_info->phy.link_info.link_cfg_err);
>>  
>> @@ -6745,7 +6753,6 @@ static int ice_up_complete(struct ice_vsi *vsi)
>>  		ice_print_link_msg(vsi, true);
>>  		netif_tx_start_all_queues(vsi->netdev);
>>  		netif_carrier_on(vsi->netdev);
>> -		ice_ptp_link_change(pf, true);
>>  	}
> 
> [Severity: Low]
> Every administrative up used to retry the PHY restart from here, and this
> removes that retry.
> 
> If ice_ptp_port_phy_restart() fails because ice_start_phy_timer_e82x() or
> ice_start_phy_timer_eth56g() returns an error, Tx timestamps stay disabled
> and ov_work is not queued.
> 
> Before, an ifdown/ifup, MTU change or ring change called
> ice_ptp_link_change(pf, true) again and retried the restart. That call is
> gone now. Any later call hits the "link_up == linkup" early return while
> the physical link stays up.
> 
> A later commit in the series, "ice: E825: stop clearing
> PHY_REG_TX_OFFSET_READY", says a link reset (e.g. via ethtool) is the
> intended way to recover. The dev_err() in ice_ptp_port_phy_restart() also
> says "A link-toggle may recover". So this looks deliberate.
> 
> Could the commit message mention that an administrative down/up no longer
> retries a failed PHY restart?
> 
> [ ... ]
> 
>> @@ -7794,6 +7798,14 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
>>  
>>  	ice_update_pf_netdev_link(pf);
>>  
>> +	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
>> +		bool link_up;
>> +
>> +		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
>> +			     ICE_AQ_LINK_UP);
>> +		ice_ptp_link_change(pf, link_up);
>> +	}
>> +
> 
> [Severity: Medium]
> After a CORER, GLOBR or EMPR reset, does this call restart the PHY on
> non-owner ports?
> 
> Before this patch, each PF's VSI rebuild called
> ice_up_complete() -> ice_ptp_link_change(pf, true). That restarted the
> PF's PHY after its own control and sideband queues had been reinitialized.
> 
> Also, ice_down() no longer clears ptp_port->link_up during
> ice_prepare_for_reset(), so link_up stays true across the reset.
> 
> The only restart left comes from the owner PF's rebuild:
> 
> ice_rebuild() (owner PF)
>   ice_ptp_rebuild()
>     ice_ptp_rebuild_owner()
>       ice_ptp_restart_all_phy()
>         ice_ptp_port_phy_restart(port)
> 
> This path uses ptp_port_to_pf(port)->hw, which is the non-owner PF's own
> sideband queue. Each PF handles the reset on its own from its service task
> (ice_prepare_for_reset() shuts its control queues down and ice_rebuild()
> brings them back up), and nothing orders one PF against another.
> 
> If the owner gets to ice_ptp_restart_all_phy() before a non-owner has run
> ice_init_all_ctrlq(), the PHY access fails. On E82x, tx.disabled has
> already been set by then and ov_work is not queued.
> 
> The non-owner's own call here then returns at the "link_up == linkup"
> check, because the link was up both before and after the reset. Would
> timestamping on that port stay disabled until a physical link toggle?
> 
> This still appears to be present at the end of the series.
> 


I think this needs to remove the check against what the current value is
and just unconditionally restart the PHY on every link change. :\ But
I'm not sure if that wouldn't cause any other issues...


>>  	/* tell the firmware we are up */
>>  	err = ice_send_version(pf);
>>  	if (err) {
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 1e6ba6bbf6c2..f43918f334e8 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -1319,6 +1319,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>>  
>>  	mutex_lock(&pf->adapter->ps_lock);
>>  
>> +	if (READ_ONCE(ptp_port->link_up) == linkup)
>> +		goto out_unlock;
>> +
>>  	WRITE_ONCE(ptp_port->link_up, linkup);
>>  
>>  	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
> 
> [Severity: Medium]
> Can the cached link_up drift away from what was actually applied to the
> hardware?
> 
> link_up is written before the READY/reset_ongoing check, so an event that
> skips the PHY action still updates the cache. Nothing on the reset path
> (ice_ptp_prepare_for_reset() or ice_ptp_rebuild()) invalidates it.
> 
> The first case is an LSE handled while hw->reset_ongoing is set (it is set
> without ps_lock in ice_misc_intr()) or while PTP is in ICE_PTP_RESETTING.
> link_up is updated but the PHY is not touched.
> 
> If the owner's ice_ptp_restart_all_phy() has already run or skipped this
> port, the ice_ptp_link_change() call at the end of ice_rebuild() returns
> early. The PHY then stays stopped while the link is up.
> 
> The second case is PFR. ice_ptp_rebuild() skips the owner restart for
> ICE_RESET_PFR:
> 
> 	if (ice_pf_src_tmr_owned(pf) && reset_type != ICE_RESET_PFR) {
> 		err = ice_ptp_rebuild_owner(pf);
> 
> So if the link does not change, nothing restarts the PHY after a PF reset.
> 
> Later in the series, "ice: E822: cancel offset verification work during
> reset preparation" adds an unconditional
> kthread_cancel_delayed_work_sync(&ptp->port.ov_work) to
> ice_ptp_prepare_for_reset() for ICE_MAC_GENERIC, before the PFR early
> return. Its justification is "the ice_ptp_link_change() call at the end of
> the rebuild".
> 
> Also, "ice: E822: keep Tx timestamps disabled during offset calibration"
> makes ov_work the only place that clears tx.disabled.
> 
> With both of those applied, a PFR while Vernier calibration is still
> pending cancels ov_work. The rebuild call then returns at this check
> without requeuing it. Would Tx timestamps and the Rx offset stay disabled
> until a physical link toggle?
> 
> [ ... ]
> 

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

[Jacob Keller]

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.
> 

Re: [PATCH net v5 08/13] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Jacob Keller]

On 10/25/2026 4:44 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] The commit message for "ice: E825: stop clearing
>   PHY_REG_TX_OFFSET_READY" names a Tx tracker field that does not exist.
> 
> --
> 
> ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
> 
> On E825, ice_stop_phy_timer_eth56g() no longer clears
> PHY_REG_TX_OFFSET_READY. Clearing it left the hardware's outstanding Tx
> timestamp counter out of sync. Instead, new Tx timestamp requests are
> blocked in software while the PHY is stopped or restarting, and
> ice_ptp_mark_tx_tracker_stale() now checks tx->init.
> 
>> Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
>> requests could happen while the PHY is calibrating. To avoid this, set the
>> tx.calibrating field of the Tx timestamp tracker when stopping the timer
>> and clear it when finishing the restart.
> 
> [ ... ]
> 
>> Unlike E822 devices, set the calibrating flag when "stopping" the PHY and
>> clear it immediately after the start procedure.
> 
> [ ... ]
> 
>> In the event that the PHY timer start procedure fails, the device is in an
>> unknown state and timestamps will not behave properly. As such, and similar
>> to E822 devices, the calibrating field is not cleared on failure.
> 
> [Severity: Low]
> Should these references to tx.calibrating say tx.disabled instead?
> 
> struct ice_ptp_tx in ice_ptp.h has no calibrating member. Its bitfields
> are:
> 
> 	u8 init : 1;
> 	u8 disabled : 1;
> 	u8 has_ready_bitmap : 1;
> 
> The new E825 code in ice_ptp_port_phy_stop() and
> ice_ptp_port_phy_restart() sets and clears ptp_port->tx.disabled:
> 
> 	spin_lock_irqsave(&ptp_port->tx.lock, flags);
> 	ptp_port->tx.disabled = true;
> 	spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> 
> ice_ptp_wait_for_offsets() uses the same field for E82x, and the
> kernel-doc update in this patch is for @disabled.
> 
> The behaviour the commit message describes matches the code. Only the
> field name is wrong.
> 

Yes, these doc changes should have been fixed but I missed them while
renaming the field.

Re: [PATCH net v5 09/13] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[Jacob Keller]

On 10/25/2026 4:44 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] In ice_ptp_phy_soft_reset_eth56g()
>   (drivers/net/ethernet/intel/ice/ice_ptp_hw.c), the read sweep…
> 
> --
> 
> ice: E825: clear PHY_TSTAMP memory prior to soft reset
> 
> This commit removes the write-based ice_ptp_reset_ts_memory_eth56g(). In
> its place, a new ice_ptp_clear_tx_memory_status_eth56g() reads every
> PHY_TSTAMP index for a port. That sweep is now called at the start of
> ice_ptp_phy_soft_reset_eth56g(), so the soft reset clears the internal Tx
> timestamp counter after the reads. E825 is also dropped from
> ice_ptp_reset_ts_memory().
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 07b55fbb88dd..5637925c0e68 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> @@ -2311,6 +2312,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>>  	u32 global_val;
>>  	int err;
>>  
>> +	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
>> +
>>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
>>  	if (err) {
>>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
> 
> [Severity: Low]
> The sweep has no return value and always runs before the PHY_REG_GLOBAL
> read and the three PHY_REG_GLOBAL writes. Any of those sideband accesses
> can fail and make ice_ptp_phy_soft_reset_eth56g() return early. If that
> happens, the port has been swept but its internal counter was never
> reset.
> 
> The new kdoc for ice_ptp_clear_tx_memory_status_eth56g() says:
> 
>   Due to interactions with an internal HW counter for the number of
>   outstanding Tx timestamps, this *must* only be called as part of the
>   ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
>   may become out of sync and prevent new timestamp interrupts.
> 
> Doesn't this error path leave the port in the state that comment warns
> about?
> 
> I could not find any visible effect from this today:
> 
> - ice_ptp_init_phc_e825c() stops at the first port that fails.
> - The error reaches ice_ptp_init_owner() or ice_ptp_rebuild_owner(), and
>   both treat it as fatal for PTP.
> - Recovery paths, such as a link toggle or a driver reload, run the sweep
>   and the soft reset again.
> 
> So this looks like a robustness and documentation mismatch rather than a
> functional regression.
> 
> Could PHY_REG_GLOBAL be read before the sweep, so that a failure at that
> first step happens before the counter is touched? Alternatively, the kdoc
> could be updated to cover the error path.
> 


We can update the kdoc to better clarify. The intention is that if any
of the writes during the restart fail we will error out to the top level
of the PHY restart and log an error that a link restart is necessary,
rather than trying to implment more complex restart or other error
handling logic in the lower level of the driver.

Re: [PATCH net v5 10/13] ice: E825: perform a soft reset when starting the PHY timer

[Jacob Keller]

On 10/25/2026 4:44 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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).
> 
> - [Medium] ice_start_phy_timer_eth56g()
>   (drivers/net/ethernet/intel/ice/ice_ptp_hw.c) now calls…
> - [Medium] With this patch, the destructive E825 PHY soft-reset sequence
>   runs at runtime from ice_start_phy_timer_eth56g(), mainly on E825…
> 
> --
> 
> ice: E825: perform a soft reset when starting the PHY timer
> 
> This patch makes ice_start_phy_timer_eth56g() call
> ice_ptp_phy_soft_reset_eth56g() before it programs the E825 PHY. The goal
> is to clear any outstanding Tx timestamp memory and the PHY timestamp
> counter. Only the timer block of the PHY is reset.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 5637925c0e68..2d000251b185 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> @@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>>  	if (err)
>>  		return err;
>>  
>> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
>> +	if (err)
>> +		return err;
>> +
> 
> [Severity: Medium]
> Should the software Tx timestamp tracker be flushed before this reset
> wipes the hardware timestamp memory?
> 

I am getting very tired with this... I tried flushing in a previous
version and that got a ton of complaints.

> On E825 link-up the path is ice_ptp_link_change() ->
> ice_ptp_port_phy_restart() -> ice_start_phy_timer_eth56g(). The E825 case
> in ice_ptp_port_phy_restart() only marks the outstanding entries stale:
> 
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
>     ...
> 		ptp_port->tx.disabled = true;
> 		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> 
> 		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
> 
> 		err = ice_start_phy_timer_eth56g(hw, port);
>     ...
> }
> 
> The in_use bits and tstamps[idx].skb stay in place. After that,
> ice_ptp_phy_soft_reset_eth56g() calls
> ice_ptp_clear_tx_memory_status_eth56g() and pulses the soft reset, which
> clears every ready bit on the port.
> 
> Take an entry whose timestamp was captured but not read yet. Before this
> patch it would have been read through the ready bit path and quietly
> dropped as stale. Now its ready bit never comes back, so
> ice_ptp_process_tx_tstamp() can only release it through the timeout:
> 
> 		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
> 			drop_ts = true;
> 
> 			/* Count the number of Tx timestamps that timed out */
> 			pf->ptp.tx_hwtstamp_timeouts++;
> 		}
> 
> Does this hold the slot and the skb from ice_ptp_request_ts() for about
> 2 seconds? Does it also count a timestamp the driver threw away itself in
> tx_hwtstamp_timeouts, which ethtool -S reports?
> 
> At this commit there is also no time bound on the hold. The periodic
> fallback in ice_ptp_maybe_trigger_tx_interrupt() fires only when the
> hardware ready bits are set, and the reset has just cleared them.
> 
> The later commit "ice: keep Tx timestamp slots tracked until completion
> or timeout" changes that trigger to check the software in_use bitmap.
> That bounds the reclaim to about 2.5 seconds. The miscounted timeouts and
> the temporary slot and skb hold are still there at the end of the series.
> 
> ice_ptp_rebuild_owner() already flushes the software state before it
> restarts the PHYs:
> 
> 	/* Flush software tracking of any outstanding timestamps since we're
> 	 * about to flush the PHY timestamp block.
> 	 */
> 	ice_ptp_flush_all_tx_tracker(pf);
> 

I'm actually not entirely convinced that is safe either :/ I suspect
that if we did not have that and tried to add it now there would be
similar complaints to what I got with trying to use it during link change...

> Would the link change restart path need a matching flush? The commit
> message says only that the reset will "clear any outstanding Tx timestamp
> memory". It doesn't mention what happens to the software tracker.
> 

The current intention of the series that that those indexes remain
blocked until they time out. I could try to adjust the counting logic so
that stale packets don't count as a timeout. But the way the current
logic works is entirely built around a flow where the only thread that
clears in_use is the IRQ thread. Flushing the tracker breaks that and
caused a lot of complaints.

> [Severity: Medium]
> What keeps the Tx timestamp interrupt thread and in-flight hardware
> captures away from this port while the soft reset sequence runs?
> 
> Before this patch, the only caller of ice_ptp_phy_soft_reset_eth56g() was
> ice_ptp_init_phc_e825c(), at PHC init. Now it also runs on every E825
> link-up restart. The sequence sweeps all slots and sleeps before the
> reset is asserted:
> 
> drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_ptp_phy_soft_reset_eth56g() {
> 	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
>     ...
> 	usleep_range(5000, 6000);
> 
> 	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
>     ...
> 	usleep_range(5000, 6000);
>     ...
> }
> 
> ice_stop_phy_timer_eth56g() leaves PHY_REG_TX_OFFSET_READY set, so the
> PHY keeps capturing timestamps for descriptors that were posted before
> tx.disabled was set. A capture can land on an index the sweep has already
> passed, during the 5 ms gap before the reset is asserted.
> 
> The consumer path is ice_misc_intr_thread_fn() -> ice_ptp_process_ts() ->
> ice_ptp_tx_tstamp_owner() -> ice_ptp_process_tx_tstamp(). It never takes
> ps_lock. Its ice_is_reset_in_progress() check doesn't cover a link
> triggered PHY restart. It snapshots the ready bitmap and then calls
> ice_read_phy_tstamp() based on that snapshot.
> 
> Suppose the thread snapshots that new ready bit and then reads the slot
> after the soft reset. It would read a slot whose valid bit is still set,
> against a counter that was just reset to zero. The kdoc for
> ice_ptp_clear_tx_memory_status_eth56g() says the soft reset does not clear
> the timestamp memory.

Since the previous sweep cleared the bit, this read should do nothing
because the timestamp is not marked valid. A new timestamp shouldn't be
possible to land on this index because the index is still locked by
software. The existing timestamp that was on that index should be
cleared by the sweep and soft reset, so this should not trigger a faulty
flow.
> 
> The earlier commit "ice: E825: clear PHY_TSTAMP memory prior to soft
> reset" says an unbalanced read "underflows the counter, and prevents new
> timestamp interrupts from being triggered".
> 

Yes, *if* the valid bit is set which the sweep before the soft reset is
supposed to clear.

> Could this stall Tx timestamp interrupts on the port until the next soft
> reset? That is the failure this patch is meant to prevent. Whether it
> happens depends on PHY behavior described only in this series and on a
> narrow timing window. Even so, nothing in the code seems to prevent the
> interleaving.

I don't believe this could stall. I am also not entirely certain what
the best mechanism to safely stop the IRQ from triggering is.
Theoretically I guess we could disable interrupts, do this work, then
re-enable interrupts? But at least currently soft reset can take a while
(we're under investigation as to whether those waits are necessary but
intended to change that as a followup).

> 
> Does this path need to synchronize with the misc IRQ thread, or wait for
> in-flight captures, before it runs the sweep and the reset?
> 

Waiting for in-flight captures is problematic as we saw with the older
series and its complaints. I suspect that synchronizing to stop the IRQ
is ok, but I am worried about something trying to disable the HW
register and getting in such a state that we actually never re-enable
interrupts...

>>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
>>  
>>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);
> 

Re: [PATCH net v5 11/13] ice: keep Tx timestamp slots tracked until completion or timeout

[Jacob Keller]

On 10/25/2026 4:44 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: 0 · Medium: 1 · 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).
> 
> - [Medium] The commit message says link-down requests are marked stale,
>   but the patch never marks them stale on E810, E830 or E82x.
> 
> --
> 
> ice: keep Tx timestamp slots tracked until completion or timeout
> 
> This patch stops dropping outstanding Tx timestamp requests on link down
> and rejects new requests while the link is down. Slots that are not ready
> yet are now released by the existing 2 second timeout. It also makes the
> interrupt re-arm checks ignore stale slots, and lets
> ice_ptp_maybe_trigger_tx_interrupt() run for PFs that handle their own Tx
> timestamp interrupt.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index b7c539f98276..309fa2877cc3 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -582,14 +584,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>>  			return;
>>  	}
>>  
>> -	/* Drop packets if the link went down */
>> -	link_up = READ_ONCE(ptp_port->link_up);
>> -
>>  	for_each_set_bit(idx, tx->in_use, tx->len) {
>>  		struct skb_shared_hwtstamps shhwtstamps = {};
>>  		u8 phy_idx = idx + tx->offset;
>>  		u64 raw_tstamp = 0, tstamp;
>> -		bool drop_ts = !link_up;
>> +		bool drop_ts = false;
>>  		struct sk_buff *skb;
> 
> [Severity: Medium]
> The commit message says:
> 
>     Mark the outstanding requests stale so their completions are read
>     and discarded
> 
> Where does this stale marking happen for E810, E830 and E82x? The patch
> removes the link-down drop here, but it doesn't add any new calls to
> ice_ptp_mark_tx_tracker_stale().
> 

At present it doesn't. I think this was needed for E825 because of the
complexity of what the reset flow required. I am not sure its actually
necessary on the other devices. We can improve the commit message for
clarity.

> On link down, the only caller that can be reached is the
> ICE_MAC_GENERIC_3K_E825 branch of ice_ptp_port_phy_stop() (and the same
> branch of ice_ptp_port_phy_restart()):
> 
> ice_ptp_port_phy_stop() {
>     ...
> 	case ICE_MAC_E810:
> 	case ICE_MAC_E830:
> 		err = 0;
> 		break;
> 	case ICE_MAC_GENERIC:
> 		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
> 
> 		err = ice_stop_phy_timer_e82x(hw, port, true);
> 		break;
> 	case ICE_MAC_GENERIC_3K_E825:
> 		...
> 		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
>     ...
> }
> 
> Also, ice_ptp_link_change() skips E810 and E830 entirely:
> 
> 	case ICE_MAC_E810:
> 	case ICE_MAC_E830:
> 		/* Do not reconfigure E810 or E830 PHY */
> 		break;
> 
> The new in_irq check in ice_port_has_timestamps() only excludes stale
> slots:
> 
> 	if (in_irq)
> 		return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
> 
> Given that, can a link down with a request still outstanding on E810
> (non-LL), E830 or E82x cause a back-to-back interrupt loop? The sequence
> would be:
> 
>   link goes down while a not-ready slot is set in in_use
>   ice_ptp_maybe_trigger_tx_interrupt()
>     ice_ptp_tx_tstamps_pending(pf, false) -> true
>     wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
>   ice_ptp_process_tx_tstamp()
>     slot not ready and not timed out -> continue
>   ice_misc_intr_thread_fn() (E810/E82x) or ice_ptp_ts_irq() (E830)
>     ice_ptp_tx_tstamps_pending(pf, true) -> true, slot is not stale
>     wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M)
>   ... repeats until the 2 second timeout frees the slot
> 

This ultimately depends on whether a given index actually gets stuck or
completes, which may depend on each device type. We don't really have a
good mechanism to rate limit how often the IRQ re-tries, (and at this
point I am heavily considering just trying to switch to a polling
implementation given how much trouble we've had with the IRQ to begin
with...)

> On E830 the re-arm happens in hardirq context in ice_ptp_ts_irq(). On E82x
> every pass does sideband PHY reads. Before this patch, the first pass after
> link down dropped every slot, so this loop could not happen.
> 
> A later patch in this series, "ice: don't clear in_use until HW clears
> ready bitmap", adds an ice_ptp_is_tx_tracker_up() check to the in_irq
> branch. Combined with the link_up check added here, that seems to close
> the loop.
> 
> Even after that patch, though, E810, E830 and E82x still don't get any
> stale marking on link down. So this commit isn't bisect-safe on its own,
> and the commit message describes a mechanism the code doesn't implement.
> 
> Should the commit message be corrected, or should the stale marking be
> added for the other MAC types?
> 

I can certainly try to reword the commit message...

Re: [PATCH net v5 13/13] ice: don't clear in_use until HW clears ready bitmap

[Jacob Keller]

On 10/25/2026 4:44 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> 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).
> 
> - [Medium] The patch removes the hardware ready-bitmap check
>   (ice_check_phy_tx_tstamp_ready()) from ice_ptp_tx_tstamps_pending().
> - [Low] The kernel-doc of ice_ptp_process_tx_tstamp() (ice_ptp.c:514-562)
>   no longer matches the code.
> 
> --
> 
> ice: don't clear in_use until HW clears ready bitmap
> 
> After each PHY timestamp read, this patch re-reads the Tx timestamp ready
> bitmap. If the index's ready bit has not cleared, the index is marked
> stale and kept locked, and its SKB is freed. The patch also drops the
> hardware ready bitmap check from the in-IRQ re-arm decision. That check
> now runs only from the periodic watchdog.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index b0ea01cfcc4f..c0d056896825 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -557,7 +557,9 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx)
>>   * extremely unlikely that a packet will ever take this long to timestamp. If
>>   * we detect a Tx timestamp request that has waited for this long we assume
>>   * the packet will never be sent by hardware and discard it without reading
>> - * the timestamp register.
>> + * the timestamp register. Note that in the unusual case where the PHY
>> + * continuously fails to clear the ready bitmap index, the slot may remain
>> + * locked for more than two seconds.
>>   */
>>  static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>>  {
> 
> [Severity: Low]
> Is the rest of this kernel-doc still accurate after this change? The
> numbered list earlier in the same comment still says:
> 
>  * 3) read and copy the timestamp out of the PHY register
>  * 4) unlock the index by clearing the associated in_use bit
>  * 5) check if the timestamp is stale, and discard if so
> 
> Look at the new "verify ready bit cleared" block in
> ice_ptp_process_tx_tstamp(). If the ready bit is still set after the read,
> or the re-read of the ready bitmap fails, the index is marked stale and
> the SKB is freed. in_use stays set, so step 4 never happens.
> 
> The opening line, "loop over each index with a waiting skb", is also no
> longer true. in_use slots with a NULL skb are now visited on every pass
> until the hardware clears the bit.
> 
> This paragraph also still says timed-out requests are discarded "without
> reading the timestamp register". A timed-out index whose ready bit is set
> is still read through ice_read_phy_tstamp(), and with this patch it can
> stay locked.
> 
> Could the kernel-doc be updated to describe the new flow?
> 

I can try to reword this doc flow (again..)

> [ ... ]
> 
>> @@ -2766,10 +2787,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
>>  		if (!tx->init)
>>  			return false;
>>  
>> -		if (in_irq)
>> +		if (in_irq) {
>> +			if (!ice_ptp_is_tx_tracker_up(tx))
>> +				return false;
>> +
>>  			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
>> -		else
>> +		} else {
>>  			return !bitmap_empty(tx->in_use, tx->len);
>> +		}
>>  	}
>>  }
>>  
>> @@ -2801,41 +2826,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
>>  
>>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
>>  {
>> -	struct ice_hw *hw = &pf->hw;
>> -	int ret;
>> -
>> -	/* Check software indicator */
>>  	switch (pf->ptp.tx_interrupt_mode) {
>>  	case ICE_PTP_TX_INTERRUPT_NONE:
>>  		return false;
>>  	case ICE_PTP_TX_INTERRUPT_SELF:
>> -		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
>> -			return true;
>> -		break;
>> +		return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq);
>>  	case ICE_PTP_TX_INTERRUPT_ALL:
>> -		if (ice_any_port_has_timestamps(pf, in_irq))
>> -			return true;
>> -		break;
>> +		return ice_any_port_has_timestamps(pf, in_irq);
>>  	default:
>>  		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n",
>>  			  pf->ptp.tx_interrupt_mode);
>> -		break;
>> -	}
>> -
>> -	/* Check hardware indicator */
>> -	ret = ice_check_phy_tx_tstamp_ready(hw);
>> -	if (ret < 0) {
>> -		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
>> -			ret);
>> -		/* Stop triggering IRQs if we're unable to read PHY */
>>  		return false;
>>  	}
>> -
>> -	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
>> -	 * available, 0 if there are no waiting timestamps, and a negative
>> -	 * value if there was an error (which we checked for above).
>> -	 */
>> -	return ret > 0;
>>  }
> 
> [Severity: Medium]
> Can removing this hardware fallback leave the PHY Tx timestamp interrupt
> blocked until the watchdog runs?
> 

Theoretically? Yes. But we don't really have a good alternative. If we
keep it, we end up with a storm of IRQ retries because the IRQ sees a
ready timestamp and retries but its stuck so it keeps retrying and
burning CPU for 2 seconds. I don't have a mechanism to properly rate
limit this to try more frequently than the watchdog but not at insane
rates that re-triggering the IRQ does. Unfortunately the hardware
interrupt throttling doesn't apply to software-triggered IRQ like we do
here. I guess I could try a timer but.. at some point this just becomes
"why not rip out the IRQ stuff and just poll for timestamps to begin with".

> ice_misc_intr_thread_fn() and the E830 path in ice_ptp_ts_irq() call
> ice_ptp_tx_tstamps_pending(pf, true) to decide whether to re-arm:
> 
> drivers/net/ethernet/intel/ice/ice_main.c:ice_misc_intr_thread_fn() {
>     ...
> 	if (ice_ptp_tx_tstamps_pending(pf, true)) {
> 		/* If any new Tx timestamps happened while in interrupt,
> 		 * re-arm the interrupt to trigger it again.
> 		 */
> 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> 		ice_flush(hw);
> 	}
>     ...
> }
> 
> This change, combined with the new ice_ptp_is_tx_tracker_up() check in
> ice_port_has_timestamps(), means the re-arm only looks at non-stale
> in_use slots on trackers that are up.
> 
> The kernel-doc of ice_ptp_maybe_trigger_tx_interrupt() says the PHY "will
> not interrupt again until all current timestamp data is read". So if a
> ready entry is still unread at the end of an IRQ pass, nothing
> retriggers the interrupt. It stays blocked until the 500 ms watchdog,
> ice_ptp_periodic_work()->ice_ptp_maybe_trigger_tx_interrupt(), runs.
> 
> A few ways such an entry can be left behind:
> 
>  - a stale request whose timestamp lands mid-pass, after the ready
>    bitmap was read
>  - a late completion on a port that is link down, or (on E82x) in
>    Vernier calibration with tx.disabled set until
>    ice_ptp_wait_for_offsets() succeeds
>  - an index this patch keeps locked on purpose because its ready bit
>    did not clear
> 
> Before this patch, the in-IRQ ice_check_phy_tx_tstamp_ready() call
> retriggered the interrupt right away and drained these entries.
> 
> On E82x, timestamp memory and the interrupt are per quad (see
> ice_phy_cfg_intr_e82x() and the quad split in ice_ptp_init_tx_e82x()).
> In ICE_PTP_TX_INTERRUPT_ALL mode the clock owner services every port.
> 
> Could one leftover entry on a port that is going down or calibrating
> then delay Tx timestamps for healthy link-up ports in the same quad by
> up to about 500 ms? That is far beyond the 10 ms ptp4l default the
> commit message cites. The race window looks narrow, but link flaps and
> E82x recalibration happen in normal operation.
> 
> The commit "ice: keep Tx timestamp slots tracked until completion or
> timeout" in this series also says an orphaned ready bit "blocks the
> port interrupt".
> 

This entire mess is frustrating. If I try to fix one problem I create a
new theoretical problem, and I just am not sure there is a clean way to
handle all of the @#$%! corner cases these PHYs have boxed us into...

Implementing a polling routine kicked off by the Tx path on a timestamp
request that just checks and re-triggers with some form of back-off
polling so that it will try less and less frequently until a new request
kicks it back into higher gear seems like the most ideal solution to
bypass the complexity of all this broken IRQ nonsense.... But I don't
have a good tested implementation of that available right now.

Suggestions on how to make progress on fixing this are welcome....

Re: [PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[Paolo Abeni]

On 10/22/26 00:12, Jacob Keller 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 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. The fix also ensures that all teardown
> paths properly release the tracker and kworker in a safe order. This issue
> was found by Sashiko during review of a previous version of this series.
> 
> Next, 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 PHY restart locking is fixed to avoid an issue with a concurrent
> execution of ice_ptp_restart_all_phy() and a link change on a given port.
> Additionally, the PHY port locks are merged into a single per-adapter lock
> to reduce locking complexity.
> 
> 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, Arkadiusz and Jake modify the driver to stop pretending that the link
> has changed at administrative ice_down() and ice_up(). 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, 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, 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. Instead, the
> tx.calibrating field is used to disable new timestamps from software in a
> similar manner to the older E822 devices.
> 
> 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.
> 
> Finally, 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.
It's not trivially clear to me if the patches 2,4,6,8,9 and 12 are
functionally independent from the other ones. Are they? If so, we
could apply them, keep the remaining series smaller and hopefully
move this forward.

/P

Re: [PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[Jacob Keller]

On 10/27/2026 7:40 AM, Paolo Abeni wrote:
> On 10/22/26 00:12, Jacob Keller 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 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. The fix also ensures that all teardown
>> paths properly release the tracker and kworker in a safe order. This
>> issue
>> was found by Sashiko during review of a previous version of this series.
>>
>> Next, 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 PHY restart locking is fixed to avoid an issue with a
>> concurrent
>> execution of ice_ptp_restart_all_phy() and a link change on a given port.
>> Additionally, the PHY port locks are merged into a single per-adapter
>> lock
>> to reduce locking complexity.
>>
>> 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, Arkadiusz and Jake modify the driver to stop pretending that the
>> link
>> has changed at administrative ice_down() and ice_up(). 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, 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, 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. Instead, the
>> tx.calibrating field is used to disable new timestamps from software in a
>> similar manner to the older E822 devices.
>>
>> 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.
>>
>> Finally, 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.
> It's not trivially clear to me if the patches 2,4,6,8,9 and 12 are
> functionally independent from the other ones. Are they? If so, we
> could apply them, keep the remaining series smaller and hopefully
> move this forward.
> 
> /P
> 

Reviewing the patches, I do believe that subset can apply safely without
a high risk of regression. I would certainly be happier to have some of
this work move forward and potentially open some slots for other driver
fixes while we figure out how to fully resolve the remaining Sashiko
reports for the link issues.

Would you like me to make a respin with just that subset, or can you
apply them? I just checked and that subset successfully cherry-picked
easily with no merge conflicts net, and appears to build fine.

Thanks,
Jake

Re: [PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[Jakub Kicinski]

On Tue, 27 Oct 2026 12:32:30 -0700 Jacob Keller wrote:
> Would you like me to make a respin with just that subset, or can you
> apply them? I just checked and that subset successfully cherry-picked
> easily with no merge conflicts net, and appears to build fine.

That's too much of a cherry-pick for me TBH.
Could you please respin what you think is ready to go in?

Re: [PATCH net v5 00/13] ice: E82x: timestamp processing logic fixes

[Jacob Keller]

On 10/27/2026 3:15 PM, Jakub Kicinski wrote:
> On Tue, 27 Oct 2026 12:32:30 -0700 Jacob Keller wrote:
>> Would you like me to make a respin with just that subset, or can you
>> apply them? I just checked and that subset successfully cherry-picked
>> easily with no merge conflicts net, and appears to build fine.
> 
> That's too much of a cherry-pick for me TBH.
> Could you please respin what you think is ready to go in?
> 

Will do.

Thanks,
Jake