[PATCH net v3 00/14][pull request] ice: E82x: timestamp processing logic fixes

[PATCH net v3 00/14][pull request] ice: E82x: timestamp processing logic fixes

[Tony Nguyen]

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/
---
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.
---
v3:
- 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.

v2: https://lore.kernel.org/netdev/20261008215614.1987250-1-anthony.l.nguyen@intel.com/
- 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/

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 following are changes since commit 045958acaa75205b31578cb22c4345e28dd2e4bf:
  Merge tag 'net-7.3-rc8' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

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

Jacob Keller (9):
  ice: 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 (2):
  ice: wait for in-flight Tx timestamps before removing the tracker
  ice: keep Tx timestamp slots tracked until completion or timeout

 drivers/net/ethernet/intel/ice/ice_adapter.c |  20 +-
 drivers/net/ethernet/intel/ice/ice_adapter.h |  20 +-
 drivers/net/ethernet/intel/ice/ice_main.c    |  20 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 536 +++++++++++++------
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  16 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 153 +++---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h  |   2 +-
 7 files changed, 515 insertions(+), 252 deletions(-)

-- 
2.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@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.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.c |  17 ++-
 drivers/net/ethernet/intel/ice/ice_adapter.h |  16 ++-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 125 ++++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_ptp.h     |   4 +
 4 files changed, 120 insertions(+), 42 deletions(-)

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_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index 4f695f32da3d..0b01c7f5cf0d 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -17,15 +17,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.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);
 }
 
 /**
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 */
-- 
2.47.1

[PATCH net v3 03/14] ice: fix PHY port restart serialization

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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.

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.c |  3 ++
 drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 55 ++++++++++----------
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  2 -
 4 files changed, 35 insertions(+), 29 deletions(-)

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_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index 0b01c7f5cf0d..5f39166795b6 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -40,6 +40,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
@@ -53,6 +54,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.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 55241802d17a..d1cdd38ea27c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -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,11 +1250,11 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 	unsigned long flags;
 	int err;
 
+	lockdep_assert_held(&pf->adapter->ps_lock);
+
 	if (!ptp_port->link_up)
 		return ice_ptp_port_phy_stop(ptp_port);
 
-	mutex_lock(&ptp_port->ps_lock);
-
 	switch (hw->mac_type) {
 	case ICE_MAC_E810:
 	case ICE_MAC_E830:
@@ -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;
 }
 
@@ -1314,12 +1314,13 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 
 	ptp_port = &pf->ptp.port;
 
-	/* Update cached link status for this port immediately */
+	mutex_lock(&pf->adapter->ps_lock);
+
 	ptp_port->link_up = linkup;
 
 	/* Skip HW writes if reset is in progress */
 	if (pf->hw.reset_ongoing)
-		return;
+		goto out_unlock;
 
 	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
 	    test_bit(ICE_FLAG_DPLL, pf->flags)) {
@@ -1362,17 +1363,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 +1442,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,6 +1454,8 @@ 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)) {
@@ -1469,6 +1468,8 @@ static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 		kref_put(&port->ref, ice_ptp_release_port_srcu);
 	}
 	srcu_read_unlock(&ports->srcu, srcu_idx);
+
+	mutex_unlock(&pf->adapter->ps_lock);
 }
 
 /**
@@ -3308,8 +3309,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:
@@ -3420,7 +3419,9 @@ void ice_ptp_init(struct ice_pf *pf)
 		goto err_release_tx_tracker;
 
 	/* Start the PHY timestamping block */
-	ice_ptp_reset_phy_timestamping(pf);
+	mutex_lock(&pf->adapter->ps_lock);
+	ice_ptp_port_phy_restart(&ptp->port);
+	mutex_unlock(&pf->adapter->ps_lock);
 
 	/* Configure initial Tx interrupt settings */
 	ice_ptp_cfg_tx_interrupt(pf);
@@ -3439,7 +3440,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 +3479,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 +3497,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;
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;
-- 
2.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@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 d1cdd38ea27c..11416e23dade 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;
@@ -2674,11 +2680,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.47.1

[PATCH net v3 05/14] ice: call PTP link change only from link events

[Tony Nguyen]

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. Modify
ice_ptp_link_change() to always update port.link_up even if the PTP state
is not ready, so that it reflects the most recent link edge. Update the
ice_ptp_init() function to first set ptp->state to ICE_PTP_READY, and only
then call ice_ptp_port_phy_restart().

Finally, ensure that ice_rebuild() calls ice_ptp_link_change() to reflect
the current link status in case it changed and an edge was missed during a
reset.

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.

Note that both ice_ptp_init() and ice_rebuild() may result in a duplicate
initialization of the PHY.

In ice_ptp_init() this could happen if a link status event happens
concurrently with ice_ptp_init(). The ice_ptp_init() will call
ice_ptp_port_phy_restart() at the same time as ice_ptp_link_change() is
called by ice_link_event(). While the PHY may be initialized twice, it
should always end up in the correct state. Either the call from
ice_ptp_init() completes first, and then the correct state is set by the
ice_ptp_link_change(), or the link change completes first, and the call
from ice_ptp_init() reconfigures with the updated link_up state.

In ice_ptp_rebuild() if the clock owner has reset, it will call
ice_ptp_restart_all_phy() which will begin a restart on each port. For
large resets, like CORE reset or GLOBAL reset, each port will also reset
and then after rebuild each port will restart its PHY timestamp block
again. This is necessary because the link state may have changed between
the time the clock owner restarts and the rebuild finishes. These duplicate
restarts do force extra work on the PHY, but should only happen during
initialization or hardware reset, and are better than leaving the device
misconfigured.

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 | 18 ++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 20 ++++++++------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..b4ee2cf7cd70 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -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);
 	}
@@ -7794,6 +7800,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 11416e23dade..776f211290ec 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 = {};
@@ -1258,7 +1258,7 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 
 	lockdep_assert_held(&pf->adapter->ps_lock);
 
-	if (!ptp_port->link_up)
+	if (!READ_ONCE(ptp_port->link_up))
 		return ice_ptp_port_phy_stop(ptp_port);
 
 	switch (hw->mac_type) {
@@ -1315,17 +1315,13 @@ 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;
 
 	mutex_lock(&pf->adapter->ps_lock);
 
-	ptp_port->link_up = linkup;
+	WRITE_ONCE(ptp_port->link_up, linkup);
 
-	/* Skip HW writes if reset is in progress */
-	if (pf->hw.reset_ongoing)
+	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
 		goto out_unlock;
 
 	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
@@ -1468,7 +1464,7 @@ static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 		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);
@@ -3426,7 +3422,9 @@ void ice_ptp_init(struct ice_pf *pf)
 	if (err)
 		goto err_release_tx_tracker;
 
-	/* Start the PHY timestamping block */
+	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);
@@ -3434,8 +3432,6 @@ void ice_ptp_init(struct ice_pf *pf)
 	/* Configure initial Tx interrupt settings */
 	ice_ptp_cfg_tx_interrupt(pf);
 
-	ptp->state = ICE_PTP_READY;
-
 	/* Start periodic work now that the state is READY */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
-- 
2.47.1

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

[Tony Nguyen]

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.

