On 8/21/2026 5:13 PM, Jacob Keller wrote:
> The ice driver currently discards any outstanding timestamps that are
> happening very near to a .adjtime or .settime callback. This was originally
> add by commit d40fd6009332 ("ice: handle flushing stale Tx timestamps in
> ice_ptp_tx_tstamp").
>
> The original motivation for discarding timestamps was that extending an old
> timestamp using the new cached value of PHC was a problem, as it could
> produce incorrect results. The change did not describe what such "incorrect
> results" were.
>
> There are no such incorrect results. Extending the 32 bit timestamp with
> the new time value just means that the timestamp is reported in terms of
> the newly updated and adjusted system clock. This won't produce incorrect
> results or problematic timestamps to applications. Either the timestamp
> will be extended with the value of the PHC just prior to the time
> adjustment (if the timestamp completes prior to the adjust callback), or it
> will be extended using the new PHC value after the adjustment. In either
> case, the resulting extended timestamp value makes sense.
>
Sashiko says:
> Could removing the stale flush logic actually lead to corrupted timestamps if
> the time jumps by more than 2.14 seconds?
> Looking at ice_ptp_extend_32b_ts():
> ice_ptp_extend_32b_ts() {
> ...
> phc_time_lo = (u32)cached_phc_time;
> delta = (in_tstamp - phc_time_lo);
> ...
> }
> If a time adjustment exceeds 2.14 seconds while a Tx timestamp is in flight,
> won't the 32-bit modulo math wrap? It appears reconstructing the old hardware
> timestamp using the post-jump cached_phc_time would produce a meaningless
> value rather than a correctly extended one.
I no longer believe that result would be incorrect. I don't see other
users of similar logic around struct timecounter needing any similar
logic. The users of timecounter are very similar with the only
difference being that they maintain the "upper" bits entirely in
software, while the ice driver has hardware maintain them, effectively.
Perhaps I am wrong and missing something? If others agree I can drop
this from the series.
> The timestamp extension logic is very similar to the logic found in
> timecounter_cyc2time, the primary difference being that the ice hardware
> maintains the full 64 bits of nanoseconds in the MAC rather than being
> maintained purely by software as in the timecounter case.
>
> Indeed, I couldn't find an example of a driver using timecounter_cyc2time
> which does discard timestamps that occur nearby a time adjustment. The ice
> driver behavior of discarding such timestamps just results in failure to
> deliver a Tx timestamp to userspace, resulting in applications such as
> ptp4l to timeout and enter a fault state. Reporting the extended timestamp
> based on the updated PHC value isn't producing "garbage" results, and
> doesn't lead to incorrect behavior.
>
> This effectively reverts commit d40fd6009332 ("ice: handle flushing stale
> Tx timestamps in ice_ptp_tx_tstamp"). However, the stale logic remains, as
> we now use it to inform the driver to drop timestamps which might fail due
> to link down.
>
> Fixes: d40fd6009332 ("ice: handle flushing stale Tx timestamps in
> ice_ptp_tx_tstamp")
> Signed-off-by: Jacob Keller <[email protected]>
> Reviewed-by: Maciek Machnikowski <[email protected]>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1a9bf8839404..8c2dd83d8d6c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -826,12 +826,10 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct
> ice_ptp_tx *tx)
> * ice_ptp_mark_tx_tracker_stale - Mark unfinished timestamps as stale
> * @tx: the tracker to mark
> *
> - * Mark currently outstanding Tx timestamps as stale. This prevents sending
> - * their timestamp value to the stack. This is required to prevent extending
> - * the 40bit hardware timestamp incorrectly.
> - *
> - * This should be called when the PTP clock is modified such as after a set
> - * time request.
> + * Mark currently outstanding Tx timestamps as stale. This prevents the
> driver
> + * from reporting the timestamp to the stack. This is called to inform the
> + * driver that a timestamp is expected to fail if it was initiated as the
> link
> + * went down.
> */
> static void
> ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
> @@ -1049,13 +1047,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf
> *pf)
> kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
> msecs_to_jiffies(10));
> }
Sashiko says:
> This is a pre-existing issue, but is it possible for pf->ptp.kworker to be
> NULL here during driver teardown?
> Looking at ice_ptp_release(), it destroys the kworker before unregistering
> the PTP clock:
> ice_ptp_release() {
> if (pf->ptp.kworker) {
> kthread_destroy_worker(pf->ptp.kworker);
> pf->ptp.kworker = NULL;
> }
> if (!pf->ptp.clock)
> return;
> ...
> ptp_clock_unregister(pf->ptp.clock);
> }
> If userspace calls adjtime or settime concurrently during this window, it
> would unconditionally call ice_ptp_reset_cached_phctime(). If the update
> fails and falls back to queuing work, could this dereference the NULL
> pf->ptp.kworker pointer?
I will check this. I think the correct answer is to kref on the port
access to ensure that we don't finish teardown until the function exits,
which would be included as part of the first patch of the series.
