Re: [PATCH net-next v5 05/12] gve: refactor gve_init_priv for reset path
From: netdev-bot+sashiko
Date: Sun Sep 06 2026 - 20:05:48 EST
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 4
- [Low] gve_reset_recovery() error-path change contradicts the "only code
movement, no functional change" claim: when gve_open() fails during…
- [Low] Folding gve_adminq_write_version() into the new gve_adminq_init()
changes when and how often the driver version register is written,…
- [Low] Duplicated / misattributed error reporting: the new gve_probe()
call site prints "Could not get device information: err=%d" for any…
- [Low] Stale comment in gve_probe(): the "Set default and supported
features" block still says features "might be set in other locations…
--
gve: refactor gve_init_priv for reset path
This splits gve_init_priv() so that device property negotiation and
AdminQ setup happen once in gve_probe(), introduces gve_adminq_init()
and gve_setup_device(), and makes gve_reset_recovery() call the setup
helpers directly.
A few questions on the "no functional change" claim and on the new
diagnostics below.
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 1bc5e32b6618e..48cc8a6be186c 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -1253,6 +1253,8 @@ static inline bool gve_is_clock_enabled(struct gve_priv *priv)
> return priv->nic_ts_report;
> }
>
> +void gve_adminq_write_version(u8 __iomem *driver_version_register);
> +
> /* gqi napi handler defined in gve_main.c */
> int gve_napi_poll(struct napi_struct *napi, int budget);
>
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index f420a8e1dd3df..a62cb7a921d07 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
[ ... ]
> @@ -355,6 +357,14 @@ int gve_adminq_alloc(struct device *dev, struct gve_priv *priv)
> return 0;
> }
>
> +int gve_adminq_init(struct gve_priv *priv)
> +{
> + struct gve_registers __iomem *reg_bar = priv->reg_bar0;
> +
> + gve_adminq_write_version(®_bar->driver_version);
> + return gve_adminq_alloc(priv);
> +}
> +
[Severity: Low]
Folding the version write into gve_adminq_init() changes when and how
often the driver version register is written, so is the commit message
statement "These changes are only code movement, no functional change"
accurate here?
Before this patch the string was written exactly once per probe, from
gve_probe(), and before max_tx_queues/max_rx_queues were read. After
this patch gve_reset_recovery() also calls gve_adminq_init(), so the
version string is re-emitted to the device on every gve_reset() (ethtool
reset, device-requested reset from the service task, adminq timeout) and
on gve_resume().
struct gve_registers::driver_version is a single byte write port that
gve_adminq_write_version() feeds a byte at a time, so re-writing it
looks harmless from the driver side, but it is a device-visible change.
Is re-writing the version on each AdminQ re-init intentional, and could
the changelog say so?
> void gve_adminq_release(struct gve_priv *priv)
> {
> int i = 0;
[ ... ]
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 08859611a96e4..16ce55c6d74f6 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -2566,15 +2541,32 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> {
> int err;
>
> - err = gve_init_priv(priv, true);
> - if (err)
> + err = gve_adminq_init(priv);
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Failed to alloc admin queue: err=%d\n", err);
> goto err;
> + }
> +
> + err = gve_adminq_verify_driver_compatibility(priv);
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Could not verify driver compatibility: err=%d\n", err);
> + goto err_free_adminq;
> + }
> +
> + err = gve_setup_device(priv);
> + if (err)
> + goto err_free_adminq;
> if (was_up) {
> err = gve_open(priv->dev);
> if (err)
> - goto err;
> + return err;
[Severity: Low]
Is this error path change also covered by "These changes are only code
movement, no functional change"?
Previously a gve_open() failure here fell into the err: label:
err:
dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
gve_turndown(priv);
Now it returns silently, so the "Reset failed!" message is no longer
printed when gve_open() fails during reset or resume recovery, for
example on an -ENOMEM from gve_queues_mem_alloc(). The path is
reachable from both gve_reset()->gve_reset_recovery(priv, true) and
gve_resume()->gve_reset_recovery(priv, priv->up_before_suspend).
The gve_turndown(priv) call looks like a no-op at that point, since it
returns early unless gve_get_napi_enabled(priv) is set and every caller
reaching gve_reset_recovery() with was_up already turned the device
down, so the observable difference appears to be only the missing
dev_err().
The behaviour change is described in the post-"---" v3 notes as "don't
free device resources if gve_open() fails in the reset path", but the
old err: label did not free device resources either, it only logged and
called gve_turndown(). Could that note be reworded, or the change
mentioned in the commit message proper?
Note the next patch in the series, "gve: simplify reset logic", turns
this into gve_recover() where a gve_open() failure does
"goto teardown_device;" and the log message comes back, so this only
affects the intermediate state.
> }
> return 0;
> +
> +err_free_adminq:
> + gve_adminq_free(priv);
> err:
> dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
> gve_turndown(priv);
> @@ -2617,7 +2609,7 @@ int gve_reset(struct gve_priv *priv, bool attempt_teardown)
> return err;
> }
>
> -static void gve_write_version(u8 __iomem *driver_version_register)
> +void gve_adminq_write_version(u8 __iomem *driver_version_register)
> {
> const char *c = gve_version_prefix;
>
> @@ -2850,7 +2842,6 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto abort_with_pci_region;
> }
>
> - gve_write_version(®_bar->driver_version);
> /* Get max queues to alloc etherdev */
> max_tx_queues = ioread32be(®_bar->max_tx_queues);
> max_rx_queues = ioread32be(®_bar->max_rx_queues);
[Severity: Low]
Related to the gve_adminq_init() comment above: this removal also
reverses the probe-time ordering between the driver version write and
the reads of max_tx_queues/max_rx_queues. The version string is now
written later, from gve_adminq_init(), after the netdev has been
allocated. Does the device care about that ordering?
> @@ -2897,13 +2888,28 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> priv->rx_cfg.packet_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
> priv->max_rx_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
>
> + err = gve_adminq_init(priv);
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Failed to alloc admin queue: err=%d\n", err);
> + goto abort_with_netdev;
> + }
> +
> + priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
> + err = gve_adminq_get_device_properties(priv);
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Could not get device information: err=%d\n", err);
> + goto abort_with_adminq;
> + }
[Severity: Low]
Is this dev_err() redundant, and misleading for one of the two failure
causes? gve_adminq_get_device_properties() in gve_adminq.c already logs
each sub-step:
err = gve_adminq_verify_driver_compatibility(priv);
if (err) {
dev_err(&priv->pdev->dev,
"Could not verify driver compatibility: err=%d\n", err);
return err;
}
/* Get the initial information we need from the device */
err = gve_adminq_describe_device(priv);
if (err) {
dev_err(&priv->pdev->dev,
"Could not get device information: err=%d\n", err);
return err;
}
So a VERIFY_DRIVER_COMPATIBILITY failure during probe prints the correct
"Could not verify driver compatibility" line and is then followed by
"Could not get device information", and a DESCRIBE_DEVICE failure prints
the same line twice.
Would dropping the dev_err() at this call site, or making the helper
silent and logging only in the callers, be preferable? The reset path
added in this patch does the latter, calling
gve_adminq_verify_driver_compatibility() directly and logging itself.
[Severity: Low]
Separate from the hunk itself: while this region of gve_probe() is being
rearranged, is the feature comment a little higher up in the same
function still accurate?
/* Set default and supported features.
*
* Features might be set in other locations as well (such as
* `gve_adminq_describe_device`).
*/
dev->hw_features = NETIF_F_HIGHDMA;
After this series gve_adminq_describe_device() and
gve_enable_supported_features() only fill priv->device_info; the netdev
feature bits (NETIF_F_GRO_HW, NETIF_F_GSO_UDP_L4, NETIF_F_NTUPLE, plus
netif_set_tso_max_size()) are set in gve_init_priv() in gve_main.c. The
only remaining reference to priv->dev->features in gve_adminq.c is the
read in the create RX queue path. Should the comment point at
gve_init_priv() instead?
> /* Set adminq ctrl ops */
> priv->ctrl_ops = &gve_adminq_ops;
>
> err = priv->ctrl_ops->map_db_bar(priv);
> if (err) {
> err = -ENOMEM;
> - goto abort_with_netdev;
> + goto abort_with_adminq;
> }
>
> gve_set_probe_in_progress(priv);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903215606.31633-1-hramamurthy%40google.com