Move the tx.calibrating = false to ice_ptp_wait_for_offsets(), 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 calibrating 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.

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
calibrating flag enabled "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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 29 +++++++++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_ptp.h |  2 +-
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 776f211290ec..828d46a73f29 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -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.calibrating) {
+			port->tx.calibrating = 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));
@@ -1283,11 +1301,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 calibrating 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;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 27ea502b7576..029ee4612d76 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -107,7 +107,7 @@ enum ice_tx_tstamp_work {
  * @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.
+ *               window, timestamp requests are disabled.
  * @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.
-- 
2.47.1

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

[Tony Nguyen]

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@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 828d46a73f29..629777380694 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -3006,6 +3006,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	kthread_cancel_delayed_work_sync(&ptp->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.47.1

[PATCH net v3 08/14] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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 disabled, 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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 39 ++++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 25 ++++++-------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  2 +-
 3 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 629777380694..365cff9c1017 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -818,7 +818,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 +1229,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 +1245,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.calibrating = 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;
@@ -1309,15 +1318,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.calibrating = 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.calibrating = 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 remains disabled; 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;
 
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);
-- 
2.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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.

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 112 ++++++++++----------
 1 file changed, 58 insertions(+), 54 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..38d0ded1d0cc 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
- */
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
-{
-	unsigned int port;
-
-	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);
+ * @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_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
+{
+	int err, failed = 0;
+	u8 idx;
+
+	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",
@@ -5802,8 +5805,9 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 		ice_ptp_reset_ts_memory_e82x(hw);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		ice_ptp_reset_ts_memory_eth56g(hw);
-		break;
+		/* E825 hardware must only reset timestamp memory as part of
+		 * the soft reset procedure.
+		 */
 	case ICE_MAC_E810:
 	default:
 		return;
-- 
2.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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.

Since the E825 PHY restart procedure now performs a soft reset, flush the
outstanding timestamps instead of marking them as stale. This avoids
keeping those indexes locked in software unnecessarily for 2 seconds. This
is safe because the soft reset procedure will restore the timestamp state
and clear any desync.

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c    |  5 +++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 365cff9c1017..32be908373c3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -756,7 +756,8 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
  * @pf: Board private structure
  * @tx: the tracker to flush
  *
- * Called during teardown when a Tx tracker is being removed.
+ * Called to immediately discard any outstanding Tx timestamps, including when
+ * tearing down the Tx tracker, and on E825 when restarting the PHY.
  */
 static void
 ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
@@ -1327,7 +1328,7 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 		ptp_port->tx.calibrating = true;
 		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
 
-		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+		ice_ptp_flush_tx_tracker(pf, &ptp_port->tx);
 
 		err = ice_start_phy_timer_eth56g(hw, port);
 		if (err)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 38d0ded1d0cc..51442b26516e 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.47.1

[PATCH net v3 11/14] ice: wait for in-flight Tx timestamps before removing the tracker

[Tony Nguyen]

From: Petr Oros <poros@redhat.com>

ice_ptp_flush_tx_tracker() frees every tracked request, but a request
whose timestamp is still being captured by the PHY at that moment is
freed without touching the PHY entry. The ready bit published shortly
after has no tracked owner, and the PHY does not raise another Tx
timestamp interrupt until every outstanding ready bit is read, so
delivery for the whole quad degrades to the periodic work.

Wait up to 10 ms (per port) for in-flight captures to publish their ready
bits before flushing during tracker removal, so the flush clears them
together with the rest before removing the tracker.

Note that the ice_ptp_flush_tx_tracker() function was introduced along with
the original E810 support, but that device does not have a ready bitmap.
Only later devices (E822, E825, E830) have the bitmap and potential issues
with internal tracking. Thus, skip the wait for E810 by checking the
tx->has_ready_bitmap flag.

Only apply the wait to the ice_ptp_release_tx_tracker() call. Other callers
(such as the port restart path for E825) reset the PHY, so any desync will
be cleared immediately, making the wait redundant.

Fixes: 10e4b4a3a3e1 ("ice: check Tx timestamp memory register for ready timestamps")
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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 64 ++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 32be908373c3..ca581490f3ed 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -751,6 +751,68 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
 	return 0;
 }
 
+/**
+ * ice_ptp_is_tracker_drained - Check for outstanding timestamps
+ * @pf: Board private structure
+ * @tx: Timestamp tracker structure
+ *
+ * Return: False if there are any timestamps still waiting for hardware;
+ *         otherwise true, including when unable to read the ready bitmap.
+ */
+static bool
+ice_ptp_is_tracker_drained(struct ice_pf *pf, struct ice_ptp_tx *tx)
+{
+	struct ice_hw *hw = &pf->hw;
+	bool pending = false;
+	unsigned long flags;
+	u64 tstamp_ready;
+	u8 idx;
+
+	/* If HW reset is ongoing, we can't access SBQ */
+	if (hw->reset_ongoing)
+		return true;
+
+	if (ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready))
+		return true;
+
+	spin_lock_irqsave(&tx->lock, flags);
+	for_each_set_bit(idx, tx->in_use, tx->len) {
+		if (!(tstamp_ready & BIT_ULL(idx + tx->offset))) {
+			pending = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&tx->lock, flags);
+
+	return !pending;
+}
+
+/**
+ * ice_ptp_wait_for_tracker_drain - Wait for PHY to complete timestamps
+ * @pf: Board private structure
+ * @tx: Timestamp tracker structure
+ *
+ * Wait for up to 10 milliseconds for the PHY to complete any outstanding
+ * timestamps before flushing.
+ */
+static void
+ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
+{
+	bool drained;
+	int err;
+
+	if (!tx->has_ready_bitmap)
+		return;
+
+	err = read_poll_timeout(ice_ptp_is_tracker_drained,
+				drained, drained, 500, 10 * USEC_PER_MSEC, false,
+				pf, tx);
+	if (err) {
+		dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
+			tx->block);
+	}
+}
+
 /**
  * ice_ptp_flush_tx_tracker - Flush any remaining timestamps from the tracker
  * @pf: Board private structure
@@ -868,6 +930,8 @@ ice_ptp_release_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
 	/* wait for potentially outstanding interrupt to complete */
 	synchronize_irq(pf->oicr_irq.virq);
 
+	ice_ptp_wait_for_tracker_drain(pf, tx);
+
 	ice_ptp_flush_tx_tracker(pf, tx);
 
 	kfree(tx->tstamps);
-- 
2.47.1

[PATCH net v3 12/14] ice: keep Tx timestamp slots tracked until completion or timeout

[Tony Nguyen]

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.

Since we're adding a new call to ice_ptp_mark_tx_tracker, notice that the
function previously did not check the tracker initialization state. Fix
this by only accessing the bitmaps if the init field is set.

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c   |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 56 ++++++++++-----------
 drivers/net/ethernet/intel/ice/ice_ptp.h    |  8 ++-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c |  4 +-
 4 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index b4ee2cf7cd70..b48b9c455166 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 ca581490f3ed..57141e94c570 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->calibrating;
+	return tx->init && !tx->calibrating && 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()
@@ -1432,6 +1431,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 
 	WRITE_ONCE(ptp_port->link_up, linkup);
 
+	if (!linkup)
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
 	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
 		goto out_unlock;
 
@@ -2829,21 +2831,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;
@@ -2856,7 +2859,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);
@@ -2869,7 +2872,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;
@@ -2879,11 +2882,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:
@@ -2959,7 +2962,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.
 			 */
@@ -2981,27 +2984,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.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 029ee4612d76..7810fefc546e 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -132,6 +132,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
  *
@@ -314,7 +317,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);
 
@@ -362,7 +365,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_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 51442b26516e..bd14aa3a77f6 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.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@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 57141e94c570..a9658f859c2e 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.47.1

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

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

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. Also make sure to count Tx timestamp timeouts if we flush a
timestamp that waited for >2 seconds but had not yet had its index cleared.

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>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 106 +++++++++++++----------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a9658f859c2e..74e0c0138c50 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;
@@ -849,14 +869,20 @@ 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);
+
+		/* Count the number of Tx timestamps flushed */
+		pf->ptp.tx_hwtstamp_flushed++;
+
+		/* Also count timeouts if we flush a request that timed out */
+		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ))
+			pf->ptp.tx_hwtstamp_timeouts++;
+
 		skb = tx->tstamps[idx].skb;
 		tx->tstamps[idx].skb = NULL;
 		clear_bit(idx, tx->in_use);
 		clear_bit(idx, tx->stale);
-		spin_unlock_irqrestore(&tx->lock, flags);
 
-		/* Count the number of Tx timestamps flushed */
-		pf->ptp.tx_hwtstamp_flushed++;
+		spin_unlock_irqrestore(&tx->lock, flags);
 
 		/* Free the SKB after we've cleared the bit */
 		dev_kfree_skb_any(skb);
@@ -2839,10 +2865,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);
+		}
 	}
 }
 
@@ -2874,41 +2904,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;
 }
 
 /**
@@ -2992,6 +2999,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)
@@ -3001,7 +3009,15 @@ 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) {
+		dev_dbg(dev, "Unable to read PHY Tx timestamp ready bitmap, err %pe\n",
+			ERR_PTR(ret));
+		/* Don't trigger an IRQ if we are unable to access the PHY */
+		return;
+	}
+
+	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.47.1

Re: [PATCH net v3 01/14] 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: 1 (2) · 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).

- [High] On the clock owner, ice_ptp_init_owner() ->
  ice_ptp_create_clock() registers the PHC (/dev/ptpN) before the…
- [Medium] Because the E810 tracker is now kept across CORE/GLOBAL/EMP
  reset, the per-slot tx->tstamps[idx].cached_tstamp values survive the…

Pre-existing issues:
- [High] ice_ptp_setup_adapter() sets `pf->adapter->ctrl_pf = pf` for the
  clock-owner PF, and nothing ever clears it: not in the ice_ptp_init()…
- [High] At this commit the clock owner walks pf->adapter->ports.ports
  without adapter->ports.lock in ice_ptp_flush_all_tx_tracker(),…

--

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 can resume after a CORE, GLOBAL or EMP
reset. It also reorders ice_ptp_init() so the kworker and tracker are
created before the port is added to the adapter port list. Finally, it
makes ice_ptp_release() do a full cleanup when PTP is not in the READY
state.

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

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index eaec36ab6ae3f..e1c66cd0f94ff 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2939,8 +2939,6 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
>  		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
>  
> -	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> -

[Severity: Medium]
The tracker is now kept across CORE/GLOBAL/EMP reset. On E810, does that
leave the per-slot tx->tstamps[idx].cached_tstamp values out of sync with
the hardware?

During rebuild, the call chain is
ice_ptp_rebuild_owner()->ice_ptp_flush_all_tx_tracker()->
ice_ptp_flush_tx_tracker(). E810 reports every slot ready and
hw->reset_ongoing has already been cleared, so every in_use slot is
cleared in hardware:

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

ice_clear_phy_tstamp_e810() writes 0 to both halves of the slot. The
flush only clears skb, in_use and stale, so cached_tstamp keeps its old
non-zero value.

E810 has no ready bitmap. It detects a new capture by comparing the
register with cached_tstamp. If the slot is reused and read before the
new capture lands, the register reads 0, which does not match the cache.

In ice_ptp_process_tx_tstamp() the "not yet captured" check is then
skipped:

		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;

so the request is dropped without a timestamp.

ice_ptp_complete_tx_single_tstamp() has no valid bit check:

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

	tx->tstamps[idx].cached_tstamp = raw_tstamp;

Is 0 then taken as a fresh capture, extended through
ice_ptp_extend_40b_ts(), and delivered to the socket as a bogus Tx
timestamp?

Before this patch, and before 7a25fe5cd5fb, the tracker was either
freed or reallocated zeroed, so the cache and the hardware matched.
Should the flush also reset cached_tstamp?

[ ... ]

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

[ ... ]

> -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: High]
On the clock owner, ice_ptp_init_owner()->ice_ptp_create_clock() has
already registered the PHC before ice_ptp_init_work() and
ice_ptp_init_port() run. If ice_ptp_init_port() fails (for example with
-ENOMEM from ice_ptp_alloc_tx_tracker()), this unwind destroys the worker
and sets ptp->kworker to NULL. Only after that does err_exit call
ptp_clock_unregister().

Can a concurrent clock_settime() or clock_adjtime() on /dev/ptpN reach
the driver in that window? ice_ptp_settime64() and ice_ptp_adjtime() call
ice_ptp_reset_cached_phctime() without any state check:

ice_ptp_reset_cached_phctime() {
    ...
	err = ice_ptp_update_cached_phctime(pf);
	if (err) {
		...
		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
					   msecs_to_jiffies(10));
	}
    ...
}

That branch runs whenever ICE_CFG_BUSY is already held, for example by a
second concurrent settime or adjtime.

If this lands after ptp->kworker = NULL, is it a NULL pointer
dereference?

If it lands between kthread_cancel_delayed_work_sync() and
kthread_destroy_worker(), does it re-arm the delayed work timer on a
worker that is then freed? The timer callback would later touch the
freed worker.

ice_ptp_mark_tx_tracker_stale() also runs bitmap_or() on tx->stale
without checking tx->init. Could it race with
ice_ptp_release_tx_tracker() freeing the bitmaps when coming from
err_release_tx_tracker?

