netdev - Re: [PATCH net-next] gve: Add retry logic for recoverable adminq errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240628180206.07c0a1b2@kernel.org>
Date: Fri, 28 Jun 2024 18:02:06 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Rushil Gupta <rushilg@...gle.com>
Cc: netdev@...r.kernel.org, jeroendb@...gle.com, pkaligineedi@...gle.com,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 willemb@...gle.com, hramamurthy@...gle.com, Shailend Chand
 <shailend@...gle.com>, Ziwei Xiao <ziweixiao@...gle.com>
Subject: Re: [PATCH net-next] gve: Add retry logic for recoverable adminq
 errors

On Fri, 28 Jun 2024 20:41:39 +0000 Rushil Gupta wrote:
> An adminq command is retried if it fails with an ETIME error code
> which translates to the deadline exceeded error for the device.
> The create and destroy adminq commands are now managed via a common
> method. This method keeps track of return codes for each queue and retries
> the commands for the queues that failed with ETIME.
> Other adminq commands that do not require queue level granularity are
> simply retried in gve_adminq_execute_cmd.
> 
> Signed-off-by: Rushil Gupta <rushilg@...gle.com>
> Signed-off-by: Jeroen de Borst <jeroendb@...gle.com>
> Reviewed-by: Shailend Chand <shailend@...gle.com>
> Reviewed-by: Ziwei Xiao <ziweixiao@...gle.com>

I told you once already that you're not allowed to repost patches
within 24h. You should also include a change long when you repost.

Since Jeroen is a maintainer of this driver, and you are not listed
in the MAINTAINERS file I don't understand why you're the one sending
this. We can't teach everyone at google the upstream process one by
one so I'd like to request that only the listed maintainers post pure
GVE patches (or the folks who are heavily involved upstream).

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index c5bbc1b7524e..74c61b90ea45 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -12,7 +12,7 @@
>  
>  #define GVE_MAX_ADMINQ_RELEASE_CHECK	500
>  #define GVE_ADMINQ_SLEEP_LEN		20
> -#define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK	100
> +#define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK	1000
>  
>  #define GVE_DEVICE_OPTION_ERROR_FMT "%s option error:\n" \
>  "Expected: length=%d, feature_mask=%x.\n" \
> @@ -415,14 +415,17 @@ static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
>  /* Flushes all AQ commands currently queued and waits for them to complete.
>   * If there are failures, it will return the first error.
>   */
> -static int gve_adminq_kick_and_wait(struct gve_priv *priv)
> +static int gve_adminq_kick_and_wait(struct gve_priv *priv, int ret_cnt, int *ret_codes)
>  {
>  	int tail, head;
> -	int i;
> +	int i, j;
>  
>  	tail = ioread32be(&priv->reg_bar0->adminq_event_counter);
>  	head = priv->adminq_prod_cnt;
>  
> +	if ((head - tail) > ret_cnt)

please delete all the pointless parenthesis in + lines of this patch.
-- 
pw-bot: cr