From: Pin-yen Lin <redacted>
When modifying device features via ethtool, GVE causes a short link-down
after the ethtool command completes, which can break scripts that use
the network interface right after the ethtool command.
This issue was discovered and actually triggered by the
drivers/net/ping.py selftest. Modifying checksum offload features via
ethtool caused a transient carrier drop ("Lost carrier", DHCP lease lost),
causing the immediate ping invocation to fail.
Fix it by calling gve_turnup_and_check_status() in gve_queues_start()
instead of deferring the link status update to the service task, so the
carrier status is updated synchronously before returning to userspace.
Take netdev_lock() around the link status update in gve_service_task(),
so that it is serialized with the synchronous update. All other callers
of gve_handle_link_status() already hold the instance lock.
This change was tested using the DQO RDA queue format.
Cc: <redacted>
Fixes: 3b7cc73628ff ("gve: Use link status register to report link status")
Signed-off-by: Pin-yen Lin <redacted>
Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
Changes in v4:
- Take netdev_lock() around the link status update in
gve_service_task() to serialize it with gve_queues_start() (Sashiko)
Changes in v3:
- Retitled from "gve: Make ethtool config changes synchronous"
- Fix the Fixes tag
- Refined the commit message
drivers/net/ethernet/google/gve/gve_main.c | 70 ++++++++++++----------
1 file changed, 39 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index b5e2f39a903c..bc7d7c41948c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -17,6 +17,7 @@
#include <linux/sched.h>
#include <linux/timer.h>
#include <linux/workqueue.h>
+#include <net/netdev_lock.h>
#include <net/netdev_queues.h>
#include <net/sch_generic.h>
#include <net/xdp_sock_drv.h>
@@ -1431,6 +1432,37 @@ static void gve_queues_stop(struct gve_priv *priv)
gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
}
+static void gve_handle_link_status(struct gve_priv *priv)
+{
+ bool link_up;
+
+ netdev_assert_locked(priv->dev);
+
+ if (!gve_get_napi_enabled(priv))
+ return;
+
+ link_up = READ_ONCE(priv->link_up);
+ if (link_up == netif_carrier_ok(priv->dev))
+ return;
+
+ if (link_up) {
+ netdev_info(priv->dev, "Device link is up.\n");
+ netif_carrier_on(priv->dev);
+ } else {
+ netdev_info(priv->dev, "Device link is down.\n");
+ netif_carrier_off(priv->dev);
+ }
+}
+
+static void gve_turnup_and_check_status(struct gve_priv *priv)
+{
+ const struct gve_ctrl_ops *ops = priv->ctrl_ops;
+
+ gve_turnup(priv);
+ ops->report_link_status(priv);
+ gve_handle_link_status(priv);
+}
+
/* The passed-in queue memory is stored into priv and the queues are made live.
* No memory is allocated. Passed-in memory is freed on errors.
*/@@ -1494,8 +1526,7 @@ static int gve_queues_start(struct gve_priv *priv,
round_jiffies(jiffies +
msecs_to_jiffies(priv->stats_report_timer_period)));
- gve_turnup(priv);
- queue_work(priv->gve_wq, &priv->service_task);
+ gve_turnup_and_check_status(priv);
priv->interface_up_cnt++;
return 0;
@@ -1570,26 +1601,6 @@ static int gve_close(struct net_device *dev)
return err;
}
-static void gve_handle_link_status(struct gve_priv *priv)
-{
- bool link_up;
-
- if (!gve_get_napi_enabled(priv))
- return;
-
- link_up = READ_ONCE(priv->link_up);
- if (link_up == netif_carrier_ok(priv->dev))
- return;
-
- if (link_up) {
- netdev_info(priv->dev, "Device link is up.\n");
- netif_carrier_on(priv->dev);
- } else {
- netdev_info(priv->dev, "Device link is down.\n");
- netif_carrier_off(priv->dev);
- }
-}
-
static int gve_configure_rings_xdp(struct gve_priv *priv,
u16 num_xdp_rings)
{@@ -2064,15 +2075,6 @@ static void gve_turnup(struct gve_priv *priv)
gve_set_napi_enabled(priv);
}
-static void gve_turnup_and_check_status(struct gve_priv *priv)
-{
- const struct gve_ctrl_ops *ops = priv->ctrl_ops;
-
- gve_turnup(priv);
- ops->report_link_status(priv);
- gve_handle_link_status(priv);
-}
-
static struct gve_notify_block *gve_get_tx_notify_block(struct gve_priv *priv,
unsigned int txqueue)
{@@ -2406,8 +2408,14 @@ static void gve_service_task(struct work_struct *work)
gve_handle_status(priv, status);
gve_handle_reset(priv);
+
+ /* Serialize with the synchronous link status updates, which all run
+ * under the netdev instance lock.
+ */
+ netdev_lock(priv->dev);
ops->report_link_status(priv);
gve_handle_link_status(priv);
+ netdev_unlock(priv->dev);
}
static void gve_set_netdev_xdp_features(struct gve_priv *priv)
--
2.56.0.313.g2479322e68-goog