The new non-READY branch of ice_ptp_release() unregisters the clock
first. Should this error path use the same order?

The same ordering is still present in two other places. On the success
path the PHC is registered before ice_ptp_init_work() creates the
kworker. The READY branch of ice_ptp_release() still destroys the kworker
before ptp_clock_unregister().

The later patch "ice: use reference counting and SRCU for PTP port
access" adds a tx->init check to ice_ptp_mark_tx_tracker_stale(). It
does not change the kworker vs. unregister ordering.

> @@ -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: High]
This is a pre-existing issue, but at this commit the clock owner walks
pf->adapter->ports.ports without adapter->ports.lock in
ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
ice_ptp_prepare_rebuild_sec():

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

A peer PF's ice_ptp_cleanup_pf() removes its node under ports.lock. This
path then frees that peer's Tx tracker, kworker and ps_lock. Nothing
waits for the unlocked walkers, so can the owner still be using the
peer's port memory after it is freed?

The later patch in this series, "ice: use reference counting and SRCU
for PTP port access", seems to fix this. It converts these walkers to
SRCU with kref_get_unless_zero(), and ice_ptp_cleanup_pf() waits for
readers to drain before the tracker and kworker are freed.

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

[Severity: High]
This isn't a bug introduced by this patch, but adapter->ctrl_pf is never
cleared. ice_ptp_setup_adapter() sets it for the clock owner:

static void ice_ptp_setup_adapter(struct ice_pf *pf)
{
	pf->adapter->ctrl_pf = pf;
}

Neither branch of ice_ptp_release() resets it, and neither does the
ice_ptp_init() error path.

The ice_adapter is shared and refcounted across all PFs of the device.
The owner's ice_pf is freed when that PF unbinds. If only the owner PF is
unbound (for example through sysfs for VFIO passthrough) and the other
PFs stay bound, is adapter->ctrl_pf left pointing at freed memory?

Any user could then reach it by running ethtool -T on a remaining PF:

ice_get_ts_info()
  ice_ptp_clock_index()
    ice_get_ctrl_ptp()        <- &freed_pf->ptp
    ptp_clock_index(ctrl_ptp->clock)

Other users of the pointer are ice_ptp_setup_pf() on re-init and the
ice_txclk.c paths, which take ctrl_pf->dplls.lock and touch
ctrl_pf->ptp.tx_refclks. This is still unchanged at the end of the
series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 02/14] ice: use reference counting and SRCU for PTP port access

[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] drivers/net/ethernet/intel/ice/ice_adapter.h now embeds `struct
  srcu_struct srcu` by value in `struct ice_port_list` (line 32).

Pre-existing issues:
- [Medium] ice_ptp_prepare_rebuild_sec() (E825C owner reset path) holds
  only an SRCU read section and a kref on the peer port when it calls…

--

ice: use reference counting and SRCU for PTP port access

This commit replaces the mutex-protected adapter port list with an SRCU
protected list plus a spinlock for writers. It also adds a kref to each
ice_ptp_port. Readers take the kref with kref_get_unless_zero(), and
ice_ptp_cleanup_pf() waits for the last reference to drop before it calls
synchronize_srcu().

> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index 4f695f32da3d8..0b01c7f5cf0da 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -17,15 +17,19 @@ struct ice_pf;

[ ... ]

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

[Severity: Low]
Should ice_adapter.h include <linux/srcu.h> itself? struct ice_port_list
now embeds struct srcu_struct by value, so it needs the complete type. The
header only pulls in:

#include <linux/types.h>
#include <linux/mutex.h>
#include <linux/spinlock_types.h>
#include <linux/refcount_types.h>

#include "ice_type.h"

The patch adds <linux/srcu.h> to ice_adapter.c and ice_ptp.c instead. The
header seems to build today only through a transitive chain:

ice_type.h -> ice_osdep.h -> <net/udp_tunnel.h> -> ... -> <linux/notifier.h>

and notifier.h is what includes <linux/srcu.h>.

In the same patch, ice_ptp.h does get <linux/kref.h> for its new struct
kref member.

>  };
>  
>  /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index e1c66cd0f94ff..55241802d17ae 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -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)
>  {

[ ... ]

> +	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);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Can this
call to ice_ptp_prepare_for_reset(peer_pf) race with the peer PF's own
ice_ptp_release()?

Here the owner holds only an SRCU read section and a kref on the peer
port. ice_ptp_prepare_for_reset() then reads and writes peer_pf->ptp.state
with no lock held:

	if (ptp->state != ICE_PTP_READY)
		return;

	ptp->state = ICE_PTP_RESETTING;

	/* Disable timestamping for both Tx and Rx */
	ice_ptp_disable_timestamp_mode(pf);

	kthread_cancel_delayed_work_sync(&ptp->work);

At the same time, the peer can be in the READY path of ice_ptp_release().
That path does the following before it reaches ice_ptp_cleanup_pf():

	pf->ptp.state = ICE_PTP_UNINIT;

	/* Disable timestamping for both Tx and Rx */
	ice_ptp_disable_timestamp_mode(pf);

	ice_ptp_cleanup_pf(pf);

One possible interleaving:

owner (E825C reset)                    peer PF (unbind/remove)
ice_ptp_prepare_rebuild_sec()
  kref_get_unless_zero() succeeds
  ice_ptp_prepare_for_reset(peer_pf)
    sees state == ICE_PTP_READY
                                       ice_ptp_release()
                                         state = ICE_PTP_UNINIT
                                         ice_ptp_disable_timestamp_mode()
    state = ICE_PTP_RESETTING
    ice_ptp_disable_timestamp_mode()

Could the peer then be torn down with ptp.state left at ICE_PTP_RESETTING
instead of ICE_PTP_UNINIT? Both sides would also run
ice_ptp_disable_timestamp_mode() with nothing ordering them.

The new kref wait in ice_ptp_cleanup_pf() keeps the peer's kworker and Tx
tracker alive until the owner drops its reference, so the earlier
use-after-free window looks closed.

Before this patch, the same call ran with no locking at all. The commit
message also limits its scope to the port list.

^ permalink raw reply

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 03/14] ice: fix PHY port restart serialization

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (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] At this commit, ice_ptp_link_change() still returns early on
  `pf->ptp.state != ICE_PTP_READY` (ice_ptp.c:1312).

Pre-existing issues:
- [High] ice_ptp_release() runs its teardown without ps_lock and without
  serializing against ice_ptp_link_change().
- [Low] At this commit ptp_port->link_up is written with a plain store
  under ps_lock in ice_ptp_link_change() (ice_ptp.c:1319).

--

ice: fix PHY port restart serialization

This commit replaces the per-port ps_lock with a single ps_lock mutex in
struct ice_adapter. ice_ptp_link_change() now takes that lock to write
link_up and run the PHY restart. ice_ptp_restart_all_phy(), ice_ptp_init()
and ice_ptp_release() also take it around the PHY start/stop calls.

Closes: https://lore.kernel.org/netdev/20260916011228.1632848-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 55241802d17ae..d1cdd38ea27c8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -1314,12 +1314,13 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  
>  	ptp_port = &pf->ptp.port;
>  
> -	/* Update cached link status for this port immediately */
> +	mutex_lock(&pf->adapter->ps_lock);

[Severity: Medium]
Can a link edge still be lost here? Just above this hunk,
ice_ptp_link_change() still returns before it takes ps_lock and before it
writes link_up:

	if (pf->ptp.state != ICE_PTP_READY)
		return;

	ptp_port = &pf->ptp.port;

	mutex_lock(&pf->adapter->ps_lock);

ice_ptp_init() does its locked restart while the state is still
ICE_PTP_INITIALIZING, and sets READY only after that:

	mutex_lock(&pf->adapter->ps_lock);
	ice_ptp_port_phy_restart(&ptp->port);
	mutex_unlock(&pf->adapter->ps_lock);
	...
	ptp->state = ICE_PTP_READY;

A link event that arrives during init, or while the state is RESETTING,
is dropped without updating link_up. It does not wait on ps_lock. After
that, the init restart or ice_ptp_restart_all_phy() during rebuild
programs the PHY from a stale link_up value:

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

ice_link_event() does not replay the edge once old_link has been updated:

	if (link_up == old_link && link_speed == old_link_speed)

So on E82x/E825 the PHY timer could stay stopped, or be restarted from
stale state, until the next link toggle.

Doesn't this undercut the commit message's goal of setting link_up in the
same critical section as the restart?

The later patch in this series, "ice: call PTP link change only from link
events", appears to fix this. It writes link_up under ps_lock
unconditionally, checks the READY state inside the lock, and sets READY
before the init restart. Could that ordering be folded into this patch so
the fix stands on its own?

> +
>  	ptp_port->link_up = linkup;

[Severity: Low]
This is a pre-existing issue, but link_up is written here with a plain
store. ice_ptp_process_tx_tstamp() reads it without a lock from the misc
IRQ thread:

	/* Drop packets if the link went down */
	link_up = ptp_port->link_up;

Should these accesses use WRITE_ONCE()/READ_ONCE()? KCSAN would report
this as a data race. The only effect is a stale bool deciding whether a
batch of timestamps is dropped around a link edge.

The later patch "ice: keep Tx timestamp slots tracked until completion or
timeout" removes this read from ice_ptp_process_tx_tstamp(). In the final
series, all remaining accesses use READ_ONCE()/WRITE_ONCE().

[ ... ]

> @@ -3498,8 +3497,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;

[Severity: High]
This isn't a bug introduced by this patch, but here only phy_stop is
serialized against ice_ptp_link_change(). Can a concurrent link change
restart the PHY after release has torn things down?

ice_ptp_release() does three things outside ps_lock:

- sets pf->ptp.state = ICE_PTP_UNINIT with a plain store
- calls ice_ptp_release_tx_tracker() before taking the lock
- destroys pf->ptp.kworker after dropping the lock

ice_unload() calls ice_deinit_features()->ice_ptp_release() while the
netdev is still registered. The service task is only stopped later, in
ice_deinit_dev(). So ice_link_event(), ice_down() and ice_up_complete()
can still call ice_ptp_link_change() at this point.

