Re: [PATCH net-next v9 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: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "These changes are only code movement,
  no functional change".

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a hard reset
  leaks the AdminQ dma_pool and its buffer.

--

gve: refactor gve_init_priv for reset path

This patch splits gve_init_priv() so that gve_probe() now does the AdminQ
setup (the new gve_adminq_init()) and the device property negotiation
(gve_adminq_get_device_properties()). Device setup moves into a new
gve_setup_device() helper. gve_reset_recovery() now calls
gve_adminq_init(), gve_adminq_verify_driver_compatibility() and
gve_setup_device() directly, instead of gve_init_priv(priv, true).

> 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);

[Severity: Medium]
This isn't a bug introduced by this patch, but does a hard reset leak the
AdminQ dma_pool and its buffer here?

A hard reset is gve_reset(priv, false), or gve_reset() after gve_close()
fails. Either way it goes through:

gve_reset_and_teardown()
  gve_trigger_reset()
    gve_adminq_release()
      gve_clear_admin_queue_ok(priv);
  gve_teardown_priv_resources()
    gve_adminq_free()

gve_adminq_free() then returns early, because ADMIN_QUEUE_OK is already
clear:

void gve_adminq_free(struct gve_priv *priv)
{
	if (!gve_get_admin_queue_ok(priv))
		return;
	gve_adminq_release(priv);
	dma_pool_free(priv->adminq_pool, priv->adminq, priv->adminq_bus_addr);
	dma_pool_destroy(priv->adminq_pool);
	...
}

So dma_pool_free() and dma_pool_destroy() never run. Then
gve_adminq_init() here calls gve_adminq_alloc(), which puts new
allocations in priv->adminq_pool and priv->adminq. The old pool and
buffer are lost.

The same thing seems to happen when gve_teardown_device_resources() calls
gve_trigger_reset() after a failed AdminQ command.

The baseline gve_init_priv(priv, true) path leaked in the same way. The
later "gve: simplify reset logic" patch in this series appears to fix it.
That patch makes gve_adminq_release() static and calls it only from
gve_adminq_free(). It also stops gve_adminq_release() from clearing
ADMIN_QUEUE_OK and removes gve_trigger_reset().

> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Failed to alloc admin queue: err=%d\n", err);
>  		goto err;
> +	}
> +

[ ... ]

>  	if (was_up) {
>  		err = gve_open(priv->dev);
>  		if (err)
> -			goto err;
> +			return err;

[Severity: Medium]
The commit message says:

  These changes are only code movement, no functional change.

Is that accurate? The patch seems to change behaviour in a few places.

First, gve_adminq_init() writes the version string before it allocates
the AdminQ:

	gve_adminq_write_version(&reg_bar->driver_version);
	return gve_adminq_alloc(priv);

gve_reset_recovery() now calls this helper. That means the driver version
string is written to the BAR0 driver_version register on every reset,
resume and recovery. Before this patch, gve_write_version() ran only once,
from gve_probe(). gve_recover() still does this at the end of the series.

Second, the order of device accesses in gve_probe() changes. The version
write now comes after the max_tx_queues/max_rx_queues ioread32be() reads
and after alloc_etherdev_mqs(). AdminQ registration,
VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE now run before ctrl_ops is
set, before map_db_bar() and before alloc_ordered_workqueue().

Third, when gve_open() fails here, the old goto err path logged "Reset
failed! !!! DISABLING ALL QUEUES !!!" and called gve_turndown(). Now the
function just returns. Neither gve_reset() nor gve_resume() logs the
error, so a failed re-open after reset or resume leaves nothing in the log.

The only place this is mentioned is the v3 changelog below the ---
line, and that text is dropped when the patch is applied. The changelog
says "don't free device resources if gve_open() fails in the reset path".
The baseline err label did not free device resources either, though.

The later "gve: simplify reset logic" patch adds a "Failed to start
queues ... DISABLING ALL QUEUES" message in gve_recover(). At this commit,
though, the message is missing.

Could the commit message describe these changes, instead of calling the
patch only code movement?

>  	}
>  	return 0;

[ ... ]

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