[PATCH net-next v6 00/12] gve: AdminQ mode related refactors

[PATCH net-next v6 00/12] gve: AdminQ mode related refactors

[Harshitha Ramamurthy]

This series is preparing the driver for the addition of a new control
plane mode(MailboxQ) by commonizing and reusing as much code as possible.
MailboxQ is a new control plane infrastructure which uses mailbox queues.
This is a new control plane to communicate with the device when the driver
is running on bare-metal instances as well as newer versions of the device.

This series contains 2 main changes to the gve driver's existing control
plane(AdminQ):
1) refactor some initialization/teardown path methods to make it work for
   both control planes.
2) add gve_ctrl_ops structure to the driver for various control plane
   operations

The refactors are mainly around the init, reset and recover paths so that
functionalities could be moved into the control ops like 'set_num_queues',
'set_num_ntfy_blks' etc.

This patch series only sets the control ops for AdminQ mode. This is
intended as a preparation for adding MailboxQ mode in upcoming series
which will add the new control plane infrastructure and then introduce
the corresponding control ops for MailboxQ mode.

The entire list of future patches can be found here for reference:
https://github.com/hramamurthy12/linux/commits/gve-mailbox-queues/

Changes in v6:
- update commit message to be more consistent with code (patch 1,9 Sashiko)
- call out MTU validation change (patch 1, Sashiko)
- A few other Sashiko related fixes (error propagation in patch 2, drop double
  logging in patch 5, drop unintentional type conversion in patch 11)
- v5: https://lore.kernel.org/netdev/20260903215606.31633-1-hramamurthy@google.com/

Changes in v5:
- This series contains 12 patches since 3 patches from v4 were applied
- Picked up Przemek Kitszel's Reviewed-by tag
- Honor device provided default no. of Qs and max rx buffer size correctly (patch 1, Sashiko)
- drop unnecessary header files and __maybe_unused tag (patch 4, Sashiko)
- fix workqueue disable count, destroy rings before stopping Qs, stop queues in error path
  in gve_close, preserve ordering of gve_turndown->gve_destroy_rings->gve_queues_stop
  during close and reset (patch 6, Sashiko)
- drop double logging in all paths for queue creation/destruction (patch 11, Sashiko)
- v4: https://lore.kernel.org/netdev/20260814021406.3044324-1-hramamurthy@google.com/ 

Changes in v4:
- Reduce code churn by placing new functions directly in gve_adminq.c, introducing final tab spacing
  and using local variable (Przemek Kitszel)
- Fix kdoc formatting and drop spurious management interrupt(patch 9)
- Propagate release_db_resources naming change (patch 12)
- Remove double logging in destroy queues path (patch 14)
- v3: https://lore.kernel.org/netdev/20260803184630.3813311-1-hramamurthy@google.com/

Changes in v3:
- Fix driver compatibility verification in the reset and recovery paths (patches 2, 8).
- Mark gve_adminq_get_device_properties() with __maybe_unused ahead of its usage (patch 7).
- Refine error handling and resource cleanup in reset, probe, and queue creation paths (patches 8, 9, 11, 14).
- Keep IRQ doorbell allocation/release in control plane resource helpers (patch 12).
- Minor fixes: fix TOCTOU issue for link speed, correct kdoc comments, and remove redundant teardown calls (patches 4, 11, 15). 
- v2: https://lore.kernel.org/netdev/20260602235900.3050782-1-hramamurthy@google.com/

Changes in v2:
- Rebased against latest net-next to resolve apply failure for patch 5
- Fixed a few typos in commit messages of patch 9 and 14
- Fixed a kdoc warning in patch 9
- v1: https://lore.kernel.org/netdev/20260601175437.3767283-1-hramamurthy@google.com/

Harshitha Ramamurthy (6):
  gve: add struct gve_device_info to hold device properties
  gve: introduce control plane operations structure
  gve: introduce ctrl ops to set vectors and Qs
  gve: introduce gve_adminq_get_device_properties()
  gve: refactor gve_init_priv for reset path
  gve: add ctrl ops for queue operations

Joshua Washington (6):
  gve: simplify reset logic
  gve: add gve_ctrl_ops for gve initialization/teardown sequences
  gve: split up notify block allocation and setup paths
  gve: introduce new methods to handle IRQ doorbells
  gve: setup and teardown management interrupts
  gve: add link status/speed ctrl ops

 drivers/net/ethernet/google/gve/gve.h         |  87 +-
 drivers/net/ethernet/google/gve/gve_adminq.c  | 380 ++++++--
 drivers/net/ethernet/google/gve/gve_adminq.h  |  30 +-
 drivers/net/ethernet/google/gve/gve_ethtool.c |   5 +-
 .../net/ethernet/google/gve/gve_flow_rule.c   |  15 +-
 drivers/net/ethernet/google/gve/gve_main.c    | 881 +++++++++---------
 6 files changed, 870 insertions(+), 528 deletions(-)

-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 01/12] gve: add struct gve_device_info to hold device properties

[Harshitha Ramamurthy]

In the current AdminQ mode, device properties are written into
struct gve_device_descriptor that is allocated in shared memory
between the driver and device. In the upcoming MailboxQ mode,
these properties will be returned in the response of a mailbox
message. Hence, add struct gve_device_info as the structure that
holds all the properties that are negotiated with the device in
either mode.

Change the AdminQ mode method gve_adminq_describe_device()
and its children to fill up device information into this newly
introduced struct gve_device_info. Move a few helper functions
and code that set device properties in the priv structure into
gve_init_priv(). So now gve_init_priv() calls/does the following:

- gve_set_mtu()
- gve_set_mac()
- gve_set_queue_properties()
- gve_set_buf_sizes()
- set flow steering and RSS properties
- set other priv properties

When MailboxQ support is added, device information will be filled
into the same structure and the same gve_init_priv() path would be
used to set device properties to ensure common code reusage.

Most of these changes are refactors only, except for one:
with the introduction of the central struct gve_device_info,
in AdminQ mode, gve_set_mtu() now also validates the device
provided mtu value from jumbo frames device option. This sets
up the driver nicely for the MailboxQ mode where both the
default and the maximum mtu are provided at once and they
are validated in gve_set_mtu().

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v6:
- update commit message to call out change in devive provided MTU
  validation (Sashiko)

v5:
- ensure using default_tx/rx_queues (Sashiko)
- honor device provided rx buffer size correctly (Sashiko)

v4:
- reuse mtu variable

v3:
- Read default_min_ring_size from device info instead of priv
 drivers/net/ethernet/google/gve/gve.h        |  29 +++++
 drivers/net/ethernet/google/gve/gve_adminq.c | 128 +++++++++++--------
 drivers/net/ethernet/google/gve/gve_adminq.h |   6 -
 drivers/net/ethernet/google/gve/gve_main.c   |  86 +++++++++----
 4 files changed, 169 insertions(+), 80 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index c280ff35ee77..021adb9108df 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -797,6 +797,34 @@ struct gve_ptp {
 	struct gve_priv *priv;
 };
 
+struct gve_device_info {
+	enum gve_queue_format queue_format;
+	u16 default_tx_queues;
+	u16 default_rx_queues;
+	u16 max_tx_queues;
+	u16 max_rx_queues;
+	u16 default_tx_ring_size;
+	u16 default_rx_ring_size;
+	u16 max_tx_ring_size;
+	u16 max_rx_ring_size;
+	u16 min_tx_ring_size;
+	u16 min_rx_ring_size;
+	u16 max_mtu;
+	u8 mac[ETH_ALEN];
+	u16 max_rx_buffer_size;
+	u16 header_buf_size;
+	u32 max_flow_rules;
+	u16 rss_key_size;
+	u16 rss_lut_size;
+	u16 tx_pages_per_qpl;
+	u16 num_event_counters;
+	u64 max_registered_pages;
+	bool default_min_ring_size;
+	bool nic_timestamp_supported;
+	bool modify_ring_size_enabled;
+	bool cache_rss_config;
+};
+
 struct gve_priv {
 	struct net_device *dev;
 	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
@@ -929,6 +957,7 @@ struct gve_priv {
 	struct gve_nic_ts_report *nic_ts_report;
 	dma_addr_t nic_ts_report_bus;
 	u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
+	struct gve_device_info device_info;
 };
 
 enum gve_service_task_flags_bit {
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index f05f4895f4c7..512349c5517f 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -70,7 +70,7 @@ void gve_parse_device_option(struct gve_priv *priv,
 
 		dev_info(&priv->pdev->dev,
 			 "Gqi raw addressing device option enabled.\n");
-		priv->queue_format = GVE_GQI_RDA_FORMAT;
+		priv->device_info.queue_format = GVE_GQI_RDA_FORMAT;
 		break;
 	case GVE_DEV_OPT_ID_GQI_RDA:
 		if (option_length < sizeof(**dev_op_gqi_rda) ||
@@ -190,7 +190,7 @@ void gve_parse_device_option(struct gve_priv *priv,
 
 		/* device has not provided min ring size */
 		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
-			priv->default_min_ring_size = true;
+			priv->device_info.default_min_ring_size = true;
 		break;
 	case GVE_DEV_OPT_ID_FLOW_STEERING:
 		if (option_length < sizeof(**dev_op_flow_steering) ||
@@ -922,10 +922,13 @@ int gve_adminq_destroy_rx_queues(struct gve_priv *priv, u32 num_queues)
 
 static void gve_set_default_rss_sizes(struct gve_priv *priv)
 {
-	if (!gve_is_gqi(priv)) {
-		priv->rss_key_size = GVE_RSS_KEY_SIZE;
-		priv->rss_lut_size = GVE_RSS_INDIR_SIZE;
-		priv->cache_rss_config = true;
+	struct gve_device_info *device_info = &priv->device_info;
+
+	if (device_info->queue_format == GVE_DQO_RDA_FORMAT ||
+	    device_info->queue_format == GVE_DQO_QPL_FORMAT) {
+		device_info->rss_key_size = GVE_RSS_KEY_SIZE;
+		device_info->rss_lut_size = GVE_RSS_INDIR_SIZE;
+		device_info->cache_rss_config = true;
 	}
 }
 
@@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,
 					  const struct gve_device_option_modify_ring
 					  *dev_op_modify_ring)
 {
+	struct gve_device_info *info = &priv->device_info;
+
 	/* Before control reaches this point, the page-size-capped max MTU from
 	 * the gve_device_descriptor field has already been stored in
-	 * priv->dev->max_mtu. We overwrite it with the true max MTU below.
+	 * device_info->max_mtu. We overwrite it with the true max MTU below.
 	 */
 	if (dev_op_jumbo_frames &&
 	    (supported_features_mask & GVE_SUP_JUMBO_FRAMES_MASK)) {
 		dev_info(&priv->pdev->dev,
 			 "JUMBO FRAMES device option enabled.\n");
-		priv->dev->max_mtu = be16_to_cpu(dev_op_jumbo_frames->max_mtu);
+		info->max_mtu = be16_to_cpu(dev_op_jumbo_frames->max_mtu);
 	}
 
 	if (dev_op_buffer_sizes &&
 	    (supported_features_mask & GVE_SUP_BUFFER_SIZES_MASK)) {
-		priv->max_rx_buffer_size =
+		info->max_rx_buffer_size =
 			be16_to_cpu(dev_op_buffer_sizes->packet_buffer_size);
-		priv->header_buf_size =
+		info->header_buf_size =
 			be16_to_cpu(dev_op_buffer_sizes->header_buffer_size);
 		dev_info(&priv->pdev->dev,
 			 "BUFFER SIZES device option enabled with max_rx_buffer_size of %u, header_buf_size of %u.\n",
-			 priv->max_rx_buffer_size, priv->header_buf_size);
-		if (gve_is_dqo(priv) &&
-		    priv->max_rx_buffer_size > GVE_DEFAULT_RX_BUFFER_SIZE)
-			priv->rx_cfg.packet_buffer_size =
-				priv->max_rx_buffer_size;
+			 info->max_rx_buffer_size, info->header_buf_size);
 	}
 
 	/* Read and store ring size ranges given by device */
 	if (dev_op_modify_ring &&
 	    (supported_features_mask & GVE_SUP_MODIFY_RING_MASK)) {
-		priv->modify_ring_size_enabled = true;
-		priv->max_rx_desc_cnt =
+		info->modify_ring_size_enabled = true;
+		info->max_rx_ring_size =
 			be16_to_cpu(dev_op_modify_ring->max_rx_ring_size);
-		priv->max_tx_desc_cnt =
+		info->max_tx_ring_size =
 			be16_to_cpu(dev_op_modify_ring->max_tx_ring_size);
-		if (priv->default_min_ring_size) {
+		if (info->default_min_ring_size) {
 			/* If device hasn't provided minimums, use default minimums */
-			priv->min_tx_desc_cnt = GVE_DEFAULT_MIN_TX_RING_SIZE;
-			priv->min_rx_desc_cnt = GVE_DEFAULT_MIN_RX_RING_SIZE;
+			info->min_tx_ring_size = GVE_DEFAULT_MIN_TX_RING_SIZE;
+			info->min_rx_ring_size = GVE_DEFAULT_MIN_RX_RING_SIZE;
 		} else {
-			priv->min_rx_desc_cnt = be16_to_cpu(dev_op_modify_ring->min_rx_ring_size);
-			priv->min_tx_desc_cnt = be16_to_cpu(dev_op_modify_ring->min_tx_ring_size);
+			info->min_rx_ring_size =
+				be16_to_cpu(dev_op_modify_ring->min_rx_ring_size);
+			info->min_tx_ring_size =
+				be16_to_cpu(dev_op_modify_ring->min_tx_ring_size);
 		}
 	}
 
 	if (dev_op_flow_steering &&
 	    (supported_features_mask & GVE_SUP_FLOW_STEERING_MASK)) {
 		if (dev_op_flow_steering->max_flow_rules) {
-			priv->max_flow_rules =
+			info->max_flow_rules =
 				be32_to_cpu(dev_op_flow_steering->max_flow_rules);
-			priv->dev->hw_features |= NETIF_F_NTUPLE;
 			dev_info(&priv->pdev->dev,
 				 "FLOW STEERING device option enabled with max rule limit of %u.\n",
-				 priv->max_flow_rules);
+				 info->max_flow_rules);
 		}
 	}
 
 	if (dev_op_rss_config &&
 	    (supported_features_mask & GVE_SUP_RSS_CONFIG_MASK)) {
-		priv->rss_key_size =
+		info->rss_key_size =
 			be16_to_cpu(dev_op_rss_config->hash_key_size);
-		priv->rss_lut_size =
+		info->rss_lut_size =
 			be16_to_cpu(dev_op_rss_config->hash_lut_size);
-		priv->cache_rss_config = false;
+		info->cache_rss_config = false;
 		dev_dbg(&priv->pdev->dev,
 			"RSS device option enabled with key size of %u, lut size of %u.\n",
-			priv->rss_key_size, priv->rss_lut_size);
+			info->rss_key_size, info->rss_lut_size);
 	}
 
 	if (dev_op_nic_timestamp &&
 	    (supported_features_mask & GVE_SUP_NIC_TIMESTAMP_MASK))
-		priv->nic_timestamp_supported = true;
+		info->nic_timestamp_supported = true;
+}
+
+static void gve_fill_device_info(struct gve_priv *priv,
+				 struct gve_device_descriptor *descriptor)
+{
+	struct gve_device_info *device_info = &priv->device_info;
+	u16 default_num_queues;
+
+	device_info->tx_pages_per_qpl =
+				be16_to_cpu(descriptor->tx_pages_per_qpl);
+	device_info->max_registered_pages =
+				be64_to_cpu(descriptor->max_registered_pages);
+	device_info->num_event_counters = be16_to_cpu(descriptor->counters);
+	ether_addr_copy(device_info->mac, descriptor->mac);
+	device_info->max_mtu =  be16_to_cpu(descriptor->mtu);
+
+	default_num_queues = be16_to_cpu(descriptor->default_num_queues);
+	device_info->default_tx_queues = default_num_queues;
+	device_info->default_rx_queues = default_num_queues;
+	device_info->default_tx_ring_size =
+				be16_to_cpu(descriptor->tx_queue_entries);
+	device_info->default_rx_ring_size =
+				be16_to_cpu(descriptor->rx_queue_entries);
+
+	/* set default ranges */
+	device_info->max_tx_ring_size = device_info->default_tx_ring_size;
+	device_info->max_rx_ring_size = device_info->default_rx_ring_size;
+	device_info->min_tx_ring_size = device_info->default_tx_ring_size;
+	device_info->min_rx_ring_size = device_info->default_rx_ring_size;
 }
 
 int gve_adminq_describe_device(struct gve_priv *priv)
@@ -1027,6 +1058,7 @@ int gve_adminq_describe_device(struct gve_priv *priv)
 	struct gve_device_option_jumbo_frames *dev_op_jumbo_frames = NULL;
 	struct gve_device_option_modify_ring *dev_op_modify_ring = NULL;
 	struct gve_device_option_rss_config *dev_op_rss_config = NULL;
+	struct gve_device_info *device_info = &priv->device_info;
 	struct gve_device_option_gqi_rda *dev_op_gqi_rda = NULL;
 	struct gve_device_option_gqi_qpl *dev_op_gqi_qpl = NULL;
 	struct gve_device_option_dqo_rda *dev_op_dqo_rda = NULL;
@@ -1070,26 +1102,26 @@ int gve_adminq_describe_device(struct gve_priv *priv)
 	 * DqoRda, DqoQpl, GqiRda, GqiQpl. Use GqiQpl as default.
 	 */
 	if (dev_op_dqo_rda) {
-		priv->queue_format = GVE_DQO_RDA_FORMAT;
+		device_info->queue_format = GVE_DQO_RDA_FORMAT;
 		dev_info(&priv->pdev->dev,
 			 "Driver is running with DQO RDA queue format.\n");
 		supported_features_mask =
 			be32_to_cpu(dev_op_dqo_rda->supported_features_mask);
 	} else if (dev_op_dqo_qpl) {
-		priv->queue_format = GVE_DQO_QPL_FORMAT;
+		device_info->queue_format = GVE_DQO_QPL_FORMAT;
 		supported_features_mask =
 			be32_to_cpu(dev_op_dqo_qpl->supported_features_mask);
 	}  else if (dev_op_gqi_rda) {
-		priv->queue_format = GVE_GQI_RDA_FORMAT;
+		device_info->queue_format = GVE_GQI_RDA_FORMAT;
 		dev_info(&priv->pdev->dev,
 			 "Driver is running with GQI RDA queue format.\n");
 		supported_features_mask =
 			be32_to_cpu(dev_op_gqi_rda->supported_features_mask);
-	} else if (priv->queue_format == GVE_GQI_RDA_FORMAT) {
+	} else if (device_info->queue_format == GVE_GQI_RDA_FORMAT) {
 		dev_info(&priv->pdev->dev,
 			 "Driver is running with GQI RDA queue format.\n");
 	} else {
-		priv->queue_format = GVE_GQI_QPL_FORMAT;
+		device_info->queue_format = GVE_GQI_QPL_FORMAT;
 		if (dev_op_gqi_qpl)
 			supported_features_mask =
 				be32_to_cpu(dev_op_gqi_qpl->supported_features_mask);
@@ -1097,18 +1129,9 @@ int gve_adminq_describe_device(struct gve_priv *priv)
 			 "Driver is running with GQI QPL queue format.\n");
 	}
 
+	gve_fill_device_info(priv, descriptor);
 	gve_set_default_rss_sizes(priv);
 
-	err = gve_set_mtu(priv, descriptor);
-	if (err)
-		goto free_device_descriptor;
-
-	priv->num_event_counters = be16_to_cpu(descriptor->counters);
-
-	gve_set_mac(priv, descriptor);
-
-	gve_set_queue_properties(priv, descriptor);
-
 	gve_enable_supported_features(priv, supported_features_mask,
 				      dev_op_jumbo_frames, dev_op_dqo_qpl,
 				      dev_op_buffer_sizes, dev_op_flow_steering,
@@ -1595,6 +1618,8 @@ int gve_set_num_ntfy_blks(struct gve_priv *priv)
 
 void gve_set_num_queues(struct gve_priv *priv)
 {
+	struct gve_device_info *device_info = &priv->device_info;
+
 	priv->tx_cfg.max_queues =
 		min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
 	priv->rx_cfg.max_queues =
@@ -1602,10 +1627,13 @@ void gve_set_num_queues(struct gve_priv *priv)
 
 	priv->tx_cfg.num_queues = priv->tx_cfg.max_queues;
 	priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
-	if (priv->default_num_queues > 0) {
-		priv->tx_cfg.num_queues = min_t(int, priv->default_num_queues,
+	if (device_info->default_tx_queues > 0)
+		priv->tx_cfg.num_queues = min_t(int,
+						device_info->default_tx_queues,
 						priv->tx_cfg.num_queues);
-		priv->rx_cfg.num_queues = min_t(int, priv->default_num_queues,
+
+	if (device_info->default_rx_queues > 0)
+		priv->rx_cfg.num_queues = min_t(int,
+						device_info->default_rx_queues,
 						priv->rx_cfg.num_queues);
-	}
 }
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 68c63ce75505..a17af755b454 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -658,10 +658,4 @@ int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
 				 struct gve_ptype_lut *ptype_lut);
 int gve_set_num_ntfy_blks(struct gve_priv *priv);
 void gve_set_num_queues(struct gve_priv *priv);
-void gve_set_queue_properties(struct gve_priv *priv,
-			      struct gve_device_descriptor *descriptor);
-int gve_set_mtu(struct gve_priv *priv,
-		struct gve_device_descriptor *descriptor);
-void gve_set_mac(struct gve_priv *priv,
-		 struct gve_device_descriptor *descriptor);
 #endif /* _GVE_ADMINQ_H */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 9cc343a16271..d3882de584e3 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -2398,57 +2398,71 @@ static const struct xdp_metadata_ops gve_xdp_metadata_ops = {
 	.xmo_rx_timestamp	= gve_xdp_rx_timestamp,
 };
 
-static void gve_set_default_desc_cnt(struct gve_priv *priv,
-				     const struct gve_device_descriptor *descriptor)
+static void gve_set_desc_cnt(struct gve_priv *priv)
 {
-	priv->tx_desc_cnt = be16_to_cpu(descriptor->tx_queue_entries);
-	priv->rx_desc_cnt = be16_to_cpu(descriptor->rx_queue_entries);
+	struct gve_device_info *device_info = &priv->device_info;
 
-	/* set default ranges */
-	priv->max_tx_desc_cnt = priv->tx_desc_cnt;
-	priv->max_rx_desc_cnt = priv->rx_desc_cnt;
-	priv->min_tx_desc_cnt = priv->tx_desc_cnt;
-	priv->min_rx_desc_cnt = priv->rx_desc_cnt;
+	priv->tx_desc_cnt = device_info->default_tx_ring_size;
+	priv->rx_desc_cnt = device_info->default_rx_ring_size;
+	priv->max_tx_desc_cnt = device_info->max_tx_ring_size;
+	priv->max_rx_desc_cnt = device_info->max_rx_ring_size;
+	priv->min_tx_desc_cnt = device_info->min_tx_ring_size;
+	priv->min_rx_desc_cnt = device_info->min_rx_ring_size;
 }
 
-void gve_set_queue_properties(struct gve_priv *priv,
-			      struct gve_device_descriptor *descriptor)
+static void gve_set_queue_properties(struct gve_priv *priv)
 {
-	/* set default descriptor counts */
-	gve_set_default_desc_cnt(priv, descriptor);
+	struct gve_device_info *device_info = &priv->device_info;
 
-	priv->max_registered_pages = be64_to_cpu(descriptor->max_registered_pages);
-	priv->tx_pages_per_qpl = be16_to_cpu(descriptor->tx_pages_per_qpl);
-	priv->default_num_queues = be16_to_cpu(descriptor->default_num_queues);
+	gve_set_desc_cnt(priv);
+	priv->max_registered_pages = device_info->max_registered_pages;
+	priv->tx_pages_per_qpl = device_info->tx_pages_per_qpl;
 }
 
-int gve_set_mtu(struct gve_priv *priv,
-		struct gve_device_descriptor *descriptor)
+static int gve_set_mtu(struct gve_priv *priv)
 {
+	struct gve_device_info *device_info = &priv->device_info;
 	u16 mtu;
 
-	mtu = be16_to_cpu(descriptor->mtu);
+	mtu = device_info->max_mtu;
 	if (mtu < ETH_MIN_MTU) {
 		dev_err(&priv->pdev->dev, "MTU %d below minimum MTU\n", mtu);
 		return -EINVAL;
 	}
 	priv->dev->max_mtu = mtu;
+	priv->dev->mtu = mtu;
 
 	return 0;
 }
 
-void gve_set_mac(struct gve_priv *priv,
-		 struct gve_device_descriptor *descriptor)
+static void gve_set_mac(struct gve_priv *priv)
 {
+	struct gve_device_info *device_info = &priv->device_info;
 	u8 *mac;
 
-	mac = descriptor->mac;
+	mac = device_info->mac;
 	eth_hw_addr_set(priv->dev, mac);
 	dev_info(&priv->pdev->dev, "MAC addr: %pM\n", mac);
 }
 
+static void gve_set_buf_sizes(struct gve_priv *priv)
+{
+	struct gve_device_info *device_info = &priv->device_info;
+
+	if (device_info->max_rx_buffer_size)
+		priv->max_rx_buffer_size = device_info->max_rx_buffer_size;
+
+	if (gve_is_dqo(priv) &&
+	    priv->max_rx_buffer_size > GVE_DEFAULT_RX_BUFFER_SIZE)
+		priv->rx_cfg.packet_buffer_size = priv->max_rx_buffer_size;
+
+	if (device_info->header_buf_size)
+		priv->header_buf_size = device_info->header_buf_size;
+}
+
 static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 {
+	struct gve_device_info *device_info = &priv->device_info;
 	int err;
 
 	/* Set up the adminq */
@@ -2471,7 +2485,7 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 	if (skip_describe_device)
 		goto setup_device;
 
-	priv->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
+	device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
 	/* Get the initial information we need from the device */
 	err = gve_adminq_describe_device(priv);
 	if (err) {
@@ -2480,6 +2494,8 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 		goto err;
 	}
 
+	priv->queue_format = priv->device_info.queue_format;
+
 	err = gve_set_num_ntfy_blks(priv);
 	if (err) {
 		dev_err(&priv->pdev->dev,
@@ -2507,12 +2523,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 		netif_set_tso_max_size(priv->dev, GVE_DQO_TX_MAX);
 	}
 
-	priv->dev->mtu = priv->dev->max_mtu;
+	if (gve_set_mtu(priv)) {
+		err = -EINVAL;
+		goto err;
+	}
+
+	priv->num_event_counters = device_info->num_event_counters;
+
+	gve_set_mac(priv);
+
+	gve_set_queue_properties(priv);
+	priv->modify_ring_size_enabled = device_info->modify_ring_size_enabled;
+
+	gve_set_buf_sizes(priv);
+
+	priv->max_flow_rules = device_info->max_flow_rules;
+	if (priv->max_flow_rules)
+		priv->dev->hw_features |= NETIF_F_NTUPLE;
+
+	priv->rss_key_size = device_info->rss_key_size;
+	priv->rss_lut_size = device_info->rss_lut_size;
+	priv->cache_rss_config = device_info->cache_rss_config;
+
 	priv->numa_node = dev_to_node(&priv->pdev->dev);
 	priv->tx_cfg.num_xdp_queues = 0;
 	priv->rx_copybreak = GVE_DEFAULT_RX_COPYBREAK;
 	priv->ts_config.tx_type = HWTSTAMP_TX_OFF;
 	priv->ts_config.rx_filter = HWTSTAMP_FILTER_NONE;
+	priv->nic_timestamp_supported = device_info->nic_timestamp_supported;
 
 setup_device:
 	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 02/12] gve: introduce control plane operations structure

[Harshitha Ramamurthy]

To abstract out the difference in implementation of control
plane operations between the existing Adminq ABI and the upcoming
Mailbox ABI, introduce a new gve_ctrl_ops structure which will
contain the basic operations. At probe, these ops will be set based
on the ABI and the corresponding ops will be called in relevant
places.

As of this patch, only Adminq ops are set. In future patches,
corresponding ops will be set for the new mailbox mode.

Implement a ctrl op to map/unmap the doorbell bar. Since this
functionality has moved to a control op, call this op after control
ops are set for AdminQ mode.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v6:
- plumb through map_db_bar error instead of overwriting it

 drivers/net/ethernet/google/gve/gve.h        | 11 +++++++
 drivers/net/ethernet/google/gve/gve_adminq.c | 21 ++++++++++++
 drivers/net/ethernet/google/gve/gve_adminq.h |  2 ++
 drivers/net/ethernet/google/gve/gve_main.c   | 34 +++++++++++---------
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 021adb9108df..56148ea3cfbf 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -825,6 +825,16 @@ struct gve_device_info {
 	bool cache_rss_config;
 };
 
+/**
+ * struct gve_ctrl_ops - Control plane operations structure
+ * @map_db_bar: Maps the doorbell BAR for the device and store in @priv.
+ * @unmap_db_bar: Unmaps the doorbell BAR previously mapped by @map_db_bar.
+ */
+struct gve_ctrl_ops {
+	int (*map_db_bar)(struct gve_priv *priv);
+	void (*unmap_db_bar)(struct gve_priv *priv);
+};
+
 struct gve_priv {
 	struct net_device *dev;
 	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
@@ -958,6 +968,7 @@ struct gve_priv {
 	dma_addr_t nic_ts_report_bus;
 	u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
 	struct gve_device_info device_info;
+	const struct gve_ctrl_ops *ctrl_ops;
 };
 
 enum gve_service_task_flags_bit {
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 512349c5517f..08d8e49d563e 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1637,3 +1637,24 @@ void gve_set_num_queues(struct gve_priv *priv)
 						device_info->default_rx_queues,
 						priv->rx_cfg.num_queues);
 }
+
+int gve_adminq_map_db_bar(struct gve_priv *priv)
+{
+	struct pci_dev *pdev = priv->pdev;
+	void __iomem *db_bar;
+
+	db_bar = pci_iomap(pdev, GVE_DOORBELL_BAR, 0);
+	if (!db_bar) {
+		dev_err(&pdev->dev, "Failed to map doorbell bar!\n");
+		return -ENOMEM;
+	}
+	priv->db_bar2 = db_bar;
+	return 0;
+}
+
+void gve_adminq_unmap_db_bar(struct gve_priv *priv)
+{
+	struct pci_dev *pdev = priv->pdev;
+
+	pci_iounmap(pdev, priv->db_bar2);
+}
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index a17af755b454..93d3cabb67f1 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -658,4 +658,6 @@ int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
 				 struct gve_ptype_lut *ptype_lut);
 int gve_set_num_ntfy_blks(struct gve_priv *priv);
 void gve_set_num_queues(struct gve_priv *priv);
+int gve_adminq_map_db_bar(struct gve_priv *priv);
+void gve_adminq_unmap_db_bar(struct gve_priv *priv);
 #endif /* _GVE_ADMINQ_H */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index d3882de584e3..d721347e54b4 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -2460,6 +2460,11 @@ static void gve_set_buf_sizes(struct gve_priv *priv)
 		priv->header_buf_size = device_info->header_buf_size;
 }
 
+static const struct gve_ctrl_ops gve_adminq_ops = {
+	.map_db_bar		= gve_adminq_map_db_bar,
+	.unmap_db_bar		= gve_adminq_unmap_db_bar,
+};
+
 static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 {
 	struct gve_device_info *device_info = &priv->device_info;
@@ -2861,7 +2866,6 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	int max_tx_queues, max_rx_queues;
 	struct net_device *dev;
-	__be32 __iomem *db_bar;
 	struct gve_registers __iomem *reg_bar;
 	struct gve_priv *priv;
 	int err;
@@ -2889,13 +2893,6 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto abort_with_pci_region;
 	}
 
-	db_bar = pci_iomap(pdev, GVE_DOORBELL_BAR, 0);
-	if (!db_bar) {
-		dev_err(&pdev->dev, "Failed to map doorbell bar!\n");
-		err = -ENOMEM;
-		goto abort_with_reg_bar;
-	}
-
 	gve_write_version(&reg_bar->driver_version);
 	/* Get max queues to alloc etherdev */
 	max_tx_queues = ioread32be(&reg_bar->max_tx_queues);
@@ -2905,7 +2902,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!dev) {
 		dev_err(&pdev->dev, "could not allocate netdev\n");
 		err = -ENOMEM;
-		goto abort_with_db_bar;
+		goto abort_with_reg_bar;
 	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	pci_set_drvdata(pdev, dev);
@@ -2937,19 +2934,25 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	priv->pdev = pdev;
 	priv->msg_enable = DEFAULT_MSG_LEVEL;
 	priv->reg_bar0 = reg_bar;
-	priv->db_bar2 = db_bar;
 	priv->service_task_flags = 0x0;
 	priv->state_flags = 0x0;
 	priv->ethtool_flags = 0x0;
 	priv->rx_cfg.packet_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
 	priv->max_rx_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
 
+	/* Set adminq ctrl ops */
+	priv->ctrl_ops = &gve_adminq_ops;
+
+	err = priv->ctrl_ops->map_db_bar(priv);
+	if (err)
+		goto abort_with_netdev;
+
 	gve_set_probe_in_progress(priv);
 	priv->gve_wq = alloc_ordered_workqueue("gve", 0);
 	if (!priv->gve_wq) {
 		dev_err(&pdev->dev, "Could not allocate workqueue");
 		err = -ENOMEM;
-		goto abort_with_netdev;
+		goto abort_with_unmap_db_bar;
 	}
 	INIT_WORK(&priv->service_task, gve_service_task);
 	INIT_WORK(&priv->stats_report_task, gve_stats_report_task);
@@ -2979,12 +2982,12 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 abort_with_wq:
 	destroy_workqueue(priv->gve_wq);
 
+abort_with_unmap_db_bar:
+	priv->ctrl_ops->unmap_db_bar(priv);
+
 abort_with_netdev:
 	free_netdev(dev);
 
-abort_with_db_bar:
-	pci_iounmap(pdev, db_bar);
-
 abort_with_reg_bar:
 	pci_iounmap(pdev, reg_bar);
 
@@ -3000,14 +3003,13 @@ static void gve_remove(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct gve_priv *priv = netdev_priv(netdev);
-	__be32 __iomem *db_bar = priv->db_bar2;
 	void __iomem *reg_bar = priv->reg_bar0;
 
 	unregister_netdev(netdev);
 	gve_teardown_priv_resources(priv);
 	destroy_workqueue(priv->gve_wq);
+	priv->ctrl_ops->unmap_db_bar(priv);
 	free_netdev(netdev);
-	pci_iounmap(pdev, db_bar);
 	pci_iounmap(pdev, reg_bar);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 03/12] gve: introduce ctrl ops to set vectors and Qs

[Harshitha Ramamurthy]

Introduce following ctrl ops for Adminq mode.

- set_num_queues to set maximum and default TX/RX queues in the
  alloc structures used in the initialization flow.
- set_num_ntfy_blks op to set number of vectors(ntfy_blks) in the
  initialization flow.

This is done by attaching the adminq_ prefix to the existing
functions placed in the gve_adminq.c file.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v4: Attach 'adminq_' prefix in the function names for gve_set_num_queues
    and gve_set_num_ntfy_blks and set them as ops (Przemek Kitszel)

 drivers/net/ethernet/google/gve/gve.h        | 6 ++++++
 drivers/net/ethernet/google/gve/gve_adminq.c | 4 ++--
 drivers/net/ethernet/google/gve/gve_adminq.h | 4 ++--
 drivers/net/ethernet/google/gve/gve_main.c   | 6 ++++--
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 56148ea3cfbf..1bc5e32b6618 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -829,10 +829,16 @@ struct gve_device_info {
  * struct gve_ctrl_ops - Control plane operations structure
  * @map_db_bar: Maps the doorbell BAR for the device and store in @priv.
  * @unmap_db_bar: Unmaps the doorbell BAR previously mapped by @map_db_bar.
+ * @set_num_queues: Sets default and max TX/RX queues into allocation
+ *                  structures stored in @priv to be used during initialization.
+ * @set_num_ntfy_blks: Sets no. of vectors into @priv to be used during
+ *                     initialization.
  */
 struct gve_ctrl_ops {
 	int (*map_db_bar)(struct gve_priv *priv);
 	void (*unmap_db_bar)(struct gve_priv *priv);
+	void (*set_num_queues)(struct gve_priv *priv);
+	int (*set_num_ntfy_blks)(struct gve_priv *priv);
 };
 
 struct gve_priv {
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 08d8e49d563e..046155ae7d66 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1592,7 +1592,7 @@ int gve_adminq_query_rss_config(struct gve_priv *priv, struct ethtool_rxfh_param
 	return err;
 }
 
-int gve_set_num_ntfy_blks(struct gve_priv *priv)
+int gve_adminq_set_num_ntfy_blks(struct gve_priv *priv)
 {
 	int num_ntfy;
 
@@ -1616,7 +1616,7 @@ int gve_set_num_ntfy_blks(struct gve_priv *priv)
 	return 0;
 }
 
-void gve_set_num_queues(struct gve_priv *priv)
+void gve_adminq_set_num_queues(struct gve_priv *priv)
 {
 	struct gve_device_info *device_info = &priv->device_info;
 
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 93d3cabb67f1..ed3504853908 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -656,8 +656,8 @@ int gve_adminq_report_nic_ts(struct gve_priv *priv,
 struct gve_ptype_lut;
 int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
 				 struct gve_ptype_lut *ptype_lut);
-int gve_set_num_ntfy_blks(struct gve_priv *priv);
-void gve_set_num_queues(struct gve_priv *priv);
+int gve_adminq_set_num_ntfy_blks(struct gve_priv *priv);
+void gve_adminq_set_num_queues(struct gve_priv *priv);
 int gve_adminq_map_db_bar(struct gve_priv *priv);
 void gve_adminq_unmap_db_bar(struct gve_priv *priv);
 #endif /* _GVE_ADMINQ_H */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index d721347e54b4..da53c1fb6afb 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -2463,6 +2463,8 @@ static void gve_set_buf_sizes(struct gve_priv *priv)
 static const struct gve_ctrl_ops gve_adminq_ops = {
 	.map_db_bar		= gve_adminq_map_db_bar,
 	.unmap_db_bar		= gve_adminq_unmap_db_bar,
+	.set_num_queues		= gve_adminq_set_num_queues,
+	.set_num_ntfy_blks	= gve_adminq_set_num_ntfy_blks,
 };
 
 static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
@@ -2501,14 +2503,14 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 
 	priv->queue_format = priv->device_info.queue_format;
 
-	err = gve_set_num_ntfy_blks(priv);
+	err = priv->ctrl_ops->set_num_ntfy_blks(priv);
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Could not setup notify blocks: err=%d\n", err);
 		goto err;
 	}
 
-	gve_set_num_queues(priv);
+	priv->ctrl_ops->set_num_queues(priv);
 	dev_info(&priv->pdev->dev, "TX queues %d, RX queues %d\n",
 		 priv->tx_cfg.num_queues, priv->rx_cfg.num_queues);
 	dev_info(&priv->pdev->dev, "Max TX queues %d, Max RX queues %d\n",
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 04/12] gve: introduce gve_adminq_get_device_properties()

[Harshitha Ramamurthy]

Introduce gve_adminq_get_device_properties() which executes the first
two Adminq commands: VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE
so that this can be called during initialization.

Move these to Adminq specific files. This is just code movement, no
functional change.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v5:
- Place the utsname.h and version.h header files only where needed
- drop uneccessary __maybe_unused (Sashiko)

v3:
- move patch down so that the function is introduced just before usage
- mark function as maybe_unused
- update commit message

 drivers/net/ethernet/google/gve/gve_adminq.c | 68 ++++++++++++++++++--
 drivers/net/ethernet/google/gve/gve_adminq.h |  5 +-
 drivers/net/ethernet/google/gve/gve_main.c   | 47 +-------------
 3 files changed, 65 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 046155ae7d66..f420a8e1dd3d 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -6,6 +6,8 @@
 
 #include <linux/etherdevice.h>
 #include <linux/pci.h>
+#include <linux/utsname.h>
+#include <linux/version.h>
 #include "gve.h"
 #include "gve_adminq.h"
 #include "gve_register.h"
@@ -1143,6 +1145,27 @@ int gve_adminq_describe_device(struct gve_priv *priv)
 	return err;
 }
 
+int gve_adminq_get_device_properties(struct gve_priv *priv)
+{
+	int err;
+
+	err = gve_adminq_verify_driver_compatibility(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Could not verify driver compatibility: err=%d\n", err);
+		return err;
+	}
+
+	/* Get the initial information we need from the device */
+	err = gve_adminq_describe_device(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Could not get device information: err=%d\n", err);
+		return err;
+	}
+	return 0;
+}
+
 int gve_adminq_register_page_list(struct gve_priv *priv,
 				  struct gve_queue_page_list *qpl)
 {
@@ -1205,20 +1228,53 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
 	return gve_adminq_execute_cmd(priv, &cmd);
 }
 
-int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
-					   u64 driver_info_len,
-					   dma_addr_t driver_info_addr)
+int gve_adminq_verify_driver_compatibility(struct gve_priv *priv)
 {
+	struct gve_driver_info *driver_info;
 	union gve_adminq_command cmd;
+	dma_addr_t driver_info_bus;
+	int err;
+
+	driver_info = dma_alloc_coherent(&priv->pdev->dev,
+					 sizeof(struct gve_driver_info),
+					 &driver_info_bus, GFP_KERNEL);
+	if (!driver_info)
+		return -ENOMEM;
+
+	*driver_info = (struct gve_driver_info) {
+		.os_type = 1, /* Linux */
+		.os_version_major = cpu_to_be32(LINUX_VERSION_MAJOR),
+		.os_version_minor = cpu_to_be32(LINUX_VERSION_SUBLEVEL),
+		.os_version_sub = cpu_to_be32(LINUX_VERSION_PATCHLEVEL),
+		.driver_capability_flags = {
+			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS1),
+			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS2),
+			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS3),
+			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS4),
+		},
+	};
+	strscpy(driver_info->os_version_str1, utsname()->release,
+		sizeof(driver_info->os_version_str1));
+	strscpy(driver_info->os_version_str2, utsname()->version,
+		sizeof(driver_info->os_version_str2));
 
 	memset(&cmd, 0, sizeof(cmd));
 	cmd.opcode = cpu_to_be32(GVE_ADMINQ_VERIFY_DRIVER_COMPATIBILITY);
 	cmd.verify_driver_compatibility = (struct gve_adminq_verify_driver_compatibility) {
-		.driver_info_len = cpu_to_be64(driver_info_len),
-		.driver_info_addr = cpu_to_be64(driver_info_addr),
+		.driver_info_len = cpu_to_be64(sizeof(struct gve_driver_info)),
+		.driver_info_addr = cpu_to_be64(driver_info_bus),
 	};
 