The READY check in ice_ptp_link_change() runs before mutex_lock(), so a
caller that already passed it can block on ps_lock while release runs
phy_stop:

CPU1 (link event)                  CPU2 (ice_ptp_release)
ice_ptp_link_change()
  state == ICE_PTP_READY
                                   state = ICE_PTP_UNINIT
                                   mutex_lock(ps_lock)
  mutex_lock(ps_lock) blocks
                                   ice_ptp_port_phy_stop()
                                   mutex_unlock(ps_lock)
  ice_ptp_port_phy_restart()       kthread_destroy_worker(kworker)
                                   pf->ptp.kworker = NULL
    kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0)

On E82x, wouldn't this use the worker after it was destroyed, or
dereference NULL once kworker has been cleared?

"ice: call PTP link change only from link events" later in the series
moves the READY check under ps_lock, which closes the kworker case.

An E825 variant seems to remain in the final series, though:

- ice_ptp_link_change() can take ps_lock and see READY before release's
  unlocked UNINIT store.
- Its restart path then calls ice_ptp_flush_tx_tracker(pf, &ptp_port->tx),
  which walks tx->in_use and tx->tstamps without checking tx->init.
- At the same time, ice_ptp_release_tx_tracker() runs outside ps_lock and
  does kfree(tx->tstamps) and bitmap_free(tx->in_use).

Should the state change and the tracker and kworker teardown in
ice_ptp_release() also happen under ps_lock?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

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

[netdev-bot+sashiko]

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

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

- [High] The commit message says the new ice_ptp_link_change() call in
  ice_rebuild() (ice_main.c:7803-7808) exists 'to reflect the current…
- [Medium] This patch removes ice_ptp_link_change(pf, true) from
  ice_up_complete(), which leaves the PTP link state with no resync on…
- [Medium] The new PTP link_up seed is only in ice_init_link()
  (ice_main.c:4817).
- [Medium] ice_ptp_link_change() used to start with `if (pf->ptp.state !=
  ICE_PTP_READY) return;`.
- [Medium] ice_ptp_init() now sets `ptp->state = ICE_PTP_READY`
  (ice_ptp.c:3425) before the PHY restart and before…
- [Low] The comment above ice_ptp_rebuild() in ice_rebuild() still says
  'PTP init needs to happen before the VSI rebuild.

--

ice: call PTP link change only from link events

This patch stops calling ice_ptp_link_change() from ice_up_complete() and
ice_down(). Instead it seeds the PTP link_up field in ice_init_link() and
resyncs it from ice_rebuild(). ice_ptp_link_change() now records link_up
even when PTP is not ready, and ice_ptp_init() sets ICE_PTP_READY before
it restarts the PHY timestamping block.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3aa..b4ee2cf7cd704 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -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]
Does the devlink reload path need this seed too?

ice_init_link() is only reached from ice_init() on probe. The reload path
skips it:

ice_devlink_reinit_up()
  ice_init_hw()
  ice_init_dev()
  ice_init_pf()
  ice_load()
    ice_init_features()
      ice_ptp_init()
        ice_ptp_port_phy_restart()

ice_ptp_port_phy_restart() then uses whatever pf->ptp.port.link_up was
left from before the unload, because ice_ptp_release() does not reset it.

While the reload is in progress, the service task is stopped and the
control queues are shut down, so link events are lost. ice_init_hw() then
refreshes the link_info cache with ice_aq_get_link_info(), so no later
ice_link_event() sees a difference.

Before this patch, the ice_ptp_link_change(pf, true) call in
ice_up_complete() fixed a stale value on the next open. That call is now
gone. Say the link was down before devlink dev reload and came up during
the reload. Does PTP then stay at link_up == false until the next physical
link flap or reset? On E82x that means the PHY timer stays stopped and Tx
timestamps are dropped.

> @@ -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: Medium]
With this call removed, can PTP miss a link-up event that is still queued
when the interface is opened?

ice_open_internal() refreshes the cached link_info directly from firmware:

ice_open_internal() {
    ...
	pi = vsi->port_info;
	err = ice_update_link_info(pi);
    ...
}

ice_link_event() compares against that same cache. If nothing changed, it
returns before it reaches ice_ptp_link_change():

ice_link_event() {
    ...
	if (link_up == old_link && link_speed == old_link_speed)
		return 0;
    ...
	ice_ptp_link_change(pf, link_up);
    ...
}

Take an admin-down netdev whose PTP link_up is false from an earlier
link-down event. A link-up LSE has been posted to the ARQ but the service
task has not handled it yet:

ice_open()
  ice_open_internal()
    ice_update_link_info()     <- cache now says up
    ice_up_complete()          <- carrier on, PTP not told

service task
  ice_link_event()             <- up == up, returns early

Wouldn't pf->ptp.port.link_up stay false while the physical link is up?

On E82x, ice_ptp_port_phy_restart() would keep the PHY timer stopped.
ice_ptp_process_tx_tstamp() would drop Tx timestamps because link_up is
false. Later in the series the same link_up value gates Tx timestamp
requests, so those requests would be refused instead.

A later ifdown/ifup does not fix this either, because ice_down() and
ice_up_complete() no longer touch PTP. Only a link flap or a reset does.
This seems to contradict the commit message statement that "the link
status will reflect the current state of the physical link". Later
patches in the series leave ice_up_complete() unchanged.

[ ... ]

> @@ -7794,6 +7800,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: High]
When the netdev is administratively down, is link_info re-read from
firmware anywhere before this read?

The commit message says this call is there "to reflect the current link
status in case it changed and an edge was missed during a reset". However,
ice_ena_vsi() only calls ice_open_internal(), and through it
ice_update_link_info(), when netif_running(vsi->netdev) is true.

ice_update_pf_netdev_link() -> ice_get_link_status() only queries firmware
when pi->phy.get_link_info is set. Only the ethtool set_link_ksettings path
sets that flag. Nothing else in ice_rebuild() issues Get Link Status.

If so, for an admin-down interface this call just replays the pre-reset
state:

- the link is down before the reset (PTP link_up == false, PHY stopped)
- the link comes up during the reset, and the LSE is lost while the
  control queues are down
- the cache still says down, so this calls ice_ptp_link_change(pf, false)
- a later ice_open() refreshes the cache to up and turns the carrier on,
  but ice_up_complete() no longer notifies PTP

Wouldn't PTP then stay at link_up == false while the carrier is up, until
another link flap or reset?

The reverse case also looks possible. A stale "up" value makes
ice_ptp_link_change(pf, true) start the PHY timer before the link has
retrained. The real link-up LSE is then suppressed by the unchanged-cache
check in ice_link_event().

The end of the series does not change this code. Would calling
ice_update_link_info(pf->hw.port_info) before computing link_up fix this?

[Severity: Low]
This isn't a bug, but the comment above ice_ptp_rebuild() earlier in
ice_rebuild() still reads:

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

After this patch the VSI rebuild no longer sends PTP link status updates,
because ice_ptp_link_change() was removed from ice_up_complete() and
ice_down(). The ordering that matters now is that this new
ice_ptp_link_change() call runs after ice_ptp_rebuild() has set
ICE_PTP_READY. Should the comment say that instead?

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 11416e23dadeb..776f211290eca 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -1315,17 +1315,13 @@ 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;
>  
>  	mutex_lock(&pf->adapter->ps_lock);

[Severity: Medium]
The state check now comes after mutex_lock(), so every call dereferences
pf->adapter, even after ice_ptp_release() has set ICE_PTP_UNINIT. Can
this touch a freed ice_adapter during removal?

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

ice_remove() {
    ...
	ice_adapter_put(pdev);
	ice_deinit_hw(&pf->hw);

	ice_deinit_dev(pf);    <- ice_service_task_stop() happens here
    ...
}

ice_deinit() only sets ICE_SERVICE_DIS. It does not wait for a service
task that is already running. For the last PF on an adapter,
ice_adapter_put() can free the adapter. A service task already inside
ice_link_event() past the ice_get_main_vsi() check could then reach
ice_ptp_link_change() and lock the freed adapter's ps_lock. For example,
the task could be stalled in ice_set_link() AQ traffic.

Before this patch, the early state check made this a no-op once
ice_ptp_release() had run.

The race itself is older: the same window already has a pre-existing use
of the freed VSI from the concurrent service task. What this change adds
is the adapter access. At the end of the series the lock is still taken
unconditionally.

>  
> -	ptp_port->link_up = linkup;
> +	WRITE_ONCE(ptp_port->link_up, linkup);
>  
> -	/* Skip HW writes if reset is in progress */
> -	if (pf->hw.reset_ongoing)
> +	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
>  		goto out_unlock;

[ ... ]

> @@ -3426,7 +3422,9 @@ void ice_ptp_init(struct ice_pf *pf)
>  	if (err)
>  		goto err_release_tx_tracker;
>  
> -	/* Start the PHY timestamping block */
> +	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);
> @@ -3434,8 +3432,6 @@ void ice_ptp_init(struct ice_pf *pf)
>  	/* Configure initial Tx interrupt settings */
>  	ice_ptp_cfg_tx_interrupt(pf);
>  
> -	ptp->state = ICE_PTP_READY;
> -

[Severity: Medium]
Setting ICE_PTP_READY before ice_ptp_cfg_tx_interrupt() runs: can
ndo_hwtstamp_set now race with that call?

ice_ptp_init() runs from ice_load() -> ice_init_features() after
ice_register_netdev(), so the netdev is already visible. The only check in
ice_ptp_hwtstamp_set() is:

	if (pf->ptp.state != ICE_PTP_READY)
		return -EAGAIN;

Both paths end up in ice_ptp_cfg_tx_interrupt(). The user path gets there
through ice_ptp_set_timestamp_mode() -> ice_ptp_restore_timestamp_mode().
The function computes enable from pf->ptp.tstamp_config.tx_type and then
does an unlocked read-modify-write:

	val = rd32(hw, PFINT_OICR_ENA);
	if (enable)
		val |= PFINT_OICR_TSYN_TX_M;
	else
		val &= ~PFINT_OICR_TSYN_TX_M;
	wr32(hw, PFINT_OICR_ENA, val);

ice_ptp_init() runs under devl_lock and the ndo runs under rtnl/netdev
locking, so nothing keeps the two apart. In ICE_PTP_TX_INTERRUPT_SELF
mode:

