Re: [PATCH net-next v4 14/15] gve: add ctrl ops to for queue operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double logging remains in the create queues error path, contradicting the commit message.
--

--- Patch [14]: [PATCH net-next v4 14/15] gve: add ctrl ops to for queue operations ---
commit 026806681c18ac8de5714de37a0562162a553296
Author: Harshitha Ramamurthy <[email protected]>

gve: add ctrl ops to for queue operations

This commit adds control operations for creating and destroying queues. It moves existing admin queue functions into the new ops structure to abstract these operations.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index eecc988073951..500247b0e34aa 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -874,33 +874,18 @@ 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);
>  	if (err) {
> -		netif_err(priv, drv, priv->dev, "failed to create %d tx queues\n",
> -			  num_tx_queues);
> -		/* This failure will trigger a reset - no need to clean
> -		 * up
> -		 */
> -		return err;
> -	}
> -	netif_dbg(priv, drv, priv->dev, "created %d tx queues\n",
> -		  num_tx_queues);
> -
> -	err = gve_adminq_create_rx_queues(priv, priv->rx_cfg.num_queues);
> -	if (err) {
> -		netif_err(priv, drv, priv->dev, "failed to create %d rx queues\n",
> -			  priv->rx_cfg.num_queues);
> +		netif_err(priv, drv, priv->dev,
> +			  "failed to create queues\n");

[Severity: Medium]
The v3 changelog notes state "remove double logging in the create queues
path", but doesn't this still leave a double logging issue when
create_queues() fails?

If priv->ctrl_ops->create_queues() (which maps to gve_adminq_create_queues())
encounters an error, it logs a specific message:

drivers/net/ethernet/google/gve/gve_adminq.c:gve_adminq_create_queues() {
	...
	err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);
	if (err) {
		netif_err(priv, drv, priv->dev, "failed to create %d tx queues\n",
			  num_tx_queues);
		return err;
	}
	...
}

Then it returns the error back here to gve_create_rings(), which logs a second
generic "failed to create queues" message. Should this netif_err() in
gve_create_rings() be removed to fully resolve the double logging?

>  		/* This failure will trigger a reset - no need to clean
>  		 * up
>  		 */
>  		return err;
>  	}

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