Re: [PATCH net-next v7 11/12] gve: add ctrl ops for queue operations

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] After this patch converts gve_create_rings()/gve_destroy_rings()
  in drivers/net/ethernet/google/gve/gve_main.c to call…

--

gve: add ctrl ops for queue operations

This adds create_queues and destroy_queues to struct gve_ctrl_ops, moves the
bulk queue create/destroy sequences from gve_create_rings()/gve_destroy_rings()
in gve_main.c into gve_adminq_create_queues()/gve_adminq_destroy_queues(), and
wires them up in gve_adminq_ops. Described as code movement with no functional
change.

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index f0218d78bc5d1..450341b70968d 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -943,6 +943,30 @@ static void gve_set_default_rss_sizes(struct gve_priv *priv)
>  	}
>  }
>  
> +int gve_adminq_destroy_queues(struct gve_priv *priv)
> +{
> +	int num_tx_queues = gve_num_tx_queues(priv);
> +	int err;
> +
> +	err = gve_adminq_destroy_tx_queues(priv, 0, num_tx_queues);

[ ... ]

> +	err = gve_adminq_destroy_rx_queues(priv, priv->rx_cfg.num_queues);

[ ... ]

> @@ -1791,3 +1815,29 @@ void gve_adminq_teardown_mgmt_irq(struct gve_priv *priv)
>  		priv->mgmt_irq_requested = false;
>  	}
>  }
> +
> +int gve_adminq_create_queues(struct gve_priv *priv)
> +{
> +	int num_tx_queues = gve_num_tx_queues(priv);
> +	int err;
> +
> +	err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);

[ ... ]

> +	err = gve_adminq_create_rx_queues(priv, priv->rx_cfg.num_queues);

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> index 0b48e9f2a82ae..d696e4932a8b9 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.h
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.h
> @@ -628,6 +628,8 @@ int gve_adminq_configure_device_resources(struct gve_priv *priv,
>  					  dma_addr_t db_array_bus_addr,
>  					  u32 num_ntfy_blks);
>  int gve_adminq_deconfigure_device_resources(struct gve_priv *priv);
> +int gve_adminq_create_queues(struct gve_priv *priv);
> +int gve_adminq_destroy_queues(struct gve_priv *priv);
>  int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
>  int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
      ^^^^

[Severity: Low]
This isn't a bug, but after this patch is it still necessary to export the
four bulk helpers from gve_adminq.h?

With gve_create_rings() and gve_destroy_rings() converted to
priv->ctrl_ops->create_queues() / priv->ctrl_ops->destroy_queues(), the only
remaining references to gve_adminq_create_tx_queues(),
gve_adminq_create_rx_queues(), gve_adminq_destroy_tx_queues() and
gve_adminq_destroy_rx_queues() are their definitions in gve_adminq.c and the
calls from the two new wrappers in the same file:

  gve_adminq_create_queues()
      gve_adminq_create_tx_queues()
      gve_adminq_create_rx_queues()

  gve_adminq_destroy_queues()
      gve_adminq_destroy_tx_queues()
      gve_adminq_destroy_rx_queues()

All four definitions precede those call sites, so could they be made static
and their declarations dropped from the header, leaving only the
ctrl_ops-backed gve_adminq_create_queues()/gve_adminq_destroy_queues() in the
shared header? Keeping the AdminQ-specific helpers visible module-wide leaves
an entry point that bypasses the ops indirection this series is introducing.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 44669b2f25d7d..33af1437b3c04 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -886,33 +886,16 @@ static int gve_unregister_qpls(struct gve_priv *priv)
>  
>  static int gve_create_rings(struct gve_priv *priv)
>  {
> -	int num_tx_queues = gve_num_tx_queues(priv);
>  	int err;
>  	int i;
>  
> -	err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);
> +	err = priv->ctrl_ops->create_queues(priv);

[ ... ]

> @@ -1067,26 +1050,12 @@ static int gve_queues_mem_alloc(struct gve_priv *priv,
>  
>  static int gve_destroy_rings(struct gve_priv *priv)
>  {
> -	int num_tx_queues = gve_num_tx_queues(priv);
>  	int err;
>  
> -	err = gve_adminq_destroy_tx_queues(priv, 0, num_tx_queues);
> +	err = priv->ctrl_ops->destroy_queues(priv);

[ ... ]

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