From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A56394071C8 for ; Sun, 7 Jun 2026 22:46:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=209.85.208.172 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780872362; cv=pass; b=fwU0FIiC1NanB6bm4zP/Jb7KcURT944sJ8KHby4XLGleMFkkEtNpkLVBY/OOO+7VbVSaGW26Oc5WCeTs/YW3mGyyGLMKoKlbr4NLzq/1VKyyv4NhC7ru8sZHJGK7jjwoRX6H/4lwiz0OH9LhEWE0lxxtH6f/xxdOqwlIKBosyrI= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780872362; c=relaxed/simple; bh=Od3p1nZs4kL3mf+3N8X53ZY1nJanxX/3Ne+1K2ANvDs=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=bXtPmuQDxbWGrnLZZOehkL+PKJnijv86sw5bhgoJB1b8Y2VcdAK6vu2HgrnKrP+NRoQ92qdR9NrBj/UQ68nJIBndoggQrA4Ljf3ghZiRsRo/gKlSiHD4/z0rhTTpCqAlBB6SGITQ5E5DrGRPTnaLWDsMRs/gWLJ+dMQHLoF5gDI= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=onOFzriG; arc=pass smtp.client-ip=209.85.208.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="onOFzriG" Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-397e391cb2aso1688791fa.2 for ; Sun, 07 Jun 2026 15:46:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1780872359; cv=none; d=google.com; s=arc-20240605; b=SwMMZRGn5Q0uEBxRupCre5Wgiy2Hd9HmyMHxpDzBXSGgf9ZtSN1FNlFk3XlscPE4Mb uQBQC9nyz+q0EJ8JBvLGsSRcKMjfMQ/2VtGzgRPMG+tdbcE9j4h0yWI7iGRlAkBeT8ME M2R9O1S+Uoi78GtuySg8mU0/HvahPI68xULxD6OKVbvoCY16VYaC+12pksvQXIgj/hWB ZRnBSaVVsomeAoL/eNAXqHm8cetJXSDFfve6BobA6ICUY+NCM4q7W6tco7xeiaAyE+9h uUMZfUFgGSlW7IUvKZKM4OVQh2n0O2ye2dGwEQxcNnpQQ539IWon5ZbXy348NDUT2YJP JSbA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=mPhv20V/FZQuAxtv7EsKC5FmW4XMVDs9irxFJuAaIpw=; fh=xktiilF8HCvdNff3UXJP7DVlCLVKuRvJquGIdhjx1FI=; b=KfljzacDpdJ8vk14bNfDrl45pQUKpzGwd0GVL03e/8zS6W3JGebpYWB3MKuoG9n+aw uI8y8Z7jo37lDRmQeJ1KW+yp4aI/Gt75eReKARdxnWZt+C8EnLvm2oICHqXx22vif3Kb oDDvvoCjiDqJWAJx/f3SxjEE02V/4GYPW1cFp+miVn3j3+QlZrbl59cHilk/6FXhim/p /p1ty2LJrTP3EXfpZ5iAlRQzdHO4Odi5khE3iv4lU8eW7mQ7qAtW3+NK1UVz9Cp8NUL3 woMgARq6eyE8yWzHuAaDygx5EiMt53j0ocHQ61bVSiAd7ldMTc9PP+xfsjVP2m6oKKY1 qe4g==; darn=vger.kernel.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1780872359; x=1781477159; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=mPhv20V/FZQuAxtv7EsKC5FmW4XMVDs9irxFJuAaIpw=; b=onOFzriGuBqRkBSRoOn3IE6HC+khQcud2kKwKq/EwdBl5NH3WhH7XrfMwGs5dNjPwu unlWpCSCoPQvDo24dCisnOK15CWeoybebshz1RVEh2LhvZgaV7iWCnalneMoYzsJv9i7 V+z0WxpGLqv3OvQxnidVJ8xP/3yLWIbvkIuUq//cmlStY8r6Amf7b2ZwDu4M/BbUXHf/ wlvG1uhCHlXDvvvS5dmYyz1rFfL/KTfZDaFF2r5PKtLStiA37US636PcjT6djrDYdFrR HBIno/ND4BNsG7Wo7bs7XvwqxelViJ63ArldCUYtB/WrEcIE/wJgZw7fIgdKxzUaWV1u gDzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780872359; x=1781477159; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=mPhv20V/FZQuAxtv7EsKC5FmW4XMVDs9irxFJuAaIpw=; b=P3lATLH7RypQFhZLKi6dZf/NV838WzzqAm1KOCoWzF9Sli1LXqKB/yC6IJ7rKNjRSo mauOLbSg2Ijmdbi8gfG+QBbSAMjEUhhAqRkKcn8EWfzLYaCV/DJnNEUT3kA5d7KcwFvQ NH3dGmnTk+W3MBubtD+xXrji0hs2QmLneQmHkEssAGOz8VtYxE5t7YHuKJ04HvnDWjQu H0DAjnts7VPKVqd6+7QzPbDRt6tbt3gPSOhDuTQRX+q7QUGZADRN/7g1g9zWGxYDrVI1 CfgX9LQ3Lox65VbTzJHH9cP/mn11mp/u+KoBAc7S7do4/RIDl4IFFgWz+7TDZ2rqdEgp Y6Kg== X-Forwarded-Encrypted: i=1; AFNElJ+wSFfY8QsUQ+f79mOhN1F0Fw/7Ngcd4ZskcjkBHCdMvpnlRjiecvI9CRenqHhn8fxtrHkIylcL9GGZ63g=@vger.kernel.org X-Gm-Message-State: AOJu0YzeeUpYXAzzJhxlfQhWpwlG3LLnsC3txfQvVEsoGLIVoUBLdafJ KFq6Jv5EwGfzxidwHxso9LotcvZdWhKu+ygscEODn/v3ptv2vS80pxKvXeFj+J+rOde6MmM6f2e S5J0jD3ER5ud9EcJ0qdt7yhJYB0beHVWOBxx3g30D X-Gm-Gg: Acq92OFyhuvMzQ0xsO4ISz0uLvBsxt+lY2pqVEMKcphrsrFid6OsGL3QOpcCLRGceh+ sObOaWadUMpEQfW3+g/7Wpcrzr1D4myjI1noKXkFyFyrBPze1dv+WIO6WO84qr2CtG0qJnPkQ3W LeX9mu44jIztddatVDAY/jsy0pgJKAziWbAiIM/C+F95ciRqSt3ZKqnpcCC+3OOkGkXbrlKQp3A zHPJ+xn+9ZPuSIi/ZllDV/4btqrGVFHztchdqxwiSSZE4JMBsYeJdbYZBhJbNpaWOZ96Y5vlvRc LAZZI8dkSKcahfDErqQHPjaYo3hr3tNx8+HlZH9V0Mx19jvWOzanEnMZztAkiQ== X-Received: by 2002:a05:6512:124c:b0:5aa:8822:c947 with SMTP id 2adb3069b0e04-5aa8822c990mr3551512e87.49.1780872358521; Sun, 07 Jun 2026 15:45:58 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20260602235900.3050782-1-hramamurthy@google.com> <20260602235900.3050782-16-hramamurthy@google.com> In-Reply-To: <20260602235900.3050782-16-hramamurthy@google.com> From: Joshua Washington Date: Sun, 7 Jun 2026 15:45:45 -0700 X-Gm-Features: AVVi8CfPT-3xuSXsUVnCEjKf1TGsQF-RmVKjI02NtfRoQCkSd3d7nafaupH8rR0 Message-ID: Subject: Re: [PATCH net-next v2 15/15] gve: add link status/speed ctrl ops To: Harshitha Ramamurthy Cc: netdev@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org, john.fastabend@gmail.com, sdf@fomichev.me, willemb@google.com, jordanrhee@google.com, jfraker@google.com, nktgrg@google.com, bpf@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Jun 2, 2026 at 4:59=E2=80=AFPM Harshitha Ramamurthy wrote: > > From: Joshua Washington > > Refactor link status check to use a control plane op. Introduce > new op for retrieving the link status in AQ mode. This op reads > the link status from the device status register and stores the > value in priv. > > Also add an op for retrieving link speed in AQ mode which calls into > gve_adminq_report_link_speed. > > Reviewed-by: Willem de Bruijn > Reviewed-by: Jordan Rhee > Signed-off-by: Joshua Washington > Signed-off-by: Harshitha Ramamurthy > --- > drivers/net/ethernet/google/gve/gve.h | 5 ++++ > drivers/net/ethernet/google/gve/gve_adminq.c | 9 ++++++++ > drivers/net/ethernet/google/gve/gve_adminq.h | 1 + > drivers/net/ethernet/google/gve/gve_ethtool.c | 3 +-- > drivers/net/ethernet/google/gve/gve_main.c | 23 +++++++++++-------- > 5 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet= /google/gve/gve.h > index 4283cb9b49de..c8ebbbb93942 100644 > --- a/drivers/net/ethernet/google/gve/gve.h > +++ b/drivers/net/ethernet/google/gve/gve.h > @@ -843,6 +843,8 @@ struct gve_device_info { > * @reset_flow_rules: Flush all flow rules from device > * @create_queues: Sends commands to the device to create TX/RX queues. > * @destroy_queues: Sends commands to the device to destroy TX/RX queues= . > + * @report_link_status: Set link status into @priv->link_up > + * @report_link_speed: Set link status into @priv->link_speed Sashiko says: Should this read "Set link speed into @priv->link_speed" instead? ... Will fix in v3. > */ > struct gve_ctrl_ops { > int (*map_db_bar)(struct gve_priv *priv); > @@ -863,6 +865,8 @@ struct gve_ctrl_ops { > int (*reset_flow_rules)(struct gve_priv *priv); > int (*create_queues)(struct gve_priv *priv); > int (*destroy_queues)(struct gve_priv *priv); > + int (*report_link_status)(struct gve_priv *priv); > + int (*report_link_speed)(struct gve_priv *priv); > }; > > struct gve_priv { > @@ -964,6 +968,7 @@ struct gve_priv { > > /* Gvnic device link speed from hypervisor. */ > u64 link_speed; > + bool link_up; > bool up_before_suspend; /* True if dev was up before suspend */ > > bool mgmt_irq_requested; > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/e= thernet/google/gve/gve_adminq.c > index 799381d24c71..b33633555b71 100644 > --- a/drivers/net/ethernet/google/gve/gve_adminq.c > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c > @@ -1872,3 +1872,12 @@ int gve_adminq_create_queues(struct gve_priv *priv= ) > > return err; > } > + > +int gve_adminq_report_link_status(struct gve_priv *priv) > +{ > + u32 status; > + > + status =3D ioread32be(&priv->reg_bar0->device_status); > + priv->link_up =3D !!(GVE_DEVICE_STATUS_LINK_STATUS_MASK & status)= ; > + return 0; > +} > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/e= thernet/google/gve/gve_adminq.h > index 03e8318f7327..82413369dec9 100644 > --- a/drivers/net/ethernet/google/gve/gve_adminq.h > +++ b/drivers/net/ethernet/google/gve/gve_adminq.h > @@ -643,6 +643,7 @@ int gve_adminq_report_stats(struct gve_priv *priv, u6= 4 stats_report_len, > dma_addr_t stats_report_addr, u64 interval); > int gve_adminq_verify_driver_compatibility(struct gve_priv *priv); > int gve_adminq_get_device_properties(struct gve_priv *priv); > +int gve_adminq_report_link_status(struct gve_priv *priv); > int gve_adminq_report_link_speed(struct gve_priv *priv); > int gve_adminq_add_flow_rule(struct gve_priv *priv, struct gve_adminq_fl= ow_rule *rule, u32 loc); > int gve_adminq_del_flow_rule(struct gve_priv *priv, u32 loc); > diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/= ethernet/google/gve/gve_ethtool.c > index 54b623b678ce..aaf5b8b1671b 100644 > --- a/drivers/net/ethernet/google/gve/gve_ethtool.c > +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c > @@ -749,12 +749,11 @@ static int gve_get_link_ksettings(struct net_device= *netdev, > int err =3D 0; > > if (priv->link_speed =3D=3D 0) > - err =3D gve_adminq_report_link_speed(priv); > + err =3D priv->ctrl_ops->report_link_speed(priv); Sashiko says: --- This is a pre-existing issue, but I have a question about the timeout handl= ing inside gve_adminq_report_link_speed(). If gve_adminq_execute_cmd() returns a timeout error, the function drops the error and immediately frees the DMA memory via dma_free_coherent(). However= , since a host CPU timeout does not guarantee the NIC has aborted the DMA operation, could the hardware eventually execute a DMA write to the provide= d bus address after the memory has been reallocated, causing memory corruptio= n? This is another pre-existing issue, but could this ethtool call trigger a use-after-free if a previous device reset failed? If a device reset fails, gve_adminq_free() destroys the adminq_pool and fre= es the DMA memory backing priv->adminq. If user-space subsequently issues an ethtool command like gve_get_link_ksettings, the driver invokes report_link_speed() and eventually gve_adminq_issue_cmd(). If the device's tail counter was reset to 0, and the host's head counter is still 0, could gve_adminq_issue_cmd() write the command payload into the now-dangling priv->adminq buffer? --- I think that this is a larger scale issue that affects all admin queue commands that could time out. I think a solution here would be: 1) submit a net fix to allocate from dma pool in gve_amdinq_report_link_spe= ed 2) follow up with a patch that invokes a reset on a timed out AQ command. As far as any cross-reset failures go, that would require a much larger behavioral change in the driver, as the driver assumes that if the actual hardware reset fails, it is an =E2=80=9Cunrecoverable platform error=E2=80=9D and makes no promises about future driver behavior. > > cmd->base.speed =3D priv->link_speed; > > cmd->base.duplex =3D DUPLEX_FULL; > - > return err; > } > > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/eth= ernet/google/gve/gve_main.c > index dfc734d8bec9..181ec6919051 100644 > --- a/drivers/net/ethernet/google/gve/gve_main.c > +++ b/drivers/net/ethernet/google/gve/gve_main.c > @@ -1475,15 +1475,15 @@ static int gve_close(struct net_device *dev) > return err; > } > > -static void gve_handle_link_status(struct gve_priv *priv, bool link_stat= us) > +static void gve_handle_link_status(struct gve_priv *priv) > { > if (!gve_get_napi_enabled(priv)) > return; > > - if (link_status =3D=3D netif_carrier_ok(priv->dev)) > + if (priv->link_up =3D=3D netif_carrier_ok(priv->dev)) > return; > > - if (link_status) { > + if (priv->link_up) { Sashiko says: Is there a time-of-check to time-of-use race here? Because priv->link_up is read twice in quick succession without protection, and could be modified concurrently by gve_adminq_report_link_status() runni= ng in the lockless service_task workqueue, can the value change between the tw= o checks? Would using READ_ONCE() and saving the result to a local variable prevent unintended compiler optimizations and state changes? Will fix in v3. > netdev_info(priv->dev, "Device link is up.\n"); > netif_carrier_on(priv->dev); > } else { > @@ -1510,7 +1510,6 @@ static int gve_set_xdp(struct gve_priv *priv, struc= t bpf_prog *prog, > { > struct bpf_prog *old_prog; > int err =3D 0; > - u32 status; > > old_prog =3D READ_ONCE(priv->xdp_prog); > if (!netif_running(priv->dev)) { > @@ -1537,8 +1536,8 @@ static int gve_set_xdp(struct gve_priv *priv, struc= t bpf_prog *prog, > bpf_prog_put(old_prog); > > out: > - status =3D ioread32be(&priv->reg_bar0->device_status); > - gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK &= status); > + priv->ctrl_ops->report_link_status(priv); > + gve_handle_link_status(priv); > return err; > } > > @@ -1971,11 +1970,11 @@ static void gve_turnup(struct gve_priv *priv) > > static void gve_turnup_and_check_status(struct gve_priv *priv) > { > - u32 status; > + const struct gve_ctrl_ops *ops =3D priv->ctrl_ops; > > gve_turnup(priv); > - status =3D ioread32be(&priv->reg_bar0->device_status); > - gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK &= status); > + ops->report_link_status(priv); > + gve_handle_link_status(priv); > } > > static struct gve_notify_block *gve_get_tx_notify_block(struct gve_priv = *priv, > @@ -2299,12 +2298,14 @@ static void gve_service_task(struct work_struct *= work) > { > struct gve_priv *priv =3D container_of(work, struct gve_priv, > service_task); > + const struct gve_ctrl_ops *ops =3D priv->ctrl_ops; > u32 status =3D ioread32be(&priv->reg_bar0->device_status); > > gve_handle_status(priv, status); > > gve_handle_reset(priv); > - gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK &= status); > + ops->report_link_status(priv); > + gve_handle_link_status(priv); > } > > static void gve_set_netdev_xdp_features(struct gve_priv *priv) > @@ -2427,6 +2428,8 @@ static const struct gve_ctrl_ops gve_adminq_ops =3D= { > .teardown_mgmt_irq =3D gve_adminq_teardown_mgmt_irq, > .create_queues =3D gve_adminq_create_queues, > .destroy_queues =3D gve_adminq_destroy_queues, > + .report_link_status =3D gve_adminq_report_link_status, > + .report_link_speed =3D gve_adminq_report_link_speed, > }; > > static int gve_init_priv(struct gve_priv *priv) > -- > 2.54.0.1013.g208068f2d8-goog > --=20 Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-442= 3