Re: [PATCH net-next v6 1/3] gve: skip error logging for retryable AdminQ commands

[Jordan Rhee]

On Thu, May 7, 2026 at 2:15 PM Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
>
> On 5/7/2026 2:13 PM, Harshitha Ramamurthy wrote:
> > From: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> >
> > AdminQ commands may return -EAGAIN under certain transient conditions.
> > These commands are intended to be retried by the driver, so logging
> > a formal error to the system log is misleading and creates
> > unnecessary noise.
> >
> > Modify the logging logic to skip the error message when the result
> > is -EAGAIN, and move logging to dev_err_ratelimited() to avoid
> > spamming the log.
> >
>
> Excellent!
>
> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
>
> > Reviewed-by: Joshua Washington <joshwash@xxxxxxxxxx>
> > Signed-off-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> > ---
> > Changes in v4:
> > - call out change to dev_err_ratelimited() in the commit message (Jacob Keller)
> > - remove extra print when adminQ status is GVE_ADMINQ_COMMAND_UNSET (Jacob Keller)
> > ---
> >  drivers/net/ethernet/google/gve/gve_adminq.c | 27 +++++++++++++++-----
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 08587bf40ed4..a65b14835aa0 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -416,16 +416,10 @@ static bool gve_adminq_wait_for_cmd(struct gve_priv *priv, u32 prod_cnt)
> >
> >  static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
> >  {
> > -     if (status != GVE_ADMINQ_COMMAND_PASSED &&
> > -         status != GVE_ADMINQ_COMMAND_UNSET) {
> > -             dev_err(&priv->pdev->dev, "AQ command failed with status %d\n", status);
> > -             priv->adminq_cmd_fail++;
> > -     }
> >       switch (status) {
> >       case GVE_ADMINQ_COMMAND_PASSED:
> >               return 0;
> >       case GVE_ADMINQ_COMMAND_UNSET:
> > -             dev_err(&priv->pdev->dev, "parse_aq_err: err and status both unset, this should not be possible.\n");
> >               return -EINVAL;
> >       case GVE_ADMINQ_COMMAND_ERROR_ABORTED:
> >       case GVE_ADMINQ_COMMAND_ERROR_CANCELLED:
> > @@ -455,6 +449,16 @@ static int gve_adminq_parse_err(struct gve_priv *priv, u32 status)
> >       }
> >  }
> >
> > +static bool gve_adminq_is_retryable(enum gve_adminq_opcodes opcode)
> > +{
> > +     switch (opcode) {
> > +     case GVE_ADMINQ_REPORT_NIC_TIMESTAMP:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> >  /* Flushes all AQ commands currently queued and waits for them to complete.
> >   * If there are failures, it will return the first error.
> >   */
> > @@ -482,9 +486,18 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
> >               cmd = &priv->adminq[i & priv->adminq_mask];
> >               status = be32_to_cpu(READ_ONCE(cmd->status));
> >               err = gve_adminq_parse_err(priv, status);
> > -             if (err)
> > +             if (err) {
> > +                     enum gve_adminq_opcodes opcode =
> > +                             be32_to_cpu(READ_ONCE(cmd->opcode));
> > +                     priv->adminq_cmd_fail++;
> > +                     if (!gve_adminq_is_retryable(opcode) || err != -EAGAIN)

Sashiko says:
---
This isn't a bug, but should the variable err be declared as an int?
The variable err is declared at the beginning of this loop as a u32, but it
receives the return value of gve_adminq_parse_err() which returns signed int
error codes such as -EAGAIN.

When checking err != -EAGAIN here, the code compares an unsigned 32-bit
integer to a negative constant. While this may evaluate correctly due to two's
complement wrap-around, it can trigger compiler warnings for signed and
unsigned integer comparisons.
---

Yes, it should be an int, will fix in v7.

> > +                             dev_err_ratelimited(&priv->pdev->dev,
> > +                                                 "AQ command %d failed with status %d\n",
> > +                                                 opcode, status);
> > +
> >                       // Return the first error if we failed.
> >                       return err;
> > +             }
> >       }
> >
> >       return 0;
>