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]

Date: Sat, 29 Jun 2024 10:56:27 -0700
From: Rushil Gupta <rushilg@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
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, Jun 28, 2024 at 6:02 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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.
I am sorry about the email mix-up. I will be careful about that in the future.
>
> 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).
I could not find one single documentation that says only listed
maintainers can post pure patches.
Authors of some of the recently accepted patches were in fact not in
the MAINTAINERS file.
I am sending this patch as I was involved in getting this code to the
upstream-ready state and testing it internally.
However, if other GVE maintainers wish to follow this rule; I am ok
with your suggestion.
>
> > 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