ice_ptp_init()                        ice_ptp_hwtstamp_set()
  enable = false (tx_type OFF)
                                        tx_type = HWTSTAMP_TX_ON
                                        sets PFINT_OICR_TSYN_TX_M
  rd32(PFINT_OICR_ENA)
  clears PFINT_OICR_TSYN_TX_M
  wr32(PFINT_OICR_ENA)

Wouldn't the config then report HWTSTAMP_TX_ON while the Tx timestamp
interrupt is masked? Tx timestamps would then time out until the next
hwtstamp_set or reset.

Before this patch, hwtstamp_set returned -EAGAIN until init had finished
ice_ptp_cfg_tx_interrupt(). The end of the series keeps the new ordering.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 06/14] ice: E822: keep Tx timestamps disabled during offset calibration

[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 kernel-doc for ice_ptp_tx::calibrating (ice_ptp.h:109-110)
  still says "if true, the PHY is calibrating the Tx offset".

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  ice_ptp_settime64() leaves the E82x PHY offset-ready bits cleared with…

--

ice: E822: keep Tx timestamps disabled during offset calibration

ice_ptp_port_phy_restart() no longer clears tx.calibrating right after
ice_start_phy_timer_e82x() succeeds. The flag is now cleared in
ice_ptp_wait_for_offsets(), once ice_phy_cfg_tx_offset_e82x() has
succeeded. The patch also adds debug messages for pending and completed
offset calibration.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 776f211290eca..828d46a73f292 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -1283,11 +1301,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 calibrating 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;

[Severity: Medium]
This is a pre-existing issue, and this patch did not introduce it. ov_work
is the only thing that re-arms the E82x PHY offsets. Can
ice_ptp_settime64() leave Tx and Rx timestamps invalid on every port if
ice_ptp_lock() fails?

ice_ptp_settime64() clears the offset-ready bits first. It can then bail
out before the PHYs are restarted:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_settime64() {
    ...
	if (hw->mac_type == ICE_MAC_GENERIC) {
		err = ice_ptp_clear_phy_offset_ready_e82x(hw);
		...
	}

	if (!ice_ptp_lock(hw)) {
		err = -EBUSY;
		goto exit;
	}
    ...
	if (hw->mac_type == ICE_MAC_GENERIC)
		ice_ptp_restart_all_phy(pf);
exit:
    ...
}

ice_ptp_clear_phy_offset_ready_e82x() writes 0 to P_REG_TX_OR and
P_REG_RX_OR on every port. ice_ptp_lock() polls the shared PFTSYN_SEM up
to MAX_TRIES times, with usleep_range(5000, 6000) between polls, and then
gives up. So this path can be hit if another function holds the semaphore
for that long.

The goto skips ice_ptp_restart_all_phy(). That is the only route to
ice_ptp_port_phy_restart(), and so the only way ov_work gets queued. If
ov_work had already finished, nothing calls
ice_phy_cfg_tx_offset_e82x() or ice_phy_cfg_rx_offset_e82x() again.

Would every Tx and Rx timestamp then be marked invalid and dropped by the
ICE_PTP_TS_VALID checks? That would last until a link flap, a reset, or a
later successful settime. tx.calibrating stays false the whole time, so
Tx timestamp requests are still accepted.

Could the offset-ready clear move after the lock is taken, or could the
error path still restart the PHYs? This still looks present at the end of
the series.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index 27ea502b75768..029ee4612d765 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -107,7 +107,7 @@ enum ice_tx_tstamp_work {
>   * @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.
> + *               window, timestamp requests are disabled.

[Severity: Low]
This isn't a bug, but is the first half of this kernel-doc still accurate
after this patch?

On E822, ice_ptp_wait_for_offsets() is now the only place that clears the
flag, so it can stay true while nothing is calibrating. Say link goes down
before ice_phy_cfg_tx_offset_e82x() has succeeded. ice_ptp_port_phy_stop()
then cancels ov_work and stops the PHY timer without touching the flag:

	case ICE_MAC_GENERIC:
		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);

		err = ice_stop_phy_timer_e82x(hw, port, true);
		break;

Before this patch that could not happen, because the flag was cleared
right after a successful start. The same is true when
ice_start_phy_timer_e82x() fails: ice_ptp_port_phy_restart() breaks out
with calibrating still set and no ov_work queued.

Later in the series, the E825 ice_ptp_port_phy_stop() path also sets
calibrating to true on link-down. That moves the meaning further from
this description.

This has no functional effect. The next ice_ptp_port_phy_restart() sets
the flag again and requeues ov_work.

The flag now reads more like "Tx timestamp requests are blocked until the
Tx offset has been configured" than "the PHY is calibrating". The
"temporarily disable Tx timestamps while calibrating PHY offset" comment
in ice_ptp_port_phy_restart() has the same mismatch.

Could both comments be updated to match the new behaviour?

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

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

[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] Possible requeue of ov_work after the new cancel (judged harmless
  and by design).

--

ice: E822: cancel offset verification work during reset preparation

This patch cancels the E822 offset verification work (ov_work) in
ice_ptp_prepare_for_reset(). Without the cancel, the work can keep running
or rescheduling itself while a reset is in progress. The PHY gets
reconfigured after the reset anyway, either by the clock owner or by
ice_ptp_link_change() at the end of the rebuild.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 828d46a73f292..6297773806949 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -3006,6 +3006,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>
>  	kthread_cancel_delayed_work_sync(&ptp->port.work);
>
> +	if (hw->mac_type == ICE_MAC_GENERIC)
> +		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);
> +

[Severity: Low]
This is a minor observation and it may well be intended. This cancel runs
without adapter->ps_lock held, and nothing stops ov_work from being queued
again afterwards. Can another PF queue a peer's ov_work again while that
peer is still resetting?

On a multi-PF E822 adapter, the clock owner can reach this producer for
every port that has link_up set:

ice_ptp_rebuild_owner() / ice_ptp_settime64()
  ice_ptp_restart_all_phy()
    ice_ptp_port_phy_restart(port)
      kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work, 0);

ice_ptp_restart_all_phy() checks only this before restarting the peer's PHY:

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

It does not look at the peer's ptp.state or its reset state.

The impact looks small. A requeued ice_ptp_wait_for_offsets() sees
ice_is_reset_in_progress() and only reschedules, with no hardware access:

	if (ice_is_reset_in_progress(pf->state)) {
		/* wait for device driver to complete reset */
		kthread_queue_delayed_work(pf->ptp.kworker,
					   &port->ov_work,
					   msecs_to_jiffies(100));
		return;
	}

Each PF's ice_rebuild() also calls ice_ptp_link_change(). That path goes
through ice_ptp_port_phy_restart(), which cancels and requeues the work.

The commit message also says the PHY is reconfigured "as part of the clock
owner resetting", so the requeue may be the intended recalibration path. If
so, the only effect left is a few no-op 100 ms reschedules during a reset.

Is that the expected behaviour, or should the restart path skip ports whose
PF is in reset?

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 08/14] 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 kernel-doc no longer matches how the code uses `struct
  ice_ptp_tx::calibrating` and `ice_ptp_mark_tx_tracker_stale()`.
- [Low] The new dev_err in `ice_ptp_port_phy_restart()`
  (ice_ptp.c:1347-1349) says "Timestamping remains disabled", which is…

--

ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY, because clearing it puts the hardware Tx timestamp
counter out of sync. Instead, the PHY stop and restart paths set
tx.calibrating to block new Tx timestamp requests, and mark outstanding
timestamps stale. The unused soft_reset parameter is removed and the
restart failure message is reworded.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 6297773806949..365cff9c10173 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -818,7 +818,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);
>  }
>  

[ ... ]

> @@ -1243,7 +1245,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.calibrating = true;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

[Severity: Low]
This isn't a bug, but should the kernel-doc for the calibrating field be
updated to match how E825 now uses it? In ice_ptp.h, struct ice_ptp_tx
still says:

 * @calibrating: if true, the PHY is calibrating the Tx offset. During this
 *               window, timestamp requests are disabled.

After this patch, E825 sets the bit in ice_ptp_port_phy_stop() on link
down, where no calibration runs. The commit message also says E825 "does
not perform vernier calibration".

ice_ptp_port_phy_restart() leaves the bit set on failure, so it acts as a
"timestamping disabled until link toggle" state. ice_ptp_release() also
leaves the bit set after teardown.

The kernel-doc for ice_ptp_mark_tx_tracker_stale() has the same problem. It
still says:

 * This should be called when the PTP clock is modified such as after a set
 * time request.

It doesn't mention the new PHY stop and restart callers. It also doesn't say
that the helper now does nothing when tx->init is clear. The teardown order
in ice_ptp_release() depends on that check, because the tracker bitmaps have
already been freed:

ice_ptp_release()
    ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
    ...
    ice_ptp_port_phy_stop(&pf->ptp.port);
        ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

Could both comments be updated to describe the new behavior?

> +
> +		err = ice_stop_phy_timer_eth56g(hw, port);
>  		break;

[ ... ]

> @@ -1309,15 +1318,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)

[ ... ]

>  		err = ice_start_phy_timer_eth56g(hw, port);
> +		if (err)
> +			break;
> +
> +		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +		ptp_port->tx.calibrating = false;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
>  		break;
>  	default:

[ ... ]

>  	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 remains disabled; A link-toggle may recover.\n",
> +			port, ERR_PTR(err));

[Severity: Low]
Is "Timestamping remains disabled" accurate for Rx on E825 in every
failure case?

Tx is now blocked only in software, through tx.calibrating. Rx is blocked
only in hardware, through PHY_REG_RX_OFFSET_READY.

Suppose the first register write in ice_start_phy_timer_eth56g() fails:

ice_start_phy_timer_eth56g()
    err = ice_stop_phy_timer_eth56g(hw, port);
    if (err)
        return err;

PHY_REG_RX_OFFSET_READY then keeps its previous value, for example 1 from
an earlier successful start. The hardware may keep marking Rx descriptor
timestamps valid, even though ice_sync_phy_timer_eth56g() never ran.

If any later step fails, RX_OFFSET_READY has already been cleared and the
message is accurate. The early return is not new; only the message's
absolute claim is.

