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

On Thu, Sep 10, 2026 at 3:13 PM [off-list ref] 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.

quoted

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

[ ... ]

quoted

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

quoted

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

[ ... ]

quoted

-     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