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

[Harshitha Ramamurthy]

On Thu, Sep 10, 2026 at 3:13 PM <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: 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().

As the AI notes, both the issues it has flagged are resolved in the
next patch in the series.

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