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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactored gve_set_buf_sizes() logic silently ignores hardware-requested max_rx_buffer_size limits if they are smaller than the default.
--

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

gve: add struct gve_device_info to hold device properties

In the current AdminQ mode, device properties are written into struct
gve_device_descriptor that is allocated in shared memory between the driver
and device. This patch introduces struct gve_device_info to hold all
properties negotiated in either mode to prepare for the upcoming MailboxQ
mode. Device initialization logic is refactored to use this new structure
without any intended functional changes.

> 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

[ ... ]

> +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: Medium]
Does this logic silently ignore hardware-requested buffer size limits if
they are smaller than the initial default?

Before this refactoring, gve_enable_supported_features() applied the
hardware limit unconditionally:

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

With this conditional check in gve_set_buf_sizes(), if the device advertises
a max_rx_buffer_size smaller than what priv->max_rx_buffer_size was initially
set to, the driver will ignore the hardware limit.

Could this cause the device configuration to fail or buffers to overflow if
the hardware cannot handle the larger default buffers?

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

[ ... ]

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