Re: [PATCH net-next v6 01/12] gve: add struct gve_device_info to hold device properties

On Thu, Sep 10, 2026 at 3:13 PM [off-list ref] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] After this patch, struct gve_priv still declares `u16
  default_num_queues; /* default num queues to set up */` (gve.h:856)…
- [Low] Stale comment: gve_probe() still states "Features might be set in
  other locations as well (such as `gve_adminq_describe_device`)."…

--

gve: add struct gve_device_info to hold device properties

This adds struct gve_device_info as a single place to hold the properties
negotiated with the device, converts gve_adminq_describe_device() and its
helpers to fill that structure, and moves the code that applies those
properties to priv/netdev into gve_init_priv().

A couple of questions on leftovers from the conversion.

quoted

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index c280ff35ee771..021adb9108dff 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h

@@ -797,6 +797,34 @@ struct gve_ptp {
      struct gve_priv *priv;
 };

+struct gve_device_info {
+     enum gve_queue_format queue_format;
+     u16 default_tx_queues;
+     u16 default_rx_queues;
+     u16 max_tx_queues;
+     u16 max_rx_queues;
+     u16 default_tx_ring_size;
+     u16 default_rx_ring_size;
+     u16 max_tx_ring_size;
+     u16 max_rx_ring_size;
+     u16 min_tx_ring_size;
+     u16 min_rx_ring_size;
+     u16 max_mtu;
+     u8 mac[ETH_ALEN];
+     u16 max_rx_buffer_size;
+     u16 header_buf_size;
+     u32 max_flow_rules;
+     u16 rss_key_size;
+     u16 rss_lut_size;
+     u16 tx_pages_per_qpl;
+     u16 num_event_counters;
+     u64 max_registered_pages;
+     bool default_min_ring_size;
+     bool nic_timestamp_supported;
+     bool modify_ring_size_enabled;
+     bool cache_rss_config;
+};
+

[ ... ]

quoted

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index f05f4895f4c74..512349c5517f4 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

quoted

@@ -190,7 +190,7 @@ void gve_parse_device_option(struct gve_priv *priv,

              /* device has not provided min ring size */
              if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
-                     priv->default_min_ring_size = true;
+                     priv->device_info.default_min_ring_size = true;
              break;

[ ... ]

quoted

@@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,

[ ... ]

quoted

      if (dev_op_flow_steering &&
          (supported_features_mask & GVE_SUP_FLOW_STEERING_MASK)) {
              if (dev_op_flow_steering->max_flow_rules) {
-                     priv->max_flow_rules =
+                     info->max_flow_rules =
                              be32_to_cpu(dev_op_flow_steering->max_flow_rules);
-                     priv->dev->hw_features |= NETIF_F_NTUPLE;
                      dev_info(&priv->pdev->dev,
                               "FLOW STEERING device option enabled with max rule limit of %u.\n",
-                              priv->max_flow_rules);
+                              info->max_flow_rules);
              }
      }

[Severity: Low]
With this hunk, the NETIF_F_NTUPLE assignment leaves the describe-device
path entirely, so is the comment in gve_probe() now stale?

drivers/net/ethernet/google/gve/gve_main.c:gve_probe() {
        /* Set default and supported features.
         *
         * Features might be set in other locations as well (such as
         * `gve_adminq_describe_device`).
         */
        dev->hw_features = NETIF_F_HIGHDMA;
        ...
}

A grep of gve_adminq.c at this commit shows no remaining write to
priv->dev->hw_features (only dev_err() logging and a read of
priv->dev->features in gve_adminq_create_rx_queue()), since the only such
write moved to gve_init_priv():

        priv->max_flow_rules = device_info->max_flow_rules;
        if (priv->max_flow_rules)
                priv->dev->hw_features |= NETIF_F_NTUPLE;

Should the comment point at gve_init_priv() instead, given a second
describe-device implementation (MailboxQ) is being added on top of this?

[ ... ]

quoted

@@ -1595,6 +1618,8 @@ int gve_set_num_ntfy_blks(struct gve_priv *priv)

 void gve_set_num_queues(struct gve_priv *priv)
 {
+     struct gve_device_info *device_info = &priv->device_info;
+
      priv->tx_cfg.max_queues =
              min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
      priv->rx_cfg.max_queues =

@@ -1602,10 +1627,13 @@ void gve_set_num_queues(struct gve_priv *priv)

      priv->tx_cfg.num_queues = priv->tx_cfg.max_queues;
      priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
-     if (priv->default_num_queues > 0) {
-             priv->tx_cfg.num_queues = min_t(int, priv->default_num_queues,
+     if (device_info->default_tx_queues > 0)
+             priv->tx_cfg.num_queues = min_t(int,
+                                             device_info->default_tx_queues,
                                              priv->tx_cfg.num_queues);
-             priv->rx_cfg.num_queues = min_t(int, priv->default_num_queues,
+
+     if (device_info->default_rx_queues > 0)
+             priv->rx_cfg.num_queues = min_t(int,
+                                             device_info->default_rx_queues,
                                              priv->rx_cfg.num_queues);
-     }
 }

[Severity: Low]
Should the now-unused copies of these fields be dropped from struct
gve_priv in the same patch?

After this conversion, struct gve_priv still declares:

        bool default_min_ring_size;
        ...
        u16 default_num_queues; /* default num queues to set up */

but nothing in drivers/net/ethernet/google/gve/ writes or reads either one
any more.  The last writer of priv->default_num_queues was
gve_set_queue_properties(), which became gve_fill_device_info() setting
device_info->default_tx_queues/default_rx_queues, and the last reader is
this gve_set_num_queues() hunk.  Similarly gve_parse_device_option() now
sets priv->device_info.default_min_ring_size and
gve_enable_supported_features() reads info->default_min_ring_size.

A grep at this commit only finds the declarations:

        drivers/net/ethernet/google/gve/gve.h
          822:  bool default_min_ring_size;    /* gve_device_info */
          849:  bool default_min_ring_size;    /* gve_priv, no users */
          856:  u16 default_num_queues;        /* gve_priv, no users */

Since the leftover gve_priv fields share names with live gve_device_info
members, could later code (for instance the MailboxQ path) read
priv->default_num_queues or priv->default_min_ring_size and silently get
0/false?

No, MailboxQ won't use these fields. Since this patch was already
quite substantial, I wanted it focused on the introduction of the
struct gve_device_info and its usage in the driver. I can send a clean
up patch later for this and the comment clean-up comment above.

quoted

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 9cc343a162712..d3882de584e36 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

quoted

@@ -2507,12 +2523,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
+     priv->max_flow_rules = device_info->max_flow_rules;
+     if (priv->max_flow_rules)
+             priv->dev->hw_features |= NETIF_F_NTUPLE;
+

[ ... ]

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