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

[netdev-bot+sashiko@kernel.org]

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?

> 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