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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Dead and unused struct fields after the refactor.
- [Low] Stale comment in gve_probe().
- [Low] The commit message does not mention a behaviour change in
  gve_set_buf_sizes().

--

gve: add struct gve_device_info to hold device properties

This patch adds struct gve_device_info to hold the properties negotiated
with the device. gve_adminq_describe_device() now fills that struct
instead of writing directly into gve_priv. The helpers that copy these
properties into priv and the netdev now run from gve_init_priv(), so the
upcoming MailboxQ mode can reuse them.

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

[Severity: Low]
Are max_tx_queues and max_rx_queues meant to be used somewhere?
gve_fill_device_info() doesn't set them, and nothing reads them.

gve_set_num_queues() still takes its limits from priv->tx_cfg.max_queues
and priv->rx_cfg.max_queues. gve_probe() reads those from BAR0:

	priv->tx_cfg.max_queues =
		min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);

If a later path such as MailboxQ fills device_info->max_*_queues, would
that value be ignored without any warning?

Every reader and writer of these gve_priv fields has also moved to
device_info, but the old declarations are still there:

struct gve_priv {
	...
	bool default_min_ring_size;
	...
	u16 default_num_queues; /* default num queues to set up */
	...
};

Nothing assigns them any more, so they are always 0. The comment still
describes default_num_queues as if it holds a real value. Could these
fields be removed as part of this change?

> +	u16 default_tx_ring_size;

[ ... ]

> 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

[ ... ]

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

[Severity: Low]
This isn't a bug, but NETIF_F_NTUPLE is now set in gve_init_priv(), so
gve_adminq_describe_device() no longer changes netdev features.

The comment above the netdev feature setup in gve_probe() still names
gve_adminq_describe_device as another place where features might be set.
Should it name gve_init_priv() instead?

[ ... ]

> 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
> @@ -2398,57 +2398,71 @@ static const struct xdp_metadata_ops gve_xdp_metadata_ops = {

[ ... ]

> +static void gve_set_buf_sizes(struct gve_priv *priv)
> +{
> +	struct gve_device_info *device_info = &priv->device_info;
> +
> +	if (device_info->max_rx_buffer_size)
> +		priv->max_rx_buffer_size = device_info->max_rx_buffer_size;
> +
> +	if (gve_is_dqo(priv) &&
> +	    priv->max_rx_buffer_size > GVE_DEFAULT_RX_BUFFER_SIZE)
> +		priv->rx_cfg.packet_buffer_size = priv->max_rx_buffer_size;
> +
> +	if (device_info->header_buf_size)
> +		priv->header_buf_size = device_info->header_buf_size;
> +}

[Severity: Low]
The commit message leaves out a behaviour change here. It says the only
change that isn't a refactor is the MTU validation.

Before this patch, gve_enable_supported_features() copied the
BUFFER_SIZES option into priv whenever the option was present, zeros
included:

	priv->max_rx_buffer_size =
		be16_to_cpu(dev_op_buffer_sizes->packet_buffer_size);
	priv->header_buf_size =
		be16_to_cpu(dev_op_buffer_sizes->header_buffer_size);

With the new nonzero guards, a device that sends packet_buffer_size == 0
now leaves priv->max_rx_buffer_size at the default that gve_probe() sets:

	priv->max_rx_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;

Before the patch it would have been 0. This value is the upper limit
used when choosing the DQO packet_buffer_size and when checking ethtool
rx-buf-len.

For header_buf_size, a negotiated 0 can no longer clear a nonzero value
already in priv. Today this code in gve_init_priv() only runs from
probe, where priv is zeroed, so nothing changes yet. It still doesn't
fit the goal of device_info being the one source of negotiated state.

Should the guards be dropped, or should the commit message mention this
change?

[ ... ]

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