Re: [PATCH net-next v6 01/12] gve: add struct gve_device_info to hold 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 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.
>
> > 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;
> > +};
> > +
>
> [ ... ]
>
> > 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
>
> [ ... ]
>
> > @@ -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;
>
> [ ... ]
>
> > @@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,
>
> [ ... ]
>
> >       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?
>
> [ ... ]
>
> > @@ -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.

>
> > 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
>
> [ ... ]
>
> > @@ -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