-	return gve_adminq_execute_cmd(priv, &cmd);
+	err = gve_adminq_execute_cmd(priv, &cmd);
+
+	/* It's ok if the device doesn't support this */
+	if (err == -EOPNOTSUPP)
+		err = 0;
+
+	dma_free_coherent(&priv->pdev->dev,
+			  sizeof(struct gve_driver_info),
+			  driver_info, driver_info_bus);
+	return err;
 }
 
 int gve_adminq_report_link_speed(struct gve_priv *priv)
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index ed3504853908..2ab68c822e22 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -640,9 +640,8 @@ int gve_adminq_register_page_list(struct gve_priv *priv,
 int gve_adminq_unregister_page_list(struct gve_priv *priv, u32 page_list_id);
 int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
 			    dma_addr_t stats_report_addr, u64 interval);
-int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
-					   u64 driver_info_len,
-					   dma_addr_t driver_info_addr);
+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_speed(struct gve_priv *priv);
 int gve_adminq_add_flow_rule(struct gve_priv *priv, struct gve_adminq_flow_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_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index da53c1fb6afb..addfd8dc1a1a 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -17,8 +17,6 @@
 #include <linux/sched.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
-#include <linux/utsname.h>
-#include <linux/version.h>
 #include <net/netdev_queues.h>
 #include <net/sch_generic.h>
 #include <net/xdp_sock_drv.h>
@@ -41,49 +39,6 @@ char gve_driver_name[] = "gve";
 const char gve_version_str[] = GVE_VERSION;
 static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
 
-static int gve_verify_driver_compatibility(struct gve_priv *priv)
-{
-	int err;
-	struct gve_driver_info *driver_info;
-	dma_addr_t driver_info_bus;
-
-	driver_info = dma_alloc_coherent(&priv->pdev->dev,
-					 sizeof(struct gve_driver_info),
-					 &driver_info_bus, GFP_KERNEL);
-	if (!driver_info)
-		return -ENOMEM;
-
-	*driver_info = (struct gve_driver_info) {
-		.os_type = 1, /* Linux */
-		.os_version_major = cpu_to_be32(LINUX_VERSION_MAJOR),
-		.os_version_minor = cpu_to_be32(LINUX_VERSION_SUBLEVEL),
-		.os_version_sub = cpu_to_be32(LINUX_VERSION_PATCHLEVEL),
-		.driver_capability_flags = {
-			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS1),
-			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS2),
-			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS3),
-			cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS4),
-		},
-	};
-	strscpy(driver_info->os_version_str1, utsname()->release,
-		sizeof(driver_info->os_version_str1));
-	strscpy(driver_info->os_version_str2, utsname()->version,
-		sizeof(driver_info->os_version_str2));
-
-	err = gve_adminq_verify_driver_compatibility(priv,
-						     sizeof(struct gve_driver_info),
-						     driver_info_bus);
-
-	/* It's ok if the device doesn't support this */
-	if (err == -EOPNOTSUPP)
-		err = 0;
-
-	dma_free_coherent(&priv->pdev->dev,
-			  sizeof(struct gve_driver_info),
-			  driver_info, driver_info_bus);
-	return err;
-}
-
 static netdev_features_t gve_features_check(struct sk_buff *skb,
 					    struct net_device *dev,
 					    netdev_features_t features)
@@ -2480,7 +2435,7 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 		return err;
 	}
 
-	err = gve_verify_driver_compatibility(priv);
+	err = gve_adminq_verify_driver_compatibility(priv);
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Could not verify driver compatibility: err=%d\n", err);
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 05/12] gve: refactor gve_init_priv for reset path

[Harshitha Ramamurthy]

The driver does not need to renegotiate all properties with
the device on a reset since those should stay constant through
a reset. Hence change gve_init_priv() into a method that only
sets these properties into the priv structure and hence needs
to be only called once during gve_probe().

To achieve this end state of gve_init_priv(), do the following:
- introduce gve_adminq_init() which writes the driver version register
  and allocates the AdminQ and call it in gve_probe()
- call gve_adminq_get_device_properties() into gve_probe() to learn device
  properties
- introduce gve_setup_device() which deals with device setup logic and
  call it in gve_probe()
- resetting no. of registered pages is moved into gve_setup_device() since
  that needs to be reset every time queues are re-created.

With these changes, gve_adminq_get_device_properties() and
gve_init_priv() are only called once during gve_probe.
gve_reset_recovery() now calls targeted setup functions directly.

This prepares the driver to add mailbox mode's control plane
initialization and device properties negotiation in the same place
as is done in AdminQ mode in the upcoming patches when adding the
mailbox ABI.

These changes are only code movement, no functional change.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v6:
- drop double logging on err of gve_adminq_get_device_properties()

v3:
 - gve_reset_recovery also calls verify driver compatibility
 - don't free device resources if gve_open() fails in the reset path
 - move resetting no. of registered pages to gve_setup_device()

 drivers/net/ethernet/google/gve/gve.h        |   2 +
 drivers/net/ethernet/google/gve/gve_adminq.c |  12 +-
 drivers/net/ethernet/google/gve/gve_adminq.h |   2 +-
 drivers/net/ethernet/google/gve/gve_main.c   | 143 ++++++++++---------
 4 files changed, 92 insertions(+), 67 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 1bc5e32b6618..48cc8a6be186 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -1253,6 +1253,8 @@ static inline bool gve_is_clock_enabled(struct gve_priv *priv)
 	return priv->nic_ts_report;
 }
 
+void gve_adminq_write_version(u8 __iomem *driver_version_register);
+
 /* gqi napi handler defined in gve_main.c */
 int gve_napi_poll(struct napi_struct *napi, int budget);
 
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index f420a8e1dd3d..a62cb7a921d0 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -298,8 +298,10 @@ gve_process_device_options(struct gve_priv *priv,
 	return 0;
 }
 
-int gve_adminq_alloc(struct device *dev, struct gve_priv *priv)
+static int gve_adminq_alloc(struct gve_priv *priv)
 {
+	struct device *dev = &priv->pdev->dev;
+
 	priv->adminq_pool = dma_pool_create("adminq_pool", dev,
 					    GVE_ADMINQ_BUFFER_SIZE, 0, 0);
 	if (unlikely(!priv->adminq_pool))
@@ -355,6 +357,14 @@ int gve_adminq_alloc(struct device *dev, struct gve_priv *priv)
 	return 0;
 }
 
+int gve_adminq_init(struct gve_priv *priv)
+{
+	struct gve_registers __iomem *reg_bar = priv->reg_bar0;
+
+	gve_adminq_write_version(&reg_bar->driver_version);
+	return gve_adminq_alloc(priv);
+}
+
 void gve_adminq_release(struct gve_priv *priv)
 {
 	int i = 0;
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 2ab68c822e22..78eee3b5cb7f 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -619,7 +619,7 @@ union gve_adminq_command {
 
 static_assert(sizeof(union gve_adminq_command) == 64);
 
-int gve_adminq_alloc(struct device *dev, struct gve_priv *priv);
+int gve_adminq_init(struct gve_priv *priv);
 void gve_adminq_free(struct gve_priv *priv);
 void gve_adminq_release(struct gve_priv *priv);
 int gve_adminq_describe_device(struct gve_priv *priv);
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index addfd8dc1a1a..2fe280cf7e68 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -2415,6 +2415,35 @@ static void gve_set_buf_sizes(struct gve_priv *priv)
 		priv->header_buf_size = device_info->header_buf_size;
 }
 
+static int gve_setup_device(struct gve_priv *priv)
+{
+	int err;
+
+	priv->num_registered_pages = 0;
+
+	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
+	if (!priv->xsk_pools) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	gve_set_netdev_xdp_features(priv);
+	if (!gve_is_gqi(priv))
+		priv->dev->xdp_metadata_ops = &gve_xdp_metadata_ops;
+
+	err = gve_setup_device_resources(priv);
+	if (err)
+		goto err_free_xsk_bitmap;
+
+	return 0;
+
+err_free_xsk_bitmap:
+	bitmap_free(priv->xsk_pools);
+	priv->xsk_pools = NULL;
+err:
+	return err;
+}
+
 static const struct gve_ctrl_ops gve_adminq_ops = {
 	.map_db_bar		= gve_adminq_map_db_bar,
 	.unmap_db_bar		= gve_adminq_unmap_db_bar,
@@ -2422,47 +2451,18 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.set_num_ntfy_blks	= gve_adminq_set_num_ntfy_blks,
 };
 
-static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
+static int gve_init_priv(struct gve_priv *priv)
 {
 	struct gve_device_info *device_info = &priv->device_info;
 	int err;
 
-	/* Set up the adminq */
-	err = gve_adminq_alloc(&priv->pdev->dev, priv);
-	if (err) {
-		dev_err(&priv->pdev->dev,
-			"Failed to alloc admin queue: err=%d\n", err);
-		return err;
-	}
-
-	err = gve_adminq_verify_driver_compatibility(priv);
-	if (err) {
-		dev_err(&priv->pdev->dev,
-			"Could not verify driver compatibility: err=%d\n", err);
-		goto err;
-	}
-
-	priv->num_registered_pages = 0;
-
-	if (skip_describe_device)
-		goto setup_device;
-
-	device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
-	/* Get the initial information we need from the device */
-	err = gve_adminq_describe_device(priv);
-	if (err) {
-		dev_err(&priv->pdev->dev,
-			"Could not get device information: err=%d\n", err);
-		goto err;
-	}
-
 	priv->queue_format = priv->device_info.queue_format;
 
 	err = priv->ctrl_ops->set_num_ntfy_blks(priv);
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Could not setup notify blocks: err=%d\n", err);
-		goto err;
+		return err;
 	}
 
 	priv->ctrl_ops->set_num_queues(priv);
@@ -2485,10 +2485,8 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 		netif_set_tso_max_size(priv->dev, GVE_DQO_TX_MAX);
 	}
 
-	if (gve_set_mtu(priv)) {
-		err = -EINVAL;
-		goto err;
-	}
+	if (gve_set_mtu(priv))
+		return -EINVAL;
 
 	priv->num_event_counters = device_info->num_event_counters;
 
@@ -2513,30 +2511,7 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 	priv->ts_config.tx_type = HWTSTAMP_TX_OFF;
 	priv->ts_config.rx_filter = HWTSTAMP_FILTER_NONE;
 	priv->nic_timestamp_supported = device_info->nic_timestamp_supported;
