Re: [PATCH net-next v6 05/12] gve: refactor gve_init_priv for reset path

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] In `gve_reset_recovery()`
  (drivers/net/ethernet/google/gve/gve_main.c:2561-2564) a `gve_open()`…

Pre-existing issues:
- [Medium] Hardware-state divergence after a failed queue bring-up in
  reset recovery: when `gve_reset_recovery()` gets an error from…

--

gve: refactor gve_init_priv for reset path

gve_init_priv() is reduced to only populating priv with device properties
so it can be called once from gve_probe(), with the new gve_adminq_init()
and gve_setup_device() helpers taking over adminq allocation and device
setup. gve_reset_recovery() now calls those helpers directly instead of
going through gve_init_priv().

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index addfd8dc1a1a2..2fe280cf7e680 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]

The gve_open() failure in gve_reset_recovery() changes from "goto err" to
"return err", so it no longer reaches the shared label:

err:
	dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
	gve_turndown(priv);
	return err;

Is dropping the "Reset failed" message and the gve_turndown(priv) call on
that path intended?  A failing gve_open() during recovery (for example
gve_queues_mem_alloc() returning -ENOMEM under memory pressure) now
returns silently to gve_reset(), which only prints its own message.

The commit message says:

    These changes are only code movement, no functional change.

Would it be worth mentioning this behaviour change above the "---" line so
it survives in git history?  The v3 changelog note "don't free device
resources if gve_open() fails in the reset path" is below the cut and does
not mention the lost diagnostic or the skipped gve_turndown().

For what it is worth, the later commit "gve: simplify reset logic"
(84559f54c263) restores both: gve_recover() sends a gve_open() failure to
its teardown_device label, which logs "Recover failed! !!! DISABLING ALL
QUEUES !!!" and calls gve_teardown_device().

[Severity: Medium]

This isn't a bug introduced by this patch, but the same "return err" path
leaves the device without any reset after a partially completed queue
bring-up, and the refactor is a good opportunity to ask about it.

When gve_open() -> gve_queues_start() fails after gve_register_qpls() or
after some queues were created, it takes its reset label, and because
GVE_PRIV_FLAGS_RESET_IN_PROGRESS is set it skips the reset and frees the
ring and QPL memory instead:

reset:
	if (gve_get_reset_in_progress(priv))
		goto stop_and_free_rings;
	gve_reset_and_teardown(priv, true);
...
stop_and_free_rings:
	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
	gve_queues_mem_remove(priv);
	return err;

gve_register_qpls() and gve_create_rings() rely on the assumption that
"this failure will trigger a reset - no need to clean up", but on this path
no gve_adminq_free()/gve_adminq_release() runs, so nothing writes
GVE_DRIVER_STATUS_RESET_MASK.

Can the NIC then be left with queues created and page lists registered that
reference DMA memory the driver has just released?  The baseline code had
the same gap, since its "goto err" only logged and called gve_turndown().

Here too the end of the series appears to close this: gve_recover() routes
the gve_open() failure to gve_teardown_device(), which calls
gve_adminq_free() and therefore gve_adminq_release().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com