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

[Przemek Kitszel]

On 8/3/26 20:46, Harshitha Ramamurthy wrote:
> 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. In the upcoming MailboxQ mode,
> these properties will be returned in the response of a mailbox
> message. Hence, add struct gve_device_info as the structure that
> holds all the properties that are negotiated with the device in
> either mode.
>
> Change the AdminQ mode method gve_adminq_describe_device()
> and its children to fill up device information into this newly
> introduced struct gve_device_info. Move a few helper functions
> and code that set device properties in the priv structure into
> gve_init_priv(). So now gve_init_priv() calls/does the following:
>
> - gve_set_mtu()
> - gve_set_mac()
> - gve_set_queue_properties()
> - gve_set_buf_sizes()
> - set flow steering and RSS properties
> - set other priv properties
>
> When MailboxQ support is added, device information will be filled
> into the same structure and the same gve_init_priv() path would be
> used to set device properties to ensure common code reusage.
>
> These changes are refactors only, no functional change.
>
> Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> Reviewed-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> ---
> Changes in v3:
> - Read default_min_ring_size from device info instead of priv
>
>   drivers/net/ethernet/google/gve/gve.h        |  29 +++++
>   drivers/net/ethernet/google/gve/gve_adminq.c | 115 +++++++++++--------
>   drivers/net/ethernet/google/gve/gve_adminq.h |   6 -
>   drivers/net/ethernet/google/gve/gve_main.c   | 101 +++++++++++-----
>   4 files changed, 170 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index c280ff35ee77..021adb9108df 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;
> +};

you could save some bytes of the struct size by reordering fields

> +
>   struct gve_priv {
>   	struct net_device *dev;
>   	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> @@ -929,6 +957,7 @@ struct gve_priv {
>   	struct gve_nic_ts_report *nic_ts_report;
>   	dma_addr_t nic_ts_report_bus;
>   	u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
> +	struct gve_device_info device_info;
>   };


[..]

> -int gve_set_mtu(struct gve_priv *priv,
> -		struct gve_device_descriptor *descriptor)
> +static int gve_set_mtu(struct gve_priv *priv)
>   {
> +	struct gve_device_info *device_info = &priv->device_info;
>   	u16 mtu;
>   
> -	mtu = be16_to_cpu(descriptor->mtu);
> +	mtu = device_info->max_mtu;
>   	if (mtu < ETH_MIN_MTU) {
>   		dev_err(&priv->pdev->dev, "MTU %d below minimum MTU\n", mtu);
>   		return -EINVAL;
>   	}
>   	priv->dev->max_mtu = mtu;
> +	priv->dev->mtu = priv->dev->max_mtu;

nit: I would just use `= mtu`

>   
>   	return 0;
>   }
>   
> -void gve_set_mac(struct gve_priv *priv,
> -		 struct gve_device_descriptor *descriptor)
> +static void gve_set_mac(struct gve_priv *priv)
>   {
> -	eth_hw_addr_set(priv->dev, descriptor->mac);
> -	dev_info(&priv->pdev->dev, "MAC addr: %pM\n", descriptor->mac);
> +	struct gve_device_info *device_info = &priv->device_info;
> +
> +	eth_hw_addr_set(priv->dev, device_info->mac);
> +	dev_info(&priv->pdev->dev, "MAC addr: %pM\n", device_info->mac);
> +}

whole function is rewritten (in git sense), and it was just added in
previous commit

one way to avoid that is to extract "mac" variable in the prev patch,
and here only update the assignement and function signature

> +
> +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;
> +
> +	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;
>   }
>   
>   static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
>   {
> +	struct gve_device_info *device_info = &priv->device_info;
>   	int err;
>   
>   	/* Set up the adminq */
> @@ -2514,7 +2533,7 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
>   	if (skip_describe_device)
>   		goto setup_device;
>   
> -	priv->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
> +	device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
>   	/* Get the initial information we need from the device */
>   	err = gve_adminq_describe_device(priv);
>   	if (err) {
> @@ -2523,6 +2542,8 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
>   		goto err;
>   	}
>   
> +	priv->queue_format = priv->device_info.queue_format;
> +
>   	err = gve_set_num_ntfy_blks(priv);
>   	if (err) {
>   		dev_err(&priv->pdev->dev,
> @@ -2546,12 +2567,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
>   		netif_set_tso_max_size(priv->dev, GVE_DQO_TX_MAX);
>   	}
>   
> -	priv->dev->mtu = priv->dev->max_mtu;
> +	if (gve_set_mtu(priv)) {
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
> +	priv->num_event_counters = device_info->num_event_counters;
> +
> +	gve_set_mac(priv);
> +
> +	gve_set_queue_properties(priv);
> +	priv->modify_ring_size_enabled = device_info->modify_ring_size_enabled;
> +
> +	gve_set_buf_sizes(priv);
> +
> +	priv->max_flow_rules = device_info->max_flow_rules;
> +	if (priv->max_flow_rules)
> +		priv->dev->hw_features |= NETIF_F_NTUPLE;
> +
> +	priv->rss_key_size = device_info->rss_key_size;
> +	priv->rss_lut_size = device_info->rss_lut_size;
> +	priv->cache_rss_config = device_info->cache_rss_config;

I don't know, especially with your MailboxQ coming next, if following
makes sense, but I wonder why you have to copy whole device info into
priv (or IOW, hold two copies of most params)?

Alternative will be to use variables from device info everywhere,
you could even achieve that without any code changes via struct_group

> +
>   	priv->numa_node = dev_to_node(&priv->pdev->dev);
>   	priv->tx_cfg.num_xdp_queues = 0;
>   	priv->rx_copybreak = GVE_DEFAULT_RX_COPYBREAK;
>   	priv->ts_config.tx_type = HWTSTAMP_TX_OFF;
>   	priv->ts_config.rx_filter = HWTSTAMP_FILTER_NONE;
> +	priv->nic_timestamp_supported = device_info->nic_timestamp_supported;
>   
>   setup_device:
>   	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);