[PATCH iwl-next v2 0/4] ice: E825 timer adjustment fixes

[PATCH iwl-next v2 0/4] ice: E825 timer adjustment fixes

[Jacob Keller]

Recently we've had reports of bad timer adjustment on E825. This series
fixes the issues by replacing direct timer adjustments with a struct
timecounter to maintain the phase in software instead.

To motivate this, the following is an analysis of how the E825 PHY
timestamps function.

    The ice driver maintains PTP clock time in the MAC with a 96-bit
    register. Timestamps are captured in the PHY which only stores 64
    bits, only including the lower 32-bits of nanoseconds. The driver
    performs clock adjustments using a hardware timer command mechanism,
    where the clock adjustment is programmed into shadow registers in
    both the MAC and PHY, and then a timer command is initiated to
    synchronously update the timers.

    Since the PHY only maintains the lower 32 bits of nanoseconds,
    adjustments are also limited such that the maximum atomic adjustment
    is ~2.1 seconds (2^31-1 nanoseconds).

    For large offsets, the driver uses a non-atomic flow where the
    current time is read, an adjustment is applied in software, and then
    written back. This necessarily loses some elapsed time, which
    applications must deal with. For particular servo configurations,
    where the parameters are tuned for a very slow servo, this error can
    take a long time to finish correction.

    On E825, the adjustment in the PHY does not always work reliably.
    The reason for this is subtle, and is a quirk of how the timestamps
    are actually generated. The timestamps for Tx and Rx packets come
    from the PCS timer rather than directly from the PHY timer. The PCS
    timer is derived from internal PHY signaling.

    To keep the PCS timer and PHY timer in sync, every few cycles
    (testing seems to indicate every 8 cycles) the PHY timer is copied
    to the PCS timer. On other cycles, the PCS timer is incremented by a
    value determined from the PCS clock rate.

    Crucially, the copy of the PHY timer is skipped if the current value
    of the PHY timer is lower than the PCS timer (*and* on this cycle
    the regular PCS clock increment is also skipped).

    During normal operation, the PCS timer will tick from its own source
    for 7 ticks then copy the PCS timer. If the PCS timer is slightly
    ahead, then PHY timer update is skipped, and the PCS timer skips an
    update every 8 clock cycles until it is back in alignment with the
    PHY timer.

    Unfortunately, this hardware logic did not take into account manual
    adjustments to the PHY. If the PHY timer is adjusted backwards, (or
    if a forward adjustment causes the PHY timer to overflow), the PHY
    to PCS sync will not occur again until the PCS timer wraps back
    around to zero. In the worst case, an adjustment could land the PHY
    timer to just below the PCS timer just after it wraps around. PHY to
    PCS syncs would stop for ~3.6 seconds.

    During this time, the Tx and Rx timestamps will read from the PCS
    timer. These timestamps will be fed into the timestamp extension
    algorithm which will produce incoherent results, and data will be
    fed into the PTP application servo. During the invalid window,
    faulty timestamps will cause the servo to steer incorrectly, likely
    away from the true target time. After the PCS timer synchronizes
    again, the timestamps will return to normal. However, the servo may
    have steered the clock incorrectly. In default ptp4l configurations,
    direct clock adjustment is only performed once during startup. This
    forces the application to use frequency slewing to correct any
    remaining offset. With slow servo parameters, this can take a *very*
    long time. In some setups, we had reports of taking several minutes
    to nearly an hour before stabilizing, and in some configurations the
    servo was unable to ever recover.

    Note that adjustments work properly on the MAC timer, and indeed on
    the first-layer PHY timer value that the driver can read over the
    sideband queue. Only the PCS timer (which is inaccessible to the
    driver) fails to adjust. This made tracking the precise origin of
    the failure difficult.

Testing shows that pretty much all "negative" adjustments and about half
of the positive adjustments can trigger this failure. It is also
difficult to see without using extremely slow servo parameters. With a
servo tuned for fast reaction, the incorrect timestamps will cause some
difficulty in the first adjustment, but the servo will quickly correct
and recover within a few seconds at most.

I used the following P/I servo parameters in my testing:

    pi_proportional_const 0.60
    pi_integral_const 0.001
    pi_proportional_scale 0.0
    pi_proportional_exponent -0.3
    pi_proportional_norm_max 0.7
    pi_integral_scale 0.0
    pi_integral_exponent 0.4
    pi_integral_norm_max 0.3

In particular, the pi_integral_const will force the servo to be very
slow at reacting.

Even trying to compare the MAC timer with the Tx and Rx timestamps is
not trivial. There is no mechanism to synchronously read the MAC timer
along with a timestamp. Additionally, timestamp extension makes this even
harder to track. The MAC timer is immediately adjusted and the cached PHC
time is updated. Normally timestamps from before the adjustment should
reasonably convert, however if the adjustment is large enough the timestamp
can flip to being detected as "in the past" and in the worst cases the
resulting extended timestamps will have a second sudden jump of -4.2
seconds before the PCS timer adjustment is applied and things settle. All
of this while being fed into a servo trying to steer the clock can make it
difficult to understand what is going on.

With that background out of the way, here's a summary of the patch content:

First, the SO_TX_TIME support in ice_tx_map() is adjusted to compute the
timestamp from the full nanoseconds instead of only the sub-second portion
of the timestamp.

Next, the PTP callback functions are adjusted to check that the PTP state
is ready. This makes them fail early if the driver is resetting or enters
an error state. This isn't a complete synchronization fix as it doesn't
protect against an already-running command when a reset starts. More work
is underway to investigate and determine the best synchronization solution.

Next, the timestamp extension logic is refactored to store the cached
PHC time value inside the ice_adapter structure under a new ice_phc
structure which will be used to store timecounter data. This change
reduces the number of threads that need to perform timestamp extension
and makes introduction of timecounter simpler as it gives a natural
place for accessing the structure and simplifies modifications to the Rx
hotpath in particular. This does further solidify the dependency on the
clock owning PF.

Finally, the driver is modified to allow use of a timecounter for
representing the time instead of directly adjusting the phase in
hardware. Frequency adjustments continue to be handled in hardware. For
now, only the E825 and E830 devices are switched. The legacy
implementation remains for supporting older installations of E810 and
E822 platform devices. We plan to complete migration in the future, but
felt that immediate conversion would slow down ability to validate that
all older installations will behave correctly. Since the E825 device
timer adjustment logic is completely useless, this can speed up ability
to adopt the fix. Although E830 does not appear to have the same timer
command issue, it was decided to convert both since they are the most
recent devices and undergoing the most testing and active development.

Because the timecounter already exists within the kernel, it was deemed the
best solution, despite its trade offs required for implementation. I'm
open to other alternative fixes if anyone has a better solution. One key
thing is that we need the 32-bit cycle counter mask in order to enable
timestamp extension, but this means we effectively (mostly) ignore the
upper 32-bits of the MAC timer when they could be used for tracking time
over the wraparound. Sashiko is not happy about this dependency on
requiring the periodic 2 second check on the clock owner, and complains
about ways it could break if that stops.

This series fixes a separate issue from [1] and does not *directly* depend
on it. The fixes in that series are necessary but not sufficient to
completely eliminate PHY timer desync issues on E825.

There are some structural conflicts with the ctrl_pf RCU changes at [2]
which I have tried to integrate, so this won't be a straightforward
cherry-pick to trees without that series.

One final note, the conversion to timecounter supersedes the changes
from [3], and that patch has been removed from more recent versions of
that series. The reset partially helped, but was only applied to the
non-atomic settime interface. In particular, the settime interface was
vulnerable to the same PCS issue, but the PHY soft reset *does* clear
both the PHY and PCS timers back to 0. Performing a reset after the
adjustment will force both timers back to zero and then perform a
forward adjustment that won't trigger the failure condition. However,
attempting to add this reset to *every* timer adjustment comes with
unnecessary delay since the whole process takes a long time especially
with the PHY soft reset. It was ultimately deemed as not a proper fix
suitable for deployment.

Link: [1] https://lore.kernel.org/intel-wired-lan/20260925222824.270640-1-grzegorz.nitka@intel.com/
Link: [2] https://lore.kernel.org/intel-wired-lan/20261013091235.1423979-1-sergey.temerkhanov@intel.com/
Link: [3] https://lore.kernel.org/netdev/20261008215614.1987250-16-anthony.l.nguyen@intel.com/

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes in v2:
- Replace the seqlock_t with a seqcount_latch_t and a mutex that protects
  serialization of the periodic update with the direct updates from the PTP
  hardware clock callbacks.
- Replace use of "0" as a canary value of cached_time and apply the
  cached_jiffies stale discard check to both Tx and Rx timestamps.
- Fix the rx_hwtstamp trace to record the timecounter values and properly
  use the same copied value of the cached_time on non-timecounter devices.
- Fix the SO_TX_TIME support on E830 to account for timecounter phase
  offset.
- Modify ice_ptp_read_cycles() to always read and report the full timer
  value, enabling proper phase calculation even as the MAC timer rolls
  over.
- Add ice_phc_get_phase() to compute the timecounter phase, currently used
  only by SO_TX_TIME, but likely necessary for VF Rx timestamping in the
  future.
- Disable VF Rx timestamping support for devices using the timecounter.
  This will need some investigation and virtchnl development work to enable
  properly, which I plan on doing in a different series. I don't like
  regressing this, but I don't have a simple alternative. I don't believe
  that simply accounting for the phase in the GET_TIME would be sufficient,
  as I am not certain it produces the correct math in the iavf timestamp
  extension algorithm.
- Fix the phase offset calculation in ice_ptp_cfg_perout(), computing the
  timecounter domain first before adding the 500 msec.
- Separate the E830 ETF fix out to its own patch, as it seems like the
  driver is incorrectly programming all ETF timestamps.
- Add a patch to have all the PTP callbacks check for the PTP ready state.
- Target net-next despite being a bug fix since this is unlikely to be
  accepted to net due to the scope.
- Link to v1: https://patch.msgid.link/20261023-jk-e825-e830-timecounter-v1-0-e932602f0d16@intel.com

---
Jacob Keller (4):
      ice: fix computation of ETF timestamps for E830
      ice: exit PTP callbacks early if not ICE_PTP_READY
      ice: store cached PHC time in ice_adapter
      ice: use struct timecounter for E825 and E830

 drivers/net/ethernet/intel/ice/ice_adapter.h   |  54 +++
 drivers/net/ethernet/intel/ice/ice_ptp.h       |  18 +-
 drivers/net/ethernet/intel/ice/ice_trace.h     |  18 +-
 drivers/net/ethernet/intel/ice/ice_txrx.h      |   5 +-
 drivers/net/ethernet/intel/ice/ice_adapter.c   |  51 +++
 drivers/net/ethernet/intel/ice/ice_ethtool.c   |   1 -
 drivers/net/ethernet/intel/ice/ice_lib.c       |   2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c       | 607 ++++++++++++++++++-------
 drivers/net/ethernet/intel/ice/ice_txrx.c      |  16 +-
 drivers/net/ethernet/intel/ice/virt/virtchnl.c |   7 +-
 10 files changed, 608 insertions(+), 171 deletions(-)
---
base-commit: c3ceba9087610ff11af6406df68acabeb2d4c34c
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
change-id: 20261022-jk-e825-e830-timecounter-06c15de02e76

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

[PATCH iwl-next v2 1/4] ice: fix computation of ETF timestamps for E830

[Jacob Keller]

When Earliest TxTime First offload is enabled, the ice_tx_map() function
computes the appropriate timestamps for the timestamp ring used to set the
transmit times for each packet.

It computes this by converting the skb->tstamp into a timespec and then
taking the sub-second portion from tv_nsec and dividing it by the timestamp
resolution. This value is then inserted into the timestamp descriptor using
ice_build_tstamp_desc().

At first glance this may seem reasonable. After all, the Tx timestamp
descriptor field has a mask defined by ICE_TXTIME_STAMP_M which is
GENMASK(31,13). This is a 19-bit wide field. With a unit size of 128
nanoseconds, the maximum representable time is only ~67 milliseconds, far
below the size of the tv_nsec field (just shy of one second).

The E830 device time is represented as a single 64-bit nanosecond value as
seen by ice_ptp_read_src_clk_reg() using a simple rd64 on the GLTSYN_TIME_L
register.