Could the message be more precise here, for example by saying that Tx
timestamping remains disabled?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 09/14] 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 patch puts the read…
- [Low] The concern is that the kernel-doc of ice_ptp_reset_ts_memory()
  ('Reset timestamp memory for all blocks') no longer matches the code,…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This patch replaces the E825 timestamp memory reset, which wrote
PHY_REG_TX_MEMORY_STATUS and had no effect, with a sweep that reads every
PHY_TSTAMP index on the port. The sweep now runs at the start of
ice_ptp_phy_soft_reset_eth56g(), so the soft reset that follows resets the
internal counter. ice_ptp_reset_ts_memory() no longer does anything for
E825.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dde..38d0ded1d0cc0 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 runs before any of the PHY_REG_GLOBAL accesses that can fail.
What happens to the internal counter if one of those later steps fails?

ice_ptp_clear_tx_memory_status_eth56g() returns void and always reads
every index. After it runs, ice_ptp_phy_soft_reset_eth56g() can still
return early. That can happen at the PHY_REG_GLOBAL read or at any of the
three PHY_REG_GLOBAL writes:

	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
	if (err) {
		...
		return err;
	}
	...
	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, global_val);
	if (err) {
		...
		return err;
	}

The commit message says reading a garbage entry after a cold power-on can
underflow the counter, and only the soft reset sets it back to zero. If
the reset pulse is never issued, or only partly issued, is the port then
left in the state that the new kernel-doc of
ice_ptp_clear_tx_memory_status_eth56g() warns about?

    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.

ice_ptp_init_phc_e825c() then stops at the first port that fails:

	for (int port = 0; port < hw->ptp.num_lports; port++) {
		err = ice_ptp_phy_soft_reset_eth56g(hw, port);
		if (err) {
			...
			return err;
		}
	}

That port stays swept but not reset, and ice_ptp_init_owner() and
ice_ptp_rebuild_owner() only return the error. Before this patch, a failed
soft reset could not cause this desync, because nothing destructive ran
before it.

Later in the series, ice_start_phy_timer_eth56g() also calls
ice_ptp_phy_soft_reset_eth56g() every time the PHY timer starts, so this
path runs more often.

Could the PHY_REG_GLOBAL read be done before the sweep? Alternatively,
could the reset be completed or retried once the sweep has run?

[ ... ]

> @@ -5802,8 +5805,9 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
>  		ice_ptp_reset_ts_memory_e82x(hw);
>  		break;
>  	case ICE_MAC_GENERIC_3K_E825:
> -		ice_ptp_reset_ts_memory_eth56g(hw);
> -		break;
> +		/* E825 hardware must only reset timestamp memory as part of
> +		 * the soft reset procedure.
> +		 */
>  	case ICE_MAC_E810:
>  	default:
>  		return;

[Severity: Low]
This isn't a bug, but the kernel-doc for ice_ptp_reset_ts_memory() still
reads:

 * ice_ptp_reset_ts_memory - Reset timestamp memory for all blocks

E825 is now a no-op here, like the E810 and default (E830) branches. The
only explanation is the inline comment.

Could the kernel-doc list which MAC types are skipped? It could also say
that E825 clears its memory through ice_ptp_phy_soft_reset_eth56g()
instead.

This doesn't change how ice_ptp_cfg_phy_interrupt() behaves. Both
ice_ptp_init_owner() and ice_ptp_rebuild_owner() call ice_ptp_init_phc()
before it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

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

[netdev-bot+sashiko]

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

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

- [High] This patch makes the E825 PHY restart path in
  ice_ptp_port_phy_restart()…
- [Medium] On E825, ice_ptp_port_phy_restart() now calls
  ice_ptp_flush_tx_tracker() on a live tracker without synchronizing…
- [Medium] ice_ptp_port_phy_restart() on E825 now flushes, i.e. releases
  in software, every outstanding Tx timestamp index right away.
- [Low] The kernel-doc for struct ice_ptp
  (drivers/net/ethernet/intel/ice/ice_ptp.h:250) still says…

--

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

This commit adds a PHY soft reset to ice_start_phy_timer_eth56g() so the
E825 PHY timer starts in a clean state. Because the restart path now
performs a soft reset, ice_ptp_port_phy_restart() flushes outstanding Tx
timestamps instead of marking them stale.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 365cff9c10173..32be908373c30 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -756,7 +756,8 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
>   * @pf: Board private structure
>   * @tx: the tracker to flush
>   *
> - * Called during teardown when a Tx tracker is being removed.
> + * Called to immediately discard any outstanding Tx timestamps, including when
> + * tearing down the Tx tracker, and on E825 when restarting the PHY.
>   */

[Severity: Low]
This isn't a functional bug, but should the kernel-doc for
tx_hwtstamp_flushed in struct ice_ptp also be updated? ice_ptp.h still
says:

 * @tx_hwtstamp_flushed: number of Tx skbs flushed due to interface closed

On E825, the flush is now called from ice_ptp_port_phy_restart(). Every
PHY restart that has outstanding timestamps now increments this counter.
That covers link-up via ice_ptp_link_change(), the initial restart in
ice_ptp_init(), and ice_ptp_restart_all_phy() during owner rebuild. None
of these is an interface close.

The counter is exported as tx_hwtstamp_flushed in ethtool -S. It is also
added into the standard timestamping err stat in ice_get_ts_stats():

	ts_stats->err = ptp->tx_hwtstamp_skipped +
			ptp->tx_hwtstamp_flushed +
			ptp->tx_hwtstamp_discarded;

As a result, link-up flushes now show up as timestamp errors. Before this
patch, stale-marked requests were dropped without being counted in err.

[ ... ]

> @@ -1327,7 +1328,7 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
>  		ptp_port->tx.calibrating = true;
>  		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
>  
> -		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
> +		ice_ptp_flush_tx_tracker(pf, &ptp_port->tx);

[Severity: High]
Does this drop the tx->init protection that this call site had before?

ice_ptp_mark_tx_tracker_stale() only touches the bitmaps under tx->lock,
and only when tx->init is set:

	spin_lock_irqsave(&tx->lock, flags);
	if (tx->init)
		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
	spin_unlock_irqrestore(&tx->lock, flags);

An earlier commit in this series, "ice: E825: stop clearing
PHY_REG_TX_OFFSET_READY", added that guard here to prevent a possible
use-after-free on the bitmap.

ice_ptp_flush_tx_tracker() never checks tx->init. It walks tx->in_use
without tx->lock and dereferences tx->tstamps[idx]. It also makes
sleeping sideband calls (ice_get_phy_tx_tstamp_ready(),
ice_clear_phy_tstamp()) while it does so.

Can this race with ice_ptp_release() during driver unload or devlink
reload? The service task path:

ice_link_event()
  ice_ptp_link_change()
    mutex_lock(&pf->adapter->ps_lock)
    pf->ptp.state == ICE_PTP_READY check passes
    ice_ptp_port_phy_restart()
      ice_ptp_flush_tx_tracker()

can run at the same time as:

ice_unload()
  ice_deinit_features()
    ice_ptp_release()
      pf->ptp.state = ICE_PTP_UNINIT    (ps_lock not held)
      ice_ptp_cleanup_pf()
      ice_ptp_release_tx_tracker()
        tx->init = 0
        synchronize_irq(pf->oicr_irq.virq)
        ice_ptp_flush_tx_tracker()
        kfree(tx->tstamps); tx->tstamps = NULL;
        bitmap_free(tx->in_use); tx->in_use = NULL;

synchronize_irq() waits only for the interrupt, not for the service
task. ice_service_task_stop() runs later, in ice_deinit_dev().

If an E825 link-up or speed change is processed during unload, could the
restart's flush read and write tx->in_use and tx->tstamps after they
have been freed or set to NULL?

The commit message doesn't mention losing this guard. The unguarded
flush in ice_ptp_port_phy_restart() is still there at the end of the
series.

[Severity: Medium]
Is this flush synchronized with the clock owner's Tx timestamp
processing?

ice_ptp_process_tx_tstamp() runs from
ice_misc_intr_thread_fn()->ice_ptp_process_ts()->ice_ptp_tx_tstamp_owner()
without adapter->ps_lock. It reads tstamp_ready, the in_use bit and
raw_tstamp without tx->lock. It takes the lock only at skip_ts_read,
where it claims whatever skb is in the slot at that moment:

skip_ts_read:
		spin_lock_irqsave(&tx->lock, flags);
		...
		clear_bit(idx, tx->in_use);
		skb = tx->tstamps[idx].skb;
		tx->tstamps[idx].skb = NULL;
		if (test_and_clear_bit(idx, tx->stale))
			drop_ts = true;
		spin_unlock_irqrestore(&tx->lock, flags);

Could the following sequence deliver a pre-reset timestamp to a
different packet?

owner IRQ thread:
  ice_ptp_process_tx_tstamp() reads raw_tstamp for skb A at idx,
  then is delayed

restart path:
  ice_ptp_flush_tx_tracker() frees A, clears in_use[idx] and stale[idx]
  ice_start_phy_timer_eth56g() soft reset (at least 10ms of sleeps)
  ptp_port->tx.calibrating = false

xmit path:
  ice_ptp_request_ts() reuses idx (lowest free index) for skb B

owner IRQ thread resumes:
  takes tx->lock, claims B, finds no stale bit,
  extends A's pre-reset value and calls skb_tstamp_tx() on B

There is also a variant. The thread uses a tstamp_ready snapshot taken
before the reset and reads the now-cleared register for B's index. It
then drops B for missing the valid bit. B's real capture stays ready
with no in_use owner. The comment in ice_ptp_process_tx_tstamp() warns
that this kind of state can leave interrupt generation stuck.

Before this patch, the restart only marked indices stale. The processing
thread then kept ownership of each index until it cleared in_use under
tx->lock. The other live-tracker flush, in ice_ptp_release_tx_tracker(),
first sets tx->init = 0 and calls synchronize_irq().

The flush also increments tx_hwtstamp_flushed for indices whose skb the
processing thread has already taken, when skb is NULL.

>  
>  		err = ice_start_phy_timer_eth56g(hw, port);
>  		if (err)
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 38d0ded1d0cc0..51442b26516ee 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]
Can Tx captures land after the memory sweep, into slots that software
has already released in ice_ptp_port_phy_restart()?