-
-setup_device:
-	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
-	if (!priv->xsk_pools) {
-		err = -ENOMEM;
-		goto err;
-	}
-
-	gve_set_netdev_xdp_features(priv);
-	if (!gve_is_gqi(priv))
-		priv->dev->xdp_metadata_ops = &gve_xdp_metadata_ops;
-
-	err = gve_setup_device_resources(priv);
-	if (err)
-		goto err_free_xsk_bitmap;
-
 	return 0;
-
-err_free_xsk_bitmap:
-	bitmap_free(priv->xsk_pools);
-	priv->xsk_pools = NULL;
-err:
-	gve_adminq_free(priv);
-	return err;
 }
 
 static void gve_teardown_priv_resources(struct gve_priv *priv)
@@ -2566,15 +2541,32 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
 {
 	int err;
 
-	err = gve_init_priv(priv, true);
-	if (err)
+	err = gve_adminq_init(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Failed to alloc admin queue: err=%d\n", err);
 		goto err;
+	}
+
+	err = gve_adminq_verify_driver_compatibility(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Could not verify driver compatibility: err=%d\n", err);
+		goto err_free_adminq;
+	}
+
+	err = gve_setup_device(priv);
+	if (err)
+		goto err_free_adminq;
 	if (was_up) {
 		err = gve_open(priv->dev);
 		if (err)
-			goto err;
+			return err;
 	}
 	return 0;
+
+err_free_adminq:
+	gve_adminq_free(priv);
 err:
 	dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
 	gve_turndown(priv);
@@ -2617,7 +2609,7 @@ int gve_reset(struct gve_priv *priv, bool attempt_teardown)
 	return err;
 }
 
-static void gve_write_version(u8 __iomem *driver_version_register)
+void gve_adminq_write_version(u8 __iomem *driver_version_register)
 {
 	const char *c = gve_version_prefix;
 
@@ -2850,7 +2842,6 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto abort_with_pci_region;
 	}
 
-	gve_write_version(&reg_bar->driver_version);
 	/* Get max queues to alloc etherdev */
 	max_tx_queues = ioread32be(&reg_bar->max_tx_queues);
 	max_rx_queues = ioread32be(&reg_bar->max_rx_queues);
@@ -2897,12 +2888,24 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	priv->rx_cfg.packet_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
 	priv->max_rx_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
 
+	err = gve_adminq_init(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Failed to alloc admin queue: err=%d\n", err);
+		goto abort_with_netdev;
+	}
+
+	priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
+	err = gve_adminq_get_device_properties(priv);
+	if (err)
+		goto abort_with_adminq;
+
 	/* Set adminq ctrl ops */
 	priv->ctrl_ops = &gve_adminq_ops;
 
 	err = priv->ctrl_ops->map_db_bar(priv);
 	if (err)
-		goto abort_with_netdev;
+		goto abort_with_adminq;
 
 	gve_set_probe_in_progress(priv);
 	priv->gve_wq = alloc_ordered_workqueue("gve", 0);
@@ -2916,10 +2919,17 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	priv->tx_cfg.max_queues = max_tx_queues;
 	priv->rx_cfg.max_queues = max_rx_queues;
 
-	err = gve_init_priv(priv, false);
+	err = gve_init_priv(priv);
 	if (err)
 		goto abort_with_wq;
 
+	err = gve_setup_device(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Could not setup device: err=%d\n", err);
+		goto abort_with_wq;
+	}
+
 	if (!gve_is_gqi(priv) && !gve_is_qpl(priv))
 		dev->netmem_tx = NETMEM_TX_DMA;
 
@@ -2942,6 +2952,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 abort_with_unmap_db_bar:
 	priv->ctrl_ops->unmap_db_bar(priv);
 
+abort_with_adminq:
+	gve_adminq_free(priv);
+
 abort_with_netdev:
 	free_netdev(dev);
 
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 06/12] gve: simplify reset logic

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

Current GVE reset logic is quite complex, with a number of methods with
similar names and functionalities. This complexity has allowed a number
of bugs to enter the reset/recovery path, including the potential for
reset loops if an operation fails during teardown.

Simplify the reset path by doing the following:
1) Removing recursive resets. Recursive resets have two major issues.
   First, there is the potential for stack overflows if resets are
   invoked too many times in a row. Second, long recursive calls mean
   that GVE never gives up the RTNL lock, or at the very least holds it
   for too long. If a reset must occur anywhere during the
   reset/recovery path, it should be scheduled as a separate task.
2) Removing resets during teardown. This is partly covered by removing
   recursive resets, but the primary goal in this case is to ensure that
   the driver is capable of actually executing a hardware reset if
   something goes wrong with a control plane operation. As it stands, if
   `deconfigure_device_resources` fails, for example, GVE will preempt
   its reset with another reset without actually invoking a hardware
   reset, which could actually help with recovery.
3) Decompose allocation/de-allocation and setup/teardown. Performing
   allocation and setup for each control plane system (RSS, ptype map,
   etc) leaves many more error conditions to handle, causing teardown in
   the case of failures to be much more complex than they need to be.
   This will also be useful to better align a major behavioral change
   in mailbox mode, which will use separate response buffers instead to
   get data from the device instead of a pre-allocated shared memory
   region.

With the new reset functionality, shared resources between the device
and driver are not freed until after the hardware reset has completed
in the event that `deconfigure_device_resources` fails, meaning that the
device could potentially still be holding on to shared memory.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v5:
- fix workqueue disable count imbalance in reset and suspend path (Sashiko)
- destroy rings before stopping queues (Sashiko)
- ensure to call gve_queues_stop in error path in gve_close()
- pull out gve_turndown out of gve_queues_stop so the ordering of
  gve_turndown(stop NAPIs) -> gve_destroy_rings -> gve_queues_stop(free rings) can be preserved

v4:
  - fix kdoc formatting for gve_teardown_control_plane_resources
  - ignore management interrupt if device is not okay

v3:
 - only reset when failing to program flow rules as ethtool op
 - don't attempt to teardown rings in reset path if AQ is not allocated
 - fix work queue semantics related to management IRQ handler

v2:
  - Fixed typos in commit message (recursive, preempt)
  - Fixed a kdoc warning

 drivers/net/ethernet/google/gve/gve.h         |   2 +-
 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 |   2 +-
 .../net/ethernet/google/gve/gve_flow_rule.c   |  15 +-
 drivers/net/ethernet/google/gve/gve_main.c    | 351 +++++++++---------
 6 files changed, 197 insertions(+), 183 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 48cc8a6be186..026d685ecaee 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -1348,7 +1348,7 @@ struct page_pool *gve_rx_create_page_pool(struct gve_priv *priv,
 
 /* Reset */
 void gve_schedule_reset(struct gve_priv *priv);
-int gve_reset(struct gve_priv *priv, bool attempt_teardown);
+int gve_reset(struct gve_priv *priv, bool skip_queue_setup);
 void gve_get_curr_alloc_cfgs(struct gve_priv *priv,
 			     struct gve_tx_alloc_rings_cfg *tx_alloc_cfg,
 			     struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index a62cb7a921d0..901673d2e264 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -365,7 +365,7 @@ int gve_adminq_init(struct gve_priv *priv)
 	return gve_adminq_alloc(priv);
 }
 
-void gve_adminq_release(struct gve_priv *priv)
+static void gve_adminq_release(struct gve_priv *priv)
 {
 	int i = 0;
 
@@ -394,7 +394,6 @@ void gve_adminq_release(struct gve_priv *priv)
 	}
 	gve_clear_device_rings_ok(priv);
 	gve_clear_device_resources_ok(priv);
-	gve_clear_admin_queue_ok(priv);
 }
 
 void gve_adminq_free(struct gve_priv *priv)
@@ -1377,12 +1376,8 @@ gve_adminq_configure_flow_rule(struct gve_priv *priv,
 			sizeof(struct gve_adminq_configure_flow_rule),
 			flow_rule_cmd);
 
-	if (err == -ETIME) {
-		dev_err(&priv->pdev->dev, "Timeout to configure the flow rule, trigger reset");
-		gve_reset(priv, true);
-	} else if (!err) {
+	if (!err)
 		priv->flow_rules_cache.rules_cache_synced = false;
-	}
 
 	return err;
 }
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 78eee3b5cb7f..fe1e8868cdfe 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -621,7 +621,6 @@ static_assert(sizeof(union gve_adminq_command) == 64);
 
 int gve_adminq_init(struct gve_priv *priv);
 void gve_adminq_free(struct gve_priv *priv);
-void gve_adminq_release(struct gve_priv *priv);
 int gve_adminq_describe_device(struct gve_priv *priv);
 int gve_adminq_configure_device_resources(struct gve_priv *priv,
 					  dma_addr_t counter_array_bus_addr,
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 8199738ba979..dd1c44fedc77 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -651,7 +651,7 @@ static int gve_user_reset(struct net_device *netdev, u32 *flags)
 
 	if (*flags == ETH_RESET_ALL) {
 		*flags = 0;
-		return gve_reset(priv, true);
+		return gve_reset(priv, false);
 	}
 
 	return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/google/gve/gve_flow_rule.c b/drivers/net/ethernet/google/gve/gve_flow_rule.c
index 2c80cda28ef3..fae552f4ad6f 100644
--- a/drivers/net/ethernet/google/gve/gve_flow_rule.c
+++ b/drivers/net/ethernet/google/gve/gve_flow_rule.c
@@ -278,6 +278,11 @@ int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
 		goto out;
 
 	err = gve_adminq_add_flow_rule(priv, rule, fsp->location);
+	if (err == -ETIME) {
+		dev_err(&priv->pdev->dev,
+			"Timeout to add flow rule, trigger reset.");
+		gve_reset(priv, false);
+	}
 
 out:
 	kvfree(rule);
@@ -290,9 +295,17 @@ int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
 int gve_del_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
 {
 	struct ethtool_rx_flow_spec *fsp = (struct ethtool_rx_flow_spec *)&cmd->fs;
+	int err;
 
 	if (!priv->max_flow_rules)
 		return -EOPNOTSUPP;
 
-	return gve_adminq_del_flow_rule(priv, fsp->location);
+	err = gve_adminq_del_flow_rule(priv, fsp->location);
+	if (err == -ETIME) {
+		dev_err(&priv->pdev->dev,
+			"Timeout to delete flow rule, trigger reset.");
+		gve_reset(priv, false);
+	}
+
+	return err;
 }
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 2fe280cf7e68..87388856ff6c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -270,6 +270,10 @@ static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
 {
 	struct gve_priv *priv = arg;
 
+	/* Device resources are not okay; consider the interrupt spurious. */
+	if (!gve_get_device_resources_ok(priv))
+		return IRQ_NONE;
+
 	queue_work(priv->gve_wq, &priv->service_task);
 	return IRQ_HANDLED;
 }
@@ -590,7 +594,22 @@ static void gve_free_notify_blocks(struct gve_priv *priv)
 	priv->msix_vectors = NULL;
 }
 
-static int gve_setup_device_resources(struct gve_priv *priv)
+static void gve_free_control_plane_resources(struct gve_priv *priv)
+{
+	bitmap_free(priv->xsk_pools);
+	priv->xsk_pools = NULL;
+
+	kvfree(priv->ptype_lut_dqo);
+	priv->ptype_lut_dqo = NULL;
+
+	gve_free_stats_report(priv);
+	gve_free_notify_blocks(priv);
+	gve_free_counter_array(priv);
+	gve_free_rss_config_cache(priv);
+	gve_free_flow_rule_caches(priv);
+}
+
+static int gve_alloc_control_plane_resources(struct gve_priv *priv)
 {
 	int err;
 
@@ -599,16 +618,42 @@ static int gve_setup_device_resources(struct gve_priv *priv)
 		return err;
 	err = gve_alloc_rss_config_cache(priv);
 	if (err)
-		goto abort_with_flow_rule_caches;
+		goto abort;
 	err = gve_alloc_counter_array(priv);
 	if (err)
-		goto abort_with_rss_config_cache;
+		goto abort;
 	err = gve_alloc_notify_blocks(priv);
 	if (err)
-		goto abort_with_counter;
+		goto abort;
 	err = gve_alloc_stats_report(priv);
 	if (err)
-		goto abort_with_ntfy_blocks;
+		goto abort;
+
+	if (!gve_is_gqi(priv)) {
+		priv->ptype_lut_dqo = kvzalloc_obj(*priv->ptype_lut_dqo,
+						   GFP_KERNEL);
+		if (!priv->ptype_lut_dqo) {
+			err = -ENOMEM;
+			goto abort;
+		}
+	}
+
+	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
+	if (!priv->xsk_pools) {
+		err = -ENOMEM;
+		goto abort;
+	}
+
+	return 0;
+abort:
+	gve_free_control_plane_resources(priv);
+	return err;
+}
+
+static int gve_setup_control_plane_resources(struct gve_priv *priv)
+{
+	int err = 0;
+
 	err = gve_adminq_configure_device_resources(priv,
 						    priv->counter_array_bus,
 						    priv->num_event_counters,
@@ -618,20 +663,15 @@ static int gve_setup_device_resources(struct gve_priv *priv)
 		dev_err(&priv->pdev->dev,
 			"could not setup device_resources: err=%d\n", err);
 		err = -ENXIO;
-		goto abort_with_stats_report;
+		return err;
 	}
 
 	if (!gve_is_gqi(priv)) {
-		priv->ptype_lut_dqo = kvzalloc_obj(*priv->ptype_lut_dqo);
-		if (!priv->ptype_lut_dqo) {
-			err = -ENOMEM;
-			goto abort_with_stats_report;
-		}
 		err = gve_adminq_get_ptype_map_dqo(priv, priv->ptype_lut_dqo);
 		if (err) {
 			dev_err(&priv->pdev->dev,
 				"Failed to get ptype map: err=%d\n", err);
-			goto abort_with_ptype_lut;
+			goto deconfigure_device;
 		}
 	}
 
@@ -646,7 +686,7 @@ static int gve_setup_device_resources(struct gve_priv *priv)
 	err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
 	if (err) {
 		dev_err(&priv->pdev->dev, "Failed to init RSS config");
-		goto abort_with_clock;
+		goto teardown_clock;
 	}
 
 	err = gve_adminq_report_stats(priv, priv->stats_report_len,
@@ -658,67 +698,64 @@ static int gve_setup_device_resources(struct gve_priv *priv)
 	gve_set_device_resources_ok(priv);
 	return 0;
 
-abort_with_clock:
+teardown_clock:
 	gve_teardown_clock(priv);
-abort_with_ptype_lut:
-	kvfree(priv->ptype_lut_dqo);
-	priv->ptype_lut_dqo = NULL;
-abort_with_stats_report:
-	gve_free_stats_report(priv);
-abort_with_ntfy_blocks:
-	gve_free_notify_blocks(priv);
-abort_with_counter:
-	gve_free_counter_array(priv);
-abort_with_rss_config_cache:
-	gve_free_rss_config_cache(priv);
-abort_with_flow_rule_caches:
-	gve_free_flow_rule_caches(priv);
-
+deconfigure_device:
+	gve_adminq_deconfigure_device_resources(priv);
 	return err;
 }
 
-static void gve_trigger_reset(struct gve_priv *priv);
-
-static void gve_teardown_device_resources(struct gve_priv *priv)
+/**
+ * gve_teardown_control_plane_resources() - Request the device to release any
+ * shared allocated resources.
+ *
+ * @priv: Pointer to the GVE private device data structure.
+ *
+ * If any part of the teardown step fails, the failure is documented, but is
+ * otherwise ignored. It is expected that a device reset is triggered
+ * immediately after tearing down device resources, which would clear any
+ * lingering state on the device.
+ */
+static void gve_teardown_control_plane_resources(struct gve_priv *priv)
 {
 	int err;
 
 	/* Tell device its resources are being freed */
 	if (gve_get_device_resources_ok(priv)) {
 		err = gve_flow_rules_reset(priv);
-		if (err) {
+		if (err)
 			dev_err(&priv->pdev->dev,
 				"Failed to reset flow rules: err=%d\n", err);
-			gve_trigger_reset(priv);
-		}
 		/* detach the stats report */
 		err = gve_adminq_report_stats(priv, 0, 0x0, GVE_STATS_REPORT_TIMER_PERIOD);
-		if (err) {
+		if (err)
 			dev_err(&priv->pdev->dev,
 				"Failed to detach stats report: err=%d\n", err);
-			gve_trigger_reset(priv);
-		}
+		gve_teardown_clock(priv);
 		err = gve_adminq_deconfigure_device_resources(priv);
-		if (err) {
+		if (err)
 			dev_err(&priv->pdev->dev,
 				"Could not deconfigure device resources: err=%d\n",
 				err);
-			gve_trigger_reset(priv);
-		}
 	}
 
-	kvfree(priv->ptype_lut_dqo);
-	priv->ptype_lut_dqo = NULL;
-
-	gve_free_flow_rule_caches(priv);
-	gve_free_rss_config_cache(priv);
-	gve_free_counter_array(priv);
-	gve_free_notify_blocks(priv);
-	gve_free_stats_report(priv);
-	gve_teardown_clock(priv);
 	gve_clear_device_resources_ok(priv);
 }
 
+static void gve_teardown_device(struct gve_priv *priv)
+{
+	gve_teardown_control_plane_resources(priv);
+	gve_adminq_free(priv);
+	/*
+	 * Free any resources shared with the device only after we have a
+	 * guarantee that the device will not try to access such resources.
+	 * Device commands in gve_teardown_control_plane_resources can fail, in
+	 * which case, device resources won't be relinquished until
+	 * gve_adminq_free is called to trigger a device reset.
+	 */
+	gve_free_control_plane_resources(priv);
+}
+
 static int gve_unregister_qpl(struct gve_priv *priv,
 			      struct gve_queue_page_list *qpl)
 {
@@ -1157,8 +1194,6 @@ void gve_schedule_reset(struct gve_priv *priv)
 	queue_work(priv->gve_wq, &priv->service_task);
 }
 
-static void gve_reset_and_teardown(struct gve_priv *priv, bool was_up);
-static int gve_reset_recovery(struct gve_priv *priv, bool was_up);
 static void gve_turndown(struct gve_priv *priv);
 static void gve_turnup(struct gve_priv *priv);
 
@@ -1269,11 +1304,12 @@ static int gve_reg_xdp_info(struct gve_priv *priv, struct net_device *dev)
 	return err;
 }
 
-
 static void gve_drain_page_cache(struct gve_priv *priv)
 {
 	int i;
 
+	if (!priv->rx)
+		return;
 	for (i = 0; i < priv->rx_cfg.num_queues; i++)
 		page_frag_cache_drain(&priv->rx[i].page_cache);
 }
@@ -1416,10 +1452,11 @@ static int gve_queues_start(struct gve_priv *priv,
 reset:
 	if (gve_get_reset_in_progress(priv))
 		goto stop_and_free_rings;
-	gve_reset_and_teardown(priv, true);
-	/* if this fails there is nothing we can do so just ignore the return */
-	gve_reset_recovery(priv, false);
-	/* return the original error */
+
+	/* Attempt to reset. If reset is successful, gve_queues_start was
+	 * successful.
+	 */
+	err = gve_reset(priv, false);
 	return err;
 stop_and_free_rings:
 	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
@@ -1435,6 +1472,12 @@ static int gve_open(struct net_device *dev)
 	struct gve_priv *priv = netdev_priv(dev);
 	int err;
 
+	if (!gve_get_device_resources_ok(priv)) {
+		dev_err(&priv->pdev->dev,
+			"Attempting to open netdev without resources. Device must be reset.");
+		return -ENODEV;
+	}
+
 	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
 
 	err = gve_queues_mem_alloc(priv, &tx_alloc_cfg, &rx_alloc_cfg);
@@ -1451,41 +1494,16 @@ static int gve_open(struct net_device *dev)
 	return 0;
 }
 
-static int gve_queues_stop(struct gve_priv *priv)
+static void gve_queues_stop(struct gve_priv *priv)
 {
-	int err;
+	gve_unreg_xdp_info(priv);
+	gve_drain_page_cache(priv);
 
-	netif_carrier_off(priv->dev);
-	if (gve_get_device_rings_ok(priv)) {
-		gve_turndown(priv);
-		gve_drain_page_cache(priv);
-		err = gve_destroy_rings(priv);
-		if (err)
-			goto err;
-		err = gve_unregister_qpls(priv);
-		if (err)
-			goto err;
-		gve_clear_device_rings_ok(priv);
-	}
 	timer_delete_sync(&priv->stats_report_timer);
-
-	gve_unreg_xdp_info(priv);
+	cancel_work_sync(&priv->stats_report_task);
 
 	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
 	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
-
-	priv->interface_down_cnt++;
-	return 0;
-
-err:
-	/* This must have been called from a reset due to the rtnl lock
-	 * so just return at this point.
-	 */
-	if (gve_get_reset_in_progress(priv))
-		return err;
-	/* Otherwise reset before returning */
-	gve_reset_and_teardown(priv, true);
-	return gve_reset_recovery(priv, false);
 }
 
 static int gve_close(struct net_device *dev)
@@ -1493,12 +1511,30 @@ static int gve_close(struct net_device *dev)
 	struct gve_priv *priv = netdev_priv(dev);
 	int err;
 
-	err = gve_queues_stop(priv);
-	if (err)
-		return err;
+	gve_turndown(priv);
+
+	/* Surrender to reset if the queue destroying adminq cmds fail. Reset
+	 * will not re-enable the interface.
+	 */
+	if (gve_get_device_rings_ok(priv)) {
+		gve_clear_device_rings_ok(priv);
+		err = gve_destroy_rings(priv);
+		if (err)
+			goto reset;
+		err = gve_unregister_qpls(priv);
+		if (err)
+			goto reset;
+	}
 
+	gve_queues_stop(priv);
 	gve_queues_mem_remove(priv);
+	priv->interface_down_cnt++;
 	return 0;
+
+reset:
+	gve_queues_stop(priv);
+	err = gve_reset(priv, true);
+	return err;
 }
 
 static void gve_handle_link_status(struct gve_priv *priv, bool link_status)
@@ -2421,25 +2457,17 @@ static int gve_setup_device(struct gve_priv *priv)
 
 	priv->num_registered_pages = 0;
 
-	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
-	if (!priv->xsk_pools) {
-		err = -ENOMEM;
-		goto err;
-	}
-
 	gve_set_netdev_xdp_features(priv);
 	if (!gve_is_gqi(priv))
 		priv->dev->xdp_metadata_ops = &gve_xdp_metadata_ops;
 
-	err = gve_setup_device_resources(priv);
+	err = gve_alloc_control_plane_resources(priv);
 	if (err)
-		goto err_free_xsk_bitmap;
-
+		goto err;
+	err = gve_setup_control_plane_resources(priv);
+	if (err)
+		goto err;
 	return 0;
-
-err_free_xsk_bitmap:
-	bitmap_free(priv->xsk_pools);
-	priv->xsk_pools = NULL;
 err:
 	return err;
 }
@@ -2514,30 +2542,7 @@ static int gve_init_priv(struct gve_priv *priv)
 	return 0;
 }
 
-static void gve_teardown_priv_resources(struct gve_priv *priv)
-{
-	gve_teardown_device_resources(priv);
-	gve_adminq_free(priv);
-	bitmap_free(priv->xsk_pools);
-	priv->xsk_pools = NULL;
-}
-
-static void gve_trigger_reset(struct gve_priv *priv)
-{
-	/* Reset the device by releasing the AQ */
-	gve_adminq_release(priv);
-}
-
-static void gve_reset_and_teardown(struct gve_priv *priv, bool was_up)
-{
-	gve_trigger_reset(priv);
-	/* With the reset having already happened, close cannot fail */
-	if (was_up)
-		gve_close(priv->dev);
-	gve_teardown_priv_resources(priv);
-}
-
-static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
+static int gve_recover(struct gve_priv *priv, bool setup_queues)
 {
 	int err;
 
@@ -2545,62 +2550,66 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Failed to alloc admin queue: err=%d\n", err);
-		goto err;
+		goto teardown_device;
 	}
 
 	err = gve_adminq_verify_driver_compatibility(priv);
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Could not verify driver compatibility: err=%d\n", err);
-		goto err_free_adminq;
+		goto teardown_device;
 	}
 
 	err = gve_setup_device(priv);
 	if (err)
-		goto err_free_adminq;
-	if (was_up) {
+		goto teardown_device;
+
+	if (setup_queues) {
 		err = gve_open(priv->dev);
 		if (err)
-			return err;
+			goto teardown_device;
 	}
+
 	return 0;
 
-err_free_adminq:
-	gve_adminq_free(priv);
-err:
-	dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
-	gve_turndown(priv);
+teardown_device:
+	dev_err(&priv->pdev->dev, "Recover failed! !!! DISABLING ALL QUEUES !!!\n");
+	gve_teardown_device(priv);
 	return err;
 }
 
