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

[Harshitha Ramamurthy]

On Fri, Oct 2, 2026 at 3:06 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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.

Like the AI notes, this issue is pre-exisiting and most of the reset
related issues are fixed in the following patch “"gve: simplify reset
logic"  in the same series.

> 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().

Both these two changes listed here prepare the driver for the addition
of the new control plane - the version write is now done as part of
Adminq init specifically since this won’t be needed in the mailbox
mode.

And in mailbox mode,  the control plane has to be setup and the device
properties negotiation has to happen before we try to allocate the
net_device. Since the max TX/RX queues that the device supports comes
from this negotiation.
>
> 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.

Hmm, actually the only visible effect here is the dropping of the log
line which will be back in the next patch. gve_turndown() does nothing
at this point, because NAPI is already disabled and carrier is already
off. The following patch rewrites this path anyway.

>
> 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?

I can call out these changes in the commit message and bring the log
line back if there is a need for another respin. These issues on their
own don’t merit a respin IMO.

>
> >       }
> >       return 0;
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com