All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH iwl-next v2 0/4] ice: E825 timer adjustment fixes
@ 2026-11-13 23:30 Jacob Keller
  2026-11-13 23:30 ` [PATCH iwl-next v2 1/4] ice: fix computation of ETF timestamps for E830 Jacob Keller
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Jacob Keller @ 2026-11-13 23:30 UTC (permalink / raw)
  To: Intel Wired LAN, Anthony Nguyen, Przemek Kitszel,
	Przemyslaw Korba, Grzegorz Nitka, Sergey Temerkhanov,
	Maciej Machnikowski, Arkadiusz Kubalewski, 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>


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-11-13 23:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-13 23:30 [PATCH iwl-next v2 0/4] ice: E825 timer adjustment fixes Jacob Keller
2026-11-13 23:30 ` [PATCH iwl-next v2 1/4] ice: fix computation of ETF timestamps for E830 Jacob Keller
2026-11-13 23:30 ` [PATCH iwl-next v2 2/4] ice: exit PTP callbacks early if not ICE_PTP_READY Jacob Keller
2026-11-13 23:30 ` [PATCH iwl-next v2 3/4] ice: store cached PHC time in ice_adapter Jacob Keller
2026-11-13 23:52   ` sashiko-bot
2026-11-13 23:30 ` [PATCH iwl-next v2 4/4] ice: use struct timecounter for E825 and E830 Jacob Keller
2026-11-13 23:46   ` sashiko-bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.