Re: [Intel-wired-lan] [PATCH v10 iwl-next 10/11] idpf: add Tx timestamp flows

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

On 4/8/2025 3:31 AM, Milena Olech wrote:
> +/**
> + * idpf_ptp_tstamp_extend_32b_to_64b - Convert a 32b nanoseconds Tx or Rx
> + *				       timestamp value to 64b.
> + * @cached_phc_time: recently cached copy of PHC time
> + * @in_timestamp: Ingress/egress 32b nanoseconds timestamp value
> + *
> + * Hardware captures timestamps which contain only 32 bits of nominal
> + * nanoseconds, as opposed to the 64bit timestamps that the stack expects.
> + *
> + * Return: Tx timestamp value extended to 64 bits based on cached PHC time.
> + */
> +u64 idpf_ptp_tstamp_extend_32b_to_64b(u64 cached_phc_time, u32 in_timestamp)
> +{
> +	s64 delta;
> +
> +	delta = in_timestamp - lower_32_bits(cached_phc_time);
> +
> +	return cached_phc_time + delta;
> +}


This logic looks quite different from what we did in ice and iavf, which
was based on the math from timecounters. It looks like you do check if
the value is too old which is good to verify. Perhaps I'm just
misunderstanding the math.

For clarity, here's what we have in ice:

> static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
> {
>         u32 delta, phc_time_lo;
>         u64 ns;
> 
>         /* Extract the lower 32 bits of the PHC time */
>         phc_time_lo = (u32)cached_phc_time;
> 
>         /* Calculate the delta between the lower 32bits of the cached PHC
>          * time and the in_tstamp value
>          */
>         delta = (in_tstamp - phc_time_lo);
> 
>         /* Do not assume that the in_tstamp is always more recent than the
>          * cached PHC time. If the delta is large, it indicates that the
>          * in_tstamp was taken in the past, and should be converted
>          * forward.
>          */
>         if (delta > (U32_MAX / 2)) {
>                 /* reverse the delta calculation here */
>                 delta = (phc_time_lo - in_tstamp);
>                 ns = cached_phc_time - delta;
>         } else {
>                 ns = cached_phc_time + delta;
>         }
> 
>         return ns;
> }


In particular, this ensures that we correctly handle the case where a
timestamp is captured just before an update to the cached PHC time.
Without that check, you can't guarantee that the timestamp is updated
correctly with lockess PHC updating.

With these checks, as long as the timestamp is recent, we can extend it
safely without worrying about whether the cached PHC time we are using
is slightly old or not. (As long as its no more than 2 seconds old).

Could you explain why this was changed for idpf?

Bonus points if we extracted this method into libie/libeth and shared it
across ice, idpf, and iavf, which I believe recently gained support for
timestamping as well.