Re: [PATCH v2 0/6] net/gve: add hardware timestamping support

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 15 May 2026 23:19:29 +0000
Mark Blasko <blasko@google.com> wrote:

> This patch series introduces support for GVE hardware timestamping on DQO
> queues. To support concurrent access, a mutex lock is introduced to protect
> admin queue operations. A mechanism is then added to periodically synchronize
> the NIC clock via AdminQ, and support is introduced for the read_clock ethdev
> operation. Finally, the RX datapath is updated to reconstruct full 64-bit
> timestamps from the 32-bit values in DQO descriptors.
> 
> ---
> v2:
> - Patch 1: Dropped ROBUST mutex attribute.
> - Patch 3: Added adminq timestamp counter reset to gve_adminq_alloc.
> - Patch 4:
>   - Removed redundant void* casts.
>   - Handled alarm reschedule failures by marking timestamp stale.
>   - Added transient error logging on memzone allocation failure.
> - Patch 5: Scoped read_clock ethdev operation strictly to DQO queues.
> - Patch 6:
>   - Scoped timestamp offload capability advertisement strictly to
>     DQO queues.
>   - Predicated capability advertisement directly on memzone
>     allocation.
>   - Initialized mbuf_timestamp_offset to -1.
>   - Added blank line separating release notes.
> ---
> 
> Mark Blasko (6):
>   net/gve: add thread safety to admin queue
>   net/gve: add device option support for HW timestamps
>   net/gve: add AdminQ command for NIC timestamps
>   net/gve: add periodic NIC clock synchronization
>   net/gve: support read clock ethdev op
>   net/gve: reconstruct HW timestamps from DQO
> 
>  .mailmap                               |   1 +
>  doc/guides/nics/features/gve.ini       |   1 +
>  doc/guides/nics/gve.rst                |  20 ++++
>  doc/guides/rel_notes/release_26_07.rst |   4 +
>  drivers/net/gve/base/gve_adminq.c      | 128 +++++++++++++++++----
>  drivers/net/gve/base/gve_adminq.h      |  29 +++++
>  drivers/net/gve/base/gve_desc_dqo.h    |   8 +-
>  drivers/net/gve/gve_ethdev.c           | 149 ++++++++++++++++++++++++-
>  drivers/net/gve/gve_ethdev.h           |  39 +++++++
>  drivers/net/gve/gve_rx_dqo.c           |  26 +++++
>  10 files changed, 383 insertions(+), 22 deletions(-)
> 

Looks good, once again AI followup still found some things you need to address.

Patch 5: net/gve: support read clock ethdev op

Error: gve_read_clock is defined as static but never assigned to
any eth_dev_ops table. In v1 the patch added
".read_clock = gve_read_clock" to both gve_eth_dev_ops and
gve_eth_dev_ops_dqo. The v2 changelog notes "Scoped read_clock
ethdev operation strictly to DQO queues," which should have left
the assignment in gve_eth_dev_ops_dqo and removed it from
gve_eth_dev_ops. Instead both assignments were dropped, leaving
the function unreferenced. DPDK CI builds with -Dwerror=true, so
-Wunused-function will fail the build, and the read_clock feature
is unreachable at runtime in any case.

Restore the assignment in gve_eth_dev_ops_dqo:

    static const struct eth_dev_ops gve_eth_dev_ops_dqo = {
            ...
            .reta_query           = gve_rss_reta_query,
            .read_clock           = gve_read_clock,
    };

Warning: (unchanged from v1) gve_read_clock and the periodic
gve_read_nic_clock alarm callback both issue
GVE_ADMINQ_REPORT_NIC_TIMESTAMP into the single shared DMA buffer
priv->nic_ts_report, then read it after gve_adminq_execute_cmd
has released adminq_lock. If gve_read_clock is preempted between
gve_adminq_report_nic_timestamp returning and the be64_to_cpu
read, the alarm callback can memset() and reissue its own
command, so the user thread will read either zero or another
command's response. The simplest fix is for gve_read_clock to
return the cached priv->last_read_nic_timestamp instead of
issuing a fresh adminq command - the 250ms periodic sync keeps
it fresh enough for .read_clock semantics. Once the dev_op
registration is restored this race becomes reachable.