According to the data sheet, the hardware uses the following flow to
determine when to transmit the next packet in the paced queue:

  half_window = window_size / 2
  if (ts <= timer) {
    if (timer - ts < half_window) {
      // timestamp expired
      pass
    } else {
      // wait before sending
      stall
    }
  } else {
    // Handle possible rollover case
    if (ts - timer < half_window) {
      // wait before sending
      stall
    } else {
      // timestamp expired
      pass
    }
  }

Here, window_size is the maximum value that fix in the 19 bit sized
timestamp field, ts is the next packet timestamp waiting in the queue, and
timer is the 19 bits of the GLTSYN_TIME MAC timer. The configured timestamp
"resolution" determines at which bit position to capture from the
GLTSYN_TIME registers. This logic does not perform any computation to
divide out seconds or to handle anything like a timespec. The timestamp and
timer values are compared directly with logic to account for handling
rollover.

Because of the faulty logic, all of the timestamps computed and placed in
the ring will result in the transmit time not matching the desired time per
the skb. Consider a timestamp exactly on a second boundary. The computed
timestamp will be 0, while the actual value should be (1,000,000,000 / 128)
mod 2^19 or 472,468 (0x73594).

Fix this by directly converting the ktime to nanoseconds rather than
converting to a timespec64.

Fixes: ccde82e90946 ("ice: add E830 Earliest TxTime First Offload support")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index d09a67bb6019..8576f186480f 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1519,10 +1519,9 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first,
 
 	if (ice_is_txtime_cfg(tx_ring)) {
 		struct ice_tstamp_ring *tstamp_ring;
-		u32 tstamp_count, j;
 		struct ice_ts_desc *ts_desc;
-		struct timespec64 ts;
-		u32 tstamp;
+		u32 tstamp_count, j;
+		u64 tstamp;
 
 		smp_rmb();	/* order flag read before pointer read */
 		tstamp_ring = READ_ONCE(tx_ring->tstamp_ring);
@@ -1532,8 +1531,8 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first,
 		tstamp_count = tstamp_ring->count;
 		j = tstamp_ring->next_to_use;
 
-		ts = ktime_to_timespec64(first->skb->tstamp);
-		tstamp = ts.tv_nsec >> ICE_TXTIME_CTX_RESOLUTION_128NS;
+		tstamp = ktime_to_ns(first->skb->tstamp);
+		tstamp >>= ICE_TXTIME_CTX_RESOLUTION_128NS;
 
 		ts_desc = ICE_TS_DESC(tstamp_ring, j);
 		ts_desc->tx_desc_idx_tstamp = ice_build_tstamp_desc(i, tstamp);

-- 
2.56.0.215.g47fdf2208c5b

[PATCH iwl-next v2 2/4] ice: exit PTP callbacks early if not ICE_PTP_READY

[Jacob Keller]

Add a check for the PTP state at the start of ice_ptp_settime64(),
ice_ptp_adjtime(), ice_ptp_adjfine(), and ice_ptp_gpio_enable(). If the
driver PTP state is not ICE_PTP_READY, exit with -EIO instead of continuing
to try to modify the clock while the driver may still be loading,
resetting, unloading, or after a failed rebuild.

The check is currently best effort and doesn't guarantee to prevent a race
between the start of a new command and a reset. For that, we will need more
time to investigate and develop a proper synchronization mechanism.

The ice_ptp_gpio_enable() still allows requests to disable the pins. This
is intentional as it will be called by ptp_clock_unregister() when tearing
down. Additionally, the core PTP code often ignores the return value of
ptp_disable_pinfunc() anyways. Allowing disables through ensures the
internal driver state matches, while new enable requests are blocked until
the driver is ready.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a4c7c36905e8..bf37c2a7ff86 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1607,6 +1607,9 @@ static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
 	u64 incval;
 	int err;
 
+	if (pf->ptp.state != ICE_PTP_READY)
+		return -EIO;
+
 	incval = adjust_by_scaled_ppm(ice_base_incval(pf), scaled_ppm);
 	err = ice_ptp_write_incval_locked(hw, incval);
 	if (err) {
@@ -1997,6 +2000,9 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
 		struct ptp_perout_request *cached =
 			&pf->ptp.perout_rqs[rq->perout.index];
 
+		if (on && pf->ptp.state != ICE_PTP_READY)
+			return -EIO;
+
 		err = ice_ptp_cfg_perout(pf, &rq->perout, on);
 		if (!err) {
 			*cached = rq->perout;
@@ -2011,6 +2017,9 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
 		struct ptp_extts_request *cached =
 			&pf->ptp.extts_rqs[rq->extts.index];
 
+		if (on && pf->ptp.state != ICE_PTP_READY)
+			return -EIO;
+
 		err = ice_ptp_cfg_extts(pf, &rq->extts, on);
 		if (!err)
 			*cached = rq->extts;
@@ -2065,6 +2074,9 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
 	struct ice_hw *hw = &pf->hw;
 	int err;
 
+	if (pf->ptp.state != ICE_PTP_READY)
+		return -EIO;
+
 	/* For Vernier mode on E82X, we need to recalibrate after new settime.
 	 * Start with marking timestamps as invalid.
 	 */
@@ -2134,6 +2146,9 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
 	struct device *dev;
 	int err;
 
+	if (pf->ptp.state != ICE_PTP_READY)
+		return -EIO;
+
 	dev = ice_pf_to_dev(pf);
 
 	/* Hardware only supports atomic adjustments using signed 32-bit

-- 
2.56.0.215.g47fdf2208c5b

[PATCH iwl-next v2 3/4] ice: store cached PHC time in ice_adapter

[Jacob Keller]

The ice driver uses a cached copy of the PTP Hardware Clock time for its
timestamp extension algorithm implemented in ice_ptp_extend_32b_ts(). This
algorithm is based on work originally from struct timecounter, and relies
on a cached copy of the PTP time used to extend 32-bit snapshots of the PTP
time into full 64-bit time values.

The driver currently stores this cache once in the ice_ptp structure for
each PF and once per each Rx ring on each PF. Each PF is then responsible
for calling ice_ptp_update_cached_phctime() at least once every 2 seconds.
This is handled by the per-PF periodic work task that runs on a per-PF
kthread. Each Rx ring must be updated, which requires looping the VSI and
ring structures, and might fail due to the rings being reconfigured.

In order to fix some device-specific bugs, a following change is going to
refactor the driver to support using a timecounter structure to avoid
needing to perform direct time adjustments in hardware on E825 and E830.

Use of a timecounter will require the that the Rx path have a reference to
that structure. To ensure that all PFs on the same device have the same
time domain, the structure must be part of the whole adapter, making
ice_adapter the best place to store it. The initial implementation will
focus only on migrating the E825 and E830 devices, to minimize impact and
potential regression.

Since the Rx path will need to reference a structure anyways, it makes
little sense to maintain both a cached copy of the PHC time in addition to
a pointer to the adapter.

As a first step, refactor the driver to store the existing cached PHC time
in ice_adapter, instead of per-PF in each ice_ptp structure and in the Rx
rings.

Create a struct ice_phc structure to store the PTP Hardware Clock fields,
and embed it in ice_adapter. Store a pointer to the PHC structure in the Rx
rings, replacing the cached_phctime.

Pass a pointer to the PHC structure to ice_ptp_extend_32b_ts() and obtain
the cached time from that structure. Modify the Tx and Rx paths to stop
using a zero value of cached_time as a canary for the clock owner
disappearing. The ice_ptp_read_src_clk_reg() can't fail when called by the
clock owner anyways, so this check is not very meaningful when only the
clock owner performs the cached update.

Modify the ice_ptp_update_cached_phctime(), removing logic to update the
cached time within the Rx rings. Convert it to a void function since it can
no longer fail in a meaningful way.

Change ice_ptp_periodic_work() to check ice_pf_src_tmr_owned() and only
call ice_ptp_update_cached_phctime() for the clock owner. To ensure a
consistent update after the clock adjustments, use a mutex to serialize the
calls to ice_ptp_update_cached_phctime(). This lock is acquired *before*
the read of the current time. This ensures that even if a periodic update
races with a clock adjustment, it will either happen before or will see the
clock adjusted after as it updates cached_time. Performing the update only
on the clock owner does make the late_cached_phc_updates ethtool stat only
apply for the clock owner.

Since the Rx path now has access to the PHC structure, move the timeout
check from ice_ptp_extend_40b_ts() into ice_ptp_extend_32b_ts(). This
ensures that Rx timestamps also get discarded if the cached PHC update
becomes stale.

The ice_ptp_reset_cached_phctime() function previously only applied to the
clock owner. This was wrong, as the driver should have been updating all
ports cached time copy, and marking each port tracker as stale. Introduce
the ice_ptp_mark_all_tx_tracker_stale() and use it here.

Separate the PHC reset time from the cached time value in
ice_ptp_rebuild_owner(), rather than relying on the cached_phc_time. Read
the value of the PHC time near the same time that we save ptp->reset_time.
If a hardware reset has already begun, the GLTSYN_TIME registers should
read as 0 which will trigger the rebuild path to reinitialize from the
kernel time.

These changes eliminate failure points from the cached PHC time update,
reduce needless duplicate work across PFs, and ensure consistency of the
cached time update w.r.t clock adjustments. They also pave the way for
introduction of a timecounter in a following change.

Careful readers may note that non-owner PFs now completely depend on the
clock owner PF being loaded to ensure the cached time is up to date,
as they will start discarding timestamps 2 seconds after the last update.
However, PTP already has a strong dependency on the clock owner PF. Only
the owner initializes the PHC mac timer and exposes the PTP clock device.
E822 and E825 devices also depend on the clock owner miscellaneous IRQ for
handling all Tx timestamps. PTP also will not behave right under PCIe
device passthru unless all PFs are assigned into the same VM, due to
depending on coordination via the ice_adapter structure. This is a
limitation due to the quirks of the device design.

The Rx path now requires accessing the PHC structure via a dereference and
performs the jiffies timeout check which may impact performance. However,
this ensures that Rx timestamps do not get extended on stale data and
access to the PHC is required for the timecounter refactor anyways.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.h |  22 +++
 drivers/net/ethernet/intel/ice/ice_ptp.h     |   6 +-
 drivers/net/ethernet/intel/ice/ice_txrx.h    |   4 +-
 drivers/net/ethernet/intel/ice/ice_adapter.c |   4 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c |   1 -
 drivers/net/ethernet/intel/ice/ice_lib.c     |   2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 223 +++++++++++----------------
 7 files changed, 120 insertions(+), 142 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index ce1fe0846265..acde4031d528 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -35,6 +35,25 @@ struct ice_port_list {
 	struct srcu_struct srcu;
 };
 
+/**
+ * struct ice_phc - PTP Hardware Clock fields
+ * @cached_time: Cached copy of the PHC time for timestamp extension
+ * @update_jiffies: Jiffies when PHC was last updated
+ * @lock: For protecting updates to the PHC fields.
+ *
+ * Readers are lockless and rely on READ_ONCE/WRITE_ONCE for the cached_time
+ * and smp_acquire/smp_release for the update_jiffies field.
+ *
+ * Writers must acquire the lock mutex to ensure that the periodic task cannot
+ * write back an out-of-date cached value after a clock adjustment.
+ */
+struct ice_phc {
+	u64 cached_time;
+	unsigned long update_jiffies;
+	/* Serialize updates to cached_time */
+	struct mutex lock;
+};
+
 /**
  * struct ice_adapter - PCI adapter resources shared across PFs
  * @refcount: Reference count. struct ice_pf objects hold the references.
@@ -46,6 +65,7 @@ struct ice_port_list {
  * @cgu_synce_lock: Mutex serializing changes of the CGU registers R10 and R11,
  *                  which all PFs reach. Used on E825C devices.
  * @ps_lock: Mutex to serialize PHY port start/stop across adapter.
+ * @phc: PTP Hardware Clock fields
  * @ctrl_pf_lock: Protect control PF lifetime for sleepable users
  * @ctrl_pf: Control PF of the adapter
  * @rebuild_lock: serialize PFR recovery across PFs of the same adapter
@@ -73,6 +93,8 @@ struct ice_adapter {
 	/* For serializing PHY port start/stop sequences */
 	struct mutex ps_lock;
 
+	struct ice_phc phc;
+
 	struct rw_semaphore ctrl_pf_lock;
 	struct ice_pf __rcu *ctrl_pf;
 	struct ice_port_list ports;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 40a0e654c2c6..430441b43150 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -235,8 +235,6 @@ struct ice_ptp_pin_desc {
  * @tx_interrupt_mode: the TX interrupt mode for the PTP clock
  * @port: data for the PHY port initialization procedure
  * @work: delayed work function for periodic tasks
- * @cached_phc_time: a cached copy of the PHC time for timestamp extension
- * @cached_phc_jiffies: jiffies when cached_phc_time was last updated
  * @kworker: kwork thread for handling periodic work
  * @ext_ts_irq: the external timestamp IRQ in use
  * @pin_desc: structure defining pins
@@ -248,6 +246,7 @@ struct ice_ptp_pin_desc {
  * @tstamp_config: hardware timestamping configuration
  * @tx_refclks: bitmaps table to store the information about TX reference clocks
  * @reset_time: kernel time after clock stop on reset
+ * @phc_reset_time: PHC time at clock stop on reset
  * @tx_hwtstamp_good: number of completed Tx timestamp requests
  * @tx_hwtstamp_skipped: number of Tx time stamp requests skipped
  * @tx_hwtstamp_timeouts: number of Tx skbs discarded with no time stamp
@@ -270,8 +269,6 @@ struct ice_ptp {
 	enum ice_ptp_tx_interrupt tx_interrupt_mode;
 	struct ice_ptp_port port;
 	struct kthread_delayed_work work;
-	u64 cached_phc_time;
-	unsigned long cached_phc_jiffies;
 	struct kthread_worker *kworker;
 	u8 ext_ts_irq;
 	struct ptp_pin_desc pin_desc[ICE_N_PINS_MAX];
@@ -283,6 +280,7 @@ struct ice_ptp {
 	struct kernel_hwtstamp_config tstamp_config;
 	unsigned long tx_refclks[ICE_E825_MAX_PHYS][ICE_REF_CLK_MAX];
 	u64 reset_time;
+	u64 phc_reset_time;
 	u64 tx_hwtstamp_good;
 	u32 tx_hwtstamp_skipped;
 	u32 tx_hwtstamp_timeouts;
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index 5e517f219379..83a34322e0d3 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -221,7 +221,7 @@ enum ice_tx_ring_flags {
 };
 
 struct ice_pkt_ctx {
-	u64 cached_phctime;
+	struct ice_phc *phc;
 	__be16 vlan_proto;
 };
 
@@ -321,7 +321,7 @@ struct ice_rx_ring {
 	union {
 		struct ice_pkt_ctx pkt_ctx;
 		struct {
-			u64 cached_phctime;
+			struct ice_phc *phc;
 			__be16 vlan_proto;
 		};
 	};
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 29e089a5a86e..f3ee5e60613e 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -83,6 +83,8 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 
 	mutex_init(&adapter->ps_lock);
 
+	mutex_init(&adapter->phc.lock);
+
 	return adapter;
 }
 
@@ -98,6 +100,8 @@ static void ice_adapter_free(struct ice_adapter *adapter)
 	cleanup_srcu_struct(&adapter->ports.srcu);
 	mutex_destroy(&adapter->ps_lock);
 
+	mutex_destroy(&adapter->phc.lock);
+
 	kfree(adapter);
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 90fa11b22820..ca5137724b38 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3338,7 +3338,6 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 		/* clone ring and setup updated count */
 		rx_rings[i] = *vsi->rx_rings[i];
 		rx_rings[i].count = new_rx_cnt;
-		rx_rings[i].cached_phctime = pf->ptp.cached_phc_time;
 		rx_rings[i].desc = NULL;
 		rx_rings[i].xdp_buf = NULL;
 		rx_rings[i].xdp_rxq = (struct xdp_rxq_info){ };
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 9be3f52bb9d9..f3fefab09f8c 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -1527,7 +1527,7 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
 		ring->vsi = vsi;
 		ring->netdev = vsi->netdev;
 		ring->count = vsi->num_rx_desc;
-		ring->cached_phctime = pf->ptp.cached_phc_time;
+		ring->phc = &pf->adapter->phc;
 
 		if (ice_is_feature_supported(pf, ICE_F_GCS))
 			ring->flags |= ICE_RX_FLAGS_RING_GCS;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index bf37c2a7ff86..a0451c1083a4 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -274,8 +274,9 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 
 /**
  * ice_ptp_extend_32b_ts - Convert a 32b nanoseconds timestamp to 64b
- * @cached_phc_time: recently cached copy of PHC time
+ * @phc: PTP Hardware Clock structure
  * @in_tstamp: Ingress/egress 32b nanoseconds timestamp value
+ * @trace_rx: True if this call should trigger an Rx timestamp trace
  *
  * Hardware captures timestamps which contain only 32 bits of nominal
  * nanoseconds, as opposed to the 64bit timestamps that the stack expects.
@@ -309,13 +310,27 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
  * This is prevented by (a) periodically updating the cached PHC time once
  * a second, and (b) discarding any Tx timestamp packet if it has waited for
  * a timestamp for more than one second.
+ *
+ * Return: the extended timestamp value, or 0 if phc->update_jiffies is stale.
  */
-static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
+static u64 ice_ptp_extend_32b_ts(struct ice_phc *phc,
+				 u32 in_tstamp, bool trace_rx)
 {
+	unsigned long discard_time;
+	u64 ns, cached_phc_time;
 	u32 delta, phc_time_lo;
-	u64 ns;
 
-	/* Extract the lower 32 bits of the PHC time */
+	/* Discard the timestamp if update_jiffies hasn't been initialized */
+	discard_time = smp_load_acquire(&phc->update_jiffies);
+	if (!discard_time)
+		return 0;
+
+	/* Discard the hardware timestamp if the cached PHC time is too old */
+	discard_time += msecs_to_jiffies(2000);
+	if (time_is_before_jiffies(discard_time))
+		return 0;
+
+	cached_phc_time = READ_ONCE(phc->cached_time);
 	phc_time_lo = (u32)cached_phc_time;
 
 	/* Calculate the delta between the lower 32bits of the cached PHC
@@ -336,6 +351,9 @@ static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
 		ns = cached_phc_time + delta;
 	}
 
+	if (trace_rx)
+		ice_trace(rx_hwtstamp, cached_phc_time, in_tstamp, ns);
+
 	return ns;
 }
 
@@ -359,7 +377,7 @@ static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
  * calling this function.
  *
  * Extract the 32bit nominal nanoseconds and extend them. Use the cached PHC
- * time stored in the device private PTP structure as the basis for timestamp
+ * time stored in the adapter PHC structure as the basis for timestamp
  * extension.
  *
  * See ice_ptp_extend_32b_ts for a detailed explanation of the extension
@@ -367,28 +385,17 @@ static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
  */
 static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
 {
+	struct ice_phc *phc = &pf->adapter->phc;
 	const u64 mask = GENMASK_ULL(31, 0);
-	unsigned long discard_time;
-	u64 cached_time;
+	u64 ns;
 
-	/* Discard the hardware timestamp if the cached PHC time is too old */
-	discard_time = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
-	if (time_is_before_jiffies(discard_time)) {
+	ns = ice_ptp_extend_32b_ts(phc, (in_tstamp >> 8) & mask, false);
+
+	/* A value of 0 indicates the timestamp was discarded */
+	if (!ns)
 		pf->ptp.tx_hwtstamp_discarded++;
-		return 0;
-	}
 
-	/* Discard it as well if there is no cached PHC time at all, which is
-	 * how ice_ptp_update_cached_phctime() reports that the source clock
-	 * has become unreachable.
-	 */
-	cached_time = READ_ONCE(pf->ptp.cached_phc_time);
-	if (!cached_time) {
-		pf->ptp.tx_hwtstamp_discarded++;
-		return 0;
-	}
-
-	return ice_ptp_extend_32b_ts(cached_time, (in_tstamp >> 8) & mask);
+	return ns;
 }
 
 /**
@@ -903,6 +910,31 @@ ice_ptp_flush_all_tx_tracker(struct ice_pf *pf)
 	srcu_read_unlock(&ports->srcu, srcu_idx);
 }
 
+/**
+ * ice_ptp_mark_all_tx_tracker_stale - Mark all unfinished timestamps stale
+ * @pf: Board private structure
+ *
+ * Mark currently outstanding Tx timestamps for all ports as stale. Called by
+ * the clock owner when a time adjustment is made to prevent accidentally
+ * extending timestamps captured before the adjustment.
+ */
+static void ice_ptp_mark_all_tx_tracker_stale(struct ice_pf *pf)
+{
+	struct ice_port_list *ports = &pf->adapter->ports;
+	struct ice_ptp_port *port;
+	int srcu_idx;
+
+	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_mark_tx_tracker_stale(&port->tx);
+		kref_put(&port->ref, ice_ptp_release_port_srcu);
+	}
+	srcu_read_unlock(&ports->srcu, srcu_idx);
+}
+
 /**
  * ice_ptp_release_tx_tracker - Release allocated memory for Tx tracker
  * @pf: Board private structure
@@ -987,35 +1019,28 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
 }
 
 /**
- * ice_ptp_update_cached_phctime - Update the cached PHC time values
+ * ice_ptp_update_cached_phctime - Update the cached PHC time value
  * @pf: Board specific private structure
  *
- * This function updates the system time values which are cached in the PF
- * structure and the Rx rings.
+ * Update the PTP hardware clock time values which are cached in the PHC
+ * structure.
  *
- * This function must be called periodically to ensure that the cached value
- * is never more than 2 seconds old.
- *
- * Note that the cached copy in the PF PTP structure is always updated, even
- * if we can't update the copy in the Rx rings.
- *
- * Return:
- * * 0 - OK, successfully updated
- * * -EAGAIN - PF was busy, need to reschedule the update
- * * -ENODEV - no control PF owns the source clock register space
- * * -EOPNOTSUPP - PTP support is not compiled in
+ * This function must be called periodically by the clock owner to ensure that
+ * the cached value is never more than 2 seconds old.
  */
-static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
+static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
 {
+	struct ice_phc *phc = &pf->adapter->phc;
 	struct device *dev = ice_pf_to_dev(pf);
 	unsigned long update_before;
 	u64 systime;
-	int err, i;
+	int err;
 
-	update_before = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
-	if (pf->ptp.cached_phc_time &&
-	    time_is_before_jiffies(update_before)) {
-		unsigned long time_taken = jiffies - pf->ptp.cached_phc_jiffies;
+	mutex_lock(&phc->lock);
+
+	update_before = phc->update_jiffies + msecs_to_jiffies(2000);
+	if (phc->update_jiffies && time_is_before_jiffies(update_before)) {
+		unsigned long time_taken = jiffies - phc->update_jiffies;
 
 		dev_warn(dev, "%u msecs passed between update to cached PHC time\n",
 			 jiffies_to_msecs(time_taken));
@@ -1024,44 +1049,13 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
 
 	/* Read the current PHC time */
 	err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
-	if (err) {
-		/* The source clock is gone, so the cache can no longer be
-		 * refreshed. Publish zero instead of leaving the last good
-		 * value behind: neither ice_ptp_get_rx_hwts() nor
-		 * ice_ptp_extend_40b_ts() may extend against a value that no
-		 * longer tracks the PHC, and both treat a zero cache as
-		 * "no timestamp".
-		 */
-		systime = 0;
-		WRITE_ONCE(pf->ptp.cached_phc_time, 0);
-	} else {
-		/* Update the cached PHC time stored in the PF structure */
-		WRITE_ONCE(pf->ptp.cached_phc_time, systime);
-		WRITE_ONCE(pf->ptp.cached_phc_jiffies, jiffies);
+	if (!err) {
+		WRITE_ONCE(phc->cached_time, systime);
+		/* Paired with smp_load_acquire in ice_ptp_extend_32b_ts */
+		smp_store_release(&phc->update_jiffies, jiffies | 1);
 	}
 
-	if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
-		return -EAGAIN;
-
-	ice_for_each_vsi(pf, i) {
-		struct ice_vsi *vsi = pf->vsi[i];
-		int j;
-
-		if (!vsi)
-			continue;
-
-		if (vsi->type != ICE_VSI_PF)
-			continue;
-
-		ice_for_each_rxq(vsi, j) {
-			if (!vsi->rx_rings[j])
-				continue;
-			WRITE_ONCE(vsi->rx_rings[j]->cached_phctime, systime);
-		}
-	}
-	clear_bit(ICE_CFG_BUSY, pf->state);
-
-	return err;
+	mutex_unlock(&phc->lock);
 }
 
 /**
@@ -1070,41 +1064,18 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
  *
  * This function must be called when the cached PHC time is no longer valid,
  * such as after a time adjustment. It marks any currently outstanding Tx
- * timestamps as stale and updates the cached PHC time for both the PF and Rx
- * rings.
- *
- * If updating the PHC time cannot be done immediately, a warning message is
- * logged and the work item is scheduled immediately to minimize the window
- * with a wrong cached timestamp.
+ * timestamps as stale and updates the cached PHC time.
  */
 static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
 {
-	struct device *dev = ice_pf_to_dev(pf);
-	int err;
-
-	/* Update the cached PHC time immediately if possible, otherwise
-	 * schedule the work item to execute soon.
-	 */
-	err = ice_ptp_update_cached_phctime(pf);
-	if (err) {
-		/* If another thread is updating the Rx rings, we won't
-		 * properly reset them here. This could lead to reporting of
-		 * invalid timestamps, but there isn't much we can do.
-		 */
-		dev_warn(dev, "%s: unable to immediately update cached PHC time, err %d\n",
-			 __func__, err);
-
-		/* Queue the work item to update the Rx rings when possible */
-		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
-					   msecs_to_jiffies(10));
-	}
+	ice_ptp_update_cached_phctime(pf);
 
 	/* Mark any outstanding timestamps as stale, since they might have
 	 * been captured in hardware before the time update. This could lead
 	 * to us extending them with the wrong cached value resulting in
 	 * incorrect timestamp values.
 	 */
-	ice_ptp_mark_tx_tracker_stale(&pf->ptp.port.tx);
+	ice_ptp_mark_all_tx_tracker_stale(pf);
 }
 
 /**
@@ -2479,28 +2450,14 @@ int ice_ptp_hwtstamp_set(struct net_device *netdev,
 u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 			const struct ice_pkt_ctx *pkt_ctx)
 {
-	u64 ts_ns, cached_time;
 	u32 ts_high;
+	u64 ts_ns;
 
 	if (!(rx_desc->wb.time_stamp_low & ICE_PTP_TS_VALID))
 		return 0;
 
-	cached_time = READ_ONCE(pkt_ctx->cached_phctime);
-
-	/* Do not report a timestamp if we don't have a cached PHC time */
-	if (!cached_time)
-		return 0;
-
-	/* Use ice_ptp_extend_32b_ts directly, using the ring-specific cached
-	 * PHC value, rather than accessing the PF. This also allows us to
-	 * simply pass the upper 32bits of nanoseconds directly. Calling
-	 * ice_ptp_extend_40b_ts is unnecessary as it would just discard these
-	 * bits itself.
-	 */
 	ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
-	ts_ns = ice_ptp_extend_32b_ts(cached_time, ts_high);
-
-	ice_trace(rx_hwtstamp, cached_time, ts_high, ts_ns);
+	ts_ns = ice_ptp_extend_32b_ts(pkt_ctx->phc, ts_high, true);
 
 	return ts_ns;
 }
@@ -3099,24 +3056,20 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
 {
 	struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
 	struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
-	int err;
 
 	if (pf->ptp.state != ICE_PTP_READY)
 		return;
 
 	ice_ptp_tspll_monitor(pf);
 
-	err = ice_ptp_update_cached_phctime(pf);
+	if (ice_pf_src_tmr_owned(pf))
+		ice_ptp_update_cached_phctime(pf);
 
 	ice_ptp_maybe_trigger_tx_interrupt(pf);
 
-	/* Run twice a second, or retry soon if another thread was busy
-	 * updating the Rx rings. Other errors, such as a missing control PF,
-	 * do not clear up on their own and must not turn the periodic worker
-	 * into a busy loop.
-	 */
+	/* Run twice a second */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work,
-				   msecs_to_jiffies(err == -EAGAIN ? 10 : 500));
+				   msecs_to_jiffies(500));
 }
 
 /**
@@ -3211,6 +3164,8 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	/* Acquire PHC and system timer to restore after reset */
 	ptp->reset_time = ktime_get_real_ns();
+	if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
+		ptp->phc_reset_time = 0;
 }
 
 /**
@@ -3250,13 +3205,13 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
 	if (err)
 		goto err_unlock;
 
-	/* Write the initial Time value to PHY and LAN using the cached PHC
-	 * time before the reset and time difference between stopping and
-	 * starting the clock.
+	/* Write the initial Time value to PHY and LAN using the PHC time
+	 * before the reset and time difference between stopping and starting
+	 * the clock.
 	 */
-	if (ptp->cached_phc_time) {
+	if (ptp->phc_reset_time) {
 		time_diff = ktime_get_real_ns() - ptp->reset_time;
-		ts = ns_to_timespec64(ptp->cached_phc_time + time_diff);
+		ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
 	} else {
 		ts = ktime_to_timespec64(ktime_get_real());
 	}

-- 
2.56.0.215.g47fdf2208c5b

[PATCH iwl-next v2 4/4] ice: use struct timecounter for E825 and E830

[Jacob Keller]

The ice driver maintains PTP clock time in the MAC with a 96-bit register.
Timestamps are captured in the PHY which only stores 64 bits, only
including the lower 32-bits of nanoseconds. The driver performs clock
adjustments using a hardware timer command mechanism, where the clock
adjustment is programmed into shadow registers in both the MAC and PHY, and
then a timer command is initiated to synchronously update the timers.

Since the PHY only maintains the lower 32 bits of nanoseconds, adjustments
are also limited such that the maximum atomic adjustment is ~2.1 seconds
(2^31-1 nanoseconds).

For large offsets, the driver uses a non-atomic flow where the current time
is read, an adjustment is applied in software, and then written back. This
necessarily loses some elapsed time, which applications must deal with. For
particular servo configurations, where the parameters are tuned for a very
slow servo, this error can take a long time to finish correction.

Worst, on E825 in specific, the adjustment in the PHY does not always work
reliably. The reason for this is subtle, and is a quirk of how the
timestamps are actually generated. The timestamps for Tx and Rx packets
come from the PCS timer rather than directly from the PHY timer. The PCS
timer is derived from internal PHY signaling.

To keep the PCS timer and PHY timer in sync, every few cycles (testing
seems to indicate every 8 cycles) the PHY timer is copied to the PCS timer.
On other cycles, the PCS timer is incremented by a value determined from
the PCS clock rate.

Crucially, the copy of the PHY timer is skipped if the current value of the
PHY timer is lower than the PCS timer (*and* on this cycle the regular PCS
clock increment is also skipped).

During normal operation, the PCS timer will tick from its own source for 7
ticks then copy the PCS timer. If the PCS timer is slightly ahead, then PHY
timer update is skipped, and the PCS timer skips an update every 8 clock
cycles until it is back in alignment with the PHY timer.

Unfortunately, this hardware logic did not take into account manual
adjustments to the PHY. If the PHY timer is adjusted backwards, (or if a
forward adjustment causes the PHY timer to overflow), the PHY to PCS sync
will not occur again until the PCS timer wraps back around to zero. In the
worst case, an adjustment could land the PHY timer to just below the PCS
timer just after it wraps around. PHY to PCS syncs would stop for ~3.6
seconds.

During this time, the Tx and Rx timestamps will read from the PCS timer.
These timestamps will be fed into the timestamp extension algorithm which
will produce incoherent results, and data will be fed into the PTP
application servo. During the invalid window, faulty timestamps will cause
the servo to steer incorrectly, likely away from the true target time.
After the PCS timer synchronizes again, the timestamps will return to
normal. However, the servo may have steered the clock incorrectly. In
default ptp4l configurations, direct clock adjustment is only performed
once during startup. This forces the application to use frequency slewing
to correct any remaining offset. With slow servo parameters, this can take
a *very* long time. In some setups, we had reports of taking several
minutes to nearly an hour before stabilizing, and in some configurations
the servo was unable to ever recover. Note that this issue with the PCS
synchronization happens regardless of *how* the PHY timer is set, either
with the ICE_PTP_ADJ_TIME or ICE_PTP_INIT_TIME.

Note that adjustments work properly on the MAC timer, and indeed on the
first-layer PHY timer value that the driver can read over the sideband
queue. Only the PCS timer (which is inaccessible to the driver) fails to
adjust. This made tracking the precise origin of the failure difficult.

To handle this, the most robust solution is to simply not adjust the
hardware clock directly. The kernel already has struct timecounter and
struct cyclecounter for representing hardware counters that cannot handle
64-bits of precision. Indeed, the PHY timer already is only 32 bits, and
ice has used its own custom logic in ice_ptp_extend_32b_ts to extend
timestamps without using a timecounter.

This makes switching to a timecounter fairly straight forward in most
cases. However, such a switch covers many product families and devices. To
limit the initial scope, implement the timecounter only for the newer E825
and E830 devices. Older E810 and E822 platform devices will currently
remain using the legacy approach which has been in use for years. We may
opt to convert all devices in the future, but for now limiting the scope
keeps from potentially regressing legacy deployments.

The timecounter and cyclecounter are embedded within the recently
introduced ice_phc structure. They are initialized as part of ice_adapter.
Locking and concurrency requirements for access are a bit awkward. The
timestamps are extended from multiple places including the Rx hotpath, the
Tx IRQ function on E830, and the Tx IRQ thread function on E825. The
timecounter is updated as part of .adjtime and .settime64 callbacks, as
well as periodically from the clock owner kworker. The updates are expected
to be rare relative to reads, and we want to avoidt  a global lock on the
readers.

Original versions of this used a seqlock_t which allows multiple readers,
but unfortunately had issues because the updates necessarily call the
read() function of the cyclecounter, which acquires a spinlock to access
the time registers. On RT kernels, the spinlock can sleep which violates
the requirements of the seqlock_t (according to Sashiko AI reports).

Instead, use a seqcount_latch_t along with two copies of the timecounter
structure in the ice_adapter. The sequence counter allows readers to
retry until they get a consistent snapshot. The seqcount_latch_t directs
the reader to the appropriate copy, to allow readers to fully preempt the
writer. The timecounters are fully initialized as part of ice_adapter and
all updates only ever modify their cycle_last and nsec fields.

Writers will first perform the update on the first timecounter, then latch
and finish by copying the values from the first counter to the second.
Readers that preempt the writer will be directed to the non-modified copy
and see a clean status. If the writer preempts the reader, then the
sequence counter is invalid and the reader will re-try.

The reader logic is embedded in ice_phc_cyc2time which wraps the
timecounter_cyc2time and handles the seqcount_latch_t. Similarly, writer
logic is embedded within ice_phc_refresh(), ice_phc_init(), and
ice_phc_adjtime().

In most places, the timecounter is a drop-in replacement for existing
logic, including using ice_phc_init instead of ice_ptp_write_init and
ice_phc_adjtime instead of ice_ptp_write_adj.

The ice_ptp_update_cached_phctime will conditionally handle whether to
update the cached_time value, or perform a timecounter_read(). To minimize
the risk of contention, this is the only place which actually performs a
timecounter_read(). In this way, only the periodic thread and the actual
modifications through reset, .settime64 and .adjtime need the writer lock.

All other accesses to time values use ice_phc_cyc2time when
phc->use_timecounter is enabled, ensuring that the timecounter phase offset
is handled, including external timestamp events, gettime, cross
timestamping, and periodic output signals.

The SO_TX_TIME needs to compensate for the timecounter phase and uses
ice_phc_get_phase() which computes the difference between the cycle_last
and nsec fields. Note that the cyclecounter mask is 32bits to allow
extending the shorter PHY timestamps. However, the cyclecounter still reads
and reports the "full" 64 bit value from the MAC. This is important since
phase adjustments need to convert from the timecounter to the MAC timer
which keeps counting the higher bits after a rollover.

Periodic output signal programming is a slightly tricky conversion:
The ice_ptp_cfg_perout() function programs the SDP output signals. For
timecounter, the clk time is adjusted using ice_phc_cyc2time to convert to
the timecounter domain. Then, the timecounter phase is subtracted from the
requested start time. This works because the MAC timer still maintains the
full 96 bits, so even if it overflows, we can still reliably program start
times more than 4 seconds in the future.

Finally, VF PTP features are disabled for the devices using the
timecounter. To support this feature, the virtchnl interface must be
extended to handle the timecounter phase appropriately which will require
its own development work including modification to the VF driver. This will
be investigated and planned in the future.

Careful readers may note that the driver still performs ICE_PTP_ADJ_TIME on
the PHY timers after a device reset and when the PHY needs to be
reinitialized at link up. The PHY must be adjusted to align with the MAC
time in order for timestamps to be accurate. These adjustments are safe
because the PHY restart procedure performs a PHY soft reset which forces
the PHY to reinitialize to zero, and then be forward corrected as part of
the ice_sync_phy_timer_eth56g() procedure. The soft reset was introduced as
part of commit 24e335fff462 ("ice: E825: perform a soft reset when starting
the PHY timer").

Although the particular hardware issue is only known to affect E825, this
timecounter conversion is applied to both E825 and E830 family. These two
products are the most recent devices and consistency between their behavior
is desired. Older products could benefit from the timecounter (as
arbitrarily large offsets may be programmed atomically). However, at this
time it was deemed safest to initially implement timecounter only for the
new devices, leaving legacy implementation alone. After some time is given
to prove out the implementation and ensure all remaining gaps are closed,
we may opt to convert all devices and drop the legacy code entirely.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.h   |  40 ++-
 drivers/net/ethernet/intel/ice/ice_ptp.h       |  12 +
 drivers/net/ethernet/intel/ice/ice_trace.h     |  18 +-
 drivers/net/ethernet/intel/ice/ice_txrx.h      |   1 +
 drivers/net/ethernet/intel/ice/ice_adapter.c   |  47 +++
 drivers/net/ethernet/intel/ice/ice_ptp.c       | 399 +++++++++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_txrx.c      |   7 +
 drivers/net/ethernet/intel/ice/virt/virtchnl.c |   7 +-
 8 files changed, 488 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index acde4031d528..b6af88967043 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -8,6 +8,8 @@
 #include <linux/mutex.h>
 #include <linux/rwsem.h>
 #include <linux/srcu.h>
+#include <linux/seqlock.h>
+#include <linux/timecounter.h>
 #include <linux/spinlock_types.h>
 #include <linux/refcount_types.h>
 
@@ -37,21 +39,51 @@ struct ice_port_list {
 
 /**
  * struct ice_phc - PTP Hardware Clock fields
+ *
+ * Structure used for handling the timestamp extension logic for this device.
+ * Devices will either use the legacy timestamp extension logic or
+ * a timecounter, not both.
+ *
+ * For simplicity, the timecounter fields are initialized as part of creating
+ * the ice_adapter regardless of whether the device will actually use them.
+ *
+ * Legacy timestamp extension fields:
  * @cached_time: Cached copy of the PHC time for timestamp extension
+ *
+ * struct timecounter fields:
+ * @tc: timecounter representing the current time. Accessed via seq to allow
+ *      lockless readers during updates.
+ * @cc: cyclecounter underlying the timecounter
+ * @seq: seqcount_latch_t used to protect readers and direct to the
+ *       appropriate copy during updates.
+ *
+ * General fields:
  * @update_jiffies: Jiffies when PHC was last updated
  * @lock: For protecting updates to the PHC fields.
+ * @use_timecounter: if true, the device uses the timecounter
  *
- * Readers are lockless and rely on READ_ONCE/WRITE_ONCE for the cached_time
- * and smp_acquire/smp_release for the update_jiffies field.
+ * Readers are lockless. Devices without a timecounter rely on
+ * READ_ONCE/WRITE_ONCE of the cached_time for consistency. For devices with
+ * a timecounter, readers rely on the latched sequence counter to be directed
+ * to a copy of the timecounter not being modified. Both styles depend on
+ * smp_load_acquire/smp_store_release for checking update_jiffies.
  *
  * Writers must acquire the lock mutex to ensure that the periodic task cannot
- * write back an out-of-date cached value after a clock adjustment.
+ * race with clock adjustment callbacks.
  */
 struct ice_phc {
+	/* Legacy timestamp extension fields */
 	u64 cached_time;
+
+	/* struct timecounter fields */
+	struct timecounter tc[2];
+	struct cyclecounter cc;
+	seqcount_latch_t seq;
+
 	unsigned long update_jiffies;
-	/* Serialize updates to cached_time */
+	/* Serialize updates to cached_time or the timecounter */
 	struct mutex lock;
+	bool use_timecounter;
 };
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 430441b43150..7065139005bf 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -330,6 +330,8 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf);
 bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq);
 int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 			     struct ptp_system_timestamp *sts, u64 *time);
+u64 ice_ptp_read_cycles(struct cyclecounter *cc);
+s64 ice_phc_get_phase(struct ice_phc *phc);
 
 u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 			const struct ice_pkt_ctx *pkt_ctx);
@@ -388,6 +390,16 @@ static inline int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 	return -EOPNOTSUPP;
 }
 
+static inline u64 ice_ptp_read_cycles(struct cyclecounter *cc)
+{
+	return 0;
+}
+
+static inline s64 ice_phc_get_phase(struct ice_phc *phc)
+{
+	return 0;
+}
+
 static inline u64
 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 		    const struct ice_pkt_ctx *pkt_ctx)
diff --git a/drivers/net/ethernet/intel/ice/ice_trace.h b/drivers/net/ethernet/intel/ice/ice_trace.h
index 7c43683a77d4..40c710a1d8b7 100644
--- a/drivers/net/ethernet/intel/ice/ice_trace.h
+++ b/drivers/net/ethernet/intel/ice/ice_trace.h
@@ -263,19 +263,27 @@ DEFINE_TX_TSTAMP_OP_EVENT(ice_tx_tstamp_complete);
 
 /* Trace event for Rx timestamp */
 TRACE_EVENT(ice_rx_hwtstamp,
-	    TP_PROTO(u64 cached_time, u32 ts_high, u64 ts_ns),
+	    TP_PROTO(int timecounter, u64 cycle_last, u64 nsec,
+		     u64 cached_time, u32 ts_high, u64 ts_ns),
 
-	    TP_ARGS(cached_time, ts_high, ts_ns),
+	    TP_ARGS(timecounter, cycle_last, nsec, cached_time, ts_high, ts_ns),
 
-	    TP_STRUCT__entry(__field(u64, cached_time)
+	    TP_STRUCT__entry(__field(int, timecounter)
+			     __field(u64, cycle_last)
+			     __field(u64, nsec)
+			     __field(u64, cached_time)
 			     __field(u32, ts_high)
 			     __field(u64, ts_ns)),
 
-	    TP_fast_assign(__entry->cached_time = cached_time;
+	    TP_fast_assign(__entry->timecounter = timecounter;
+			   __entry->cycle_last = cycle_last;
+			   __entry->nsec = nsec;
+			   __entry->cached_time = cached_time;
 			   __entry->ts_high = ts_high;
 			   __entry->ts_ns = ts_ns;),
 
-	    TP_printk("cached_time=%llu ts_high=%u ts_ns=%llu",
+	    TP_printk("timecounter=%d cycle_last=%llu nsec=%llu cached_time=%llu ts_high=%u ts_ns=%llu",
+		      __entry->timecounter, __entry->cycle_last, __entry->nsec,
 		      __entry->cached_time, __entry->ts_high, __entry->ts_ns)
 );
 
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index 83a34322e0d3..095513c22ebc 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -266,6 +266,7 @@ enum ice_dynamic_itr {
 /* descriptor ring, associated with a VSI */
 struct ice_tstamp_ring {
 	struct ice_tx_ring *tx_ring;	/* Backreference to associated Tx ring */
+	struct ice_phc *phc;
 	dma_addr_t dma;			/* physical address of ring */
 	struct rcu_head rcu;            /* to avoid race on free */
 	u8 __iomem *tail;
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index f3ee5e60613e..ef057fa419e0 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/seqlock.h>
 #include <linux/srcu.h>
 #include <linux/xarray.h>
 #include "ice_adapter.h"
@@ -41,6 +42,41 @@ static u64 ice_adapter_index(struct pci_dev *pdev)
 	}
 }
 
+static bool ice_adapter_use_timecounter(struct pci_dev *pdev)
+{
+	switch (pdev->device) {
+	/* E825 */
+	case ICE_DEV_ID_E825C_BACKPLANE:
+	case ICE_DEV_ID_E825C_QSFP:
+	case ICE_DEV_ID_E825C_SFP:
+	case ICE_DEV_ID_E825C_SGMII:
+	/* E830 */
+	case ICE_DEV_ID_E830CC_BACKPLANE:
+	case ICE_DEV_ID_E830CC_QSFP56:
+	case ICE_DEV_ID_E830CC_SFP:
+	case ICE_DEV_ID_E830CC_SFP_DD:
+	case ICE_DEV_ID_E830C_BACKPLANE:
+	case ICE_DEV_ID_E830_XXV_BACKPLANE:
+	case ICE_DEV_ID_E830C_QSFP:
+	case ICE_DEV_ID_E830_XXV_QSFP:
+	case ICE_DEV_ID_E830C_SFP:
+	case ICE_DEV_ID_E830_XXV_SFP:
+	case ICE_DEV_ID_E835CC_BACKPLANE:
+	case ICE_DEV_ID_E835CC_QSFP56:
+	case ICE_DEV_ID_E835CC_SFP:
+	case ICE_DEV_ID_E835C_BACKPLANE:
+	case ICE_DEV_ID_E835C_QSFP:
+	case ICE_DEV_ID_E835C_SFP:
+	case ICE_DEV_ID_E835_L_BACKPLANE:
+	case ICE_DEV_ID_E835_L_QSFP:
+	case ICE_DEV_ID_E835_L_SFP:
+		return true;
+	}
+
+	/* Keep legacy approach for older devices */
+	return false;
+}
+
 static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
 {
 	u64 index = ice_adapter_index(pdev);
@@ -83,8 +119,19 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 
 	mutex_init(&adapter->ps_lock);
 
+	adapter->phc.cc.mask = CYCLECOUNTER_MASK(32);
+	adapter->phc.cc.mult = 1;
+	adapter->phc.cc.shift = 0;
+	adapter->phc.cc.read = ice_ptp_read_cycles;
+
+	timecounter_init(&adapter->phc.tc[0], &adapter->phc.cc, 0);
+	timecounter_init(&adapter->phc.tc[1], &adapter->phc.cc, 0);
+
+	seqcount_latch_init(&adapter->phc.seq);
 	mutex_init(&adapter->phc.lock);
 
+	adapter->phc.use_timecounter = ice_adapter_use_timecounter(pdev);
+
 	return adapter;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a0451c1083a4..b48d1bf3bbf5 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -272,6 +272,202 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 	return 0;
 }
 
+/**
+ * ice_ptp_read_cycles - cyclecounter read() callback function
+ * @cc: the hardware cycle counter structure
+ *
+ * Read the PHC time via ice_ptp_read_src_clk_reg() and report it for use
+ * within timecounter_read() and timecounter_init().
+ *
+ * Although the cycle counter is masked to 32-bits for timestamp extension,
+ * this still reports the full 64-bit time to enable determining the phase
+ * difference between the PHC hardware value and the timecounter properly.
+ *
+ * Return: The current PHC timer value from the hardware on success.
+ *         Since failure cannot be meaningfully reported, return the previous
+ *         cycle_last value when unable to access the device.
+ */
+u64 ice_ptp_read_cycles(struct cyclecounter *cc)
+{
+	struct ice_adapter *adapter;
+	struct ice_phc *phc;
+	struct ice_pf *pf;
+	u64 time;
+	int err;
+
+	phc = container_of(cc, struct ice_phc, cc);
+	adapter = container_of(phc, struct ice_adapter, phc);
+
+	scoped_guard(rcu) {
+		pf = rcu_dereference(adapter->ctrl_pf);
+		if (!pf)
+			return phc->tc[0].cycle_last;
+
+		err = ice_ptp_read_src_clk_reg(pf, NULL, &time);
+		if (err)
+			return phc->tc[0].cycle_last;
+
+		/* Paired with smp_load_acquire in ice_ptp_extend_32b_ts */
+		smp_store_release(&phc->update_jiffies, jiffies | 1);
+		return time;
+	}
+}
+
+/**
+ * ice_phc_get_phase - Determine timecounter phase offset
+ * @phc: pointer to the PHC structure
+ *
+ * Compute the phase difference between the timecounter and the current
+ * hardware clock value. Note that the cycle counter reports the full
+ * 64-bits of nanoseconds, so this computes the phase difference between the
+ * timecounter and the full MAC timer which keeps counting after the lower
+ * 32-bits of GLTSYN_TIME_L overflow.
+ *
+ * Return: phase difference between the timecounter and the MAC timer.
+ */
+s64 ice_phc_get_phase(struct ice_phc *phc)
+{
+	unsigned int seq, idx;
+	s64 phase;
+
+	do {
+		seq = read_seqcount_latch(&phc->seq);
+		idx = seq & 0x1;
+
+		phase = phc->tc[idx].nsec - phc->tc[idx].cycle_last;
+	} while (read_seqcount_latch_retry(&phc->seq, seq));
+
+	return phase;
+}
+
+/**
+ * ice_phc_cyc2time_with_trace - Wrapper for timecounter_cyc2time
+ * @phc: pointer to the PHC structure
+ * @cycles: the hardware timestamp to convert
+ * @trace_rx: True if this call should trigger an Rx timestamp trace
+ *
+ * Wrapper for timecounter_cyc2time which converts hardware timestamps. Note
+ * that this works for both 32-bit PHY timestamps and 64-bit MAC timestamps,
+ * since the cyclecounter mask is set to 32-bits.
+ *
+ * The seqcount_latch protects against concurrent updates by directing the
+ * reader to use the appropriate copy of the timecounter while a writer is
+ * active. The reader may convert according to the old time domain, but will
+ * retry if the conversion could be inconsistent.
+ *
+ * Return: the timestamp converted to the timecounter domain.
+ */
+static u64 ice_phc_cyc2time_with_trace(struct ice_phc *phc,
+				       u64 cycles, bool trace_rx)
+{
+	u64 ns, cycle_last = 0, nsec = 0;
+	unsigned int seq, idx;
+
+	do {
+		seq = read_seqcount_latch(&phc->seq);
+		idx = seq & 0x1;
+		if (ice_trace_enabled(rx_hwtstamp) && trace_rx) {
+			cycle_last = phc->tc[idx].cycle_last;
+			nsec = phc->tc[idx].nsec;
+		}
+		ns = timecounter_cyc2time(&phc->tc[idx], cycles);
+	} while (read_seqcount_latch_retry(&phc->seq, seq));
+
+	if (trace_rx)
+		ice_trace(rx_hwtstamp, phc->use_timecounter,
+			  cycle_last, nsec, 0, (u32)cycles, ns);
+
+	return ns;
+}
+
+/**
+ * ice_phc_cyc2time - Wrapper for timecounter_cyc2time
+ * @phc: pointer to the PHC structure
+ * @cycles: the hardware timestamp to convert
+ *
+ * See ice_phc_cyc2time_with_trace().
+ *
+ * Return: the timestamp converted to the timecounter domain.
+ */
+static u64 ice_phc_cyc2time(struct ice_phc *phc, u64 cycles)
+{
+	return ice_phc_cyc2time_with_trace(phc, cycles, false);
+}
+
+/**
+ * ice_phc_refresh - Read PHC timecounter to refresh phase offset
+ * @phc: pointer to the PHC structure
+ *
+ * Called from ice_ptp_update_cached_phctime() to refresh the timecounter
+ * periodically. The latched sequence counter is used to ensure readers get
+ * a consistent view of the timecounter phase. The frac, mask, and cc fields
+ * always remain their initial values, so only cycle_last and nsec need to be
+ * propagated to the secondary timecounter structure.
+ *
+ * To avoid contention, timecounter_read() is not used in any other flow, and
+ * its return value is ignored.
+ */
+static void ice_phc_refresh(struct ice_phc *phc)
+{
+	lockdep_assert_held(&phc->lock);
+
+	write_seqcount_latch_begin(&phc->seq);
+	(void)timecounter_read(&phc->tc[0]);
+	write_seqcount_latch(&phc->seq);
+	/* Update the secondary copy */
+	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
+	phc->tc[1].nsec = phc->tc[0].nsec;
+	write_seqcount_latch_end(&phc->seq);
+}
+
+/**
+ * ice_phc_init - Initialize PHC timecounter to new start value
+ * @phc: pointer to the PHC structure
+ * @init_tstamp: New initial value for the timecounter
+ *
+ * Initialize the PHC timecounter to a new offset. The latched sequence
+ * counter is used to ensure readers get a consistent view of the timecounter
+ * phase. We always pass the same cyclecounter structure, so the mask, frac,
+ * and cc fields always remain the same, and only the cycle_last and nsec
+ * fields must be propagated to the secondary structure.
+ */
+static void ice_phc_init(struct ice_phc *phc, u64 init_tstamp)
+{
+	lockdep_assert_held(&phc->lock);
+
+	write_seqcount_latch_begin(&phc->seq);
+	timecounter_init(&phc->tc[0], &phc->cc, init_tstamp);
+	write_seqcount_latch(&phc->seq);
+	/* Update the secondary copy */
+	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
+	phc->tc[1].nsec = phc->tc[0].nsec;
+	write_seqcount_latch_end(&phc->seq);
+}
+
+/**
+ * ice_phc_adjtime - Adjust PHC timecounter by delta
+ * @phc: pointer to the PHC structure
+ * @delta: the delta to adjust by
+ *
+ * Adjust the PHC timecounter offset by the provided delta. The latched
+ * sequence counter is used to ensure readers get a consistent view of the
+ * timecounter phase. The frac, mask, and cc fields always remain their
+ * initial values, so only cycle_last and nsec need to be propagated to the
+ * secondary timecounter structure.
+ */
+static void ice_phc_adjtime(struct ice_phc *phc, s64 delta)
+{
+	lockdep_assert_held(&phc->lock);
+
+	write_seqcount_latch_begin(&phc->seq);
+	timecounter_adjtime(&phc->tc[0], delta);
+	write_seqcount_latch(&phc->seq);
+	/* Update the secondary copy */
+	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
+	phc->tc[1].nsec = phc->tc[0].nsec;
+	write_seqcount_latch_end(&phc->seq);
+}
+
 /**
  * ice_ptp_extend_32b_ts - Convert a 32b nanoseconds timestamp to 64b
  * @phc: PTP Hardware Clock structure
@@ -283,6 +479,12 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
  * Note that the captured timestamp values may be 40 bits, but the lower
  * 8 bits are sub-nanoseconds and generally discarded.
  *
+ * For devices which use the timecounter, call ice_phc_cyc2time_with_trace to
+ * convert the hardware cycles, accounting for the phase offset of the
+ * timecounter.
+ *
+ * For other devices not using the timecounter:
+ *
  * Extend the 32bit nanosecond timestamp using the following algorithm and
  * assumptions:
  *
@@ -330,6 +532,9 @@ static u64 ice_ptp_extend_32b_ts(struct ice_phc *phc,
 	if (time_is_before_jiffies(discard_time))
 		return 0;
 
+	if (phc->use_timecounter)
+		return ice_phc_cyc2time_with_trace(phc, in_tstamp, trace_rx);
+
 	cached_phc_time = READ_ONCE(phc->cached_time);
 	phc_time_lo = (u32)cached_phc_time;
 
@@ -352,7 +557,8 @@ static u64 ice_ptp_extend_32b_ts(struct ice_phc *phc,
 	}
 
 	if (trace_rx)
-		ice_trace(rx_hwtstamp, cached_phc_time, in_tstamp, ns);
+		ice_trace(rx_hwtstamp, phc->use_timecounter,
+			  0, 0, cached_phc_time, in_tstamp, ns);
 
 	return ns;
 }
@@ -1022,19 +1228,17 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
  * ice_ptp_update_cached_phctime - Update the cached PHC time value
  * @pf: Board specific private structure
  *
- * Update the PTP hardware clock time values which are cached in the PHC
- * structure.
+ * For devices that use a timecounter structure, update the timecounter.
+ * Otherwise, update the cached PHC time used for timestamp extension.
  *
  * This function must be called periodically by the clock owner to ensure that
- * the cached value is never more than 2 seconds old.
+ * the timecounter or cached value is never more than 2 seconds old.
  */
 static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
 {
 	struct ice_phc *phc = &pf->adapter->phc;
 	struct device *dev = ice_pf_to_dev(pf);
 	unsigned long update_before;
-	u64 systime;
-	int err;
 
 	mutex_lock(&phc->lock);
 
@@ -1047,12 +1251,20 @@ static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
 		pf->ptp.late_cached_phc_updates++;
 	}
 
-	/* Read the current PHC time */
-	err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
-	if (!err) {
-		WRITE_ONCE(phc->cached_time, systime);
-		/* Paired with smp_load_acquire in ice_ptp_extend_32b_ts */
-		smp_store_release(&phc->update_jiffies, jiffies | 1);
+	if (phc->use_timecounter) {
+		ice_phc_refresh(phc);
+	} else {
+		u64 systime;
+		int err;
+
+		err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
+		if (!err) {
+			WRITE_ONCE(phc->cached_time, systime);
+			/* Paired with smp_load_acquire in
+			 * ice_ptp_extend_32b_ts
+			 */
+			smp_store_release(&phc->update_jiffies, jiffies | 1);
+		}
 	}
 
 	mutex_unlock(&phc->lock);
@@ -1598,6 +1810,7 @@ static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
  */
 void ice_ptp_extts_event(struct ice_pf *pf)
 {
+	struct ice_phc *phc = &pf->adapter->phc;
 	struct ptp_clock_event event;
 	struct ice_hw *hw = &pf->hw;
 	u8 chan, tmr_idx;
@@ -1622,7 +1835,11 @@ void ice_ptp_extts_event(struct ice_pf *pf)
 
 		lo = rd32(hw, GLTSYN_EVNT_L(chan, tmr_idx));
 		hi = rd32(hw, GLTSYN_EVNT_H(chan, tmr_idx));
-		event.timestamp = (u64)hi << 32 | lo;
+
+		if (phc->use_timecounter)
+			event.timestamp = ice_phc_cyc2time(phc, lo);
+		else
+			event.timestamp = (u64)hi << 32 | lo;
 
 		/* Add delay compensation */
 		pin_desc_idx = ice_ptp_find_pin_idx(pf, PTP_PF_EXTTS, chan);
@@ -1824,8 +2041,9 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan,
 static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
 			      int on)
 {
+	u64 clk, period, start, phase, cycles, tc_phase;
+	struct ice_phc *phc = &pf->adapter->phc;
 	unsigned int gpio_pin, prop_delay_ns;
-	u64 clk, period, start, phase;
 	struct ice_hw *hw = &pf->hw;
 	int pin_desc_idx;
 	int err;
@@ -1870,13 +2088,23 @@ static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
 	err = ice_ptp_read_src_clk_reg(pf, NULL, &clk);
 	if (err)
 		return err;
+	if (phc->use_timecounter) {
+		cycles = clk;
+		clk = ice_phc_cyc2time(phc, cycles);
+		tc_phase = clk - cycles;
+	}
 	clk += NSEC_PER_MSEC * 500;
+
 	if (rq->flags & PTP_PEROUT_PHASE || start <= clk - prop_delay_ns)
 		start = div64_u64(clk + period - 1, period) * period + phase;
 
 	/* Compensate for propagation delay from the generator to the pin. */
 	start -= prop_delay_ns;
 
+	/* Compensate for timecounter phase offset, if necessary */
+	if (phc->use_timecounter)
+		start -= tc_phase;
+
 	return ice_ptp_write_perout(hw, rq->index, gpio_pin, start, period);
 }
 
@@ -1974,7 +2202,10 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
 		if (on && pf->ptp.state != ICE_PTP_READY)
 			return -EIO;
 
+		/* Serialize against concurrent timecounter updates */
+		mutex_lock(&pf->adapter->phc.lock);
 		err = ice_ptp_cfg_perout(pf, &rq->perout, on);
+		mutex_unlock(&pf->adapter->phc.lock);
 		if (!err) {
 			*cached = rq->perout;
 		} else {
@@ -2010,8 +2241,7 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
  * @sts: Optional parameter for holding a pair of system timestamps from
  *       the system clock. Will be ignored if NULL is given.
  *
- * Read the device clock and return the correct value on ns, after converting it
- * into a timespec struct.
+ * Read the device time from the PHC and return it as a timespec.
  */
 static int
 ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts,
@@ -2025,6 +2255,9 @@ ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts,
 	if (err)
 		return err;
 
+	if (pf->adapter->phc.use_timecounter)
+		time_ns = ice_phc_cyc2time(&pf->adapter->phc, time_ns);
+
 	*ts = ns_to_timespec64(time_ns);
 	return 0;
 }
@@ -2086,6 +2319,43 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
 	return 0;
 }
 
+/**
+ * ice_ptp_settime64_tc - Initialize the PHC timecounter to requested time
+ * @info: the driver's PTP info structure
+ * @ts: timespec64 structure that holds the new time value.
+ *
+ * Reset the PHC timecounter to the provided user input value.
+ *
+ * Return: 0 on success, -EIO if PTP state is not ready.
+ */
+static int ice_ptp_settime64_tc(struct ptp_clock_info *info,
+				const struct timespec64 *ts)
+{
+	struct ice_pf *pf = ptp_info_to_pf(info);
+	struct ice_phc *phc = &pf->adapter->phc;
+	u64 ns;
+
+	if (pf->ptp.state != ICE_PTP_READY)
+		return -EIO;
+
+	ns = timespec64_to_ns(ts);
+
+	mutex_lock(&phc->lock);
+
+	ice_ptp_disable_all_perout(pf);
+
+	ice_phc_init(phc, ns);
+
+	ice_ptp_mark_all_tx_tracker_stale(pf);
+
+	/* Reenable periodic outputs */
+	ice_ptp_enable_all_perout(pf);
+
+	mutex_unlock(&phc->lock);
+
+	return 0;
+}
+
 /**
  * ice_ptp_adjtime_nonatomic - Do a non-atomic clock adjustment
  * @info: the driver's PTP info structure
@@ -2156,6 +2426,37 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
 	return 0;
 }
 
+/**
+ * ice_ptp_adjtime_tc - Adjust the timecounter offset by the indicated delta
+ * @info: the driver's PTP info structure
+ * @delta: Offset in nanoseconds to adjust the time by
+ *
+ * Return: 0 on success, -EIO if PTP state is not ready.
+ */
+static int ice_ptp_adjtime_tc(struct ptp_clock_info *info, s64 delta)
+{
+	struct ice_pf *pf = ptp_info_to_pf(info);
+	struct ice_phc *phc = &pf->adapter->phc;
+
+	if (pf->ptp.state != ICE_PTP_READY)
+		return -EIO;
+
+	mutex_lock(&phc->lock);
+
+	ice_ptp_disable_all_perout(pf);
+
+	ice_phc_adjtime(phc, delta);
+
+	ice_ptp_mark_all_tx_tracker_stale(pf);
+
+	/* Reenable periodic outputs */
+	ice_ptp_enable_all_perout(pf);
+
+	mutex_unlock(&phc->lock);
+
+	return 0;
+}
+
 /**
  * struct ice_crosststamp_cfg - Device cross timestamp configuration
  * @lock_reg: The hardware semaphore lock to use
@@ -2291,6 +2592,11 @@ static int ice_capture_crosststamp(ktime_t *device,
 	ts_lo = rd32(hw, cfg->dev_time_l[tmr_idx]);
 	ts_hi = rd32(hw, cfg->dev_time_h[tmr_idx]);
 	ts = ((u64)ts_hi << 32) | ts_lo;
+
+	/* Account for timecounter phase offset if necessary */
+	if (pf->adapter->phc.use_timecounter)
+		ts = ice_phc_cyc2time(&pf->adapter->phc, ts);
+
 	*device = ns_to_ktime(ts);
 
 err_timeout:
@@ -2678,15 +2984,21 @@ static void ice_ptp_set_caps(struct ice_pf *pf)
 		 dev_driver_string(dev), dev_name(dev));
 	info->owner = THIS_MODULE;
 	info->max_adj = 100000000;
-	info->adjtime = ice_ptp_adjtime;
 	info->adjfine = ice_ptp_adjfine;
 	info->gettimex64 = ice_ptp_gettimex64;
-	info->settime64 = ice_ptp_settime64;
 	info->n_per_out = GLTSYN_TGT_H_IDX_MAX;
 	info->n_ext_ts = GLTSYN_EVNT_H_IDX_MAX;
 	info->enable = ice_ptp_gpio_enable;
 	info->verify = ice_verify_pin;
 
+	if (pf->adapter->phc.use_timecounter) {
+		info->adjtime = ice_ptp_adjtime_tc;
+		info->settime64 = ice_ptp_settime64_tc;
+	} else {
+		info->adjtime = ice_ptp_adjtime;
+		info->settime64 = ice_ptp_settime64;
+	}
+
 	info->supported_extts_flags = PTP_RISING_EDGE |
 				      PTP_FALLING_EDGE |
 				      PTP_STRICT_FLAGS;
@@ -3134,6 +3446,7 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	struct ice_ptp *ptp = &pf->ptp;
 	struct ice_hw *hw = &pf->hw;
 	u8 src_tmr;
+	u64 ns;
 
 	if (ptp->state != ICE_PTP_READY)
 		return;
@@ -3164,8 +3477,11 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	/* Acquire PHC and system timer to restore after reset */
 	ptp->reset_time = ktime_get_real_ns();
-	if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
-		ptp->phc_reset_time = 0;
+	if (ice_ptp_read_src_clk_reg(pf, NULL, &ns))
+		ns = 0;
+	if (ns && pf->adapter->phc.use_timecounter)
+		ns = ice_phc_cyc2time(&pf->adapter->phc, ns);
+	ptp->phc_reset_time = ns;
 }
 
 /**
@@ -3177,10 +3493,10 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
  */
 static int ice_ptp_rebuild_owner(struct ice_pf *pf)
 {
+	struct ice_phc *phc = &pf->adapter->phc;
 	struct ice_ptp *ptp = &pf->ptp;
 	struct ice_hw *hw = &pf->hw;
-	struct timespec64 ts;
-	u64 time_diff;
+	u64 time_diff, ns;
 	int err;
 
 	err = ice_ptp_init_phc(hw);
@@ -3211,13 +3527,22 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
 	 */
 	if (ptp->phc_reset_time) {
 		time_diff = ktime_get_real_ns() - ptp->reset_time;
-		ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
+		ns = ptp->phc_reset_time + time_diff;
 	} else {
-		ts = ktime_to_timespec64(ktime_get_real());
+		ns = ktime_get_real_ns();
+	}
+
+	if (phc->use_timecounter) {
+		mutex_lock(&phc->lock);
+		ice_phc_init(phc, ns);
+		mutex_unlock(&phc->lock);
+	} else {
+		struct timespec64 ts = ns_to_timespec64(ns);
+
+		err = ice_ptp_write_init(pf, &ts);
+		if (err)
+			goto err_unlock;
 	}
-	err = ice_ptp_write_init(pf, &ts);
-	if (err)
-		goto err_unlock;
 
 	/* Release the global hardware lock */
 	ice_ptp_unlock(hw);
@@ -3406,8 +3731,8 @@ int ice_ptp_clock_index(struct ice_pf *pf)
  */
 static int ice_ptp_init_owner(struct ice_pf *pf)
 {
+	struct ice_phc *phc = &pf->adapter->phc;
 	struct ice_hw *hw = &pf->hw;
-	struct timespec64 ts;
 	int err;
 
 	err = ice_ptp_init_phc(hw);
@@ -3435,11 +3760,19 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
 	if (err)
 		goto err_unlock;
 
-	ts = ktime_to_timespec64(ktime_get_real());
-	/* Write the initial Time value to PHY and LAN */
-	err = ice_ptp_write_init(pf, &ts);
-	if (err)
-		goto err_unlock;
+	if (phc->use_timecounter) {
+		mutex_lock(&phc->lock);
+		ice_phc_init(phc, ktime_get_real_ns());
+		mutex_unlock(&phc->lock);
+	} else {
+		struct timespec64 ts;
+
+		/* Write the initial Time value to PHY and LAN */
+		ktime_get_real_ts64(&ts);
+		err = ice_ptp_write_init(pf, &ts);
+		if (err)
+			goto err_unlock;
+	}
 
 	/* Release the global hardware lock */
 	ice_ptp_unlock(hw);
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 8576f186480f..6d4d97ac1d6d 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -395,6 +395,7 @@ static bool ice_clean_tx_irq(struct ice_tx_ring *tx_ring, int napi_budget)
  */
 static int ice_alloc_tstamp_ring(struct ice_tx_ring *tx_ring)
 {
+	struct ice_pf *pf = tx_ring->vsi->back;
 	struct ice_tstamp_ring *tstamp_ring;
 
 	/* allocate with kzalloc(), free with kfree_rcu() */
@@ -403,6 +404,7 @@ static int ice_alloc_tstamp_ring(struct ice_tx_ring *tx_ring)
 		return -ENOMEM;
 
 	tstamp_ring->tx_ring = tx_ring;
+	tstamp_ring->phc = &pf->adapter->phc;
 	tx_ring->tstamp_ring = tstamp_ring;
 	tstamp_ring->desc = NULL;
 	tstamp_ring->count = ice_calc_ts_ring_count(tx_ring);
@@ -1520,6 +1522,7 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first,
 	if (ice_is_txtime_cfg(tx_ring)) {
 		struct ice_tstamp_ring *tstamp_ring;
 		struct ice_ts_desc *ts_desc;
+		struct ice_phc *phc;
 		u32 tstamp_count, j;
 		u64 tstamp;
 
@@ -1528,10 +1531,14 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first,
 		if (unlikely(!tstamp_ring))
 			goto ring_kick;
 
+		phc = tstamp_ring->phc;
+
 		tstamp_count = tstamp_ring->count;
 		j = tstamp_ring->next_to_use;
 
 		tstamp = ktime_to_ns(first->skb->tstamp);
+		if (phc->use_timecounter)
+			tstamp -= ice_phc_get_phase(phc);
 		tstamp >>= ICE_TXTIME_CTX_RESOLUTION_128NS;
 
 		ts_desc = ICE_TS_DESC(tstamp_ring, j);
diff --git a/drivers/net/ethernet/intel/ice/virt/virtchnl.c b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
index 66332d02c5ba..d54cd1d34646 100644
--- a/drivers/net/ethernet/intel/ice/virt/virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/virt/virtchnl.c
@@ -326,7 +326,12 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg)
 	if (vf->driver_caps & VIRTCHNL_VF_OFFLOAD_QOS)
 		vfres->vf_cap_flags |= VIRTCHNL_VF_OFFLOAD_QOS;
 
-	if (vf->driver_caps & VIRTCHNL_VF_CAP_PTP)
+	/* Do not enable VF PTP capability on devices which use a timecounter.
+	 * Proper support for these devices requires extensions to the
+	 * virtchnl interface that have not yet been developed.
+	 */
+	if (vf->driver_caps & VIRTCHNL_VF_CAP_PTP &&
+	    !vf->pf->adapter->phc.use_timecounter)
 		vfres->vf_cap_flags |= VIRTCHNL_VF_CAP_PTP;
 
 	vfres->num_vsis = 1;

-- 
2.56.0.215.g47fdf2208c5b

Re: [PATCH iwl-next v2 4/4] ice: use struct timecounter for E825 and E830

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ice: perout_rqs updated outside phc.lock in
  ice_ptp_gpio_enable()
- [Medium] ice: premature update_jiffies store in ice_ptp_read_cycles()
--

commit 2dbce654e6813d30b319e71f8e68ca2b82a4b244
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: use struct timecounter for E825 and E830

This commit switches E825 and E830 devices from direct hardware PHC time
adjustments and legacy 32-bit timestamp extension to a latched
timecounter/cyclecounter stored in struct ice_phc.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a0451c1083a41..b48d1bf3bbf59 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -272,6 +272,202 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
>  	return 0;
>  }
>  
> +/**
> + * ice_ptp_read_cycles - cyclecounter read() callback function
> + * @cc: the hardware cycle counter structure
> + *
> + * Read the PHC time via ice_ptp_read_src_clk_reg() and report it for use
> + * within timecounter_read() and timecounter_init().
> + *
> + * Although the cycle counter is masked to 32-bits for timestamp extension,
> + * this still reports the full 64-bit time to enable determining the phase
> + * difference between the PHC hardware value and the timecounter properly.
> + *
> + * Return: The current PHC timer value from the hardware on success.
> + *         Since failure cannot be meaningfully reported, return the previous
> + *         cycle_last value when unable to access the device.
> + */
> +u64 ice_ptp_read_cycles(struct cyclecounter *cc)
> +{
> +	struct ice_adapter *adapter;
> +	struct ice_phc *phc;
> +	struct ice_pf *pf;
> +	u64 time;
> +	int err;
> +
> +	phc = container_of(cc, struct ice_phc, cc);
> +	adapter = container_of(phc, struct ice_adapter, phc);
> +
> +	scoped_guard(rcu) {
> +		pf = rcu_dereference(adapter->ctrl_pf);
> +		if (!pf)
> +			return phc->tc[0].cycle_last;
> +
> +		err = ice_ptp_read_src_clk_reg(pf, NULL, &time);
> +		if (err)
> +			return phc->tc[0].cycle_last;
> +
> +		/* Paired with smp_load_acquire in ice_ptp_extend_32b_ts */
> +		smp_store_release(&phc->update_jiffies, jiffies | 1);
> +		return time;
> +	}
> +}

[ ... ]

> +static void ice_phc_refresh(struct ice_phc *phc)
> +{
> +	lockdep_assert_held(&phc->lock);
> +
> +	write_seqcount_latch_begin(&phc->seq);
> +	(void)timecounter_read(&phc->tc[0]);
> +	write_seqcount_latch(&phc->seq);
> +	/* Update the secondary copy */
> +	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
> +	phc->tc[1].nsec = phc->tc[0].nsec;
> +	write_seqcount_latch_end(&phc->seq);
> +}
> +
> +/**
> + * ice_phc_init - Initialize PHC timecounter to new start value
> + * @phc: pointer to the PHC structure
> + * @init_tstamp: New initial value for the timecounter
> + *
> + * Initialize the PHC timecounter to a new offset. The latched sequence
> + * counter is used to ensure readers get a consistent view of the timecounter
> + * phase. We always pass the same cyclecounter structure, so the mask, frac,
> + * and cc fields always remain the same, and only the cycle_last and nsec
> + * fields must be propagated to the secondary structure.
> + */
> +static void ice_phc_init(struct ice_phc *phc, u64 init_tstamp)
> +{
> +	lockdep_assert_held(&phc->lock);
> +
> +	write_seqcount_latch_begin(&phc->seq);
> +	timecounter_init(&phc->tc[0], &phc->cc, init_tstamp);
> +	write_seqcount_latch(&phc->seq);
> +	/* Update the secondary copy */
> +	phc->tc[1].cycle_last = phc->tc[0].cycle_last;
> +	phc->tc[1].nsec = phc->tc[0].nsec;
> +	write_seqcount_latch_end(&phc->seq);
> +}

[Severity: Medium]
Does storing phc->update_jiffies inside ice_ptp_read_cycles() publish
the timestamp before either phc->tc[] copy is updated?

timecounter_init() and timecounter_read() invoke cc->read() before
writing tc[0].cycle_last and tc[0].nsec, while phc->seq is odd and
directing readers to phc->tc[1]:

CPU0                                  CPU1
ice_phc_init()
  write_seqcount_latch_begin(&phc->seq)
  timecounter_init(&phc->tc[0], ...)
    ice_ptp_read_cycles()
      smp_store_release(&phc->update_jiffies, ...)
                                      ice_ptp_extend_32b_ts()
                                        smp_load_acquire(&phc->update_jiffies)
                                        ice_phc_cyc2time_with_trace()
                                          seq = read_seqcount_latch(&phc->seq)
                                          /* idx == 1 */
                                          timecounter_cyc2time(&phc->tc[1], ...)
                                            <-- uses uninitialized or stale tc[1]
    tc[0].cycle_last = ...
    tc[0].nsec = ...
  write_seqcount_latch(&phc->seq)
  phc->tc[1] = phc->tc[0]
  write_seqcount_latch_end(&phc->seq)

During initial bringup or after a reset or delayed refresh (>2s), CPU1
sees a fresh update_jiffies and extends a packet's 32-bit hardware
timestamp against an uninitialized (zero) or stale phc->tc[1] snapshot
instead of discarding it or using the updated timecounter.  Recording the
new jiffies value and calling smp_store_release(&phc->update_jiffies, ...)
after write_seqcount_latch(&phc->seq) (or write_seqcount_latch_end()) when
cc->read() succeeded would ensure readers only see the fresh timestamp
once an updated timecounter copy is active.

[ ... ]

> @@ -1974,7 +2202,10 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
>  		if (on && pf->ptp.state != ICE_PTP_READY)
>  			return -EIO;
>  
> +		/* Serialize against concurrent timecounter updates */
> +		mutex_lock(&pf->adapter->phc.lock);
>  		err = ice_ptp_cfg_perout(pf, &rq->perout, on);
> +		mutex_unlock(&pf->adapter->phc.lock);
>  		if (!err) {
>  			*cached = rq->perout;
>  		} else {

[ ... ]

> @@ -2086,6 +2319,43 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
> +static int ice_ptp_settime64_tc(struct ptp_clock_info *info,
> +				const struct timespec64 *ts)
> +{
> +	struct ice_pf *pf = ptp_info_to_pf(info);
> +	struct ice_phc *phc = &pf->adapter->phc;
> +	u64 ns;
> +
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		return -EIO;
> +
> +	ns = timespec64_to_ns(ts);
> +
> +	mutex_lock(&phc->lock);
> +
> +	ice_ptp_disable_all_perout(pf);
> +
> +	ice_phc_init(phc, ns);
> +
> +	ice_ptp_mark_all_tx_tracker_stale(pf);\n+
> +	/* Reenable periodic outputs */
> +	ice_ptp_enable_all_perout(pf);
> +
> +	mutex_unlock(&phc->lock);
> +
> +	return 0;
> +}

[Severity: Medium]
Can releasing pf->adapter->phc.lock before updating *cached
(pf->ptp.perout_rqs[rq->perout.index]) in ice_ptp_gpio_enable() race with
ice_ptp_settime64_tc() or ice_ptp_adjtime_tc()?

The POSIX clock layer only holds down_read(&clk->rwsem) across ioctl,
clock_settime, and clock_adjtime calls, so ice_ptp_gpio_enable() and
ice_ptp_settime64_tc() can run concurrently:

CPU0                                  CPU1
ice_ptp_gpio_enable()
  mutex_lock(&pf->adapter->phc.lock)
  ice_ptp_cfg_perout(pf, &rq->perout, 1)
  mutex_unlock(&pf->adapter->phc.lock)
                                      ice_ptp_settime64_tc()
                                        mutex_lock(&phc->lock)
                                        ice_ptp_disable_all_perout(pf)
                                        ice_phc_init(phc, ns)
                                        ice_ptp_enable_all_perout(pf)
                                          /* cached->period is still 0 */
                                        mutex_unlock(&phc->lock)
  *cached = rq->perout;                 <-- HW keeps pre-settime phase

When enabling an output, ice_ptp_enable_all_perout() sees a zero period in
perout_rqs[] and skips reprogramming GLTSYN_TGT for the new timecounter
phase; when disabling an output, it sees the old non-zero period and
re-enables the output in hardware right before *cached is cleared.
Writing *cached without holding phc.lock also races with
ice_ptp_disable_all_perout() and ice_ptp_enable_all_perout() reading the
struct under phc.lock.  Holding pf->adapter->phc.lock across the *cached
update in ice_ptp_gpio_enable() avoids the race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-jk-e825-e830-timecounter-v2-0-9c357a7ed349@intel.com?part=4

Re: [PATCH iwl-next v2 3/4] ice: store cached PHC time in ice_adapter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ice: stale adapter->phc cache across reset in
  ice_ptp_rebuild_owner()
--

commit 3e2553cade32eeca4938b383704866bc53bd38ea
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: store cached PHC time in ice_adapter

This commit moves the cached PHC time and its update timestamp from the
per-PF ice_ptp and Rx ring structures into a shared struct ice_phc embedded
in ice_adapter, updated only by the clock owner PF.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index bf37c2a7ff863..a0451c1083a41 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
[ ... ]
> @@ -3099,24 +3056,20 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
>  {
>  	struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
>  	struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
> -	int err;
>  
>  	if (pf->ptp.state != ICE_PTP_READY)
>  		return;
>  
>  	ice_ptp_tspll_monitor(pf);
>  
> -	err = ice_ptp_update_cached_phctime(pf);
> +	if (ice_pf_src_tmr_owned(pf))
> +		ice_ptp_update_cached_phctime(pf);
>  
>  	ice_ptp_maybe_trigger_tx_interrupt(pf);
>  
> -	/* Run twice a second, or retry soon if another thread was busy
> -	 * updating the Rx rings. Other errors, such as a missing control PF,
> -	 * do not clear up on their own and must not turn the periodic worker
> -	 * into a busy loop.
> -	 */
> +	/* Run twice a second */
>  	kthread_queue_delayed_work(ptp->kworker, &ptp->work,
> -				   msecs_to_jiffies(err == -EAGAIN ? 10 : 500));
> +				   msecs_to_jiffies(500));
>  }
>  
>  /**
> @@ -3211,6 +3164,8 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	/* Acquire PHC and system timer to restore after reset */
>  	ptp->reset_time = ktime_get_real_ns();
> +	if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
> +		ptp->phc_reset_time = 0;
>  }
>  
>  /**
> @@ -3250,13 +3205,13 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
>  	if (err)
>  		goto err_unlock;
>  
> -	/* Write the initial Time value to PHY and LAN using the cached PHC
> -	 * time before the reset and time difference between stopping and
> -	 * starting the clock.
> +	/* Write the initial Time value to PHY and LAN using the PHC time
> +	 * before the reset and time difference between stopping and starting
> +	 * the clock.
>  	 */
> -	if (ptp->cached_phc_time) {
> +	if (ptp->phc_reset_time) {
>  		time_diff = ktime_get_real_ns() - ptp->reset_time;
> -		ts = ns_to_timespec64(ptp->cached_phc_time + time_diff);
> +		ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
>  	} else {
>  		ts = ktime_to_timespec64(ktime_get_real());
>  	}

[Severity: Medium]
Does ice_ptp_rebuild_owner() leave a stale cached_time in pf->adapter->phc
across a non-PFR reset?

When a reset such as EMPR zeroes GLTSYN_TIME before
ice_ptp_prepare_for_reset() reads it, ptp->phc_reset_time is 0 and
ice_ptp_rebuild_owner() reinitializes the hardware clock to
ktime_get_real() (UTC) via ice_ptp_write_init(), which can differ from a
pre-reset TAI PHC by 37 seconds.  Neither ice_ptp_prepare_for_reset() nor
ice_ptp_rebuild_owner() invalidates or refreshes pf->adapter->phc after
writing the new time, and non-owner PFs no longer update the cache in
ice_ptp_periodic_work().

If the reset and rebuild finish within 2000 ms of the last pre-reset
update, ice_ptp_extend_32b_ts() still sees a fresh phc->update_jiffies and
extends post-reset Rx and Tx hardware timestamps against the pre-reset
phc->cached_time until the owner PF's kworker runs
ice_ptp_periodic_work().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-jk-e825-e830-timecounter-v2-0-9c357a7ed349@intel.com?part=3