Thanks for the review, and apologies for the accidental spam with v1! We'll address most of the initial comments exactly as suggested in v2, but have a few follow-ups:
> [PATCH 4/6] net/gve: add periodic NIC clock synchronization > > gve_setup_nic_timestamp() discards the gve_alloc_nic_ts_report() error. > If the memzone allocation fails, priv->nic_timestamp_supported (set in > 2/6) remains true, so dev_info_get() in 6/6 will advertise > RTE_ETH_RX_OFFLOAD_TIMESTAMP and read_clock() in 5/6 will return -EIO > on every call. Clear priv->nic_timestamp_supported on alloc failure so > the capability is advertised honestly. To avoid dishonest capability advertisements and -EIO loops while preserving hardware capability tracking, we plan to leave priv->nic_timestamp_supported untouched (as it represents immutable hardware state). Instead, in v2 we'll predicate capability advertisement in gve_dev_info_get() directly on memzone allocation (priv->nic_ts_report_mz) and add an explicit error log advising a device reset for transient recovery. On Wed, May 13, 2026 at 7:41 AM Stephen Hemminger <[email protected]> wrote: > Patch 5: net/gve: support read clock ethdev op > > Warning: gve_read_clock and the periodic alarm callback > gve_read_nic_clock share a single DMA buffer (priv->nic_ts_report) > for the GVE_ADMINQ_REPORT_NIC_TIMESTAMP response. The adminq_lock > introduced in patch 1 serializes the adminq command exchange, but > is released inside gve_adminq_execute_cmd before either caller > reads the buffer: > > err = gve_adminq_report_nic_timestamp(priv, > priv->nic_ts_report_mz->iova); > /* lock has already been released here */ > if (err != 0) > return err; > ts = be64_to_cpu(priv->nic_ts_report->nic_timestamp); > > If the user thread (gve_read_clock) is preempted between returning > from gve_adminq_report_nic_timestamp and the be64_to_cpu read, the > alarm callback can run, memset() the buffer to zero, take the > adminq lock, issue its own command, and either leave its own > response in the buffer or be mid-DMA when the user thread reads. > The user thread will then read zero or a different command's > response. > > The simplest fix is to return the cached > priv->last_read_nic_timestamp from gve_read_clock rather than > issuing a fresh adminq command. The periodic sync runs every > 250ms, so the cached value is recent; the .read_clock semantics > do not require a brand new device read. Alternatively, use a > separate per-caller DMA buffer or extend the critical section > to cover the buffer read. Before returning the cached timestamp as suggested, we wanted to clarify the API contract: - Is the fact that .read_clock does not require a fresh device read documented in the DPDK specification, or is this based on typical application use cases? If the latter, what are those typical use cases?
