On Mon, 5 Oct 2026 12:39:31 -0700
Joshua Washington <[email protected]> wrote:
> This patch series includes a number of precursor changes to the GVE
> ethdev driver layer in preparation for introducing support for a new
> GVE Mailbox control plane (an alternative to the existing AdminQ control
> plane).
>
> The most major change in this series is the introduction of a new
> control_ops struct that will be used by both AQ and Mailbox for various
> operations which may need to communicate with the device. There are also
> some changes to how RSS and timestamping are handled in ethdev due to
> slight feature differences between the control planes.
>
> ---
Lots of issues found with manual run of AI review.
Review: [PATCH v2 0/8] net/gve control ops rework (bundle 2148)
Series summary
Patches 1 and 6 add the query_rss op and NULL handling for "optional"
ops, but no op table in this series sets query_rss and the only table
fills every optional op. None of that code can run until the mailbox
backend lands. The abstraction is easier to judge posted together with
its second user.
Errors
[PATCH v2 1/8] net/gve: refactor ethdev for control ops interface
1. Driver compatibility is no longer sent to the device after reset.
gve_verify_driver_compatibility() moved into
gve_adminq_get_device_properties(), which is skipped on reset:
if (skip_describe_device)
goto setup_device;
...
err = priv->ctrl_ops->get_device_properties(priv);
Before this patch it ran right after gve_adminq_alloc() on every
gve_init_priv() call, including gve_dev_reset() ->
gve_init_priv(priv, true). Functional change in a refactor patch,
not mentioned in the commit message. Keep it on the path that runs
on reset, e.g. an AdminQ init_ctrl_plane op that does
gve_adminq_alloc() followed by the compatibility check.
2. Function pointer table stored in shared memory.
priv->ctrl_ops = &gve_adminq_ops;
priv is dev->data->dev_private, shared with secondary processes, and
holds the primary's address of gve_adminq_ops. A secondary installs
dev_ops in gve_dev_init() and returns; gve_link_update() has no
process type check:
err = priv->ctrl_ops->report_link_speed(priv);
GVE has no LSC interrupt, so rte_eth_link_get() from a secondary on
a started port calls link_update and jumps through the primary's
address. That crashes whenever the driver is mapped at a different
address (shared build, PIE with ASLR). Same for read_clock, mtu_set,
RSS and flow ops. Before this patch the secondary called the AdminQ
functions directly. Keep the table process local: store a control
plane mode enum in priv and resolve the ops with an inline helper,
or keep the pointer in eth_dev->process_private and set it in both
primary and secondary init.
[PATCH v2 6/8] net/gve: add RSS cache boolean flag
3. One failed AdminQ RSS command locks out RSS configuration for the
life of the port.
err = gve_adminq_execute_cmd(priv, &cmd);
priv->rss_cache_dirty = true;
if (err == 0)
gve_update_priv_rss_config(priv, rss_config);
On error the flag stays set. gve_adminq_ops has no query_rss, so
gve_rss_update_cache() returns -ENOENT from gve_rss_hash_update(),
gve_rss_hash_conf_get(), gve_rss_reta_update() and
gve_rss_reta_query(), and gve_dev_configure() skips the RETA reset
for the new queue count. Only gve_update_priv_rss_config() clears
the flag, and it is reached only through configure_rss, which every
caller gates on gve_rss_update_cache(). gve_dev_reset() does not
recover: gve_init_priv() touches the flag only when query_rss is
set. The same lockout follows a successful command when
gve_update_priv_rss_config() fails with -ENOMEM; its return value
is ignored since patch 5.
Before this patch a failed AdminQ command left the cached config in
place. AdminQ has no query, so its cache is authoritative. Drop the
dirty write from gve_adminq_configure_rss(), leave it to a backend
that implements query_rss, and propagate the update result:
err = gve_adminq_execute_cmd(priv, &cmd);
if (err == 0)
err = gve_update_priv_rss_config(priv, rss_config);
Warnings
[PATCH v2 6/8] net/gve: add RSS cache boolean flag
4. priv->rss_config is read before the cache is refreshed.
The commit message says the config must not be read while dirty,
but gve_rss_hash_update() checks priv->rss_config.key_size, copies
it into rss_conf->rss_key_len, and sizes the new table from the
cache before refreshing it:
rss_reta_size = priv->rss_config.indir ?
priv->rss_config.indir_size :
GVE_RSS_INDIR_SIZE;
err = gve_init_rss_config(&gve_rss_conf, rss_conf->rss_key_len,
rss_reta_size);
...
err = gve_rss_update_cache(priv);
The later copy then takes its length from the pre-refresh cache and
its source from the post-refresh one:
memcpy(gve_rss_conf.indir, priv->rss_config.indir,
gve_rss_conf.indir_size * sizeof(*priv->rss_config.indir));
gve_dev_configure() likewise tests priv->rss_config.indir before
refreshing. Call gve_rss_update_cache() before the first
priv->rss_config access in both functions.
[PATCH v2 7/8] net/gve: fix RSS config memory leak on close
5. Wrong Fixes tag. priv->rss_config has been allocated by
gve_update_priv_rss_config() since RSS support was added and was
never freed, on close or on remove, before or after 7ba84453bacf.
Use:
Fixes: 63ef54569760 ("net/gve: support RSS configuration update")
6. Stable fix sits behind six refactor patches. It applies to main on
its own; move it to the front of the series.
[PATCH v2 8/8] net/gve: refactor timestamp support to clock read type
7. RTE_ETH_RX_OFFLOAD_TIMESTAMP is now advertised when timestamp setup
failed.
- if (!gve_is_gqi(priv) && priv->nic_ts_report_mz)
+ if (priv->clk_read_type != GVE_DEV_CLK_UNSUPPORTED)
gve_setup_nic_timestamp() leaves nic_ts_report_mz NULL when the
memzone reservation fails and frees it when the sync thread cannot
be created; clk_read_type stays GVE_DEV_CLK_CMD in both cases.
nic_ts_stale stays set, so the Rx path never stamps packets. Before
this patch rte_eth_dev_configure() rejected the offload in that
state. Keep the nic_ts_report_mz test, or set clk_read_type to
GVE_DEV_CLK_UNSUPPORTED on setup failure.
Info
[PATCH v2 1/8] net/gve: refactor ethdev for control ops interface
8. The comment marks free_db_resources, setup_stats_report,
report_nic_timestamp and the page list ops optional, but only the
set_mtu caller checks for NULL. gve_teardown_device_resources()
calls free_db_resources unconditionally. Either check at each call
site or drop the "optional" claim until a backend omits them.
[PATCH v2 6/8] net/gve: add RSS cache boolean flag
9. Pre-existing, not introduced by this patch, but the lines are
re-indented here: gve_dev_configure() ignores the return of
gve_init_rss_config_from_priv(). If the key allocation fails,
update_reta_config.indir is uninitialized stack and
gve_generate_rss_reta() writes through it. If the indir allocation
fails, gve_init_rss_config() frees key without clearing it and
gve_free_rss_config() frees it again.
10. Pre-existing: gve_update_priv_rss_config() assigns rte_realloc()
straight back to the pointer, leaking the old buffer on failure:
priv_config->key = rte_realloc(priv_config->key, key_bytes,
RTE_CACHE_LINE_SIZE);
Same for indir.
[PATCH v2 7/8] net/gve: fix RSS config memory leak on close
11. gve_teardown_device_resources() also runs on gve_dev_reset(). After
a reset key and indir are NULL but key_size, indir_size and
hash_types survive, so gve_rss_hash_conf_get() reports the old
rss_key_len and rss_hf. Zero priv->rss_config after freeing.
[PATCH v2 8/8] net/gve: refactor timestamp support to clock read type
12. The GVE_DEV_CLK_UNSUPPORTED check added to
gve_alloc_nic_ts_report() is dead: its only caller,
gve_setup_nic_timestamp(), already returns on that value.
Review-Result: ERROR
