Re: [RFC PATCH v3] ethdev: update read time API in PMD to enable crosstimestamp

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 30 Dec 2025 08:57:19 +0000
Soumyadeep Hore <soumyadeep.hore@intel.com> wrote:

> - flags indicates cross timestamp is enabled or not
> 
> For backward compatibility:
> - Existing rte_eth_read_clock() remains unchanged
> - Drivers may implement either or both APIs
> 
> Capability Discovery
> -------------------
> Will  be enabled via traditional way of enabling PTP
> 
> Driver Implementation
> ---------------------
> Drivers implementing timesync_read_time() API may:
> - Use hardware-assisted cross timestamping if available
> - Fall back to serialized software reads of PHC and system clock
> 
> Drivers that do not support cross timestamping may:
> - Fill only device_time_ns
> - Return 0 to indicate a successful PHC read
> 
> ABI / API Impact
> ----------------
> This change introduces:
> - A new flag
> - A change in existing API
> - A new device capability flag
> 
> No existing APIs or structures are modified, preserving ABI
> compatibility.
> 
> Alternatives Considered
> ----------------------
> A separate cross timestamp API was considered but rejected to avoid
> duplication and to keep PTP clock access consolidated under read_time.
> 
> Another option was to extend rte_eth_read_clock() directly, but this
> would break ABI and existing applications.
> 
> Performance and Risks
> ---------------------
> The extended read_time API is expected to be called infrequently and
> does not affect the packet data path.
> 
> Accuracy depends on hardware support. Software-based cross timestamping
> may introduce jitter, which should be documented by drivers.
> 
> Future Work
> -----------
> - Exposing estimated error bounds in flags or an extended structure
> - Alignment with Linux PHC cross timestamp semantics
> - Support for multiple system clock domains (TAI, REALTIME)
> 
> Feedback Requested
> ------------------
> Feedback is requested on:
> - Whether extending read_time is preferred over a standalone API
> - Structure and flag naming
> - Expected semantics when system_time is unavailable

Having this as compile time choice is the wrong architecture.
Let me summarize my thoughts (with AI verbosity)...

The core problem is using RTE_LIBRTE_CROSSTIMESTAMP as a compile-time
switch for the behavior of an exported function. DPDK often ships as a
prebuilt shared library. A build flag can't change the meaning of an
exported symbol: with the flag set, rte_eth_timesync_read_time(port,
timestamp) treats timestamp as a two-element array; without it, one
element. Same prototype, two incompatible contracts, chosen at library
build time. An app built against one config and linked against another
overflows or reads garbage. So "no structures modified, ABI preserved"
isn't accurate — the pointer's contract is silently redefined, which is
worse than an honest bump.

My preference for new API, in order:

    Do the right thing by default: a dedicated cross-timestamp call returns the pair inherently — no flag needed.
    Else make it a runtime option, never #ifdef — one binary must serve both callers.
    Else add a new, typed API — don't overload struct timespec * to mean timespec[2].

Concretely I'd drop the #ifdef and the bool, and add
rte_eth_timesync_read_cross_timestamp() returning a struct
(device_time, system_time, room for clock id / error bound), gated by a
capability bit in dev_info — matching the Linux PTP_SYS_OFFSET_PRECISE
model. That leaves timesync_read_time and its implementers alone.

Blocking issues as it stands:

* Build break: the typedef gains a third arg but only ice is updated.
  The other 13 implementers (txgbe, axgbe, dpaa, bnxt, i40e, idpf, igc,
  igb, ixgbe, ngbe, cnxk, hns3, dpaa2) keep the 2-arg signature —
  incompatible-pointer-type, which is -Werror in CI.

* Uninitialized read: ice doesn't support cross timestamping
  (it just warns and fills one timespec), yet the example reads sys_time = crosstimestamp[1],
  which is never written. The one path this enables produces a garbage delta.

* Silent failure: a driver that can't do it still returns 0,
  so the caller can't tell the second timestamp wasn't filled.
  Should be -ENOTSUP or gated on an advertised capability.

Minor: the new build flag isn't wired into meson, so the path is dead code as submitted; 
no capability flag is added despite the cover letter; and the typedef doc comment isn't updated.