Re: [PATCH net] gve: Add adminq lock for creating and destroying multiple queues

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 4/17/25 22:43, Harshitha Ramamurthy wrote:
> From: Ziwei Xiao <ziweixiao@google.com>
> 
> The original adminq lock is only protecting the gve_adminq_execute_cmd
> which is aimed for sending out single adminq command. However, there are
> other callers of gve_adminq_kick_and_wait and gve_adminq_issue_cmd that
> need to take the mutex lock for mutual exclusion between them, which are
> creating and destroying rx/tx queues. Add the adminq lock for those
> unprotected callers.
> 
> Also this patch cleans up the error handling code of
> gve_adminq_destroy_tx_queue.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1108566ca509 ("gve: Add adminq mutex lock")

This looks like a correct fix, it is also nice that you have added
lockdep annotations.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
>   drivers/net/ethernet/google/gve/gve_adminq.c | 54 ++++++++++++++------
>   1 file changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 3e8fc33cc11f..659460812276 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -442,6 +442,8 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
>   	int tail, head;
>   	int i;
>   
> +	lockdep_assert_held(&priv->adminq_lock);
> +
>   	tail = ioread32be(&priv->reg_bar0->adminq_event_counter);
>   	head = priv->adminq_prod_cnt;
>   
> @@ -467,9 +469,6 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
>   	return 0;
>   }
>   
> -/* This function is not threadsafe - the caller is responsible for any
> - * necessary locks.
> - */
>   static int gve_adminq_issue_cmd(struct gve_priv *priv,
>   				union gve_adminq_command *cmd_orig)
>   {
> @@ -477,6 +476,8 @@ static int gve_adminq_issue_cmd(struct gve_priv *priv,
>   	u32 opcode;
>   	u32 tail;
>   
> +	lockdep_assert_held(&priv->adminq_lock);
> +
>   	tail = ioread32be(&priv->reg_bar0->adminq_event_counter);
>   
>   	// Check if next command will overflow the buffer.
> @@ -709,13 +710,19 @@ int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_que
>   	int err;
>   	int i;
>   
> +	mutex_lock(&priv->adminq_lock);
> +
>   	for (i = start_id; i < start_id + num_queues; i++) {
>   		err = gve_adminq_create_tx_queue(priv, i);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
> -	return gve_adminq_kick_and_wait(priv);
> +	err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> +	mutex_unlock(&priv->adminq_lock);
> +	return err;
>   }
>   
>   static void gve_adminq_get_create_rx_queue_cmd(struct gve_priv *priv,
> @@ -788,19 +795,24 @@ int gve_adminq_create_rx_queues(struct gve_priv *priv, u32 num_queues)
>   	int err;
>   	int i;
>   
> +	mutex_lock(&priv->adminq_lock);
> +
>   	for (i = 0; i < num_queues; i++) {
>   		err = gve_adminq_create_rx_queue(priv, i);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
> -	return gve_adminq_kick_and_wait(priv);
> +	err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> +	mutex_unlock(&priv->adminq_lock);
> +	return err;
>   }
>   
>   static int gve_adminq_destroy_tx_queue(struct gve_priv *priv, u32 queue_index)
>   {
>   	union gve_adminq_command cmd;
> -	int err;
>   
>   	memset(&cmd, 0, sizeof(cmd));
>   	cmd.opcode = cpu_to_be32(GVE_ADMINQ_DESTROY_TX_QUEUE);
> @@ -808,11 +820,7 @@ static int gve_adminq_destroy_tx_queue(struct gve_priv *priv, u32 queue_index)
>   		.queue_id = cpu_to_be32(queue_index),
>   	};
>   
> -	err = gve_adminq_issue_cmd(priv, &cmd);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return gve_adminq_issue_cmd(priv, &cmd);
>   }
>   
>   int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues)
> @@ -820,13 +828,19 @@ int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_qu
>   	int err;
>   	int i;
>   
> +	mutex_lock(&priv->adminq_lock);
> +
>   	for (i = start_id; i < start_id + num_queues; i++) {
>   		err = gve_adminq_destroy_tx_queue(priv, i);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
> -	return gve_adminq_kick_and_wait(priv);
> +	err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> +	mutex_unlock(&priv->adminq_lock);
> +	return err;
>   }
>   
>   static void gve_adminq_make_destroy_rx_queue_cmd(union gve_adminq_command *cmd,
> @@ -861,13 +875,19 @@ int gve_adminq_destroy_rx_queues(struct gve_priv *priv, u32 num_queues)
>   	int err;
>   	int i;
>   
> +	mutex_lock(&priv->adminq_lock);
> +
>   	for (i = 0; i < num_queues; i++) {
>   		err = gve_adminq_destroy_rx_queue(priv, i);
>   		if (err)
> -			return err;
> +			goto out;
>   	}
>   
> -	return gve_adminq_kick_and_wait(priv);
> +	err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> +	mutex_unlock(&priv->adminq_lock);
> +	return err;
>   }
>   
>   static void gve_set_default_desc_cnt(struct gve_priv *priv,