Re: [PATCH net-next v6 04/12] gve: introduce gve_adminq_get_device_properties()

[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] gve_adminq_get_device_properties()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:1148) bundles…
>
> Pre-existing issues:
> - [Medium] gve_adminq_verify_driver_compatibility()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:1264, moved verbatim…
>
> --
>
> gve: introduce gve_adminq_get_device_properties()
>
> This adds gve_adminq_get_device_properties(), a helper that runs the
> VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE admin queue commands in
> sequence, and moves the driver-info allocation and fill-in from
> gve_main.c into gve_adminq_verify_driver_compatibility().  The commit
> message describes it as pure code movement with no functional change.
>
> No Link: tags present in the commit header.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 046155ae7d669..f420a8e1dd3df 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
>
> [ ... ]
>
> > @@ -1143,6 +1145,27 @@ int gve_adminq_describe_device(struct gve_priv *priv)
> >       return err;
> >  }
> >
> > +int gve_adminq_get_device_properties(struct gve_priv *priv)
> > +{
> > +     int err;
> > +
> > +     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;
> > +     }
> > +     return 0;
> > +}
>
> [Severity: Low]
> Should this helper also carry the state handling that gve_init_priv()
> currently performs around the same two commands?
>
> gve_init_priv() in gve_main.c does this before calling
> gve_adminq_describe_device():
>
>         priv->num_registered_pages = 0;
>
>         if (skip_describe_device)
>                 goto setup_device;
>
>         device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
>         /* Get the initial information we need from the device */
>         err = gve_adminq_describe_device(priv);
>
> The queue_format reset matters because gve_adminq_describe_device()
> consumes the incoming value:
>
>         } else if (device_info->queue_format == GVE_GQI_RDA_FORMAT) {
>                 dev_info(&priv->pdev->dev,
>                          "Driver is running with GQI RDA queue format.\n");
>
> A caller that invokes gve_adminq_get_device_properties() with a stale
> non-zero device_info.queue_format would keep the previous format and end
> up with supported_features_mask left at 0, silently disabling jumbo
> frames, buffer sizes, modify-ring, flow steering, RSS and timestamps.
>
> For the record, this does not appear to be reachable anywhere: at this
> commit gve_adminq_get_device_properties() has no callers, only the
> prototype in gve_adminq.h and the definition, and gve_init_priv() still
> calls gve_adminq_describe_device() directly with the reset in place.  By
> the end of the series the concern is resolved rather than left latent:
> the single call site in gve_probe() does
>
>         priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
>         err = gve_adminq_get_device_properties(priv);

Like the AI notes, when gve_adminq_get_device_properties() is actually
used later in the series, things are setup to run properly.
>
> skip_describe_device no longer exists in the driver, and
> priv->num_registered_pages = 0 has moved into gve_setup_device().  Would
> it be worth a note in the commit message that the surrounding state
> handling stays with the caller?
>
> > @@ -1205,20 +1228,53 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
> >       return gve_adminq_execute_cmd(priv, &cmd);
> >  }
> >
> > -int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> > -                                        u64 driver_info_len,
> > -                                        dma_addr_t driver_info_addr)
> > +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv)
> >  {
> > +     struct gve_driver_info *driver_info;
> >       union gve_adminq_command cmd;
> > +     dma_addr_t driver_info_bus;
> > +     int err;
> > +
> > +     driver_info = dma_alloc_coherent(&priv->pdev->dev,
> > +                                      sizeof(struct gve_driver_info),
> > +                                      &driver_info_bus, GFP_KERNEL);
> > +     if (!driver_info)
> > +             return -ENOMEM;
>
> [ ... ]
>
> > -     return gve_adminq_execute_cmd(priv, &cmd);
> > +     err = gve_adminq_execute_cmd(priv, &cmd);
> > +
> > +     /* It's ok if the device doesn't support this */
> > +     if (err == -EOPNOTSUPP)
> > +             err = 0;
> > +
> > +     dma_free_coherent(&priv->pdev->dev,
> > +                       sizeof(struct gve_driver_info),
> > +                       driver_info, driver_info_bus);
> > +     return err;
> >  }
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but is it safe to hand
> driver_info back to the DMA allocator when the admin queue timed out?
>
> The identical sequence lived in gve_verify_driver_compatibility() in
> gve_main.c before the move, so this is only being relocated here.
>
> On the timeout path gve_adminq_kick_and_wait() has already rung the
> doorbell and gives up without quiescing the queue:
>
>         gve_adminq_kick_cmd(priv, head);
>         if (!gve_adminq_wait_for_cmd(priv, head)) {
>                 dev_err(&priv->pdev->dev, "AQ commands timed out, need to reset AQ\n");
>                 priv->adminq_timeouts++;
>                 return -ENOTRECOVERABLE;
>         }
>
> The VERIFY_DRIVER_COMPATIBILITY descriptor still carries
> driver_info_bus, so after gve_adminq_execute_cmd() returns
> -ENOTRECOVERABLE the buffer is released while the device may still
> fetch it.  Can the NIC read driver_info after dma_free_coherent() has
> returned it to the allocator?

This is a non-issue. If there is a timeout and the device is not
responding, the driver resets the device and frees the AdminQ.
>
> The same unconditional-free-after-execute pattern is used by
> gve_adminq_register_page_list(), gve_adminq_execute_extended_cmd() and
> gve_adminq_describe_device(), so if this is worth addressing it looks
> like a separate change rather than something for this code movement.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com