-int gve_reset(struct gve_priv *priv, bool attempt_teardown)
+int gve_reset(struct gve_priv *priv, bool skip_queue_setup)
 {
 	bool was_up = netif_running(priv->dev);
 	int err;
 
+	if (gve_get_reset_in_progress(priv))
+		return 0;
+
 	dev_info(&priv->pdev->dev, "Performing reset\n");
 	gve_clear_do_reset(priv);
 	gve_set_reset_in_progress(priv);
-	/* If we aren't attempting to teardown normally, just go turndown and
-	 * reset right away.
-	 */
-	if (!attempt_teardown) {
+
+	if (was_up) {
 		gve_turndown(priv);
-		gve_reset_and_teardown(priv, was_up);
-	} else {
-		/* Otherwise attempt to close normally */
-		if (was_up) {
-			err = gve_close(priv->dev);
-			/* If that fails reset as we did above */
-			if (err)
-				gve_reset_and_teardown(priv, was_up);
+		if (gve_get_device_rings_ok(priv)) {
+			gve_clear_device_rings_ok(priv);
+			gve_destroy_rings(priv);
+			gve_unregister_qpls(priv);
 		}
-		/* Clean up any remaining resources */
-		gve_teardown_priv_resources(priv);
+		gve_queues_stop(priv);
 	}
 
-	/* Set it all back up */
-	err = gve_reset_recovery(priv, was_up);
+	disable_work(&priv->service_task);
+	gve_teardown_device(priv);
+	gve_queues_mem_remove(priv);
+
+	enable_work(&priv->service_task);
+	err = gve_recover(priv, was_up && !skip_queue_setup);
+	if (err)
+		dev_info(&priv->pdev->dev,
+			 "Failed to recover in reset: %d\n", err);
+
 	gve_clear_reset_in_progress(priv);
 	priv->reset_cnt++;
 	priv->interface_up_cnt = 0;
@@ -2927,7 +2936,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err) {
 		dev_err(&priv->pdev->dev,
 			"Could not setup device: err=%d\n", err);
-		goto abort_with_wq;
+		goto abort_teardown_device;
 	}
 
 	if (!gve_is_gqi(priv) && !gve_is_qpl(priv))
@@ -2935,7 +2944,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	err = register_netdev(dev);
 	if (err)
-		goto abort_with_gve_init;
+		goto abort_teardown_device;
 
 	dev_info(&pdev->dev, "GVE version %s\n", gve_version_str);
 	dev_info(&pdev->dev, "GVE queue format %d\n", (int)priv->queue_format);
@@ -2943,8 +2952,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	queue_work(priv->gve_wq, &priv->service_task);
 	return 0;
 
-abort_with_gve_init:
-	gve_teardown_priv_resources(priv);
+abort_teardown_device:
+	disable_work(&priv->stats_report_task);
+	gve_teardown_device(priv);
 
 abort_with_wq:
 	destroy_workqueue(priv->gve_wq);
@@ -2976,7 +2986,8 @@ static void gve_remove(struct pci_dev *pdev)
 	void __iomem *reg_bar = priv->reg_bar0;
 
 	unregister_netdev(netdev);
-	gve_teardown_priv_resources(priv);
+	disable_work_sync(&priv->service_task);
+	gve_teardown_device(priv);
 	destroy_workqueue(priv->gve_wq);
 	priv->ctrl_ops->unmap_db_bar(priv);
 	free_netdev(netdev);
@@ -2992,16 +3003,13 @@ static void gve_shutdown(struct pci_dev *pdev)
 	bool was_up = netif_running(priv->dev);
 
 	netif_device_detach(netdev);
+	disable_work_sync(&priv->service_task);
 
 	rtnl_lock();
 	netdev_lock(netdev);
-	if (was_up && gve_close(priv->dev)) {
-		/* If the dev was up, attempt to close, if close fails, reset */
-		gve_reset_and_teardown(priv, was_up);
-	} else {
-		/* If the dev wasn't up or close worked, finish tearing down */
-		gve_teardown_priv_resources(priv);
-	}
+	if (was_up)
+		gve_close(priv->dev);
+	gve_teardown_device(priv);
 	netdev_unlock(netdev);
 	rtnl_unlock();
 }
@@ -3013,16 +3021,14 @@ static int gve_suspend(struct device *dev)
 	struct gve_priv *priv = netdev_priv(netdev);
 	bool was_up = netif_running(priv->dev);
 
+	disable_work_sync(&priv->service_task);
+
 	priv->suspend_cnt++;
 	rtnl_lock();
 	netdev_lock(netdev);
-	if (was_up && gve_close(priv->dev)) {
-		/* If the dev was up, attempt to close, if close fails, reset */
-		gve_reset_and_teardown(priv, was_up);
-	} else {
-		/* If the dev wasn't up or close worked, finish tearing down */
-		gve_teardown_priv_resources(priv);
-	}
+	if (was_up)
+		gve_close(priv->dev);
+	gve_teardown_device(priv);
 	priv->up_before_suspend = was_up;
 	netdev_unlock(netdev);
 	rtnl_unlock();
@@ -3039,7 +3045,8 @@ static int gve_resume(struct device *dev)
 	priv->resume_cnt++;
 	rtnl_lock();
 	netdev_lock(netdev);
-	err = gve_reset_recovery(priv, priv->up_before_suspend);
+	enable_work(&priv->service_task);
+	err = gve_recover(priv, priv->up_before_suspend);
 	netdev_unlock(netdev);
 	rtnl_unlock();
 	return err;
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

Driver initialization and teardown involve a number of control plane
operations that need to be defined for gve_probe to operate in both
mailbox and adminq modes. This list includes:

- get_ptype_map: a mapping of packet types (L3+L4) held in RX completion
  descriptors
- configure_rss: set up default RSS configuration if the device is not
  queryable
- setup_stats_report: set up DMA region for stats report (AQ-only)
- reset_flow_rules: needed in teardown; flushes all flow rules from
  device

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 drivers/net/ethernet/google/gve/gve.h        | 12 ++++++++++
 drivers/net/ethernet/google/gve/gve_adminq.c |  7 +++---
 drivers/net/ethernet/google/gve/gve_adminq.h |  3 +--
 drivers/net/ethernet/google/gve/gve_main.c   | 24 +++++++++++++++-----
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 026d685ecaee..e0583e8cd2cd 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -833,12 +833,24 @@ struct gve_device_info {
  *                  structures stored in @priv to be used during initialization.
  * @set_num_ntfy_blks: Sets no. of vectors into @priv to be used during
  *                     initialization.
+ * @get_ptype_map: Learn packet type map from device and store it in @priv
+ * @configure_rss: Set up default RSS configuration
+ * @setup_stats_report: Set up DMA region for stats report (AdminQ only)
+ * @reset_flow_rules: Flush all flow rules from device
  */
 struct gve_ctrl_ops {
 	int (*map_db_bar)(struct gve_priv *priv);
 	void (*unmap_db_bar)(struct gve_priv *priv);
 	void (*set_num_queues)(struct gve_priv *priv);
 	int (*set_num_ntfy_blks)(struct gve_priv *priv);
+	int (*get_ptype_map)(struct gve_priv *priv);
+	int (*configure_rss)(struct gve_priv *priv,
+			     struct ethtool_rxfh_param *param);
+	int (*setup_stats_report)(struct gve_priv *priv,
+				  u64 stats_report_len,
+				  dma_addr_t stats_report_addr,
+				  u64 interval_ms); /* AQ-specific */
+	int (*reset_flow_rules)(struct gve_priv *priv);
 };
 
 struct gve_priv {
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 901673d2e264..1176e13fafc0 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1329,8 +1329,7 @@ int gve_adminq_report_nic_ts(struct gve_priv *priv,
 	return gve_adminq_execute_cmd(priv, &cmd);
 }
 
-int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
-				 struct gve_ptype_lut *ptype_lut)
+int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv)
 {
 	struct gve_ptype_map *ptype_map;
 	union gve_adminq_command cmd;
@@ -1356,9 +1355,9 @@ int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
 
 	/* Populate ptype_lut. */
 	for (i = 0; i < GVE_NUM_PTYPES; i++) {
-		ptype_lut->ptypes[i].l3_type =
+		priv->ptype_lut_dqo->ptypes[i].l3_type =
 			ptype_map->ptypes[i].l3_type;
-		ptype_lut->ptypes[i].l4_type =
+		priv->ptype_lut_dqo->ptypes[i].l4_type =
 			ptype_map->ptypes[i].l4_type;
 	}
 err:
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index fe1e8868cdfe..5e51c060e237 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -652,8 +652,7 @@ int gve_adminq_report_nic_ts(struct gve_priv *priv,
 			     dma_addr_t nic_ts_report_addr);
 
 struct gve_ptype_lut;
-int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
-				 struct gve_ptype_lut *ptype_lut);
+int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv);
 int gve_adminq_set_num_ntfy_blks(struct gve_priv *priv);
 void gve_adminq_set_num_queues(struct gve_priv *priv);
 int gve_adminq_map_db_bar(struct gve_priv *priv);
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 87388856ff6c..cc27141240fd 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -652,7 +652,8 @@ static int gve_alloc_control_plane_resources(struct gve_priv *priv)
 
 static int gve_setup_control_plane_resources(struct gve_priv *priv)
 {
-	int err = 0;
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
+	int err;
 
 	err = gve_adminq_configure_device_resources(priv,
 						    priv->counter_array_bus,
@@ -667,7 +668,7 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
 	}
 
 	if (!gve_is_gqi(priv)) {
-		err = gve_adminq_get_ptype_map_dqo(priv, priv->ptype_lut_dqo);
+		err = ops->get_ptype_map(priv);
 		if (err) {
 			dev_err(&priv->pdev->dev,
 				"Failed to get ptype map: err=%d\n", err);
@@ -689,12 +690,13 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
 		goto teardown_clock;
 	}
 
-	err = gve_adminq_report_stats(priv, priv->stats_report_len,
+	err = ops->setup_stats_report(priv, priv->stats_report_len,
 				      priv->stats_report_bus,
 				      GVE_STATS_REPORT_TIMER_PERIOD);
 	if (err)
 		dev_err(&priv->pdev->dev,
 			"Failed to report stats: err=%d\n", err);
+
 	gve_set_device_resources_ok(priv);
 	return 0;
 
@@ -718,6 +720,7 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
  */
 static void gve_teardown_control_plane_resources(struct gve_priv *priv)
 {
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	int err;
 
 	/* Tell device its resources are being freed */
@@ -727,11 +730,13 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
 			dev_err(&priv->pdev->dev,
 				"Failed to reset flow rules: err=%d\n", err);
 		/* detach the stats report */
-		err = gve_adminq_report_stats(priv, 0, 0x0, GVE_STATS_REPORT_TIMER_PERIOD);
+		err = ops->setup_stats_report(priv, 0, 0x0,
+					      GVE_STATS_REPORT_TIMER_PERIOD);
 		if (err)
 			dev_err(&priv->pdev->dev,
 				"Failed to detach stats report: err=%d\n", err);
 		gve_teardown_clock(priv);
+
 		err = gve_adminq_deconfigure_device_resources(priv);
 		if (err)
 			dev_err(&priv->pdev->dev,
@@ -1815,6 +1820,7 @@ static int gve_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 
 int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
 {
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	struct gve_rss_config *rss_config = &priv->rss_config;
 	struct ethtool_rxfh_param rxfh = {0};
 	u16 i;
@@ -1830,15 +1836,17 @@ int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
 
 	rxfh.hfunc = ETH_RSS_HASH_TOP;
 
-	return gve_adminq_configure_rss(priv, &rxfh);
+	return ops->configure_rss(priv, &rxfh);
 }
 
 int gve_flow_rules_reset(struct gve_priv *priv)
 {
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
+
 	if (!priv->max_flow_rules)
 		return 0;
 
-	return gve_adminq_reset_flow_rules(priv);
+	return ops->reset_flow_rules(priv);
 }
 
 int gve_adjust_config(struct gve_priv *priv,
@@ -2477,6 +2485,10 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.unmap_db_bar		= gve_adminq_unmap_db_bar,
 	.set_num_queues		= gve_adminq_set_num_queues,
 	.set_num_ntfy_blks	= gve_adminq_set_num_ntfy_blks,
+	.get_ptype_map		= gve_adminq_get_ptype_map_dqo,
+	.reset_flow_rules	= gve_adminq_reset_flow_rules,
+	.setup_stats_report	= gve_adminq_report_stats,
+	.configure_rss		= gve_adminq_configure_rss,
 };
 
 static int gve_init_priv(struct gve_priv *priv)
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 08/12] gve: split up notify block allocation and setup paths

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

Before this patch, notify block allocation and setup occurred in the same
method. This all occurred before gve_adminq_configure_device_resources,
which populates the irq_db_indicies array, a DMA region with BAR offsets
for MSI-X vectors.

The coming mailbox mode will require notify blocks to be set up only
after receiving the IRQ doorbell offsets, as the request does not work
with a supplied DMA buffer in the way that admin queue mode does. The
intended flow in that case would be:

1) allocate notify blocks
2) request doorbell information
3) set up MSI-X vectors based on doorbell info

This ordering also works for admin queue mode, so it will be updated to
match.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v3:
- remove redundant call to gve_teardown_clock()

 drivers/net/ethernet/google/gve/gve.h      |   2 +
 drivers/net/ethernet/google/gve/gve_main.c | 155 +++++++++++----------
 2 files changed, 86 insertions(+), 71 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index e0583e8cd2cd..f624a3e385e4 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -674,6 +674,7 @@ struct gve_notify_block {
 	struct gve_tx_ring *tx; /* tx rings on this block */
 	struct gve_rx_ring *rx; /* rx rings on this block */
 	u32 irq;
+	bool irq_requested;
 };
 
 /* Tracks allowed and current rx queue settings */
@@ -954,6 +955,7 @@ struct gve_priv {
 	u64 link_speed;
 	bool up_before_suspend; /* True if dev was up before suspend */
 
+	bool mgmt_irq_requested;
 	struct gve_ptype_lut *ptype_lut_dqo;
 
 	/* Must be a power of two. */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index cc27141240fd..d2f974a0cc87 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -428,6 +428,24 @@ int gve_napi_poll_dqo(struct napi_struct *napi, int budget)
 	return work_done;
 }
 
+static void gve_free_notify_blocks(struct gve_priv *priv)
+{
+	pci_disable_msix(priv->pdev);
+	if (priv->irq_db_indices) {
+		dma_free_coherent(&priv->pdev->dev,
+				  priv->num_ntfy_blks *
+				  sizeof(*priv->irq_db_indices),
+				  priv->irq_db_indices,
+				  priv->irq_db_indices_bus);
+		priv->irq_db_indices = NULL;
+	}
+
+	kvfree(priv->ntfy_blocks);
+	priv->ntfy_blocks = NULL;
+	kvfree(priv->msix_vectors);
+	priv->msix_vectors = NULL;
+}
+
 static const struct cpumask *gve_get_node_mask(struct gve_priv *priv)
 {
 	if (priv->numa_node == NUMA_NO_NODE)
@@ -439,11 +457,9 @@ static const struct cpumask *gve_get_node_mask(struct gve_priv *priv)
 static int gve_alloc_notify_blocks(struct gve_priv *priv)
 {
 	int num_vecs_requested = priv->num_ntfy_blks + 1;
-	const struct cpumask *node_mask;
-	unsigned int cur_cpu;
 	int vecs_enabled;
-	int i, j;
 	int err;
+	int i;
 
 	priv->msix_vectors = kvzalloc_objs(*priv->msix_vectors,
 					   num_vecs_requested);
@@ -457,7 +473,7 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 		dev_err(&priv->pdev->dev, "Could not enable min msix %d/%d\n",
 			GVE_MIN_MSIX, vecs_enabled);
 		err = vecs_enabled;
-		goto abort_with_msix_vectors;
+		goto abort;
 	}
 	if (vecs_enabled != num_vecs_requested) {
 		int new_num_ntfy_blks = (vecs_enabled - 1) & ~0x1;
@@ -480,15 +496,6 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 			priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
 	}
 
-	/* Setup Management Vector  - the last vector */
-	snprintf(priv->mgmt_msix_name, sizeof(priv->mgmt_msix_name), "gve-mgmnt@pci:%s",
-		 pci_name(priv->pdev));
-	err = request_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector,
-			  gve_mgmnt_intr, 0, priv->mgmt_msix_name, priv);
-	if (err) {
-		dev_err(&priv->pdev->dev, "Did not receive management vector.\n");
-		goto abort_with_msix_enabled;
-	}
 	priv->irq_db_indices =
 		dma_alloc_coherent(&priv->pdev->dev,
 				   priv->num_ntfy_blks *
@@ -496,15 +503,65 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 				   &priv->irq_db_indices_bus, GFP_KERNEL);
 	if (!priv->irq_db_indices) {
 		err = -ENOMEM;
-		goto abort_with_mgmt_vector;
+		goto abort;
 	}
 
 	priv->ntfy_blocks = kvzalloc(priv->num_ntfy_blks *
 				     sizeof(*priv->ntfy_blocks), GFP_KERNEL);
 	if (!priv->ntfy_blocks) {
 		err = -ENOMEM;
-		goto abort_with_irq_db_indices;
+		goto abort;
+	}
+	return 0;
+
+abort:
+	gve_free_notify_blocks(priv);
+	return err;
+}
+
+static void gve_teardown_notify_blocks(struct gve_priv *priv)
+{
+	int i;
+
+	if (!priv->ntfy_blocks)
+		return;
+
+	for (i = 0; i < priv->num_ntfy_blks; i++) {
+		struct gve_notify_block *block = &priv->ntfy_blocks[i];
+
+		if (!block->irq_requested)
+			continue;
+
+		irq_set_affinity_hint(priv->msix_vectors[i].vector,
+				      NULL);
+		free_irq(priv->msix_vectors[i].vector, block);
+		block->irq = 0;
+		block->irq_requested = false;
+	}
+
+	if (priv->mgmt_irq_requested) {
+		free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
+		priv->mgmt_irq_requested = false;
+	}
+}
+
+static int gve_setup_notify_blocks(struct gve_priv *priv)
+{
+	const struct cpumask *node_mask;
+	unsigned int cur_cpu;
+	int i;
+	int err;
+
+	/* Setup Management Vector  - the last vector */
+	snprintf(priv->mgmt_msix_name, sizeof(priv->mgmt_msix_name),
+		 "gve-mgmnt@pci:%s", pci_name(priv->pdev));
+	err = request_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector,
+			  gve_mgmnt_intr, 0, priv->mgmt_msix_name, priv);
+	if (err) {
+		dev_err(&priv->pdev->dev, "Did not receive management vector.\n");
+		return err;
 	}
+	priv->mgmt_irq_requested = true;
 
 	/* Setup the other blocks - the first n-1 vectors */
 	node_mask = gve_get_node_mask(priv);
@@ -522,9 +579,10 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 		if (err) {
 			dev_err(&priv->pdev->dev,
 				"Failed to receive msix vector %d\n", i);
-			goto abort_with_some_ntfy_blocks;
+			goto abort;
 		}
 		block->irq = priv->msix_vectors[msix_idx].vector;
+		block->irq_requested = true;
 		irq_set_affinity_and_hint(block->irq,
 					  cpumask_of(cur_cpu));
 		block->irq_db_index = &priv->irq_db_indices[i].index;
@@ -538,61 +596,12 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 			cur_cpu = cpumask_first(node_mask);
 	}
 	return 0;
-abort_with_some_ntfy_blocks:
-	for (j = 0; j < i; j++) {
-		struct gve_notify_block *block = &priv->ntfy_blocks[j];
-		int msix_idx = j;
 
-		irq_set_affinity_hint(priv->msix_vectors[msix_idx].vector,
-				      NULL);
-		free_irq(priv->msix_vectors[msix_idx].vector, block);
-		block->irq = 0;
-	}
-	kvfree(priv->ntfy_blocks);
-	priv->ntfy_blocks = NULL;
-abort_with_irq_db_indices:
-	dma_free_coherent(&priv->pdev->dev, priv->num_ntfy_blks *
-			  sizeof(*priv->irq_db_indices),
-			  priv->irq_db_indices, priv->irq_db_indices_bus);
-	priv->irq_db_indices = NULL;
-abort_with_mgmt_vector:
-	free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
-abort_with_msix_enabled:
-	pci_disable_msix(priv->pdev);
-abort_with_msix_vectors:
-	kvfree(priv->msix_vectors);
-	priv->msix_vectors = NULL;
+abort:
+	gve_teardown_notify_blocks(priv);
 	return err;
 }
 
-static void gve_free_notify_blocks(struct gve_priv *priv)
-{
-	int i;
-
-	if (!priv->msix_vectors)
-		return;
-
-	/* Free the irqs */
-	for (i = 0; i < priv->num_ntfy_blks; i++) {
-		struct gve_notify_block *block = &priv->ntfy_blocks[i];
-		int msix_idx = i;
-
-		irq_set_affinity_hint(priv->msix_vectors[msix_idx].vector,
-				      NULL);
-		free_irq(priv->msix_vectors[msix_idx].vector, block);
-		block->irq = 0;
-	}
-	free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
-	kvfree(priv->ntfy_blocks);
-	priv->ntfy_blocks = NULL;
-	dma_free_coherent(&priv->pdev->dev, priv->num_ntfy_blks *
-			  sizeof(*priv->irq_db_indices),
-			  priv->irq_db_indices, priv->irq_db_indices_bus);
-	priv->irq_db_indices = NULL;
-	pci_disable_msix(priv->pdev);
-	kvfree(priv->msix_vectors);
-	priv->msix_vectors = NULL;
-}
 
 static void gve_free_control_plane_resources(struct gve_priv *priv)
 {
@@ -749,6 +758,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
 
 static void gve_teardown_device(struct gve_priv *priv)
 {
+	gve_teardown_notify_blocks(priv);
 	gve_teardown_control_plane_resources(priv);
 	gve_adminq_free(priv);
 	/*
@@ -2471,13 +2481,16 @@ static int gve_setup_device(struct gve_priv *priv)
 
 	err = gve_alloc_control_plane_resources(priv);
 	if (err)
-		goto err;
+		return err;
+
 	err = gve_setup_control_plane_resources(priv);
 	if (err)
-		goto err;
+		return err;
+
+	err = gve_setup_notify_blocks(priv);
+	if (err)
+		return err;
 	return 0;
-err:
-	return err;
 }
 
 static const struct gve_ctrl_ops gve_adminq_ops = {
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 09/12] gve: introduce new methods to handle IRQ doorbells

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

Introduce `request_db_info` and `release_db_resources` to
`struct gve_ctrl_ops`. These encapsulate the configuration of device
resources (counter arrays and IRQ doorbell indices) which vary between
Admin Queue and Mailbox modes.

Establish the relationship between IRQ doorbell indices and notify
blocks in these new methods instead of directly in notify_block
setup/teardown methods.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v6:
- update commit message

v4:
  - propagate code changes as specified by v3.
v3:
  - move allocation of IRQ DB indices and counter array back into
    gve_alloc_control_plane_resources() from
    gve_adminq_request_db_info().
  - Similar to above, move free logic out of
    gve_adminq_free_db_resources() and rename all introduced methods
    from *free_db_resources to *release_db_resources to reflect the
    behavioral change.

 drivers/net/ethernet/google/gve/gve.h        | 10 ++
 drivers/net/ethernet/google/gve/gve_adminq.c | 37 ++++++++
 drivers/net/ethernet/google/gve/gve_adminq.h |  2 +
 drivers/net/ethernet/google/gve/gve_main.c   | 98 ++++++++++----------
 4 files changed, 97 insertions(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index f624a3e385e4..6c46c842070b 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -834,6 +834,9 @@ struct gve_device_info {
  *                  structures stored in @priv to be used during initialization.
  * @set_num_ntfy_blks: Sets no. of vectors into @priv to be used during
  *                     initialization.
+ * @request_db_info: Request and store doorbell information into @priv
+ * @release_db_resources: Release device hold on DMA memory holding doorbell
+ *			  info (AdminQ only)
  * @get_ptype_map: Learn packet type map from device and store it in @priv
  * @configure_rss: Set up default RSS configuration
  * @setup_stats_report: Set up DMA region for stats report (AdminQ only)
@@ -844,6 +847,8 @@ struct gve_ctrl_ops {
 	void (*unmap_db_bar)(struct gve_priv *priv);
 	void (*set_num_queues)(struct gve_priv *priv);
 	int (*set_num_ntfy_blks)(struct gve_priv *priv);
+	int (*request_db_info)(struct gve_priv *priv);
+	void (*release_db_resources)(struct gve_priv *priv);
 	int (*get_ptype_map)(struct gve_priv *priv);
 	int (*configure_rss)(struct gve_priv *priv,
 			     struct ethtool_rxfh_param *param);
@@ -1164,6 +1169,11 @@ static inline u32 gve_rx_idx_to_ntfy(struct gve_priv *priv, u32 queue_idx)
 	return (priv->num_ntfy_blks / 2) + queue_idx;
 }
 
+static inline u32 gve_ntfy_to_msix_idx(struct gve_priv *priv, u32 ntfy_blk_idx)
+{
+	return ntfy_blk_idx;
+}
+
 static inline bool gve_is_qpl(struct gve_priv *priv)
 {
 	return priv->queue_format == GVE_GQI_QPL_FORMAT ||
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 1176e13fafc0..b89825a85db8 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1718,3 +1718,40 @@ void gve_adminq_unmap_db_bar(struct gve_priv *priv)
 
 	pci_iounmap(pdev, priv->db_bar2);
 }
+
+int gve_adminq_request_db_info(struct gve_priv *priv)
+{
+	int err;
+	int i;
+
+	err = gve_adminq_configure_device_resources(priv,
+						    priv->counter_array_bus,
+						    priv->num_event_counters,
+						    priv->irq_db_indices_bus,
+						    priv->num_ntfy_blks);
+	if (unlikely(err)) {
+		dev_err(&priv->pdev->dev,
+			"could not setup device_resources: err=%d\n", err);
+		return -ENXIO;
+	}
+
+	for (i = 0; i < priv->num_ntfy_blks; i++)
+		priv->ntfy_blocks[i].irq_db_index =
+			&priv->irq_db_indices[i].index;
+	return 0;
+}
+
+void gve_adminq_release_db_resources(struct gve_priv *priv)
+{
+	int err;
+
+	/* Log error in deconfigure device, but don't fail. This is only ever
+	 * called as a reset is about to be triggered, so it would be redundant
+	 * to trigger a reset.
+	 */
+	err = gve_adminq_deconfigure_device_resources(priv);
+	if (err)
+		dev_err(&priv->pdev->dev,
+			"Could not deconfigure device resources: err=%d\n",
+			err);
+}
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 5e51c060e237..52172bf228eb 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -657,4 +657,6 @@ int gve_adminq_set_num_ntfy_blks(struct gve_priv *priv);
 void gve_adminq_set_num_queues(struct gve_priv *priv);
 int gve_adminq_map_db_bar(struct gve_priv *priv);
 void gve_adminq_unmap_db_bar(struct gve_priv *priv);
+int gve_adminq_request_db_info(struct gve_priv *priv);
+void gve_adminq_release_db_resources(struct gve_priv *priv);
 #endif /* _GVE_ADMINQ_H */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index d2f974a0cc87..84efcdea6be9 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -203,6 +203,30 @@ static void gve_free_counter_array(struct gve_priv *priv)
 	priv->counter_array = NULL;
 }
 