The restart flushes every in_use index immediately. Packets that already
hold those indices may still be in the hardware Tx ring with
ICE_TX_CTX_DESC_TSYN set and the index encoded by ice_tstamp().

Setting tx.calibrating only blocks new ice_ptp_request_ts() calls. Also,
ice_stop_phy_timer_eth56g() leaves PHY_REG_TX_OFFSET_READY set:

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

ice_ptp_phy_soft_reset_eth56g() runs the sweep first and sleeps before
it asserts the reset:

	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
	...
	usleep_range(5000, 6000);

	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;

If a capture lands before the reset is asserted, the counter and status
are zeroed while the memory still holds a valid entry. If it lands
after, there is a ready bit with no in_use owner.

ice_ptp_process_tx_tstamp() only walks tx->in_use, so that slot is never
read. At this commit, ice_ptp_tx_tstamps_pending() also checks the
hardware ready bitmap, so it would keep re-arming the interrupt.

Later, ice_ptp_request_ts() hands that index to a new skb C. Wouldn't
the processing loop then trust the old ready bit and deliver packet A's
capture to C? C's own capture would then be orphaned, and this would
repeat for each later user of the index until the next restart.

The commit message says:

    This is safe because the soft reset procedure will restore the
    timestamp state and clear any desync.

That seems to hold only for captures that land before the sweep. The
previous stale-marking approach kept these indices in_use until the late
capture was read and discarded, or until the 2 second timeout.

This patch also applies the soft reset to live ports: link-up, up to up
speed changes via ice_link_event(), and ice_ptp_restart_all_phy() on
other PFs' ports during owner rebuild.

A later commit in this series, "ice: keep Tx timestamp slots tracked
until completion or timeout", describes hardware completing captures
after software released the slot around a link bounce. The flush in
ice_ptp_port_phy_restart() is unchanged at the end of the series.

>  	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/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 11/14] ice: wait for in-flight Tx timestamps before removing the tracker

[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] On E830, the new drain wait runs, but the flush that follows
  cannot clear the ready entries it waited for.
- [Medium] The tracker is released while the PF's Tx path is still live,
  and the new drain wait gives up after 10 ms.

--

ice: wait for in-flight Tx timestamps before removing the tracker

This patch adds ice_ptp_wait_for_tracker_drain(). It polls the PHY Tx
timestamp ready bitmap for up to 10 ms, until every in-use tracker index
reports ready. ice_ptp_release_tx_tracker() now calls it before
ice_ptp_flush_tx_tracker(), so in-flight captures get cleared instead of
being left as orphaned ready bits, and E810 is skipped via
tx->has_ready_bitmap.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 32be908373c30..ca581490f3edb 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -751,6 +751,68 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)

[ ... ]

> +static void
> +ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
> +{
> +	bool drained;
> +	int err;
> +
> +	if (!tx->has_ready_bitmap)
> +		return;

[Severity: Medium]
Does this wait do anything useful on E830? ice_ptp_init_tx() sets:

	tx->has_ready_bitmap = pf->hw.mac_type != ICE_MAC_E810;

E830 passes this check. ice_ptp_is_tracker_drained() then polls the
E830 valid bits through ice_get_phy_tx_tstamp_ready_e830() for up to
10 ms.

The flush that follows calls ice_clear_phy_tstamp() for each ready
index, and that function has no E830 case:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_clear_phy_tstamp() {
	switch (hw->mac_type) {
	case ICE_MAC_E810:
		return ice_clear_phy_tstamp_e810(hw, block, idx);
	case ICE_MAC_GENERIC:
		return ice_clear_phy_tstamp_e82x(hw, block, idx);
	case ICE_MAC_GENERIC_3K_E825:
		return ice_clear_ptp_tstamp_eth56g(hw, block, idx);
	default:
		return -EOPNOTSUPP;
	}
}

ice_ptp_flush_tx_tracker() ignores the return value, so the failure is
silent.

The commit message says:

  Only later devices (E822, E825, E830) have the bitmap and potential
  issues with internal tracking.

On E830, won't the valid bits this wait collected still be set after
the flush? If so, the patch adds up to 10 ms of latency there without
fixing the desync.

ice_read_phy_tstamp() already has an E830 case that reads
E830_PRTTSYN_TXTIME_H/L, but the clear path never uses it. Should
ice_clear_phy_tstamp() get an E830 case, or should E830 skip the wait?
The E830 case is still missing at the end of the series.

> +
> +	err = read_poll_timeout(ice_ptp_is_tracker_drained,
> +				drained, drained, 500, 10 * USEC_PER_MSEC, false,
> +				pf, tx);
> +	if (err) {
> +		dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
> +			tx->block);
> +	}
> +}

[ ... ]

> @@ -868,6 +930,8 @@ ice_ptp_release_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>  	/* wait for potentially outstanding interrupt to complete */
>  	synchronize_irq(pf->oicr_irq.virq);
>  
> +	ice_ptp_wait_for_tracker_drain(pf, tx);
> +
>  	ice_ptp_flush_tx_tracker(pf, tx);

[Severity: Medium]
Is the Tx path still live when this runs? On the remove and devlink
reload paths the order is:

ice_unload()
  ice_deinit_features()
    ice_ptp_release()
      ice_ptp_release_tx_tracker()
        ice_ptp_wait_for_tracker_drain()
        ice_ptp_flush_tx_tracker()
  ...
  ice_unregister_netdev()

Nothing quiesces the Tx queues before the tracker is released.
ice_ptp_release() masks PFINT_OICR_TSYN_TX and turns off Rx
timestamping. Clearing tx->init only stops ice_ptp_request_ts() from
handing out new indexes.

Two cases seem to slip past the 10 ms wait:

  ice_tstamp() has already reserved an index through
  ice_ptp_request_ts(), but the TSYN context descriptor has not been
  posted yet.

  A TSYN descriptor stays in the hardware Tx ring for more than 10 ms,
  for example at low link speed, under PFC/pause backpressure, or with
  rate limiting.

In either case read_poll_timeout() times out with only a dev_dbg().
ice_ptp_flush_tx_tracker() then sees the index as not ready, skips
ice_clear_phy_tstamp(), and frees the entry. When the frame finally
goes out, the PHY sets a ready bit (Q_REG_TX_MEMORY_STATUS on E82x)
that no tracker owns.

On E82x that quad memory is shared with the other ports of the quad,
possibly including ports owned by other PFs. ice_ptp_process_tx_tstamp()
only walks in_use, so nothing ever reads the orphaned entry.

Isn't that the same "delivery for the whole quad degrades to the
periodic work" situation the commit message describes? Would it be more
robust to flush after Tx has been stopped, instead of relying on a fixed
timeout? The ordering is the same at the end of the series.

