On Wed, 2 Sep 2026 11:21:22 +0530
Rajesh Kumar <[email protected]> wrote:
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_timestamp_slot_alloc,
> 26.11)
> +int
> +rte_eth_timesync_tx_timestamp_slot_alloc(uint16_t port_id,
> + uint32_t *slot_id)
Could join to one line, max line line is now 100
> +{
> + struct rte_eth_dev *dev;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + dev = &rte_eth_devices[port_id];
> +
> + if (slot_id == NULL) {
> + RTE_ETHDEV_LOG_LINE(ERR,
> + "Cannot allocate ethdev port %u Tx timestamp slot to
> NULL",
> + port_id);
Minor nit the wording of that error message is awkward.
Similar problem in other messages.
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_dynfield_register,
> 26.11)
> +int
> +rte_eth_timesync_tx_slot_dynfield_register(void)
> +{
> + const struct rte_mbuf_dynfield slot_dynfield = {
> + .name = RTE_ETH_TIMESYNC_TX_SLOT_DYNFIELD_NAME,
> + .size = sizeof(uint32_t),
> + .align = alignof(uint32_t),
> + };
> + uint16_t port_id;
> +
> + if (rte_eth_timesync_tx_slot_dynfield_offset >= 0)
> + return 0;
> +
> + rte_eth_timesync_tx_slot_dynfield_offset =
> + rte_mbuf_dynfield_register(&slot_dynfield);
> + if (rte_eth_timesync_tx_slot_dynfield_offset < 0)
> + rte_eth_timesync_tx_slot_dynfield_offset =
> + rte_mbuf_dynfield_lookup(
> + RTE_ETH_TIMESYNC_TX_SLOT_DYNFIELD_NAME,
> NULL);
> + if (rte_eth_timesync_tx_slot_dynfield_offset < 0)
> + return -ENOTSUP;
> +
> + {
> + int flag_bit = rte_mbuf_dynflag_register(
> + &(const struct rte_mbuf_dynflag){
> + .name = RTE_ETH_TIMESYNC_TX_SLOT_DYNFLAG_NAME});
> + if (flag_bit < 0)
> + flag_bit = rte_mbuf_dynflag_lookup(
> + RTE_ETH_TIMESYNC_TX_SLOT_DYNFLAG_NAME, NULL);
> + if (flag_bit < 0)
> + return -ENOTSUP;
> + rte_eth_timesync_tx_slot_dynflag = RTE_BIT64(flag_bit);
> + }
No need for basic block {} here.
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_dynfield_unregister,
> 26.11)
> +int
> +rte_eth_timesync_tx_slot_dynfield_unregister(void)
> +{
> + uint16_t port_id;
> +
> + /* Reset cached state without freeing dynamic-field bytes. */
> + rte_eth_timesync_tx_slot_dynfield_offset = -1;
> + rte_eth_timesync_tx_slot_dynflag = 0;
> +
> + RTE_ETH_FOREACH_VALID_DEV(port_id)
> + eth_timesync_tx_slot_info_refresh(port_id);
> +
> + return 0;
> +}
> +
If it always returns 0 why not void.
Not sure what the point of this function is. It doesn't really do anything.
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_timestamp_stamp_mbuf,
> 26.11)
> +int
> +rte_eth_timesync_tx_timestamp_stamp_mbuf(uint16_t port_id,
> + uint32_t slot_id, struct
> rte_mbuf *m)
> +{
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + if (m == NULL)
> + return -EINVAL;
> + if (rte_eth_timesync_tx_slot_dynfield_register() != 0)
> + return -ENOTSUP;
> + *RTE_MBUF_DYNFIELD(m, rte_eth_timesync_tx_slot_dynfield_offset,
> + uint32_t *) = slot_id;
> + m->ol_flags |= rte_eth_timesync_tx_slot_dynflag;
> + return 0;
> +}
This is possibly in data path, use unlikely() here.
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index ee400b386f..bde391dea4 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5513,6 +5513,19 @@ int rte_eth_timesync_read_rx_timestamp(uint16_t
> port_id,
> /**
> * Read an IEEE1588/802.1AS Tx timestamp from an Ethernet device.
> *
> + * This is the legacy Tx timestamp API and is intended for register-based
> + * timestamp reads. It does not provide per-packet correlation.
> + *
Rather than weak guidance which will get ignored and stale.
1. Convert all in-tree uses of old API
2. Announce deprecation in this release
3. Mark legacy API as deprecated
AI had even more observations (Fable 5.1)
> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_eth_timesync_tx_slot_infos, 26.11)
> +struct rte_eth_timesync_tx_slot_info
> +rte_eth_timesync_tx_slot_infos[RTE_MAX_ETHPORTS];
Exporting a RTE_MAX_ETHPORTS sized array from the public header bakes
build config into ABI. rte_eth_fp_ops lives in ethdev_driver.h, this
should too; only PMDs read it.
The per-port array also has no per-port content. offset and dynflag are
process globals; the only per-port part is "caps say PER_PACKET". Put
the two globals in ethdev_driver.h and let the PMD that implements
slots check them. Drops the array, the refresh loop, and the forward
declaration.
> +static void eth_timesync_tx_slot_info_refresh(uint16_t port_id);
Move the definitions above first use instead.
> + ret = eth_err(port_id, dev->dev_ops->timesync_enable(dev));
> + if (ret == 0)
> + eth_timesync_tx_slot_info_refresh(port_id);
No matching reset in timesync_disable. Info stays stale after disable.
> +int
> +rte_eth_timesync_tx_timestamp_stamp_mbuf(uint16_t port_id,
> + uint32_t slot_id, struct
> rte_mbuf *m)
> +{
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + if (m == NULL)
> + return -EINVAL;
> + if (rte_eth_timesync_tx_slot_dynfield_register() != 0)
> + return -ENOTSUP;
Calling register from the per-packet path is wrong. First call takes
the mbuf dyn lock and walks every port calling into driver dev_ops.
Header says "safe for concurrent callers"; it is not, two threads
racing on first stamp both run registration on plain globals.
port_id is validated but otherwise unused. Stamping a SINGLE_REG port
succeeds and sets a flag nothing reads. Check the port's slot info,
return -ENOTSUP if dynflag == 0, and require the app to have called
register up front (which the doc already says it must, before pool
create).
> + rte_eth_timesync_tx_slot_dynfield_offset = -1;
> + rte_eth_timesync_tx_slot_dynflag = 0;
Written unlocked, read from Tx datapath on other cores. Also cannot
free the dynfield. Agree with dropping unregister entirely.
> + * -ENOTSUP and the PMD TX path falls back to the port-level ptp_tx_index
> + * (legacy mode) on every port.
ptp_tx_index is an Intel driver internal. Does not belong in rte_ethdev.h.
> + * The underlying DPDK dynfield bytes are NOT freed — DPDK provides no
> dynfield
Non-ASCII dash in source.
> +typedef int (*eth_timesync_tx_ts_get_caps_t)(struct rte_eth_dev *dev,
...
> + eth_timesync_tx_ts_get_caps_t timesync_tx_ts_get_capabilities;
...
> +int rte_eth_timesync_tx_timestamp_slot_get_capabilities(uint16_t port_id,
...
> +int rte_eth_timesync_read_tx_timestamp_slot(uint16_t port_id,
Three spellings of the same op, and the read function breaks the
rte_eth_timesync_tx_timestamp_slot_* prefix the release note
advertises. One prefix for all of it, rte_eth_timesync_tx_slot_{caps,
alloc,read,release,stamp} is shorter and consistent.
Also TX/Tx mixed throughout comments and docs. Tx.
> +struct rte_eth_timesync_dual_domain_timestamp {
> + int64_t adjusted_ns;
> + int64_t raw_ns;
> + uint32_t valid_mask;
> +};
4 byte tail hole. Fine for experimental, but say so or reorder before
it goes stable.
> +++ b/doc/guides/prog_guide/ethdev/timesync.rst
Lines up to 150+ chars. Doc guideline is one sentence per line. Half
of this file documents existing clock/Rx API, which is a separate
patch from the slot feature.