+static int gve_alloc_irq_db_indices(struct gve_priv *priv)
+{
+	priv->irq_db_indices =
+		dma_alloc_coherent(&priv->pdev->dev,
+				   priv->num_ntfy_blks *
+				   sizeof(*priv->irq_db_indices),
+				   &priv->irq_db_indices_bus, GFP_KERNEL);
+	if (!priv->irq_db_indices)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void gve_free_irq_db_indices(struct gve_priv *priv)
+{
+	if (!priv->irq_db_indices)
+		return;
+
+	dma_free_coherent(&priv->pdev->dev, priv->num_ntfy_blks *
+			  sizeof(*priv->irq_db_indices),
+			  priv->irq_db_indices, priv->irq_db_indices_bus);
+	priv->irq_db_indices = NULL;
+}
+
 /* NIC requests to report stats */
 static void gve_stats_report_task(struct work_struct *work)
 {
@@ -431,15 +455,6 @@ int gve_napi_poll_dqo(struct napi_struct *napi, int budget)
 static void gve_free_notify_blocks(struct gve_priv *priv)
 {
 	pci_disable_msix(priv->pdev);
-	if (priv->irq_db_indices) {
-		dma_free_coherent(&priv->pdev->dev,
-				  priv->num_ntfy_blks *
-				  sizeof(*priv->irq_db_indices),
-				  priv->irq_db_indices,
-				  priv->irq_db_indices_bus);
-		priv->irq_db_indices = NULL;
-	}
-
 	kvfree(priv->ntfy_blocks);
 	priv->ntfy_blocks = NULL;
 	kvfree(priv->msix_vectors);
@@ -496,24 +511,14 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 			priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
 	}
 
-	priv->irq_db_indices =
-		dma_alloc_coherent(&priv->pdev->dev,
-				   priv->num_ntfy_blks *
-				   sizeof(*priv->irq_db_indices),
-				   &priv->irq_db_indices_bus, GFP_KERNEL);
-	if (!priv->irq_db_indices) {
-		err = -ENOMEM;
-		goto abort;
-	}
-
 	priv->ntfy_blocks = kvzalloc(priv->num_ntfy_blks *
 				     sizeof(*priv->ntfy_blocks), GFP_KERNEL);
 	if (!priv->ntfy_blocks) {
 		err = -ENOMEM;
 		goto abort;
 	}
-	return 0;
 
+	return 0;
 abort:
 	gve_free_notify_blocks(priv);
 	return err;
@@ -528,13 +533,14 @@ static void gve_teardown_notify_blocks(struct gve_priv *priv)
 
 	for (i = 0; i < priv->num_ntfy_blks; i++) {
 		struct gve_notify_block *block = &priv->ntfy_blocks[i];
+		int msix_idx = gve_ntfy_to_msix_idx(priv, i);
 
 		if (!block->irq_requested)
 			continue;
 
-		irq_set_affinity_hint(priv->msix_vectors[i].vector,
+		irq_set_affinity_hint(priv->msix_vectors[msix_idx].vector,
 				      NULL);
-		free_irq(priv->msix_vectors[i].vector, block);
+		free_irq(priv->msix_vectors[msix_idx].vector, block);
 		block->irq = 0;
 		block->irq_requested = false;
 	}
@@ -563,12 +569,11 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
 	}
 	priv->mgmt_irq_requested = true;
 
-	/* Setup the other blocks - the first n-1 vectors */
 	node_mask = gve_get_node_mask(priv);
 	cur_cpu = cpumask_first(node_mask);
 	for (i = 0; i < priv->num_ntfy_blks; i++) {
 		struct gve_notify_block *block = &priv->ntfy_blocks[i];
-		int msix_idx = i;
+		int msix_idx = gve_ntfy_to_msix_idx(priv, i);
 
 		snprintf(block->name, sizeof(block->name), "gve-ntfy-blk%d@pci:%s",
 			 i, pci_name(priv->pdev));
@@ -578,14 +583,13 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
 				  IRQF_NO_AUTOEN, block->name, block);
 		if (err) {
 			dev_err(&priv->pdev->dev,
-				"Failed to receive msix vector %d\n", i);
+				"Failed to receive msix vector %d\n", msix_idx);
 			goto abort;
 		}
 		block->irq = priv->msix_vectors[msix_idx].vector;
 		block->irq_requested = true;
 		irq_set_affinity_and_hint(block->irq,
 					  cpumask_of(cur_cpu));
-		block->irq_db_index = &priv->irq_db_indices[i].index;
 
 		cur_cpu = cpumask_next(cur_cpu, node_mask);
 		/* Wrap once CPUs in the node have been exhausted, or when
@@ -602,7 +606,6 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
 	return err;
 }
 
-
 static void gve_free_control_plane_resources(struct gve_priv *priv)
 {
 	bitmap_free(priv->xsk_pools);
@@ -611,9 +614,10 @@ static void gve_free_control_plane_resources(struct gve_priv *priv)
 	kvfree(priv->ptype_lut_dqo);
 	priv->ptype_lut_dqo = NULL;
 
-	gve_free_stats_report(priv);
-	gve_free_notify_blocks(priv);
+	gve_free_irq_db_indices(priv);
 	gve_free_counter_array(priv);
+	gve_free_notify_blocks(priv);
+	gve_free_stats_report(priv);
 	gve_free_rss_config_cache(priv);
 	gve_free_flow_rule_caches(priv);
 }
@@ -626,15 +630,18 @@ static int gve_alloc_control_plane_resources(struct gve_priv *priv)
 	if (err)
 		return err;
 	err = gve_alloc_rss_config_cache(priv);
-	if (err)
-		goto abort;
-	err = gve_alloc_counter_array(priv);
 	if (err)
 		goto abort;
 	err = gve_alloc_notify_blocks(priv);
 	if (err)
 		goto abort;
 	err = gve_alloc_stats_report(priv);
+	if (err)
+		goto abort;
+	err = gve_alloc_counter_array(priv);
+	if (err)
+		goto abort;
+	err = gve_alloc_irq_db_indices(priv);
 	if (err)
 		goto abort;
 
@@ -664,15 +671,9 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
 	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	int err;
 
-	err = gve_adminq_configure_device_resources(priv,
-						    priv->counter_array_bus,
-						    priv->num_event_counters,
-						    priv->irq_db_indices_bus,
-						    priv->num_ntfy_blks);
-	if (unlikely(err)) {
-		dev_err(&priv->pdev->dev,
-			"could not setup device_resources: err=%d\n", err);
-		err = -ENXIO;
+	err = ops->request_db_info(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev, "Failed to get db info");
 		return err;
 	}
 
@@ -681,7 +682,7 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
 		if (err) {
 			dev_err(&priv->pdev->dev,
 				"Failed to get ptype map: err=%d\n", err);
-			goto deconfigure_device;
+			goto release_db_resources;
 		}
 	}
 
@@ -711,8 +712,8 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
 
 teardown_clock:
 	gve_teardown_clock(priv);
-deconfigure_device:
-	gve_adminq_deconfigure_device_resources(priv);
+release_db_resources:
+	ops->release_db_resources(priv);
 	return err;
 }
 
@@ -745,12 +746,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
 			dev_err(&priv->pdev->dev,
 				"Failed to detach stats report: err=%d\n", err);
 		gve_teardown_clock(priv);
-
-		err = gve_adminq_deconfigure_device_resources(priv);
-		if (err)
-			dev_err(&priv->pdev->dev,
-				"Could not deconfigure device resources: err=%d\n",
-				err);
+		ops->release_db_resources(priv);
 	}
 
 	gve_clear_device_resources_ok(priv);
@@ -2502,6 +2498,8 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.reset_flow_rules	= gve_adminq_reset_flow_rules,
 	.setup_stats_report	= gve_adminq_report_stats,
 	.configure_rss		= gve_adminq_configure_rss,
+	.request_db_info	= gve_adminq_request_db_info,
+	.release_db_resources	= gve_adminq_release_db_resources,
 };
 
 static int gve_init_priv(struct gve_priv *priv)
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 10/12] gve: setup and teardown management interrupts

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

Introduce control ops to setup/teardown control plane IRQs. Admin
queue-specific functionality is moved to admin queue files.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 drivers/net/ethernet/google/gve/gve.h        |  4 +++
 drivers/net/ethernet/google/gve/gve_adminq.c | 36 ++++++++++++++++++++
 drivers/net/ethernet/google/gve/gve_adminq.h |  2 ++
 drivers/net/ethernet/google/gve/gve_main.c   | 32 +++++------------
 4 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 6c46c842070b..016e85ed91a5 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -837,6 +837,8 @@ struct gve_device_info {
  * @request_db_info: Request and store doorbell information into @priv
  * @release_db_resources: Release device hold on DMA memory holding doorbell
  *			  info (AdminQ only)
+ * @setup_mgmt_irq: Setup control plane IRQ
+ * @teardown_mgmt_irq: Teardown control plane IRQ
  * @get_ptype_map: Learn packet type map from device and store it in @priv
  * @configure_rss: Set up default RSS configuration
  * @setup_stats_report: Set up DMA region for stats report (AdminQ only)
@@ -849,6 +851,8 @@ struct gve_ctrl_ops {
 	int (*set_num_ntfy_blks)(struct gve_priv *priv);
 	int (*request_db_info)(struct gve_priv *priv);
 	void (*release_db_resources)(struct gve_priv *priv);
+	int (*setup_mgmt_irq)(struct gve_priv *priv);
+	void (*teardown_mgmt_irq)(struct gve_priv *priv);
 	int (*get_ptype_map)(struct gve_priv *priv);
 	int (*configure_rss)(struct gve_priv *priv,
 			     struct ethtool_rxfh_param *param);
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index b89825a85db8..c3c10d5062f5 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1755,3 +1755,39 @@ void gve_adminq_release_db_resources(struct gve_priv *priv)
 			"Could not deconfigure device resources: err=%d\n",
 			err);
 }
+
+static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
+{
+	struct gve_priv *priv = arg;
+
+	/* Device resources are not okay; consider the interrupt spurious. */
+	if (!gve_get_device_resources_ok(priv))
+		return IRQ_NONE;
+
+	queue_work(priv->gve_wq, &priv->service_task);
+	return IRQ_HANDLED;
+}
+
+int gve_adminq_setup_mgmt_irq(struct gve_priv *priv)
+{
+	int err;
+
+	snprintf(priv->mgmt_msix_name, sizeof(priv->mgmt_msix_name),
+		 "gve-mgmnt@pci:%s", pci_name(priv->pdev));
+	err = request_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector,
+			  gve_mgmnt_intr, 0, priv->mgmt_msix_name, priv);
+	if (err)
+		return err;
+
+	priv->mgmt_irq_requested = true;
+
+	return 0;
+}
+
+void gve_adminq_teardown_mgmt_irq(struct gve_priv *priv)
+{
+	if (priv->mgmt_irq_requested) {
+		free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
+		priv->mgmt_irq_requested = false;
+	}
+}
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 52172bf228eb..0b48e9f2a82a 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -659,4 +659,6 @@ int gve_adminq_map_db_bar(struct gve_priv *priv);
 void gve_adminq_unmap_db_bar(struct gve_priv *priv);
 int gve_adminq_request_db_info(struct gve_priv *priv);
 void gve_adminq_release_db_resources(struct gve_priv *priv);
+int gve_adminq_setup_mgmt_irq(struct gve_priv *priv);
+void gve_adminq_teardown_mgmt_irq(struct gve_priv *priv);
 #endif /* _GVE_ADMINQ_H */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 84efcdea6be9..e7ebc4e0f24b 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -290,18 +290,6 @@ static void gve_free_stats_report(struct gve_priv *priv)
 	priv->stats_report = NULL;
 }
 
-static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
-{
-	struct gve_priv *priv = arg;
-
-	/* Device resources are not okay; consider the interrupt spurious. */
-	if (!gve_get_device_resources_ok(priv))
-		return IRQ_NONE;
-
-	queue_work(priv->gve_wq, &priv->service_task);
-	return IRQ_HANDLED;
-}
-
 static irqreturn_t gve_intr(int irq, void *arg)
 {
 	struct gve_notify_block *block = arg;
@@ -526,6 +514,7 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
 
 static void gve_teardown_notify_blocks(struct gve_priv *priv)
 {
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	int i;
 
 	if (!priv->ntfy_blocks)
@@ -545,29 +534,24 @@ static void gve_teardown_notify_blocks(struct gve_priv *priv)
 		block->irq_requested = false;
 	}
 
-	if (priv->mgmt_irq_requested) {
-		free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
-		priv->mgmt_irq_requested = false;
-	}
+	ops->teardown_mgmt_irq(priv);
 }
 
 static int gve_setup_notify_blocks(struct gve_priv *priv)
 {
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	const struct cpumask *node_mask;
 	unsigned int cur_cpu;
 	int i;
 	int err;
 
-	/* Setup Management Vector  - the last vector */
-	snprintf(priv->mgmt_msix_name, sizeof(priv->mgmt_msix_name),
-		 "gve-mgmnt@pci:%s", pci_name(priv->pdev));
-	err = request_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector,
-			  gve_mgmnt_intr, 0, priv->mgmt_msix_name, priv);
+	/* Setup Management Vector */
+	err = ops->setup_mgmt_irq(priv);
 	if (err) {
-		dev_err(&priv->pdev->dev, "Did not receive management vector.\n");
+		dev_err(&priv->pdev->dev,
+			"Did not receive management vector.\n");
 		return err;
 	}
-	priv->mgmt_irq_requested = true;
 
 	node_mask = gve_get_node_mask(priv);
 	cur_cpu = cpumask_first(node_mask);
@@ -2500,6 +2484,8 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.configure_rss		= gve_adminq_configure_rss,
 	.request_db_info	= gve_adminq_request_db_info,
 	.release_db_resources	= gve_adminq_release_db_resources,
+	.setup_mgmt_irq		= gve_adminq_setup_mgmt_irq,
+	.teardown_mgmt_irq	= gve_adminq_teardown_mgmt_irq,
 };
 
 static int gve_init_priv(struct gve_priv *priv)
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 11/12] gve: add ctrl ops for queue operations

[Harshitha Ramamurthy]

Add the next few control ops to

- create queues
- destroy queues

These changes are just moving existing code into ops, no functional
change.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v6:
- drop unintentional change of gve_priv from const to non-const

v5:
- remove double logging during queue creation/destruction on err also (Sashiko)

 drivers/net/ethernet/google/gve/gve.h        |  4 ++
 drivers/net/ethernet/google/gve/gve_adminq.c | 50 ++++++++++++++++++++
 drivers/net/ethernet/google/gve/gve_adminq.h |  2 +
 drivers/net/ethernet/google/gve/gve_main.c   | 43 +++--------------
 4 files changed, 63 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 016e85ed91a5..00966b6a730f 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 {
  * @configure_rss: Set up default RSS configuration
  * @setup_stats_report: Set up DMA region for stats report (AdminQ only)
  * @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.
  */
 struct gve_ctrl_ops {
 	int (*map_db_bar)(struct gve_priv *priv);
@@ -861,6 +863,8 @@ struct gve_ctrl_ops {
 				  dma_addr_t stats_report_addr,
 				  u64 interval_ms); /* AQ-specific */
 	int (*reset_flow_rules)(struct gve_priv *priv);
+	int (*create_queues)(struct gve_priv *priv);
+	int (*destroy_queues)(struct gve_priv *priv);
 };
 
 struct gve_priv {
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index c3c10d5062f5..9a0a94ebdcd8 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -943,6 +943,30 @@ static void gve_set_default_rss_sizes(struct gve_priv *priv)
 	}
 }
 
+int gve_adminq_destroy_queues(struct gve_priv *priv)
+{
+	int num_tx_queues = gve_num_tx_queues(priv);
+	int err;
+
+	err = gve_adminq_destroy_tx_queues(priv, 0, num_tx_queues);
+	if (err) {
+		netif_err(priv, drv, priv->dev,
+			  "failed to destroy tx queues\n");
+		/* This failure will trigger a reset - no need to clean up */
+		return err;
+	}
+	netif_dbg(priv, drv, priv->dev, "destroyed tx queues\n");
+	err = gve_adminq_destroy_rx_queues(priv, priv->rx_cfg.num_queues);
+	if (err) {
+		netif_err(priv, drv, priv->dev,
+			  "failed to destroy rx queues\n");
+		/* This failure will trigger a reset - no need to clean up */
+		return err;
+	}
+	netif_dbg(priv, drv, priv->dev, "destroyed rx queues\n");
+	return err;
+}
+
 static void gve_enable_supported_features(struct gve_priv *priv,
 					  u32 supported_features_mask,
 					  const struct gve_device_option_jumbo_frames
@@ -1791,3 +1815,29 @@ void gve_adminq_teardown_mgmt_irq(struct gve_priv *priv)
 		priv->mgmt_irq_requested = false;
 	}
 }
+
+int gve_adminq_create_queues(struct gve_priv *priv)
+{
+	int num_tx_queues = gve_num_tx_queues(priv);
+	int err;
+
+	err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);
+	if (err) {
+		netif_err(priv, drv, priv->dev, "failed to create %d tx queues\n",
+			  num_tx_queues);
+		return err;
+	}
+	netif_dbg(priv, drv, priv->dev, "created %d tx queues\n",
+		  num_tx_queues);
+
+	err = gve_adminq_create_rx_queues(priv, priv->rx_cfg.num_queues);
+	if (err) {
+		netif_err(priv, drv, priv->dev, "failed to create %d rx queues\n",
+			  priv->rx_cfg.num_queues);
+		return err;
+	}
+	netif_dbg(priv, drv, priv->dev, "created %d rx queues\n",
+		  priv->rx_cfg.num_queues);
+
+	return err;
+}
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 0b48e9f2a82a..d696e4932a8b 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -628,6 +628,8 @@ int gve_adminq_configure_device_resources(struct gve_priv *priv,
 					  dma_addr_t db_array_bus_addr,
 					  u32 num_ntfy_blks);
 int gve_adminq_deconfigure_device_resources(struct gve_priv *priv);
+int gve_adminq_create_queues(struct gve_priv *priv);
+int gve_adminq_destroy_queues(struct gve_priv *priv);
 int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
 int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
 int gve_adminq_create_single_rx_queue(struct gve_priv *priv, u32 queue_index);
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index e7ebc4e0f24b..c9abb9681dca 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -873,33 +873,16 @@ static int gve_unregister_qpls(struct gve_priv *priv)
 
 static int gve_create_rings(struct gve_priv *priv)
 {
-	int num_tx_queues = gve_num_tx_queues(priv);
 	int err;
 	int i;
 
-	err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);
+	err = priv->ctrl_ops->create_queues(priv);
 	if (err) {
-		netif_err(priv, drv, priv->dev, "failed to create %d tx queues\n",
-			  num_tx_queues);
 		/* This failure will trigger a reset - no need to clean
 		 * up
 		 */
 		return err;
 	}
-	netif_dbg(priv, drv, priv->dev, "created %d tx queues\n",
-		  num_tx_queues);
-
-	err = gve_adminq_create_rx_queues(priv, priv->rx_cfg.num_queues);
-	if (err) {
-		netif_err(priv, drv, priv->dev, "failed to create %d rx queues\n",
-			  priv->rx_cfg.num_queues);
-		/* This failure will trigger a reset - no need to clean
-		 * up
-		 */
-		return err;
-	}
-	netif_dbg(priv, drv, priv->dev, "created %d rx queues\n",
-		  priv->rx_cfg.num_queues);
 
 	if (gve_is_gqi(priv)) {
 		/* Rx data ring has been prefilled with packet buffers at queue
@@ -1054,26 +1037,12 @@ static int gve_queues_mem_alloc(struct gve_priv *priv,
 
 static int gve_destroy_rings(struct gve_priv *priv)
 {
-	int num_tx_queues = gve_num_tx_queues(priv);
 	int err;
 
-	err = gve_adminq_destroy_tx_queues(priv, 0, num_tx_queues);
-	if (err) {
-		netif_err(priv, drv, priv->dev,
-			  "failed to destroy tx queues\n");
-		/* This failure will trigger a reset - no need to clean up */
-		return err;
-	}
-	netif_dbg(priv, drv, priv->dev, "destroyed tx queues\n");
-	err = gve_adminq_destroy_rx_queues(priv, priv->rx_cfg.num_queues);
-	if (err) {
-		netif_err(priv, drv, priv->dev,
-			  "failed to destroy rx queues\n");
-		/* This failure will trigger a reset - no need to clean up */
-		return err;
-	}
-	netif_dbg(priv, drv, priv->dev, "destroyed rx queues\n");
-	return 0;
+	err = priv->ctrl_ops->destroy_queues(priv);
+
+	/* A failure will trigger a reset - no need to clean up */
+	return err;
 }
 
 static void gve_queues_mem_free(struct gve_priv *priv,
@@ -2486,6 +2455,8 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.release_db_resources	= gve_adminq_release_db_resources,
 	.setup_mgmt_irq		= gve_adminq_setup_mgmt_irq,
 	.teardown_mgmt_irq	= gve_adminq_teardown_mgmt_irq,
+	.create_queues		= gve_adminq_create_queues,
+	.destroy_queues		= gve_adminq_destroy_queues,
 };
 
 static int gve_init_priv(struct gve_priv *priv)
-- 
2.55.0.979.g7e5102b832-goog

[PATCH net-next v6 12/12] gve: add link status/speed ctrl ops

[Harshitha Ramamurthy]

From: Joshua Washington <joshwash@google.com>

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 <willemb@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
v3:
  - Fix kdoc typo ("set link status" -> "set link speed")
  - Copy link status into local variable instead of reading twice to
    prevent TOCTOU issue

 drivers/net/ethernet/google/gve/gve.h         |  5 ++++
 drivers/net/ethernet/google/gve/gve_adminq.c  | 10 +++++++
 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    | 26 ++++++++++++-------
 5 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 00966b6a730f..b7ce6e8632db 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -845,6 +845,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 speed into @priv->link_speed
  */
 struct gve_ctrl_ops {
 	int (*map_db_bar)(struct gve_priv *priv);
@@ -865,6 +867,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 {
@@ -966,6 +970,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/ethernet/google/gve/gve_adminq.c
index 9a0a94ebdcd8..ed64f298e80d 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1841,3 +1841,13 @@ int gve_adminq_create_queues(struct gve_priv *priv)
 
 	return err;
 }
+
+int gve_adminq_report_link_status(struct gve_priv *priv)
+{
+	u32 status;
+
+	status = ioread32be(&priv->reg_bar0->device_status);
+	WRITE_ONCE(priv->link_up,
+		   !!(GVE_DEVICE_STATUS_LINK_STATUS_MASK & status));
+	return 0;
+}
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index d696e4932a8b..8daac5fd685a 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, u64 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_flow_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 dd1c44fedc77..a3220c651438 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 = 0;
 
 	if (priv->link_speed == 0)
-		err = gve_adminq_report_link_speed(priv);
+		err = priv->ctrl_ops->report_link_speed(priv);
 
 	cmd->base.speed = priv->link_speed;
 
 	cmd->base.duplex = DUPLEX_FULL;
-
 	return err;
 }
 
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index c9abb9681dca..cc65986276be 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1501,15 +1501,18 @@ static int gve_close(struct net_device *dev)
 	return err;
 }
 