>  
>  	kfree(tx->tstamps);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v3 12/14] ice: keep Tx timestamp slots tracked until completion or timeout

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 2 (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 commit message says this patch adds the tracker-init
  check: "Since we're adding a new call to ice_ptp_mark_tx_tracker,…
- [Medium] ice_ptp_link_change() now calls
  ice_ptp_mark_tx_tracker_stale() on every link-down, before the…
- [Low] The concern says the kernel-doc of
  ice_ptp_mark_tx_tracker_stale() ('should be called when the PTP clock…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but in the
  ICE_PTP_READY branch ice_ptp_release() calls…
- [Medium] The concern: the 2 s timeout in ice_ptp_process_tx_tstamp()
  releases a not-ready index without reading it.

--

ice: keep Tx timestamp slots tracked until completion or timeout

This commit stops dropping outstanding Tx timestamp requests when the
link goes down. Instead it marks them stale, rejects new requests while
the link is down, and frees a slot that is not ready only after the 2
second timeout. It also changes the pending-timestamp check used when
re-arming the IRQ and in ice_ptp_maybe_trigger_tx_interrupt().

> Since we're adding a new call to ice_ptp_mark_tx_tracker, notice that the
> function previously did not check the tracker initialization state. Fix
> this by only accessing the bitmaps if the init field is set.

[Severity: Medium]
Is this paragraph accurate for this patch? The diff does not modify
ice_ptp_mark_tx_tracker_stale(). The if (tx->init) guard in it comes
from the earlier commit "ice: E825: stop clearing
PHY_REG_TX_OFFSET_READY".

The message also names the function ice_ptp_mark_tx_tracker instead of
ice_ptp_mark_tx_tracker_stale.

This patch has Fixes: fcc2cef37fed, so it is likely to be picked for
stable. A backport of this patch alone would add the new
ice_ptp_link_change() caller without the guard. Then a link-down after a
failed tracker allocation would call bitmap_or() on NULL in_use and
stale bitmaps.

Could the message say that the guard is a prerequisite from the other
commit, so the two get backported together?

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index ca581490f3edb..57141e94c5708 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;
>  
>  		/* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()

[Severity: Medium]
This isn't a bug introduced by this patch, but the 2 second timeout
later in ice_ptp_process_tx_tstamp() still releases an index that is not
ready, without reading it:

		if (tx->has_ready_bitmap &&
		    !(tstamp_ready & BIT_ULL(phy_idx))) {
			if (drop_ts)
				goto skip_ts_read;

Suppose the hardware holds a timestamp-requested frame for more than 2
seconds, for example during a long pause or PFC stall. Could the capture
then land in an index that a new request has reused, or leave a ready
bit with no in_use owner?

That seems to conflict with this statement in the commit message: "This
way an index is never reused while the hardware can still write it".

For the link-down case itself, the device counts frames dropped while
the link is down (GLPRT_TDOLD), so that case looks less likely to hit
this.

[ ... ]

> @@ -1432,6 +1431,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  
>  	WRITE_ONCE(ptp_port->link_up, linkup);
>  
> +	if (!linkup)
> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
> +
>  	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
>  		goto out_unlock;
>  

[Severity: Medium]
Can this take an uninitialized spinlock on link down?

ice_ptp_mark_tx_tracker_stale() takes tx->lock before it checks
tx->init:

	spin_lock_irqsave(&tx->lock, flags);
	if (tx->init)
		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
	spin_unlock_irqrestore(&tx->lock, flags);

The only spin_lock_init() of tx->lock is in ice_ptp_alloc_tx_tracker().
That is reached only through ice_ptp_init() -> ice_ptp_init_port(), and
ice_init_features() calls ice_ptp_init() only when
ICE_FLAG_PTP_SUPPORTED is set and the driver is not in safe mode.

ice_link_event() calls ice_ptp_link_change() without checking
ICE_FLAG_PTP_SUPPORTED, and the new call comes before the ICE_PTP_READY
check. Consider a PF without 1588 support, a PF in safe mode, or a PF
where ice_ptp_init() failed before reaching ice_ptp_init_port(). Every
link-down there would do:

ice_link_event()
  ice_ptp_link_change(pf, false)
    ice_ptp_mark_tx_tracker_stale()
      spin_lock_irqsave(&tx->lock, flags)

on a lock that was never initialized. With CONFIG_DEBUG_SPINLOCK this
gives "spinlock bad magic". With lockdep it gives "trying to register
non-static key" and lockdep turns itself off.

Also, ice_ptp_alloc_tx_tracker() sets tx->init = 1 before it calls
spin_lock_init(&tx->lock).

Should this new call depend on PTP having been initialized, for example
by moving it after the ICE_PTP_READY check?

[Severity: Low]
This isn't a bug, but the kernel-doc for ice_ptp_mark_tx_tracker_stale()
still says:

 * This should be called when the PTP clock is modified such as after a set
 * time request.

Now that ice_ptp_link_change() also calls it on link down, should the
comment mention that use as well?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch.

ice_ptp_release() destroys the PTP kworker before it unregisters the
PHC:

	if (pf->ptp.kworker) {
		kthread_destroy_worker(pf->ptp.kworker);
		pf->ptp.kworker = NULL;
	}

	if (!pf->ptp.clock)
		return;

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

	ptp_clock_unregister(pf->ptp.clock);

Neither ice_ptp_settime64() nor ice_ptp_adjtime() checks ptp.state, and
on success they call ice_ptp_reset_cached_phctime(). If
ice_ptp_update_cached_phctime() returns -EAGAIN because ICE_CFG_BUSY is
held, that function then does:

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

ICE_CFG_BUSY can be held by a concurrent settime or adjtime on the same
clock, or by ethtool set_link_ksettings or set_ringparam.

Could a clock_settime() on /dev/ptpX that races with PF removal queue
delayed work on a worker that kthread_destroy_worker() is about to free?
Could it instead call kthread_queue_delayed_work() with a NULL kworker?

The ethtool paths can still run at this point, because ice_unload()
tears down PTP before it unregisters the netdev:

ice_unload()
  ice_deinit_features()
    ice_ptp_release()
      kthread_destroy_worker()
      ...
      ptp_clock_unregister()
  ...
  ice_unregister_netdev()

Would unregistering the clock before destroying the worker close this
window?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] In ice_ptp_maybe_trigger_tx_interrupt()
  (drivers/net/ethernet/intel/ice/ice_ptp.c), the patch now reads the…
- [Medium] ice_ptp_flush_tx_tracker() now increments
  pf->ptp.tx_hwtstamp_flushed on every flushed request, and also…
- [Medium] The new 'ready bit not cleared' branch in
  ice_ptp_process_tx_tstamp() decides based on PHY reads done without…
- [Low] pf->ptp.tx_hwtstamp_timeouts now has two writers that are not
  serialized against each other.

--

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

This commit re-reads the PHY Tx timestamp ready bitmap after each read in
ice_ptp_process_tx_tstamp(). If the hardware did not clear the bit, the
index stays locked and is marked stale. It also moves the timeout count to
the point where the index is released, counts timeouts on flush, and moves
the hardware ready bitmap check out of ice_ptp_tx_tstamps_pending() and
into ice_ptp_maybe_trigger_tx_interrupt().

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a9658f859c2e8..74e0c0138c501 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

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

[Severity: Medium]
Can this branch mark a newer request in the same slot as stale and free
its skb?

The decision here comes from ice_read_phy_tstamp() and the ready bitmap
re-read. Neither is done under tx->lock. After the lock is taken, the only
check is test_bit(idx, tx->in_use), and that check can't tell the original
request from a new one that reused idx.

On E825, a link-up event can run this sequence while the IRQ thread sits
between the re-read and spin_lock_irqsave():

ice_ptp_link_change()
  ice_ptp_port_phy_restart()
    ptp_port->tx.calibrating = true;
    ice_ptp_flush_tx_tracker()   /* clears in_use[idx], frees skb */
    ice_start_phy_timer_eth56g()
    ptp_port->tx.calibrating = false;

ice_ptp_request_ts()
    tx->tstamps[idx].start = jiffies;
    tx->tstamps[idx].skb = skb_get(skb);
    ...
    clear_bit(idx, tx->stale);
    smp_mb__before_atomic();
    set_bit(idx, tx->in_use);

Nothing in that sequence waits for an IRQ thread that is already inside the
loop. When the IRQ thread resumes, test_bit(idx, tx->in_use) returns true.
It then sets stale[idx] and frees the new request's skb because of the old
request's hardware state. The new packet's Tx timestamp is lost and
skb_tstamp_tx() is never called for it.

The timing window is narrow, and the freed reference came from skb_get(),
so this is a lost timestamp, not a memory safety issue. The existing
skip_ts_read path has a similar check-then-act gap.

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

[Severity: Low]
Is the timeout counted here only when this path is the one that actually
releases the index?

The increment happens before tx->lock is taken, and it doesn't check that
in_use[idx] is still set. This patch also adds an increment of
tx_hwtstamp_timeouts in ice_ptp_flush_tx_tracker(), under tx->lock.

On E82x/E825, ice_ptp_process_tx_tstamp() runs in the clock owner's misc
IRQ thread without ps_lock. The flush can run at the same time from
ice_ptp_port_phy_restart() (E825 link up), from
ice_ptp_flush_all_tx_tracker() during reset, or from tracker release.

Here is one way it can happen:

1. The IRQ thread sees a request that is older than 2 seconds with its
   ready bit clear, sets timeout = true and jumps to skip_ts_read.
2. The flush takes tx->lock, increments tx_hwtstamp_timeouts and clears
   in_use.
3. The IRQ thread increments tx_hwtstamp_timeouts again, and its
   clear_bit() does nothing.

One request ends up counted twice. Also, a plain u32 increment from two
CPUs can lose updates.

The commit message says:

  "This ensures that we don't double count timeouts or count them
  prematurely."

Is that still true now that the flush path also increments this counter?

[ ... ]

> @@ -849,14 +869,20 @@ 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);
> +
> +		/* Count the number of Tx timestamps flushed */
> +		pf->ptp.tx_hwtstamp_flushed++;
> +
> +		/* Also count timeouts if we flush a request that timed out */
> +		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ))
> +			pf->ptp.tx_hwtstamp_timeouts++;
> +

[Severity: Medium]
Doesn't this count one flushed request as both err and lost in the ethtool
timestamping stats?

Both counters are incremented here for the same idx. ice_get_ts_stats()
reports them as:

drivers/net/ethernet/intel/ice/ice_ethtool.c:ice_get_ts_stats() {
    ...
	ts_stats->err = ptp->tx_hwtstamp_skipped +
			ptp->tx_hwtstamp_flushed +
			ptp->tx_hwtstamp_discarded;
	ts_stats->lost = ptp->tx_hwtstamp_timeouts;
}

The kernel-doc for struct ethtool_ts_stats in include/linux/ethtool.h says:

 * @err: Number of arbitrary timestamp generation error events that the
 *	hardware encountered, exclusive of @lost statistics.

This patch lets stuck slots stay locked for well over 2 seconds, so the
E825 link-up flush from ice_ptp_port_phy_restart() is likely to hit this
case. Should tx_hwtstamp_flushed be skipped when the request is counted as
a timeout?

A related detail: an index whose skb was already freed by the new
stuck-ready-bit path still counts towards tx_hwtstamp_flushed, even though
tx->tstamps[idx].skb is NULL. Is that intended?

[ ... ]

> @@ -3001,7 +3009,15 @@ 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) {
> +		dev_dbg(dev, "Unable to read PHY Tx timestamp ready bitmap, err %pe\n",
> +			ERR_PTR(ret));
> +		/* Don't trigger an IRQ if we are unable to access the PHY */
> +		return;
> +	}
> +
> +	if (ret > 0 || ice_ptp_tx_tstamps_pending(pf, false)) {

[Severity: Medium]
Can this early return stop the watchdog from re-triggering the interrupt
for ports that still have outstanding requests?

Before this patch, ice_ptp_tx_tstamps_pending(pf, false) checked the
software in_use bitmaps first, and returned true whenever any request was
outstanding. The hardware check only ran when software had nothing
pending, and an error from it only turned off that hardware-only trigger.

Now the hardware read happens first. On E82x and E825
(ICE_PTP_TX_INTERRUPT_ALL on the clock owner), ice_check_phy_tx_tstamp_ready()
walks every quad or port and returns the first error it hits:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_check_phy_tx_tstamp_ready_eth56g() {
	for (port = 0; port < hw->ptp.num_lports; port++) {
		...
		err = ice_get_phy_tx_tstamp_ready(hw, port, &tstamp_ready);
		if (err)
			return err;
    ...
}

ice_check_phy_tx_tstamp_ready_e82x() does the same across quads.

As long as one PHY can't be read, the watchdog triggers no IRQ for any
port. ice_ptp_process_tx_tstamp() is not re-run, so timed-out or stale
slots on healthy ports are not reaped and a stalled hardware interrupt is
not recovered.

This patch also stops the IRQ path from re-arming for stale entries and
for link-down or calibrating trackers, which leaves the watchdog as the
only fallback. The commit message also says PHY access errors can happen
during the link transition window this patch targets. The commit message
doesn't mention this ordering change.

The kernel-doc for ice_ptp_maybe_trigger_tx_interrupt() still says:

 * To avoid getting permanently stuck, periodically check if there is any
 * outstanding timestamps or if the PHY has marked any index as ready.

Would it work to check ice_ptp_tx_tstamps_pending() first, and treat a
hardware read error the same as no hardware-ready timestamps?

>  		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
>  
>  		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com