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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/27/2026 7:40 AM, Paolo Abeni wrote:
> On 10/22/26 00:12, Jacob Keller wrote:
>> Jake Keller says:
>>
>> This series contains several related fixes for the ice driver PTP logic
>> relating to timestamp handling and device (re)initialization.
>>
>> Of particular note is some changes around the handling of timestamps that
>> are requested near device state changes such as administrative up/down
>> cycles and link change events.
>>
>> The E825 device logic in the PHY has an internal counter which is used as
>> part of the "threshold" logic which determines when the device will
>> trigger
>> an interrupt signal from a given PHY port to the MAC. This internal
>> counter
>> requires some precise handling to ensure that the internal device state
>> remains in sync with software expectations. Otherwise, the device can be
>> finagled into a state where the PHY stops producing new timestamp
>> interrupt
>> notifications to the MAC indefinitely. This in turn degrades the
>> timestamp
>> processing latency and results in application failures for common
>> timestamping applications such as ptp4l.
>>
>> There are four major categories of problem resolved by this series:
>>
>>   * Timestamp requests made while the PHY_REG_TX_OFFSET_READY bit is
>> cleared
>>     will increment the internal counter, but leave their valid bit set
>> to 0.
>>     Upon read, the counter is not decremented. This leads to a desync
>> of the
>>     counter and blocks the PHY interrupt.
>>
>>   * Software logic for tracking timestamps incorrectly cleared in-use
>> bits
>>     without waiting for completion in certain cases. If the timestamp
>> *does*
>>     later complete, it leaves an "orphaned" ready bit which is not
>>     tracked by software. This results in the internal counter becoming
>>     desynced if that index is re-used.
>>
>>   * The PHY timestamp memory region lacks pull-down zero-
>> initialization at
>>     power on, resulting in uninitialized random data in the memory
>> region.
>>     If software reads these values, an entry with its valid bit set to
>> 1 can
>>     trigger a counter decrement and cause an underflow which results
>> in the
>>     counter becoming desynced.
>>
>>   * Timestamp request which complete near the beginning of the PHY losing
>>     link can become "stuck" such that the hardware logic triggered by a
>>     timestamp read does not activate. The memory status bit and the valid
>>     bit in the timestamp index are not cleared. This window where this
>>     may occur begins *before* the firmware notifies the driver of link
>> loss.
>>     When this occurs, the driver may accidentally re-use a stale
>> timestamp,
>>     and the IRQ re-trigger logic triggers a repeated IRQ "storm" that can
>>     consume significant excess CPU time.
>>
>> The series' primary focus is towards preventing driver flows that can
>> trigger the above sequences. It is based on work from Przemyslaw Korba
>> which
>> was previously posted at [1]. During that series development, Petr from
>> RedHat reported the 3rd issue mentioned above. While attempting to root
>> cause that issue, several other issues were uncovered and those fixes
>> have
>> also been included in this series.
>>
>> First, the PTP reset flow is fixed to stop tearing down the Tx tracker
>> during a CORE or GLOBAL reset. This avoids causing Tx timestamps to break
>> permanently after such a reset. The fix also ensures that all teardown
>> paths properly release the tracker and kworker in a safe order. This
>> issue
>> was found by Sashiko during review of a previous version of this series.
>>
>> Next, the locking around the PTP ports list in the adapter structure is
>> converted to use RCU primitives and a spinlock, resolving a couple of
>> reports from Petr about places where the original list was accessed
>> without
>> lock protection. Note that an older version of this fix used an xarray
>> instead of the list. The xarray has more overhead and results in an
>> increase of ~25 microseconds to the average latency for processing Tx
>> timestamps. The list is simpler and avoids this overhead.
>>
>> Next, the PHY restart locking is fixed to avoid an issue with a
>> concurrent
>> execution of ice_ptp_restart_all_phy() and a link change on a given port.
>> Additionally, the PHY port locks are merged into a single per-adapter
>> lock
>> to reduce locking complexity.
>>
>> Next, the ice_ptp_request_ts() function is updated to sequence the
>> marking
>> of the in_use bitmap in order to work properly with the lockless
>> reader in
>> the IRQ thread. This issue was reported by Sashiko during review of a
>> previous version of this series.
>>
>> Next, Arkadiusz and Jake modify the driver to stop pretending that the
>> link
>> has changed at administrative ice_down() and ice_up(). This removes
>> "virtual" PTP link changes that occurred on several flows including MTU
>> change, Eswitch setup, and others. Now, the driver only triggers a PTP
>> PHY
>> timer reinitialization when the physical PHY link has changed instead of
>> during many other actions.
>>
>> Next, come two fixes for E822 hardware that were originally posted as
>> part
>> of Przemyslaw Korba's work [1]. The E822-only "vernier" offset validation
>> work task is properly canceled during device reset, and new timestamp
>> requests are kept disabled until the validation task completes.
>>
>> Next, the driver is modified to stop clearing the PHY_REG_TX_OFFSET_READY
>> bit. This bits only purpose is to tell hardware to mark any captured
>> timestamps as invalid. Since this also disables the necessary side
>> effects
>> on read it is problematic to have cleared. According to hardware
>> engineers,
>> keeping it enabled should not have any other side effects. Instead, the
>> tx.calibrating field is used to disable new timestamps from software in a
>> similar manner to the older E822 devices.
>>
>> Next, the driver is modified to clear the PHY_REG_TX_MEMORY_STATUS by
>> reading each index *prior* to the PHY soft reset. This ensures that any
>> stale or invalid data left in the memory array is cleared, followed by
>> the
>> counter being reset via the PHY soft reset procedure.
>>
>> Next, the E825 timer start procedure is modified to first include a soft
>> reset. This ensures that upon link up the device is reconfigured from a
>> known-good state with its internal counter reset and everything cleared.
>>
>> Next, Petr modifies the ice_ptp_flush_tx_tracker() function to wait a
>> little
>> bit for any outstanding timestamps before flushing.
>>
>> Next, Petr modifies the ice_ptp_process_tx_tstamp() function to avoid
>> releasing any index from software unless either a) it is actually
>> completed
>> by hardware or b) it is timed out waiting for a full two seconds. This
>> closes the final gap from the second issue mentioned above. Instead of
>> immediately releasing the index, the software now waits until hardware
>> has
>> completed it or the driver has waited long enough to be sufficiently sure
>> that no such timestamp will be done.
>>
>> Finally, the ice_ptp_process_tx_tstamp() function is modified to verify
>> that hardware actually cleared the ready bitmap. This ensures that we do
>> not report false timestamps near a link down event.
> It's not trivially clear to me if the patches 2,4,6,8,9 and 12 are
> functionally independent from the other ones. Are they? If so, we
> could apply them, keep the remaining series smaller and hopefully
> move this forward.
> 
> /P
> 

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

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

Thanks,
Jake