-static void gve_handle_link_status(struct gve_priv *priv, bool link_status)
+static void gve_handle_link_status(struct gve_priv *priv)
 {
+	bool link_up;
+
 	if (!gve_get_napi_enabled(priv))
 		return;
 
-	if (link_status == netif_carrier_ok(priv->dev))
+	link_up = READ_ONCE(priv->link_up);
+	if (link_up == netif_carrier_ok(priv->dev))
 		return;
 
-	if (link_status) {
+	if (link_up) {
 		netdev_info(priv->dev, "Device link is up.\n");
 		netif_carrier_on(priv->dev);
 	} else {
@@ -1536,7 +1539,6 @@ static int gve_set_xdp(struct gve_priv *priv, struct bpf_prog *prog,
 {
 	struct bpf_prog *old_prog;
 	int err = 0;
-	u32 status;
 
 	old_prog = READ_ONCE(priv->xdp_prog);
 	if (!netif_running(priv->dev)) {
@@ -1563,8 +1565,8 @@ static int gve_set_xdp(struct gve_priv *priv, struct bpf_prog *prog,
 		bpf_prog_put(old_prog);
 
 out:
-	status = 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;
 }
 
@@ -1997,11 +1999,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 = priv->ctrl_ops;
 
 	gve_turnup(priv);
-	status = 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,
@@ -2325,12 +2327,14 @@ static void gve_service_task(struct work_struct *work)
 {
 	struct gve_priv *priv = container_of(work, struct gve_priv,
 					     service_task);
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
 	u32 status = 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)
@@ -2457,6 +2461,8 @@ static const struct gve_ctrl_ops gve_adminq_ops = {
 	.teardown_mgmt_irq	= gve_adminq_teardown_mgmt_irq,
 	.create_queues		= gve_adminq_create_queues,
 	.destroy_queues		= gve_adminq_destroy_queues,
+	.report_link_status	= gve_adminq_report_link_status,
+	.report_link_speed	= gve_adminq_report_link_speed,
 };
 
 static int gve_init_priv(struct gve_priv *priv)
-- 
2.55.0.979.g7e5102b832-goog

Re: [PATCH net-next v6 01/12] gve: add struct gve_device_info to hold device properties

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Leftover fields `default_min_ring_size` and `default_num_queues` in `struct gve_priv` are completely unused and should have been removed.
--

commit 1e8912b037c461a4e97b883a09cc8e52ac72a6ed
Author: Harshitha Ramamurthy <hramamurthy@google.com>

gve: add struct gve_device_info to hold device properties

This commit introduces struct gve_device_info to encapsulate device
properties in preparation for MailboxQ support. It updates the driver logic
to populate and read from this new structure instead of setting the fields
directly in struct gve_priv.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index c280ff35ee771..021adb9108dff 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -797,6 +797,34 @@ struct gve_ptp {
>  	struct gve_priv *priv;
>  };
>  
> +struct gve_device_info {
> +	enum gve_queue_format queue_format;
> +	u16 default_tx_queues;
> +	u16 default_rx_queues;
[ ... ]
> +	bool default_min_ring_size;
> +	bool nic_timestamp_supported;
> +	bool modify_ring_size_enabled;
> +	bool cache_rss_config;
> +};
> +
>  struct gve_priv {
>  	struct net_device *dev;

[Severity: Low]
With the introduction of struct gve_device_info and the migration of the logic
to use device_info.default_min_ring_size and device_info.default_tx_queues /
device_info.default_rx_queues, should the old fields default_min_ring_size
and default_num_queues be removed from struct gve_priv?

Looking at struct gve_priv in gve.h, these fields appear to be left over:

struct gve_priv {
    ...
    bool default_min_ring_size;
    ...
    u16 default_num_queues; /* default num queues to set up */
    ...
};

They seem to be completely unused now and are left occupying memory as dead
code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909191136.3811892-1-hramamurthy@google.com?part=1

Re: [PATCH net-next v6 06/12] gve: simplify reset logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Silent state loss of AF_XDP configuration and hardware flow rules after device reset.
--

commit bcda352ad7c739e9f63ce9eefb6fb5309a234230
Author: Joshua Washington <joshwash@google.com>

gve: simplify reset logic

This commit simplifies the GVE reset logic by removing recursive resets,
avoiding resets during teardown, and decomposing allocation/teardown of
control plane resources.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -590,7 +594,22 @@ static void gve_free_notify_blocks(struct gve_priv *priv)
>  	priv->msix_vectors = NULL;
>  }
>  
> -static int gve_setup_device_resources(struct gve_priv *priv)
> +static void gve_free_control_plane_resources(struct gve_priv *priv)
> +{
> +	bitmap_free(priv->xsk_pools);
> +	priv->xsk_pools = NULL;

[Severity: High]
This is a pre-existing issue, but does this cause a silent state loss of
AF_XDP configuration after a device reset?

Because the driver clears xsk_pools without restoring the state, any active
AF_XDP applications could silently stop receiving traffic and require a
manual restart to recover after a routine reset (such as from a TX timeout
or an ethtool reset). This breaks the ABI contract for AF_XDP, making the
networking features permanently unusable after a transparent recovery.

> +
> +	kvfree(priv->ptype_lut_dqo);
> +	priv->ptype_lut_dqo = NULL;
> +
> +	gve_free_stats_report(priv);
> +	gve_free_notify_blocks(priv);
> +	gve_free_counter_array(priv);
> +	gve_free_rss_config_cache(priv);
> +	gve_free_flow_rule_caches(priv);

[Severity: High]
This is a pre-existing issue, but does this destroy the software cache of
ethtool flow rules without restoring them?

User-configured hardware flow rules appear to be permanently deleted from
both the hardware and the software cache upon a device reset. They are never
restored to the hardware, requiring manual user intervention to reapply
them.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909191136.3811892-1-hramamurthy@google.com?part=6

Re: [PATCH net-next v6 08/12] gve: split up notify block allocation and setup paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cancel_work_sync() for stats_report_task leads to NULL Pointer Dereference during device teardown.
--

commit c59c716d20178f6c3e44afb298f8d5eeb8f487be
Author: Joshua Washington <joshwash@google.com>

    gve: split up notify block allocation and setup paths
    
    Before this patch, notify block allocation and setup occurred in the same
    method. This all occurred before gve_adminq_configure_device_resources,
    which populates the irq_db_indicies array, a DMA region with BAR offsets
    for MSI-X vectors.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index cc27141240fd1..d2f974a0cc877 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -749,6 +758,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
>  
>  static void gve_teardown_device(struct gve_priv *priv)
>  {
> +	gve_teardown_notify_blocks(priv);
>  	gve_teardown_control_plane_resources(priv);

[Severity: High]
This is a pre-existing issue, but does the device teardown path properly
cancel the stats_report_task?

If a user enables stats reporting while the interface is down,
stats_report_timer is armed and enqueues stats_report_task into the
workqueue. If the device is subsequently unbound, gve_teardown_device()
calls gve_teardown_control_plane_resources(), which eventually calls
gve_free_stats_report().

In drivers/net/ethernet/google/gve/gve_main.c:gve_free_stats_report():

	timer_delete_sync(&priv->stats_report_timer);
	dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
			  priv->stats_report, priv->stats_report_bus);
	priv->stats_report = NULL;

This deletes the timer and frees the memory, but leaves the pending task in
the workqueue.

Then gve_remove() continues and flushes the workqueue:

	gve_teardown_device(priv);
	destroy_workqueue(priv->gve_wq);

When the workqueue is flushed, stats_report_task will execute
gve_handle_report_stats(), which dereferences priv->stats_report:

void gve_handle_report_stats(struct gve_priv *priv)
{
	struct stats *stats = priv->stats_report->stats;

Could this lead to a NULL pointer dereference? Should cancel_work_sync() be
called on stats_report_task before freeing priv->stats_report?

>  	gve_adminq_free(priv);
>  	/*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909191136.3811892-1-hramamurthy@google.com?part=8

Re: [PATCH net-next v6 12/12] gve: add link status/speed ctrl ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] AdminQ commands that allocate a DMA buffer, send it to the device, and free it on timeout create a DMA-after-free vulnerability.
--

commit e2461d13b564589c9ca69a35d656bc9e72b3ea2b
Author: Joshua Washington <joshwash@google.com>

gve: add link status/speed ctrl ops

This commit refactors the link status check to use a control plane operation.
It introduces new operations for retrieving the link status and speed in AQ
mode, reading from the device status register.

> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index dd1c44fedc771..a3220c6514382 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 = 0;
>  
>  	if (priv->link_speed == 0)
> -		err = gve_adminq_report_link_speed(priv);
> +		err = priv->ctrl_ops->report_link_speed(priv);

[Severity: Critical]
This is a pre-existing issue, but does gve_adminq_report_link_speed() have a
DMA-after-free vulnerability when the AdminQ command times out?

Looking at the underlying implementation in gve_adminq_report_link_speed() in
drivers/net/ethernet/google/gve/gve_adminq.c:

    err = gve_adminq_execute_cmd(priv, &gvnic_cmd);

    priv->link_speed = be64_to_cpu(*link_speed_region);
    dma_free_coherent(&priv->pdev->dev, sizeof(*link_speed_region),
                      link_speed_region, link_speed_region_bus);
    return err;

If the hardware fails to respond within the loop in gve_adminq_wait_for_cmd(),
gve_adminq_execute_cmd() returns -ENOTRECOVERABLE. The driver then
unconditionally calls dma_free_coherent() on the link_speed_region buffer
while the hardware might still be processing the request.

If the hardware is merely slow and completes the operation after the driver's
timeout, it will perform a DMA write to the freed IOVA. This IOVA may have
been reallocated by the kernel for other purposes, leading to memory
corruption.

A similar pattern appears to exist in gve_adminq_execute_extended_cmd() where
the inner_cmd DMA buffer is unconditionally freed on failure:

    err = gve_adminq_execute_cmd(priv, &cmd);

    dma_free_coherent(&priv->pdev->dev, cmd_size, inner_cmd, inner_cmd_bus);
    return err;

Could this memory corruption be triggered by local users querying link
settings via ethtool if the vNIC is temporarily unresponsive?

>  
>  	cmd->base.speed = priv->link_speed;
>  
>  	cmd->base.duplex = DUPLEX_FULL;
> -
>  	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909191136.3811892-1-hramamurthy@google.com?part=12

Re: [PATCH net-next v6 01/12] gve: add struct gve_device_info to hold device properties

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] After this patch, struct gve_priv still declares `u16
  default_num_queues; /* default num queues to set up */` (gve.h:856)…
- [Low] Stale comment: gve_probe() still states "Features might be set in
  other locations as well (such as `gve_adminq_describe_device`)."…

--

gve: add struct gve_device_info to hold device properties

This adds struct gve_device_info as a single place to hold the properties
negotiated with the device, converts gve_adminq_describe_device() and its
helpers to fill that structure, and moves the code that applies those
properties to priv/netdev into gve_init_priv().

A couple of questions on leftovers from the conversion.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index c280ff35ee771..021adb9108dff 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -797,6 +797,34 @@ struct gve_ptp {
>  	struct gve_priv *priv;
>  };
>  
> +struct gve_device_info {
> +	enum gve_queue_format queue_format;
> +	u16 default_tx_queues;
> +	u16 default_rx_queues;
> +	u16 max_tx_queues;
> +	u16 max_rx_queues;
> +	u16 default_tx_ring_size;
> +	u16 default_rx_ring_size;
> +	u16 max_tx_ring_size;
> +	u16 max_rx_ring_size;
> +	u16 min_tx_ring_size;
> +	u16 min_rx_ring_size;
> +	u16 max_mtu;
> +	u8 mac[ETH_ALEN];
> +	u16 max_rx_buffer_size;
> +	u16 header_buf_size;
> +	u32 max_flow_rules;
> +	u16 rss_key_size;
> +	u16 rss_lut_size;
> +	u16 tx_pages_per_qpl;
> +	u16 num_event_counters;
> +	u64 max_registered_pages;
> +	bool default_min_ring_size;
> +	bool nic_timestamp_supported;
> +	bool modify_ring_size_enabled;
> +	bool cache_rss_config;
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index f05f4895f4c74..512349c5517f4 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

> @@ -190,7 +190,7 @@ void gve_parse_device_option(struct gve_priv *priv,
>  
>  		/* device has not provided min ring size */
>  		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> -			priv->default_min_ring_size = true;
> +			priv->device_info.default_min_ring_size = true;
>  		break;

[ ... ]

> @@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,

[ ... ]

>  	if (dev_op_flow_steering &&
>  	    (supported_features_mask & GVE_SUP_FLOW_STEERING_MASK)) {
>  		if (dev_op_flow_steering->max_flow_rules) {
> -			priv->max_flow_rules =
> +			info->max_flow_rules =
>  				be32_to_cpu(dev_op_flow_steering->max_flow_rules);
> -			priv->dev->hw_features |= NETIF_F_NTUPLE;
>  			dev_info(&priv->pdev->dev,
>  				 "FLOW STEERING device option enabled with max rule limit of %u.\n",
> -				 priv->max_flow_rules);
> +				 info->max_flow_rules);
>  		}
>  	}

[Severity: Low]
With this hunk, the NETIF_F_NTUPLE assignment leaves the describe-device
path entirely, so is the comment in gve_probe() now stale?

drivers/net/ethernet/google/gve/gve_main.c:gve_probe() {
	/* Set default and supported features.
	 *
	 * Features might be set in other locations as well (such as
	 * `gve_adminq_describe_device`).
	 */
	dev->hw_features = NETIF_F_HIGHDMA;
	...
}

A grep of gve_adminq.c at this commit shows no remaining write to
priv->dev->hw_features (only dev_err() logging and a read of
priv->dev->features in gve_adminq_create_rx_queue()), since the only such
write moved to gve_init_priv():

	priv->max_flow_rules = device_info->max_flow_rules;
	if (priv->max_flow_rules)
		priv->dev->hw_features |= NETIF_F_NTUPLE;

Should the comment point at gve_init_priv() instead, given a second
describe-device implementation (MailboxQ) is being added on top of this?

[ ... ]

> @@ -1595,6 +1618,8 @@ int gve_set_num_ntfy_blks(struct gve_priv *priv)
>  
>  void gve_set_num_queues(struct gve_priv *priv)
>  {
> +	struct gve_device_info *device_info = &priv->device_info;
> +
>  	priv->tx_cfg.max_queues =
>  		min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
>  	priv->rx_cfg.max_queues =
> @@ -1602,10 +1627,13 @@ void gve_set_num_queues(struct gve_priv *priv)
>  
>  	priv->tx_cfg.num_queues = priv->tx_cfg.max_queues;
>  	priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
> -	if (priv->default_num_queues > 0) {
> -		priv->tx_cfg.num_queues = min_t(int, priv->default_num_queues,
> +	if (device_info->default_tx_queues > 0)
> +		priv->tx_cfg.num_queues = min_t(int,
> +						device_info->default_tx_queues,
>  						priv->tx_cfg.num_queues);
> -		priv->rx_cfg.num_queues = min_t(int, priv->default_num_queues,
> +
> +	if (device_info->default_rx_queues > 0)
> +		priv->rx_cfg.num_queues = min_t(int,
> +						device_info->default_rx_queues,
>  						priv->rx_cfg.num_queues);
> -	}
>  }

[Severity: Low]
Should the now-unused copies of these fields be dropped from struct
gve_priv in the same patch?

After this conversion, struct gve_priv still declares:

	bool default_min_ring_size;
	...
	u16 default_num_queues; /* default num queues to set up */

but nothing in drivers/net/ethernet/google/gve/ writes or reads either one
any more.  The last writer of priv->default_num_queues was
gve_set_queue_properties(), which became gve_fill_device_info() setting
device_info->default_tx_queues/default_rx_queues, and the last reader is
this gve_set_num_queues() hunk.  Similarly gve_parse_device_option() now
sets priv->device_info.default_min_ring_size and
gve_enable_supported_features() reads info->default_min_ring_size.

A grep at this commit only finds the declarations:

	drivers/net/ethernet/google/gve/gve.h
	  822:	bool default_min_ring_size;    /* gve_device_info */
	  849:	bool default_min_ring_size;    /* gve_priv, no users */
	  856:	u16 default_num_queues;        /* gve_priv, no users */

Since the leftover gve_priv fields share names with live gve_device_info
members, could later code (for instance the MailboxQ path) read
priv->default_num_queues or priv->default_min_ring_size and silently get
0/false?

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 9cc343a162712..d3882de584e36 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -2507,12 +2523,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> +	priv->max_flow_rules = device_info->max_flow_rules;
> +	if (priv->max_flow_rules)
> +		priv->dev->hw_features |= NETIF_F_NTUPLE;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 04/12] gve: introduce gve_adminq_get_device_properties()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] gve_adminq_get_device_properties()
  (drivers/net/ethernet/google/gve/gve_adminq.c:1148) bundles…

Pre-existing issues:
- [Medium] gve_adminq_verify_driver_compatibility()
  (drivers/net/ethernet/google/gve/gve_adminq.c:1264, moved verbatim…

--

gve: introduce gve_adminq_get_device_properties()

This adds gve_adminq_get_device_properties(), a helper that runs the
VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE admin queue commands in
sequence, and moves the driver-info allocation and fill-in from
gve_main.c into gve_adminq_verify_driver_compatibility().  The commit
message describes it as pure code movement with no functional change.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 046155ae7d669..f420a8e1dd3df 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

> @@ -1143,6 +1145,27 @@ int gve_adminq_describe_device(struct gve_priv *priv)
>  	return err;
>  }
>  
> +int gve_adminq_get_device_properties(struct gve_priv *priv)
> +{
> +	int err;
> +
> +	err = gve_adminq_verify_driver_compatibility(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not verify driver compatibility: err=%d\n", err);
> +		return err;
> +	}
> +
> +	/* Get the initial information we need from the device */
> +	err = gve_adminq_describe_device(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not get device information: err=%d\n", err);
> +		return err;
> +	}
> +	return 0;
> +}

[Severity: Low]
Should this helper also carry the state handling that gve_init_priv()
currently performs around the same two commands?

gve_init_priv() in gve_main.c does this before calling
gve_adminq_describe_device():

	priv->num_registered_pages = 0;

	if (skip_describe_device)
		goto setup_device;

	device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
	/* Get the initial information we need from the device */
	err = gve_adminq_describe_device(priv);

The queue_format reset matters because gve_adminq_describe_device()
consumes the incoming value:

	} else if (device_info->queue_format == GVE_GQI_RDA_FORMAT) {
		dev_info(&priv->pdev->dev,
			 "Driver is running with GQI RDA queue format.\n");

A caller that invokes gve_adminq_get_device_properties() with a stale
non-zero device_info.queue_format would keep the previous format and end
up with supported_features_mask left at 0, silently disabling jumbo
frames, buffer sizes, modify-ring, flow steering, RSS and timestamps.

For the record, this does not appear to be reachable anywhere: at this
commit gve_adminq_get_device_properties() has no callers, only the
prototype in gve_adminq.h and the definition, and gve_init_priv() still
calls gve_adminq_describe_device() directly with the reset in place.  By
the end of the series the concern is resolved rather than left latent:
the single call site in gve_probe() does

	priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
	err = gve_adminq_get_device_properties(priv);

skip_describe_device no longer exists in the driver, and
priv->num_registered_pages = 0 has moved into gve_setup_device().  Would
it be worth a note in the commit message that the surrounding state
handling stays with the caller?

> @@ -1205,20 +1228,53 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
>  	return gve_adminq_execute_cmd(priv, &cmd);
>  }
>  
> -int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> -					   u64 driver_info_len,
> -					   dma_addr_t driver_info_addr)
> +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv)
>  {
> +	struct gve_driver_info *driver_info;
>  	union gve_adminq_command cmd;
> +	dma_addr_t driver_info_bus;
> +	int err;
> +
> +	driver_info = dma_alloc_coherent(&priv->pdev->dev,
> +					 sizeof(struct gve_driver_info),
> +					 &driver_info_bus, GFP_KERNEL);
> +	if (!driver_info)
> +		return -ENOMEM;

[ ... ]

> -	return gve_adminq_execute_cmd(priv, &cmd);
> +	err = gve_adminq_execute_cmd(priv, &cmd);
> +
> +	/* It's ok if the device doesn't support this */
> +	if (err == -EOPNOTSUPP)
> +		err = 0;
> +
> +	dma_free_coherent(&priv->pdev->dev,
> +			  sizeof(struct gve_driver_info),
> +			  driver_info, driver_info_bus);
> +	return err;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but is it safe to hand
driver_info back to the DMA allocator when the admin queue timed out?

The identical sequence lived in gve_verify_driver_compatibility() in
gve_main.c before the move, so this is only being relocated here.

On the timeout path gve_adminq_kick_and_wait() has already rung the
doorbell and gives up without quiescing the queue:

	gve_adminq_kick_cmd(priv, head);
	if (!gve_adminq_wait_for_cmd(priv, head)) {
		dev_err(&priv->pdev->dev, "AQ commands timed out, need to reset AQ\n");
		priv->adminq_timeouts++;
		return -ENOTRECOVERABLE;
	}

The VERIFY_DRIVER_COMPATIBILITY descriptor still carries
driver_info_bus, so after gve_adminq_execute_cmd() returns
-ENOTRECOVERABLE the buffer is released while the device may still
fetch it.  Can the NIC read driver_info after dma_free_coherent() has
returned it to the allocator?

The same unconditional-free-after-execute pattern is used by
gve_adminq_register_page_list(), gve_adminq_execute_extended_cmd() and
gve_adminq_describe_device(), so if this is worth addressing it looks
like a separate change rather than something for this code movement.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 05/12] gve: refactor gve_init_priv for reset path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] In `gve_reset_recovery()`
  (drivers/net/ethernet/google/gve/gve_main.c:2561-2564) a `gve_open()`…

Pre-existing issues:
- [Medium] Hardware-state divergence after a failed queue bring-up in
  reset recovery: when `gve_reset_recovery()` gets an error from…

--

gve: refactor gve_init_priv for reset path

gve_init_priv() is reduced to only populating priv with device properties
so it can be called once from gve_probe(), with the new gve_adminq_init()
and gve_setup_device() helpers taking over adminq allocation and device
setup. gve_reset_recovery() now calls those helpers directly instead of
going through gve_init_priv().

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index addfd8dc1a1a2..2fe280cf7e680 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -2566,15 +2541,32 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
>  {
>  	int err;
>  
> -	err = gve_init_priv(priv, true);
> -	if (err)
> +	err = gve_adminq_init(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Failed to alloc admin queue: err=%d\n", err);
>  		goto err;
> +	}
> +
> +	err = gve_adminq_verify_driver_compatibility(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not verify driver compatibility: err=%d\n", err);
> +		goto err_free_adminq;
> +	}
> +
> +	err = gve_setup_device(priv);
> +	if (err)
> +		goto err_free_adminq;
>  	if (was_up) {
>  		err = gve_open(priv->dev);
>  		if (err)
> -			goto err;
> +			return err;
>  	}

[Severity: Low]

The gve_open() failure in gve_reset_recovery() changes from "goto err" to
"return err", so it no longer reaches the shared label:

err:
	dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
	gve_turndown(priv);
	return err;

Is dropping the "Reset failed" message and the gve_turndown(priv) call on
that path intended?  A failing gve_open() during recovery (for example
gve_queues_mem_alloc() returning -ENOMEM under memory pressure) now
returns silently to gve_reset(), which only prints its own message.

The commit message says:

    These changes are only code movement, no functional change.

Would it be worth mentioning this behaviour change above the "---" line so
it survives in git history?  The v3 changelog note "don't free device
resources if gve_open() fails in the reset path" is below the cut and does
not mention the lost diagnostic or the skipped gve_turndown().

For what it is worth, the later commit "gve: simplify reset logic"
(84559f54c263) restores both: gve_recover() sends a gve_open() failure to
its teardown_device label, which logs "Recover failed! !!! DISABLING ALL
QUEUES !!!" and calls gve_teardown_device().

[Severity: Medium]

This isn't a bug introduced by this patch, but the same "return err" path
leaves the device without any reset after a partially completed queue
bring-up, and the refactor is a good opportunity to ask about it.

When gve_open() -> gve_queues_start() fails after gve_register_qpls() or
after some queues were created, it takes its reset label, and because
GVE_PRIV_FLAGS_RESET_IN_PROGRESS is set it skips the reset and frees the
ring and QPL memory instead:

reset:
	if (gve_get_reset_in_progress(priv))
		goto stop_and_free_rings;
	gve_reset_and_teardown(priv, true);
...
stop_and_free_rings:
	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
	gve_queues_mem_remove(priv);
	return err;

gve_register_qpls() and gve_create_rings() rely on the assumption that
"this failure will trigger a reset - no need to clean up", but on this path
no gve_adminq_free()/gve_adminq_release() runs, so nothing writes
GVE_DRIVER_STATUS_RESET_MASK.

Can the NIC then be left with queues created and page lists registered that
reference DMA memory the driver has just released?  The baseline code had
the same gap, since its "goto err" only logged and called gve_turndown().

Here too the end of the series appears to close this: gve_recover() routes
the gve_open() failure to gve_teardown_device(), which calls
gve_adminq_free() and therefore gve_adminq_release().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 06/12] gve: simplify reset logic

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0

- [High] Inconsistent work quiescing across the new gve_teardown_device()
  call sites leaves priv->stats_report_task able to run after…
- [High] The new reset/close flow releases data-plane DMA before the
  hardware reset that revokes the device's access to it.
- [Medium] gve_mgmnt_intr() now returns IRQ_NONE without queueing
  priv->service_task whenever GVE_PRIV_FLAGS_DEVICE_RESOURCES_OK is…

Pre-existing issues:
- [High] After gve_reset() fails to recover (gve_recover() ->
  teardown_device), the netdev is left administratively UP (nothing…

--

Here is the report.

gve: simplify reset logic

This reworks the gve reset path: recursive resets and resets during
teardown are removed, allocation is split from device setup, and a single
gve_teardown_device() / gve_recover() pair replaces the old
gve_reset_and_teardown() / gve_reset_recovery() helpers.

A few questions on the new teardown ordering and work-item quiescing
below.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2fe280cf7e680..87388856ff6c5 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -270,6 +270,10 @@ static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
>  {
>  	struct gve_priv *priv = arg;
>  
> +	/* Device resources are not okay; consider the interrupt spurious. */
> +	if (!gve_get_device_resources_ok(priv))
> +		return IRQ_NONE;
> +
>  	queue_work(priv->gve_wq, &priv->service_task);
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Can this drop a management notification instead of latching it?

The mgmt vector is live from gve_alloc_notify_blocks()/setup_mgmt_irq()
until it is released, but device_resources_ok is only set at the end of
gve_setup_control_plane_resources() and is cleared at the very start of
gve_teardown_control_plane_resources(). So for the whole teardown window
inside gve_teardown_device(), and for gve_recover()'s
adminq_init/verify/setup_device phase, this returns IRQ_NONE without
queueing the service task.

gve_service_task() is the only reader of reg_bar0->device_status (it feeds
gve_handle_status() for the device-requested-reset bit and the link
handling), and the only other re-sample point is the
queue_work(priv->gve_wq, &priv->service_task) at the end of
gve_queues_start(). When gve_recover() runs with setup_queues == false
(was_up false, or gve_reset(priv, true) from gve_close()'s reset path),
gve_open() is never called:

	if (setup_queues) {
		err = gve_open(priv->dev);

Is a device reset request or link transition asserted in that window
recovered anywhere, or is it lost until the next unrelated open?

Since gve_reset() already brackets the teardown with
disable_work()/enable_work(&priv->service_task), would keeping the
unconditional queue_work() here be equivalent for exclusion purposes
without discarding the notification? Also, for a dedicated (non-shared)
MSI-X vector, repeatedly returning IRQ_NONE feeds the core spurious
interrupt detector.

[ ... ]

> -static void gve_trigger_reset(struct gve_priv *priv);
> -
> -static void gve_teardown_device_resources(struct gve_priv *priv)
> +/**
> + * gve_teardown_control_plane_resources() - Request the device to release any
> + * shared allocated resources.

[ ... ]

> +static void gve_teardown_device(struct gve_priv *priv)
> +{
> +	gve_teardown_control_plane_resources(priv);
> +	gve_adminq_free(priv);
> +	/*
> +	 * Free any resources shared with the device only after we have a
> +	 * guarantee that the device will not try to access such resources.
> +	 * Device commands in gve_teardown_control_plane_resources can fail, in
> +	 * which case, device resources won't be relinquished until
> +	 * gve_adminq_free is called to trigger a device reset.
> +	 */
> +	gve_free_control_plane_resources(priv);
> +}
> +

[Severity: High]
Is priv->stats_report_task guaranteed to be quiesced by the time
gve_free_control_plane_resources() -> gve_free_stats_report() frees the
coherent buffer?

gve_free_stats_report() only stops the timer and frees:

	if (!priv->stats_report)
		return;

	timer_delete_sync(&priv->stats_report_timer);
	dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
			  priv->stats_report, priv->stats_report_bus);
	priv->stats_report = NULL;

The only cancel_work_sync(&priv->stats_report_task) added by this patch is
in gve_queues_stop(), which the teardown paths only reach when the
interface was up (gve_reset() calls it under if (was_up); gve_remove(),
gve_shutdown() and gve_suspend() reach it only via gve_close()).

gve_set_priv_flags() in gve_ethtool.c arms the timer with no
netif_running() check:

	if (flags & BIT(0)) {
		mod_timer(&priv->stats_report_timer,
			  round_jiffies(jiffies +
					msecs_to_jiffies(priv->stats_report_timer_period)));
	}

so with report-stats enabled on a down interface the chain
gve_stats_report_timer() -> gve_stats_report_schedule() ->
queue_work(priv->gve_wq, &priv->stats_report_task) can leave the work
pending across gve_teardown_device(). gve_handle_report_stats() then does:

	struct stats *stats = priv->stats_report->stats;
	...
	be64_add_cpu(&priv->stats_report->written_count, 1);

which is either a write into the just-freed DMA coherent buffer or a NULL
dereference once priv->stats_report has been set to NULL. In gve_remove()
the pending item is only drained by destroy_workqueue(), i.e. after
gve_teardown_device() has already freed it.

Would it be simpler to do the cancel_work_sync(&priv->stats_report_task)
inside gve_free_stats_report() (or in gve_teardown_device()) so every call
site is covered regardless of whether the interface was up?

[ ... ]

> @@ -1435,6 +1472,12 @@ static int gve_open(struct net_device *dev)
>  	struct gve_priv *priv = netdev_priv(dev);
>  	int err;
>  
> +	if (!gve_get_device_resources_ok(priv)) {
> +		dev_err(&priv->pdev->dev,
> +			"Attempting to open netdev without resources. Device must be reset.");
> +		return -ENODEV;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch (the baseline
gve_reset_recovery() error path also left the interface up with
priv->xsk_pools == NULL after gve_teardown_priv_resources()), but since
this patch restructures exactly that path and adds the resources_ok gate
here, should the other user-reachable entry points get the same gate?

After gve_recover() takes its teardown_device label, the netdev is still
administratively up (nothing calls dev_close()), while
gve_teardown_device() has set priv->xsk_pools = NULL,
priv->ptype_lut_dqo = NULL, priv->tx/priv->rx = NULL, freed the notify
blocks, and dma_pool_free()d the adminq buffer while priv->adminq keeps
its stale pointer.

gve_xsk_pool_disable() only bounds-checks the queue id:

	if (qid >= priv->rx_cfg.num_queues)
		return -EINVAL;

	clear_bit(qid, priv->xsk_pools);

and closing an AF_XDP zero-copy socket reaches ndo_bpf(XDP_SETUP_XSK_POOL,
pool == NULL) without any netif_running() test (xp_clear_dev() /
xp_disable_drv_zc()), so this is a clear_bit() on a NULL bitmap.
gve_set_priv_flags() likewise memsets priv->stats_report->stats when
report-stats is turned off, and gve_adjust_queues()/gve_adjust_config()
issue adminq commands through the freed priv->adminq while netif_running()
is true.

The failed-recovery precondition is easy to reach now that
gve_add_flow_rule()/gve_del_flow_rule() call gve_reset(priv, false) on
-ETIME, and gve_recover() bails out to teardown_device when e.g.
gve_adminq_verify_driver_compatibility() times out.

> @@ -1451,41 +1494,16 @@ static int gve_open(struct net_device *dev)
>  	return 0;
>  }
>  
> -static int gve_queues_stop(struct gve_priv *priv)
> +static void gve_queues_stop(struct gve_priv *priv)
>  {
> -	int err;
> +	gve_unreg_xdp_info(priv);
> +	gve_drain_page_cache(priv);
>  
> -	netif_carrier_off(priv->dev);
> -	if (gve_get_device_rings_ok(priv)) {
> -		gve_turndown(priv);
> -		gve_drain_page_cache(priv);
> -		err = gve_destroy_rings(priv);
> -		if (err)
> -			goto err;
> -		err = gve_unregister_qpls(priv);
> -		if (err)
> -			goto err;
> -		gve_clear_device_rings_ok(priv);
> -	}
>  	timer_delete_sync(&priv->stats_report_timer);
> -
> -	gve_unreg_xdp_info(priv);
> +	cancel_work_sync(&priv->stats_report_task);
>  
>  	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>  	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
> -
> -	priv->interface_down_cnt++;
> -	return 0;
> -
> -err:
> -	/* This must have been called from a reset due to the rtnl lock
> -	 * so just return at this point.
> -	 */
> -	if (gve_get_reset_in_progress(priv))
> -		return err;
> -	/* Otherwise reset before returning */
> -	gve_reset_and_teardown(priv, true);
> -	return gve_reset_recovery(priv, false);
>  }
>  
>  static int gve_close(struct net_device *dev)
> @@ -1493,12 +1511,30 @@ static int gve_close(struct net_device *dev)
>  	struct gve_priv *priv = netdev_priv(dev);
>  	int err;
>  
> -	err = gve_queues_stop(priv);
> -	if (err)
> -		return err;
> +	gve_turndown(priv);
> +
> +	/* Surrender to reset if the queue destroying adminq cmds fail. Reset
> +	 * will not re-enable the interface.
> +	 */
> +	if (gve_get_device_rings_ok(priv)) {
> +		gve_clear_device_rings_ok(priv);
> +		err = gve_destroy_rings(priv);
> +		if (err)
> +			goto reset;
> +		err = gve_unregister_qpls(priv);
> +		if (err)
> +			goto reset;
> +	}
>  
> +	gve_queues_stop(priv);
>  	gve_queues_mem_remove(priv);
> +	priv->interface_down_cnt++;
>  	return 0;
> +
> +reset:
> +	gve_queues_stop(priv);
> +	err = gve_reset(priv, true);
> +	return err;
>  }

[Severity: High]
On this new reset: label, is it safe to release the ring buffers before
the hardware reset?

gve_destroy_rings() or gve_unregister_qpls() failing (typically -ETIME
from gve_adminq_kick_and_wait() -> gve_adminq_wait_for_cmd()) means the
NIC never acknowledged that it stopped using those queues. gve_queues_stop()
then does:

	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);

which for DQO returns RX buffer pages via gve_free_to_page_pool()/
gve_free_qpl_page_dqo(), dma_unmaps pending TX buffers
(gve_tx_clean_pending_packets() -> gve_unmap_packet()) and memsets the
descriptor/completion rings and q_resources.

The point at which the device is guaranteed to have stopped touching that
memory only comes later, in gve_reset() -> gve_teardown_device() ->
gve_adminq_free() -> gve_adminq_release():

	iowrite32be(GVE_DRIVER_STATUS_RESET_MASK, &priv->reg_bar0->driver_status);
	while (!(ioread32be(&priv->reg_bar0->device_status)
			& GVE_DEVICE_STATUS_DEVICE_IS_RESET)) { ... }

The pre-patch code returned from gve_queues_stop() before stopping the
rings on a destroy failure, and gve_reset_and_teardown() called
gve_trigger_reset() (which blocks until DEVICE_IS_RESET) first. Should the
hardware reset happen before gve_queues_stop() here?

The commit message applies exactly this reasoning to the control plane:

    With the new reset functionality, shared resources between the device
    and driver are not freed until after the hardware reset has completed

Does the same invariant hold for the data-plane rings and buffers on this
path?

[ ... ]

> -int gve_reset(struct gve_priv *priv, bool attempt_teardown)
> +int gve_reset(struct gve_priv *priv, bool skip_queue_setup)
>  {
>  	bool was_up = netif_running(priv->dev);
>  	int err;
>  
> +	if (gve_get_reset_in_progress(priv))
> +		return 0;
> +
>  	dev_info(&priv->pdev->dev, "Performing reset\n");
>  	gve_clear_do_reset(priv);
>  	gve_set_reset_in_progress(priv);
> -	/* If we aren't attempting to teardown normally, just go turndown and
> -	 * reset right away.
> -	 */
> -	if (!attempt_teardown) {
> +
> +	if (was_up) {
>  		gve_turndown(priv);
> -		gve_reset_and_teardown(priv, was_up);
> -	} else {
> -		/* Otherwise attempt to close normally */
> -		if (was_up) {
> -			err = gve_close(priv->dev);
> -			/* If that fails reset as we did above */
> -			if (err)
> -				gve_reset_and_teardown(priv, was_up);
> +		if (gve_get_device_rings_ok(priv)) {
> +			gve_clear_device_rings_ok(priv);
> +			gve_destroy_rings(priv);
> +			gve_unregister_qpls(priv);
>  		}
> -		/* Clean up any remaining resources */
> -		gve_teardown_priv_resources(priv);
> +		gve_queues_stop(priv);
>  	}

[Severity: High]
Same ordering question here, with the added detail that the return values
of gve_destroy_rings() and gve_unregister_qpls() are not inspected at all,
so gve_queues_stop() unmaps/recycles the ring memory unconditionally,
including for a device whose adminq is not responding.

There is a mirror of this on the recovery side: with RESET_IN_PROGRESS
set, a create-rings failure makes gve_queues_start() take
stop_and_free_rings:

	stop_and_free_rings:
		gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
		gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
		gve_queues_mem_remove(priv);

which frees the coherent rings and QPL pages even though some TX queues
may already have been created in hardware, with the reset handshake only
happening later when gve_recover() calls gve_teardown_device(). Can the
NIC still be writing into those pages at that point?

>  
> -	/* Set it all back up */
> -	err = gve_reset_recovery(priv, was_up);
> +	disable_work(&priv->service_task);
> +	gve_teardown_device(priv);
> +	gve_queues_mem_remove(priv);
> +
> +	enable_work(&priv->service_task);
> +	err = gve_recover(priv, was_up && !skip_queue_setup);
> +	if (err)
> +		dev_info(&priv->pdev->dev,
> +			 "Failed to recover in reset: %d\n", err);
> +

[Severity: High]
Only priv->service_task is disabled around gve_teardown_device() here.
When was_up is false, gve_queues_stop() (the sole
cancel_work_sync(&priv->stats_report_task) call site) is skipped, so a
stats work item queued by the timer can run concurrently with
gve_free_stats_report(). Should priv->stats_report_task be quiesced here
too?

[ ... ]

> @@ -2943,8 +2952,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	queue_work(priv->gve_wq, &priv->service_task);
>  	return 0;
>  
> -abort_with_gve_init:
> -	gve_teardown_priv_resources(priv);
> +abort_teardown_device:
> +	disable_work(&priv->stats_report_task);
> +	gve_teardown_device(priv);

[Severity: High]
Is this disabling the intended work item? During probe, the stats work has
no producer yet (its only producer is the timer armed by
gve_queues_start() or by ethtool on a registered netdev), while
priv->service_task is queueable at this point because gve_setup_device()
already set device_resources_ok and the mgmt IRQ is requested. Every other
teardown site in this patch uses disable_work_sync(&priv->service_task).

Should this be disable_work_sync(&priv->service_task) instead, or in
addition?

> @@ -2976,7 +2986,8 @@ static void gve_remove(struct pci_dev *pdev)
>  	void __iomem *reg_bar = priv->reg_bar0;
>  
>  	unregister_netdev(netdev);
> -	gve_teardown_priv_resources(priv);
> +	disable_work_sync(&priv->service_task);
> +	gve_teardown_device(priv);
>  	destroy_workqueue(priv->gve_wq);

[Severity: High]
For an interface that was already down, unregister_netdev() does not call
gve_close(), so nothing cancels priv->stats_report_task before
gve_teardown_device() frees priv->stats_report. destroy_workqueue() then
drains the pending stats work after the free. Does the ordering here need
a disable_work_sync(&priv->stats_report_task) as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 09/12] gve: introduce new methods to handle IRQ doorbells

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] Undisclosed changes in the commit message: besides the two new
  `gve_ctrl_ops` callbacks, this patch (a) adds a brand-new no-op inline…
- [Low] The new error log in `gve_setup_control_plane_resources()` —
  `dev_err(&priv->pdev->dev, "Failed to get db info");` at…
- [Low] The kernel-doc added for `struct gve_ctrl_ops` understates the
  contract of the two new callbacks.

--

gve: introduce new methods to handle IRQ doorbells

This patch adds request_db_info and release_db_resources callbacks to
struct gve_ctrl_ops so that the device-resource configuration can differ
between Admin Queue and Mailbox modes. It also moves the association of
IRQ doorbell indices with notify blocks into the new callbacks.

A few questions on the changes below.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index f624a3e385e4c..6c46c842070b7 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -834,6 +834,9 @@ struct gve_device_info {
>   *                  structures stored in @priv to be used during initialization.
>   * @set_num_ntfy_blks: Sets no. of vectors into @priv to be used during
>   *                     initialization.
> + * @request_db_info: Request and store doorbell information into @priv
> + * @release_db_resources: Release device hold on DMA memory holding doorbell
> + *			  info (AdminQ only)

[Severity: Low]
Does this kernel-doc describe the full contract of the two callbacks?

The only implementation also registers and revokes the event counter
array, not just the doorbell index array:

  gve_adminq_request_db_info()
    gve_adminq_configure_device_resources(priv,
                                         priv->counter_array_bus,
                                         priv->num_event_counters,
                                         priv->irq_db_indices_bus,
                                         priv->num_ntfy_blks);

  gve_adminq_release_db_resources()
    gve_adminq_deconfigure_device_resources(priv);

The commit message itself says the ops "encapsulate the configuration of
device resources (counter arrays and IRQ doorbell indices)", which
matches the code but not the doc.

This matters because the same patch moves gve_free_counter_array() into
gve_free_control_plane_resources(), so the counter-array coherent buffer
is now freed on the strength of release_db_resources() having told the
device to stop using it. Should the doc mention the counter array so a
future mailbox implementation does not skip that part and leave the
device DMAing into freed memory?

>   * @get_ptype_map: Learn packet type map from device and store it in @priv
>   * @configure_rss: Set up default RSS configuration
>   * @setup_stats_report: Set up DMA region for stats report (AdminQ only)

[ ... ]

> @@ -1164,6 +1169,11 @@ static inline u32 gve_rx_idx_to_ntfy(struct gve_priv *priv, u32 queue_idx)
>  	return (priv->num_ntfy_blks / 2) + queue_idx;
>  }
>  
> +static inline u32 gve_ntfy_to_msix_idx(struct gve_priv *priv, u32 ntfy_blk_idx)
> +{
> +	return ntfy_blk_idx;
> +}
> +

[Severity: Low]
Could the commit message say something about this helper? It is a new
identity function whose priv argument is unused, and it is wired into
both gve_setup_notify_blocks() and gve_teardown_notify_blocks() in this
patch, but the changelog only mentions the two new gve_ctrl_ops members.

As written it encodes the AdminQ assumption that the management vector
is the last one (priv->mgmt_msix_idx == priv->num_ntfy_blks), which is
not stated anywhere. Would a short comment help?

The same applies to the resource movement further down: the
irq_db_indices DMA buffer is moved out of gve_alloc_notify_blocks() /
gve_free_notify_blocks() into new helpers, and the alloc/free order in
gve_alloc_control_plane_resources() / gve_free_control_plane_resources()
changes. The only note about that is in the v3 changelog below the ---
line, which does not end up in git history.

>  static inline bool gve_is_qpl(struct gve_priv *priv)
>  {
>  	return priv->queue_format == GVE_GQI_QPL_FORMAT ||
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 1176e13fafc0c..b89825a85db82 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -1718,3 +1718,40 @@ void gve_adminq_unmap_db_bar(struct gve_priv *priv)
>  
>  	pci_iounmap(pdev, priv->db_bar2);
>  }
> +
> +int gve_adminq_request_db_info(struct gve_priv *priv)
> +{
> +	int err;
> +	int i;
> +
> +	err = gve_adminq_configure_device_resources(priv,
> +						    priv->counter_array_bus,
> +						    priv->num_event_counters,
> +						    priv->irq_db_indices_bus,
> +						    priv->num_ntfy_blks);
> +	if (unlikely(err)) {
> +		dev_err(&priv->pdev->dev,
> +			"could not setup device_resources: err=%d\n", err);
> +		return -ENXIO;
> +	}
> +
> +	for (i = 0; i < priv->num_ntfy_blks; i++)
> +		priv->ntfy_blocks[i].irq_db_index =
> +			&priv->irq_db_indices[i].index;
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index d2f974a0cc877..84efcdea6be96 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -203,6 +203,30 @@ static void gve_free_counter_array(struct gve_priv *priv)
>  	priv->counter_array = NULL;
>  }
>  
> +static int gve_alloc_irq_db_indices(struct gve_priv *priv)
> +{
> +	priv->irq_db_indices =
> +		dma_alloc_coherent(&priv->pdev->dev,
> +				   priv->num_ntfy_blks *
> +				   sizeof(*priv->irq_db_indices),
> +				   &priv->irq_db_indices_bus, GFP_KERNEL);
> +	if (!priv->irq_db_indices)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void gve_free_irq_db_indices(struct gve_priv *priv)
> +{
> +	if (!priv->irq_db_indices)
> +		return;
> +
> +	dma_free_coherent(&priv->pdev->dev, priv->num_ntfy_blks *
> +			  sizeof(*priv->irq_db_indices),
> +			  priv->irq_db_indices, priv->irq_db_indices_bus);
> +	priv->irq_db_indices = NULL;
> +}
> +

[ ... ]

> @@ -611,9 +614,10 @@ static void gve_free_control_plane_resources(struct gve_priv *priv)
>  	kvfree(priv->ptype_lut_dqo);
>  	priv->ptype_lut_dqo = NULL;
>  
> -	gve_free_stats_report(priv);
> -	gve_free_notify_blocks(priv);
> +	gve_free_irq_db_indices(priv);
>  	gve_free_counter_array(priv);
> +	gve_free_notify_blocks(priv);
> +	gve_free_stats_report(priv);
>  	gve_free_rss_config_cache(priv);
>  	gve_free_flow_rule_caches(priv);
>  }
> @@ -626,15 +630,18 @@ static int gve_alloc_control_plane_resources(struct gve_priv *priv)
>  	if (err)
>  		return err;
>  	err = gve_alloc_rss_config_cache(priv);
> -	if (err)
> -		goto abort;
> -	err = gve_alloc_counter_array(priv);
>  	if (err)
>  		goto abort;
>  	err = gve_alloc_notify_blocks(priv);
>  	if (err)
>  		goto abort;
>  	err = gve_alloc_stats_report(priv);
> +	if (err)
> +		goto abort;
> +	err = gve_alloc_counter_array(priv);
> +	if (err)
> +		goto abort;
> +	err = gve_alloc_irq_db_indices(priv);
>  	if (err)
>  		goto abort;
>  
> @@ -664,15 +671,9 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
>  	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
>  	int err;
>  
> -	err = gve_adminq_configure_device_resources(priv,
> -						    priv->counter_array_bus,
> -						    priv->num_event_counters,
> -						    priv->irq_db_indices_bus,
> -						    priv->num_ntfy_blks);
> -	if (unlikely(err)) {
> -		dev_err(&priv->pdev->dev,
> -			"could not setup device_resources: err=%d\n", err);
> -		err = -ENXIO;
> +	err = ops->request_db_info(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev, "Failed to get db info");
>  		return err;
>  	}

[Severity: Low]
Is the missing "\n" here intentional? Without a terminating newline the
printk record stays open for continuation, so unrelated output can be
appended to this line. The message it replaces, and the next dev_err()
in this same function, both terminate and include the error code:

	dev_err(&priv->pdev->dev,
		"Failed to get ptype map: err=%d\n", err);

The message also looks redundant, since the only implementation,
gve_adminq_request_db_info(), already logs the same failure with the
error code before returning -ENXIO.

>  
> @@ -681,7 +682,7 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
>  		if (err) {
>  			dev_err(&priv->pdev->dev,
>  				"Failed to get ptype map: err=%d\n", err);
> -			goto deconfigure_device;
> +			goto release_db_resources;
>  		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 01/12] gve: add struct gve_device_info to hold device properties

[Harshitha Ramamurthy]

On Thu, Sep 10, 2026 at 3:13 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 · Low: 2
>
> - [Low] After this patch, struct gve_priv still declares `u16
>   default_num_queues; /* default num queues to set up */` (gve.h:856)…
> - [Low] Stale comment: gve_probe() still states "Features might be set in
>   other locations as well (such as `gve_adminq_describe_device`)."…
>
> --
>
> gve: add struct gve_device_info to hold device properties
>
> This adds struct gve_device_info as a single place to hold the properties
> negotiated with the device, converts gve_adminq_describe_device() and its
> helpers to fill that structure, and moves the code that applies those
> properties to priv/netdev into gve_init_priv().
>
> A couple of questions on leftovers from the conversion.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index c280ff35ee771..021adb9108dff 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -797,6 +797,34 @@ struct gve_ptp {
> >       struct gve_priv *priv;
> >  };
> >
> > +struct gve_device_info {
> > +     enum gve_queue_format queue_format;
> > +     u16 default_tx_queues;
> > +     u16 default_rx_queues;
> > +     u16 max_tx_queues;
> > +     u16 max_rx_queues;
> > +     u16 default_tx_ring_size;
> > +     u16 default_rx_ring_size;
> > +     u16 max_tx_ring_size;
> > +     u16 max_rx_ring_size;
> > +     u16 min_tx_ring_size;
> > +     u16 min_rx_ring_size;
> > +     u16 max_mtu;
> > +     u8 mac[ETH_ALEN];
> > +     u16 max_rx_buffer_size;
> > +     u16 header_buf_size;
> > +     u32 max_flow_rules;
> > +     u16 rss_key_size;
> > +     u16 rss_lut_size;
> > +     u16 tx_pages_per_qpl;
> > +     u16 num_event_counters;
> > +     u64 max_registered_pages;
> > +     bool default_min_ring_size;
> > +     bool nic_timestamp_supported;
> > +     bool modify_ring_size_enabled;
> > +     bool cache_rss_config;
> > +};
> > +
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index f05f4895f4c74..512349c5517f4 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
>
> [ ... ]
>
> > @@ -190,7 +190,7 @@ void gve_parse_device_option(struct gve_priv *priv,
> >
> >               /* device has not provided min ring size */
> >               if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> > -                     priv->default_min_ring_size = true;
> > +                     priv->device_info.default_min_ring_size = true;
> >               break;
>
> [ ... ]
>
> > @@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,
>
> [ ... ]
>
> >       if (dev_op_flow_steering &&
> >           (supported_features_mask & GVE_SUP_FLOW_STEERING_MASK)) {
> >               if (dev_op_flow_steering->max_flow_rules) {
> > -                     priv->max_flow_rules =
> > +                     info->max_flow_rules =
> >                               be32_to_cpu(dev_op_flow_steering->max_flow_rules);
> > -                     priv->dev->hw_features |= NETIF_F_NTUPLE;
> >                       dev_info(&priv->pdev->dev,
> >                                "FLOW STEERING device option enabled with max rule limit of %u.\n",
> > -                              priv->max_flow_rules);
> > +                              info->max_flow_rules);
> >               }
> >       }
>
> [Severity: Low]
> With this hunk, the NETIF_F_NTUPLE assignment leaves the describe-device
> path entirely, so is the comment in gve_probe() now stale?
>
> drivers/net/ethernet/google/gve/gve_main.c:gve_probe() {
>         /* Set default and supported features.
>          *
>          * Features might be set in other locations as well (such as
>          * `gve_adminq_describe_device`).
>          */
>         dev->hw_features = NETIF_F_HIGHDMA;
>         ...
> }
>
> A grep of gve_adminq.c at this commit shows no remaining write to
> priv->dev->hw_features (only dev_err() logging and a read of
> priv->dev->features in gve_adminq_create_rx_queue()), since the only such
> write moved to gve_init_priv():
>
>         priv->max_flow_rules = device_info->max_flow_rules;
>         if (priv->max_flow_rules)
>                 priv->dev->hw_features |= NETIF_F_NTUPLE;
>
> Should the comment point at gve_init_priv() instead, given a second
> describe-device implementation (MailboxQ) is being added on top of this?
>
> [ ... ]
>
> > @@ -1595,6 +1618,8 @@ int gve_set_num_ntfy_blks(struct gve_priv *priv)
> >
> >  void gve_set_num_queues(struct gve_priv *priv)
> >  {
> > +     struct gve_device_info *device_info = &priv->device_info;
> > +
> >       priv->tx_cfg.max_queues =
> >               min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
> >       priv->rx_cfg.max_queues =
> > @@ -1602,10 +1627,13 @@ void gve_set_num_queues(struct gve_priv *priv)
> >
> >       priv->tx_cfg.num_queues = priv->tx_cfg.max_queues;
> >       priv->rx_cfg.num_queues = priv->rx_cfg.max_queues;
> > -     if (priv->default_num_queues > 0) {
> > -             priv->tx_cfg.num_queues = min_t(int, priv->default_num_queues,
> > +     if (device_info->default_tx_queues > 0)
> > +             priv->tx_cfg.num_queues = min_t(int,
> > +                                             device_info->default_tx_queues,
> >                                               priv->tx_cfg.num_queues);
> > -             priv->rx_cfg.num_queues = min_t(int, priv->default_num_queues,
> > +
> > +     if (device_info->default_rx_queues > 0)
> > +             priv->rx_cfg.num_queues = min_t(int,
> > +                                             device_info->default_rx_queues,
> >                                               priv->rx_cfg.num_queues);
> > -     }
> >  }
>
> [Severity: Low]
> Should the now-unused copies of these fields be dropped from struct
> gve_priv in the same patch?
>
> After this conversion, struct gve_priv still declares:
>
>         bool default_min_ring_size;
>         ...
>         u16 default_num_queues; /* default num queues to set up */
>
> but nothing in drivers/net/ethernet/google/gve/ writes or reads either one
> any more.  The last writer of priv->default_num_queues was
> gve_set_queue_properties(), which became gve_fill_device_info() setting
> device_info->default_tx_queues/default_rx_queues, and the last reader is
> this gve_set_num_queues() hunk.  Similarly gve_parse_device_option() now
> sets priv->device_info.default_min_ring_size and
> gve_enable_supported_features() reads info->default_min_ring_size.
>
> A grep at this commit only finds the declarations:
>
>         drivers/net/ethernet/google/gve/gve.h
>           822:  bool default_min_ring_size;    /* gve_device_info */
>           849:  bool default_min_ring_size;    /* gve_priv, no users */
>           856:  u16 default_num_queues;        /* gve_priv, no users */
>
> Since the leftover gve_priv fields share names with live gve_device_info
> members, could later code (for instance the MailboxQ path) read
> priv->default_num_queues or priv->default_min_ring_size and silently get
> 0/false?

No, MailboxQ won't use these fields. Since this patch was already
quite substantial, I wanted it focused on the introduction of the
struct gve_device_info and its usage in the driver. I can send a clean
up patch later for this and the comment clean-up comment above.

>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 9cc343a162712..d3882de584e36 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -2507,12 +2523,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> > +     priv->max_flow_rules = device_info->max_flow_rules;
> > +     if (priv->max_flow_rules)
> > +             priv->dev->hw_features |= NETIF_F_NTUPLE;
> > +
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 04/12] gve: introduce gve_adminq_get_device_properties()

[Harshitha Ramamurthy]

On Thu, Sep 10, 2026 at 3:13 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1
>
> - [Low] gve_adminq_get_device_properties()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:1148) bundles…
>
> Pre-existing issues:
> - [Medium] gve_adminq_verify_driver_compatibility()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:1264, moved verbatim…
>
> --
>
> gve: introduce gve_adminq_get_device_properties()
>
> This adds gve_adminq_get_device_properties(), a helper that runs the
> VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE admin queue commands in
> sequence, and moves the driver-info allocation and fill-in from
> gve_main.c into gve_adminq_verify_driver_compatibility().  The commit
> message describes it as pure code movement with no functional change.
>
> No Link: tags present in the commit header.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 046155ae7d669..f420a8e1dd3df 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
>
> [ ... ]
>
> > @@ -1143,6 +1145,27 @@ int gve_adminq_describe_device(struct gve_priv *priv)
> >       return err;
> >  }
> >
> > +int gve_adminq_get_device_properties(struct gve_priv *priv)
> > +{
> > +     int err;
> > +
> > +     err = gve_adminq_verify_driver_compatibility(priv);
> > +     if (err) {
> > +             dev_err(&priv->pdev->dev,
> > +                     "Could not verify driver compatibility: err=%d\n", err);
> > +             return err;
> > +     }
> > +
> > +     /* Get the initial information we need from the device */
> > +     err = gve_adminq_describe_device(priv);
> > +     if (err) {
> > +             dev_err(&priv->pdev->dev,
> > +                     "Could not get device information: err=%d\n", err);
> > +             return err;
> > +     }
> > +     return 0;
> > +}
>
> [Severity: Low]
> Should this helper also carry the state handling that gve_init_priv()
> currently performs around the same two commands?
>
> gve_init_priv() in gve_main.c does this before calling
> gve_adminq_describe_device():
>
>         priv->num_registered_pages = 0;
>
>         if (skip_describe_device)
>                 goto setup_device;
>
>         device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
>         /* Get the initial information we need from the device */
>         err = gve_adminq_describe_device(priv);
>
> The queue_format reset matters because gve_adminq_describe_device()
> consumes the incoming value:
>
>         } else if (device_info->queue_format == GVE_GQI_RDA_FORMAT) {
>                 dev_info(&priv->pdev->dev,
>                          "Driver is running with GQI RDA queue format.\n");
>
> A caller that invokes gve_adminq_get_device_properties() with a stale
> non-zero device_info.queue_format would keep the previous format and end
> up with supported_features_mask left at 0, silently disabling jumbo
> frames, buffer sizes, modify-ring, flow steering, RSS and timestamps.
>
> For the record, this does not appear to be reachable anywhere: at this
> commit gve_adminq_get_device_properties() has no callers, only the
> prototype in gve_adminq.h and the definition, and gve_init_priv() still
> calls gve_adminq_describe_device() directly with the reset in place.  By
> the end of the series the concern is resolved rather than left latent:
> the single call site in gve_probe() does
>
>         priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
>         err = gve_adminq_get_device_properties(priv);

Like the AI notes, when gve_adminq_get_device_properties() is actually
used later in the series, things are setup to run properly.
>
> skip_describe_device no longer exists in the driver, and
> priv->num_registered_pages = 0 has moved into gve_setup_device().  Would
> it be worth a note in the commit message that the surrounding state
> handling stays with the caller?
>
> > @@ -1205,20 +1228,53 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
> >       return gve_adminq_execute_cmd(priv, &cmd);
> >  }
> >
> > -int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> > -                                        u64 driver_info_len,
> > -                                        dma_addr_t driver_info_addr)
> > +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv)
> >  {
> > +     struct gve_driver_info *driver_info;
> >       union gve_adminq_command cmd;
> > +     dma_addr_t driver_info_bus;
> > +     int err;
> > +
> > +     driver_info = dma_alloc_coherent(&priv->pdev->dev,
> > +                                      sizeof(struct gve_driver_info),
> > +                                      &driver_info_bus, GFP_KERNEL);
> > +     if (!driver_info)
> > +             return -ENOMEM;
>
> [ ... ]
>
> > -     return gve_adminq_execute_cmd(priv, &cmd);
> > +     err = gve_adminq_execute_cmd(priv, &cmd);
> > +
> > +     /* It's ok if the device doesn't support this */
> > +     if (err == -EOPNOTSUPP)
> > +             err = 0;
> > +
> > +     dma_free_coherent(&priv->pdev->dev,
> > +                       sizeof(struct gve_driver_info),
> > +                       driver_info, driver_info_bus);
> > +     return err;
> >  }
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but is it safe to hand
> driver_info back to the DMA allocator when the admin queue timed out?
>
> The identical sequence lived in gve_verify_driver_compatibility() in
> gve_main.c before the move, so this is only being relocated here.
>
> On the timeout path gve_adminq_kick_and_wait() has already rung the
> doorbell and gives up without quiescing the queue:
>
>         gve_adminq_kick_cmd(priv, head);
>         if (!gve_adminq_wait_for_cmd(priv, head)) {
>                 dev_err(&priv->pdev->dev, "AQ commands timed out, need to reset AQ\n");
>                 priv->adminq_timeouts++;
>                 return -ENOTRECOVERABLE;
>         }
>
> The VERIFY_DRIVER_COMPATIBILITY descriptor still carries
> driver_info_bus, so after gve_adminq_execute_cmd() returns
> -ENOTRECOVERABLE the buffer is released while the device may still
> fetch it.  Can the NIC read driver_info after dma_free_coherent() has
> returned it to the allocator?

This is a non-issue. If there is a timeout and the device is not
responding, the driver resets the device and frees the AdminQ.
>
> The same unconditional-free-after-execute pattern is used by
> gve_adminq_register_page_list(), gve_adminq_execute_extended_cmd() and
> gve_adminq_describe_device(), so if this is worth addressing it looks
> like a separate change rather than something for this code movement.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 05/12] gve: refactor gve_init_priv for reset path

[Harshitha Ramamurthy]

On Thu, Sep 10, 2026 at 3:13 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1
>
> - [Low] In `gve_reset_recovery()`
>   (drivers/net/ethernet/google/gve/gve_main.c:2561-2564) a `gve_open()`…
>
> Pre-existing issues:
> - [Medium] Hardware-state divergence after a failed queue bring-up in
>   reset recovery: when `gve_reset_recovery()` gets an error from…
>
> --
>
> gve: refactor gve_init_priv for reset path
>
> gve_init_priv() is reduced to only populating priv with device properties
> so it can be called once from gve_probe(), with the new gve_adminq_init()
> and gve_setup_device() helpers taking over adminq allocation and device
> setup. gve_reset_recovery() now calls those helpers directly instead of
> going through gve_init_priv().
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index addfd8dc1a1a2..2fe280cf7e680 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -2566,15 +2541,32 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> >  {
> >       int err;
> >
> > -     err = gve_init_priv(priv, true);
> > -     if (err)
> > +     err = gve_adminq_init(priv);
> > +     if (err) {
> > +             dev_err(&priv->pdev->dev,
> > +                     "Failed to alloc admin queue: err=%d\n", err);
> >               goto err;
> > +     }
> > +
> > +     err = gve_adminq_verify_driver_compatibility(priv);
> > +     if (err) {
> > +             dev_err(&priv->pdev->dev,
> > +                     "Could not verify driver compatibility: err=%d\n", err);
> > +             goto err_free_adminq;
> > +     }
> > +
> > +     err = gve_setup_device(priv);
> > +     if (err)
> > +             goto err_free_adminq;
> >       if (was_up) {
> >               err = gve_open(priv->dev);
> >               if (err)
> > -                     goto err;
> > +                     return err;
> >       }
>
> [Severity: Low]
>
> The gve_open() failure in gve_reset_recovery() changes from "goto err" to
> "return err", so it no longer reaches the shared label:
>
> err:
>         dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
>         gve_turndown(priv);
>         return err;
>
> Is dropping the "Reset failed" message and the gve_turndown(priv) call on
> that path intended?  A failing gve_open() during recovery (for example
> gve_queues_mem_alloc() returning -ENOMEM under memory pressure) now
> returns silently to gve_reset(), which only prints its own message.
>
> The commit message says:
>
>     These changes are only code movement, no functional change.
>
> Would it be worth mentioning this behaviour change above the "---" line so
> it survives in git history?  The v3 changelog note "don't free device
> resources if gve_open() fails in the reset path" is below the cut and does
> not mention the lost diagnostic or the skipped gve_turndown().
>
> For what it is worth, the later commit "gve: simplify reset logic"
> (84559f54c263) restores both: gve_recover() sends a gve_open() failure to
> its teardown_device label, which logs "Recover failed! !!! DISABLING ALL
> QUEUES !!!" and calls gve_teardown_device().
>
> [Severity: Medium]
>
> This isn't a bug introduced by this patch, but the same "return err" path
> leaves the device without any reset after a partially completed queue
> bring-up, and the refactor is a good opportunity to ask about it.
>
> When gve_open() -> gve_queues_start() fails after gve_register_qpls() or
> after some queues were created, it takes its reset label, and because
> GVE_PRIV_FLAGS_RESET_IN_PROGRESS is set it skips the reset and frees the
> ring and QPL memory instead:
>
> reset:
>         if (gve_get_reset_in_progress(priv))
>                 goto stop_and_free_rings;
>         gve_reset_and_teardown(priv, true);
> ...
> stop_and_free_rings:
>         gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>         gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
>         gve_queues_mem_remove(priv);
>         return err;
>
> gve_register_qpls() and gve_create_rings() rely on the assumption that
> "this failure will trigger a reset - no need to clean up", but on this path
> no gve_adminq_free()/gve_adminq_release() runs, so nothing writes
> GVE_DRIVER_STATUS_RESET_MASK.
>
> Can the NIC then be left with queues created and page lists registered that
> reference DMA memory the driver has just released?  The baseline code had
> the same gap, since its "goto err" only logged and called gve_turndown().
>
> Here too the end of the series appears to close this: gve_recover() routes
> the gve_open() failure to gve_teardown_device(), which calls
> gve_adminq_free() and therefore gve_adminq_release().

As the AI notes, both the issues it has flagged are resolved in the
next patch in the series.

>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com

Re: [PATCH net-next v6 06/12] gve: simplify reset logic

[Joshua Washington]

On Thu, Sep 10, 2026 at 3:13 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0
>
> - [High] Inconsistent work quiescing across the new gve_teardown_device()
>   call sites leaves priv->stats_report_task able to run after…
> - [High] The new reset/close flow releases data-plane DMA before the
>   hardware reset that revokes the device's access to it.
> - [Medium] gve_mgmnt_intr() now returns IRQ_NONE without queueing
>   priv->service_task whenever GVE_PRIV_FLAGS_DEVICE_RESOURCES_OK is…
>
> Pre-existing issues:
> - [High] After gve_reset() fails to recover (gve_recover() ->
>   teardown_device), the netdev is left administratively UP (nothing…
>
> --
>
> Here is the report.
>
> gve: simplify reset logic
>
> This reworks the gve reset path: recursive resets and resets during
> teardown are removed, allocation is split from device setup, and a single
> gve_teardown_device() / gve_recover() pair replaces the old
> gve_reset_and_teardown() / gve_reset_recovery() helpers.
>
> A few questions on the new teardown ordering and work-item quiescing
> below.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 2fe280cf7e680..87388856ff6c5 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -270,6 +270,10 @@ static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
> >  {
> >       struct gve_priv *priv = arg;
> >
> > +     /* Device resources are not okay; consider the interrupt spurious. */
> > +     if (!gve_get_device_resources_ok(priv))
> > +             return IRQ_NONE;
> > +
> >       queue_work(priv->gve_wq, &priv->service_task);
> >       return IRQ_HANDLED;
> >  }
>
> [Severity: Medium]
> Can this drop a management notification instead of latching it?
>
> The mgmt vector is live from gve_alloc_notify_blocks()/setup_mgmt_irq()
> until it is released, but device_resources_ok is only set at the end of
> gve_setup_control_plane_resources() and is cleared at the very start of
> gve_teardown_control_plane_resources(). So for the whole teardown window
> inside gve_teardown_device(), and for gve_recover()'s
> adminq_init/verify/setup_device phase, this returns IRQ_NONE without
> queueing the service task.
>
> gve_service_task() is the only reader of reg_bar0->device_status (it feeds
> gve_handle_status() for the device-requested-reset bit and the link
> handling), and the only other re-sample point is the
> queue_work(priv->gve_wq, &priv->service_task) at the end of
> gve_queues_start(). When gve_recover() runs with setup_queues == false
> (was_up false, or gve_reset(priv, true) from gve_close()'s reset path),
> gve_open() is never called:
>
>         if (setup_queues) {
>                 err = gve_open(priv->dev);
>
> Is a device reset request or link transition asserted in that window
> recovered anywhere, or is it lost until the next unrelated open?
>
> Since gve_reset() already brackets the teardown with
> disable_work()/enable_work(&priv->service_task), would keeping the
> unconditional queue_work() here be equivalent for exclusion purposes
> without discarding the notification? Also, for a dedicated (non-shared)
> MSI-X vector, repeatedly returning IRQ_NONE feeds the core spurious
> interrupt detector.
>
> [ ... ]

The management IRQ should not be enabled until after device resources
are okay. Any interrupt that comes in before the control plane
resources are set up and the management IRQ is enabled should be
considered spurious, and ignored. However, IRQ_NONE is not the correct
return value here, as this is not a situation in which shared
interrupts are returned. Will convert to IRQ_HANDLED.

>
> > -static void gve_trigger_reset(struct gve_priv *priv);
> > -
> > -static void gve_teardown_device_resources(struct gve_priv *priv)
> > +/**
> > + * gve_teardown_control_plane_resources() - Request the device to release any
> > + * shared allocated resources.
>
> [ ... ]
>
> > +static void gve_teardown_device(struct gve_priv *priv)
> > +{
> > +     gve_teardown_control_plane_resources(priv);
> > +     gve_adminq_free(priv);
> > +     /*
> > +      * Free any resources shared with the device only after we have a
> > +      * guarantee that the device will not try to access such resources.
> > +      * Device commands in gve_teardown_control_plane_resources can fail, in
> > +      * which case, device resources won't be relinquished until
> > +      * gve_adminq_free is called to trigger a device reset.
> > +      */
> > +     gve_free_control_plane_resources(priv);
> > +}
> > +
>
> [Severity: High]
> Is priv->stats_report_task guaranteed to be quiesced by the time
> gve_free_control_plane_resources() -> gve_free_stats_report() frees the
> coherent buffer?
>
> gve_free_stats_report() only stops the timer and frees:
>
>         if (!priv->stats_report)
>                 return;
>
>         timer_delete_sync(&priv->stats_report_timer);
>         dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
>                           priv->stats_report, priv->stats_report_bus);
>         priv->stats_report = NULL;
>
> The only cancel_work_sync(&priv->stats_report_task) added by this patch is
> in gve_queues_stop(), which the teardown paths only reach when the
> interface was up (gve_reset() calls it under if (was_up); gve_remove(),
> gve_shutdown() and gve_suspend() reach it only via gve_close()).
>
> gve_set_priv_flags() in gve_ethtool.c arms the timer with no
> netif_running() check:
>
>         if (flags & BIT(0)) {
>                 mod_timer(&priv->stats_report_timer,
>                           round_jiffies(jiffies +
>                                         msecs_to_jiffies(priv->stats_report_timer_period)));
>         }
>
> so with report-stats enabled on a down interface the chain
> gve_stats_report_timer() -> gve_stats_report_schedule() ->
> queue_work(priv->gve_wq, &priv->stats_report_task) can leave the work
> pending across gve_teardown_device(). gve_handle_report_stats() then does:
>
>         struct stats *stats = priv->stats_report->stats;
>         ...
>         be64_add_cpu(&priv->stats_report->written_count, 1);
>
> which is either a write into the just-freed DMA coherent buffer or a NULL
> dereference once priv->stats_report has been set to NULL. In gve_remove()
> the pending item is only drained by destroy_workqueue(), i.e. after
> gve_teardown_device() has already freed it.
>
> Would it be simpler to do the cancel_work_sync(&priv->stats_report_task)
> inside gve_free_stats_report() (or in gve_teardown_device()) so every call
> site is covered regardless of whether the interface was up?

Ack. Will do this in v7.

>
> [ ... ]
>
> > @@ -1435,6 +1472,12 @@ static int gve_open(struct net_device *dev)
> >       struct gve_priv *priv = netdev_priv(dev);
> >       int err;
> >
> > +     if (!gve_get_device_resources_ok(priv)) {
> > +             dev_err(&priv->pdev->dev,
> > +                     "Attempting to open netdev without resources. Device must be reset.");
> > +             return -ENODEV;
> > +     }
> > +
>
> [Severity: High]
> This isn't a bug introduced by this patch (the baseline
> gve_reset_recovery() error path also left the interface up with
> priv->xsk_pools == NULL after gve_teardown_priv_resources()), but since
> this patch restructures exactly that path and adds the resources_ok gate
> here, should the other user-reachable entry points get the same gate?
>
> After gve_recover() takes its teardown_device label, the netdev is still
> administratively up (nothing calls dev_close()), while
> gve_teardown_device() has set priv->xsk_pools = NULL,
> priv->ptype_lut_dqo = NULL, priv->tx/priv->rx = NULL, freed the notify
> blocks, and dma_pool_free()d the adminq buffer while priv->adminq keeps
> its stale pointer.
>
> gve_xsk_pool_disable() only bounds-checks the queue id:
>
>         if (qid >= priv->rx_cfg.num_queues)
>                 return -EINVAL;
>
>         clear_bit(qid, priv->xsk_pools);
>
> and closing an AF_XDP zero-copy socket reaches ndo_bpf(XDP_SETUP_XSK_POOL,
> pool == NULL) without any netif_running() test (xp_clear_dev() /
> xp_disable_drv_zc()), so this is a clear_bit() on a NULL bitmap.
> gve_set_priv_flags() likewise memsets priv->stats_report->stats when
> report-stats is turned off, and gve_adjust_queues()/gve_adjust_config()
> issue adminq commands through the freed priv->adminq while netif_running()
> is true.
>
> The failed-recovery precondition is easy to reach now that
> gve_add_flow_rule()/gve_del_flow_rule() call gve_reset(priv, false) on
> -ETIME, and gve_recover() bails out to teardown_device when e.g.
> gve_adminq_verify_driver_compatibility() times out.
>
> > @@ -1451,41 +1494,16 @@ static int gve_open(struct net_device *dev)
> >       return 0;
> >  }
> >
> > -static int gve_queues_stop(struct gve_priv *priv)
> > +static void gve_queues_stop(struct gve_priv *priv)
> >  {
> > -     int err;
> > +     gve_unreg_xdp_info(priv);
> > +     gve_drain_page_cache(priv);
> >
> > -     netif_carrier_off(priv->dev);
> > -     if (gve_get_device_rings_ok(priv)) {
> > -             gve_turndown(priv);
> > -             gve_drain_page_cache(priv);
> > -             err = gve_destroy_rings(priv);
> > -             if (err)
> > -                     goto err;
> > -             err = gve_unregister_qpls(priv);
> > -             if (err)
> > -                     goto err;
> > -             gve_clear_device_rings_ok(priv);
> > -     }
> >       timer_delete_sync(&priv->stats_report_timer);
> > -
> > -     gve_unreg_xdp_info(priv);
> > +     cancel_work_sync(&priv->stats_report_task);
> >
> >       gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
> >       gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
> > -
> > -     priv->interface_down_cnt++;
> > -     return 0;
> > -
> > -err:
> > -     /* This must have been called from a reset due to the rtnl lock
> > -      * so just return at this point.
> > -      */
> > -     if (gve_get_reset_in_progress(priv))
> > -             return err;
> > -     /* Otherwise reset before returning */
> > -     gve_reset_and_teardown(priv, true);
> > -     return gve_reset_recovery(priv, false);
> >  }
> >
> >  static int gve_close(struct net_device *dev)
> > @@ -1493,12 +1511,30 @@ static int gve_close(struct net_device *dev)
> >       struct gve_priv *priv = netdev_priv(dev);
> >       int err;
> >
> > -     err = gve_queues_stop(priv);
> > -     if (err)
> > -             return err;
> > +     gve_turndown(priv);
> > +
> > +     /* Surrender to reset if the queue destroying adminq cmds fail. Reset
> > +      * will not re-enable the interface.
> > +      */
> > +     if (gve_get_device_rings_ok(priv)) {
> > +             gve_clear_device_rings_ok(priv);
> > +             err = gve_destroy_rings(priv);
> > +             if (err)
> > +                     goto reset;
> > +             err = gve_unregister_qpls(priv);
> > +             if (err)
> > +                     goto reset;
> > +     }
> >
> > +     gve_queues_stop(priv);
> >       gve_queues_mem_remove(priv);
> > +     priv->interface_down_cnt++;
> >       return 0;
> > +
> > +reset:
> > +     gve_queues_stop(priv);
> > +     err = gve_reset(priv, true);
> > +     return err;
> >  }
>
> [Severity: High]
> On this new reset: label, is it safe to release the ring buffers before
> the hardware reset?
>
> gve_destroy_rings() or gve_unregister_qpls() failing (typically -ETIME
> from gve_adminq_kick_and_wait() -> gve_adminq_wait_for_cmd()) means the
> NIC never acknowledged that it stopped using those queues. gve_queues_stop()
> then does:
>
>         gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>         gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
>
> which for DQO returns RX buffer pages via gve_free_to_page_pool()/
> gve_free_qpl_page_dqo(), dma_unmaps pending TX buffers
> (gve_tx_clean_pending_packets() -> gve_unmap_packet()) and memsets the
> descriptor/completion rings and q_resources.
>
> The point at which the device is guaranteed to have stopped touching that
> memory only comes later, in gve_reset() -> gve_teardown_device() ->
> gve_adminq_free() -> gve_adminq_release():
>
>         iowrite32be(GVE_DRIVER_STATUS_RESET_MASK, &priv->reg_bar0->driver_status);
>         while (!(ioread32be(&priv->reg_bar0->device_status)
>                         & GVE_DEVICE_STATUS_DEVICE_IS_RESET)) { ... }
>
> The pre-patch code returned from gve_queues_stop() before stopping the
> rings on a destroy failure, and gve_reset_and_teardown() called
> gve_trigger_reset() (which blocks until DEVICE_IS_RESET) first. Should the
> hardware reset happen before gve_queues_stop() here?
>
> The commit message applies exactly this reasoning to the control plane:
>
>     With the new reset functionality, shared resources between the device
>     and driver are not freed until after the hardware reset has completed
>
> Does the same invariant hold for the data-plane rings and buffers on this
> path?
>

This is a good catch. Will remove the gve_queues_stop() from
gve_queues_start(). Because netif_running() is guaranteed to be true
when calling gve_queues_start(), was_up will be true in gve_reset().
Rely on this, reorder the teardown operations to do

```
gve_teardown_device()
if (was_up) { gve_queues_stop() }
gve_queues_mem_remove()
```

To ensure that the device is not holding onto reseources when
gve_queues_stop() is performed.

> [ ... ]
>
> > -int gve_reset(struct gve_priv *priv, bool attempt_teardown)
> > +int gve_reset(struct gve_priv *priv, bool skip_queue_setup)
> >  {
> >       bool was_up = netif_running(priv->dev);
> >       int err;
> >
> > +     if (gve_get_reset_in_progress(priv))
> > +             return 0;
> > +
> >       dev_info(&priv->pdev->dev, "Performing reset\n");
> >       gve_clear_do_reset(priv);
> >       gve_set_reset_in_progress(priv);
> > -     /* If we aren't attempting to teardown normally, just go turndown and
> > -      * reset right away.
> > -      */
> > -     if (!attempt_teardown) {
> > +
> > +     if (was_up) {
> >               gve_turndown(priv);
> > -             gve_reset_and_teardown(priv, was_up);
> > -     } else {
> > -             /* Otherwise attempt to close normally */
> > -             if (was_up) {
> > -                     err = gve_close(priv->dev);
> > -                     /* If that fails reset as we did above */
> > -                     if (err)
> > -                             gve_reset_and_teardown(priv, was_up);
> > +             if (gve_get_device_rings_ok(priv)) {
> > +                     gve_clear_device_rings_ok(priv);
> > +                     gve_destroy_rings(priv);
> > +                     gve_unregister_qpls(priv);
> >               }
> > -             /* Clean up any remaining resources */
> > -             gve_teardown_priv_resources(priv);
> > +             gve_queues_stop(priv);
> >       }
>
> [Severity: High]
> Same ordering question here, with the added detail that the return values
> of gve_destroy_rings() and gve_unregister_qpls() are not inspected at all,
> so gve_queues_stop() unmaps/recycles the ring memory unconditionally,
> including for a device whose adminq is not responding.
>
> There is a mirror of this on the recovery side: with RESET_IN_PROGRESS
> set, a create-rings failure makes gve_queues_start() take
> stop_and_free_rings:
>
>         stop_and_free_rings:
>                 gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>                 gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
>                 gve_queues_mem_remove(priv);
>
> which frees the coherent rings and QPL pages even though some TX queues
> may already have been created in hardware, with the reset handshake only
> happening later when gve_recover() calls gve_teardown_device(). Can the
> NIC still be writing into those pages at that point?

Addressed above.


>
> >
> > -     /* Set it all back up */
> > -     err = gve_reset_recovery(priv, was_up);
> > +     disable_work(&priv->service_task);
> > +     gve_teardown_device(priv);
> > +     gve_queues_mem_remove(priv);
> > +
> > +     enable_work(&priv->service_task);
> > +     err = gve_recover(priv, was_up && !skip_queue_setup);
> > +     if (err)
> > +             dev_info(&priv->pdev->dev,
> > +                      "Failed to recover in reset: %d\n", err);
> > +
>
> [Severity: High]
> Only priv->service_task is disabled around gve_teardown_device() here.
> When was_up is false, gve_queues_stop() (the sole
> cancel_work_sync(&priv->stats_report_task) call site) is skipped, so a
> stats work item queued by the timer can run concurrently with
> gve_free_stats_report(). Should priv->stats_report_task be quiesced here
> too?

Fixed by canceling the work as part of freeing stats report.

>
> [ ... ]
>
> > @@ -2943,8 +2952,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       queue_work(priv->gve_wq, &priv->service_task);
> >       return 0;
> >
> > -abort_with_gve_init:
> > -     gve_teardown_priv_resources(priv);
> > +abort_teardown_device:
> > +     disable_work(&priv->stats_report_task);
> > +     gve_teardown_device(priv);
>
> [Severity: High]
> Is this disabling the intended work item? During probe, the stats work has
> no producer yet (its only producer is the timer armed by
> gve_queues_start() or by ethtool on a registered netdev), while
> priv->service_task is queueable at this point because gve_setup_device()
> already set device_resources_ok and the mgmt IRQ is requested. Every other
> teardown site in this patch uses disable_work_sync(&priv->service_task).
>
> Should this be disable_work_sync(&priv->service_task) instead, or in
> addition?

Yes, this should be the service task being disabled.

>
> > @@ -2976,7 +2986,8 @@ static void gve_remove(struct pci_dev *pdev)
> >       void __iomem *reg_bar = priv->reg_bar0;
> >
> >       unregister_netdev(netdev);
> > -     gve_teardown_priv_resources(priv);
> > +     disable_work_sync(&priv->service_task);
> > +     gve_teardown_device(priv);
> >       destroy_workqueue(priv->gve_wq);
>
> [Severity: High]
> For an interface that was already down, unregister_netdev() does not call
> gve_close(), so nothing cancels priv->stats_report_task before
> gve_teardown_device() frees priv->stats_report. destroy_workqueue() then
> drains the pending stats work after the free. Does the ordering here need
> a disable_work_sync(&priv->stats_report_task) as well?
>

As mentioned above, when freeing the stats report, the timer will be
deleted, and the work will be canceled. With those fixes, the stats
report task won't have any queued work.

> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com


-- Josh