Re: [PATCH net-next v4 04/15] gve: add struct gve_device_info to hold device properties

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver ignores the hardware-requested default queue limits and blindly allocates the maximum number of queues due to an uninitialized variable introduced during refactoring.
- [Low] The conditional assignment of `max_rx_buffer_size` incorrectly drops valid device limits that are smaller than the driver's default size.
--

--- Patch [4]: [PATCH net-next v4 04/15] gve: add struct gve_device_info to hold device properties ---
commit 7fded5d146c6551cb7cea8119862eccd51e6f61f
Author: Harshitha Ramamurthy <[email protected]>

gve: add struct gve_device_info to hold device properties

This commit introduces a new gve_device_info structure to hold negotiated
device properties for both AdminQ and upcoming MailboxQ modes. It refactors
gve_adminq_describe_device and its children to populate this structure and
centralizes device property initialization in gve_init_priv.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> -void gve_set_queue_properties(struct gve_priv *priv,
> -			      struct gve_device_descriptor *descriptor)
> +static void gve_set_queue_properties(struct gve_priv *priv)
>  {
> -	/* set default descriptor counts */
> -	gve_set_default_desc_cnt(priv, descriptor);
> +	struct gve_device_info *device_info = &priv->device_info;
>  
> -	priv->max_registered_pages = be64_to_cpu(descriptor->max_registered_pages);
> -	priv->tx_pages_per_qpl = be16_to_cpu(descriptor->tx_pages_per_qpl);
> -	priv->default_num_queues = be16_to_cpu(descriptor->default_num_queues);
> +	gve_set_desc_cnt(priv);
> +	priv->max_registered_pages = device_info->max_registered_pages;
> +	priv->tx_pages_per_qpl = device_info->tx_pages_per_qpl;
>  }

[Severity: Medium]
Does this leave priv->default_num_queues uninitialized?

The previous code initialized it from the descriptor. The new device_info
struct holds default_tx_queues and default_rx_queues, but it appears
priv->default_num_queues is no longer being populated.

Because of this, will gve_set_num_queues() bypass device-suggested queue
limits and always allocate the maximum possible number of queues since the
check for priv->default_num_queues > 0 will fail?

[ ... ]

> +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)
> +		priv->max_rx_buffer_size = device_info->max_rx_buffer_size;

[Severity: Low]
Is it intentional to only update priv->max_rx_buffer_size if the device
limit is strictly greater than the driver's current default?

The previous implementation in gve_enable_supported_features() applied the
device value unconditionally. Does this conditional check discard valid
hardware limits if they happen to be smaller than the driver's default size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4