Re: [PATCH net-next v3 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:
- [Low] The patch introduces double logging for queue destruction failures, despite the v3 changelog explicitly stating the intention to remove double logging in the create queues path.
--

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

gve: add ctrl ops to for queue operations

This commit abstracts the creation and destruction of queues into control
operations for the driver. The patch changelog for v3 explicitly notes
that double logging was removed in the create queues path.

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

[ ... ]

> +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);
> +	if (err) {
> +		netif_err(priv, drv, priv->dev,
> +			  "failed to destroy tx queues\n");
> +		/* This failure will trigger a reset - no need to clean up */
> +		return err;
> +	}

[ ... ]

> +	err = gve_adminq_destroy_rx_queues(priv, priv->rx_cfg.num_queues);
> +	if (err) {
> +		netif_err(priv, drv, priv->dev,
> +			  "failed to destroy rx queues\n");
> +		/* This failure will trigger a reset - no need to clean up */
> +		return err;
> +	}
> +	netif_dbg(priv, drv, priv->dev, "destroyed rx queues\n");
> +	return err;
> +}

[ ... ]

> 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

[ ... ]

> @@ -1052,26 +1034,16 @@ 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);
> +
>  	if (err) {
> -		netif_err(priv, drv, priv->dev,
> -			  "failed to destroy tx queues\n");
>  		/* This failure will trigger a reset - no need to clean up */
> -		return err;
> -	}
> -	netif_dbg(priv, drv, priv->dev, "destroyed tx queues\n");
> -	err = gve_adminq_destroy_rx_queues(priv, priv->rx_cfg.num_queues);
> -	if (err) {
>  		netif_err(priv, drv, priv->dev,
> -			  "failed to destroy rx queues\n");
> -		/* This failure will trigger a reset - no need to clean up */
> -		return err;
> +			  "failed to destroy queues\n");
>  	}

[Severity: Low]
The v3 changelog notes that double logging was removed for the queue creation
path. Does this code unintentionally leave double logging in place for the
queue destruction path?

Since the underlying control operation, gve_adminq_destroy_queues(), now
prints specific error messages when tx or rx queue destruction fails,
this outer netif_err() in gve_destroy_rings() appears to log redundantly.

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