Re: [Intel-wired-lan] [PATCH iwl-next] ice: remove unnecessary discarding of timestamps

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

On 10/26/2023 3:59 AM, Karol Kolacinski wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The ice driver currently discards any outstanding timestamps that are
> happening very near to a .adjtime or .settime callback. This was
> originally added by commit b1a582e64bf2 ("ice: introduce
> ice_ptp_reset_cached_phctime function") and later extended 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.
> 
> 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.
> 
> Remove the unnecessary stale timestamp logic.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---

This doesn't compile when applied on top of dev-queue:

> ../drivers/net/ethernet/intel/ice/ice_ptp.c: In function ‘ice_ptp_rebuild_owner’:
> ../drivers/net/ethernet/intel/ice/ice_ptp.c:2509:2: error: implicit declaration of function ‘ice_ptp_flush_all_tx_tracker’; did you mean ‘ice_ptp_flush_tx_tracker’? [-Werror=implicit-function-declaration]
>   ice_ptp_flush_all_tx_tracker(pf);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ice_ptp_flush_tx_tracker

Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan