* [PATCH net-next v9 01/12] gve: add struct gve_device_info to hold device properties
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-10-02 10:06 ` netdev-bot+sashiko
2026-09-30 19:04 ` [PATCH net-next v9 02/12] gve: introduce control plane operations structure Harshitha Ramamurthy
` (10 subsequent siblings)
11 siblings, 1 reply; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 validates the final max mtu:
the mtu from the device descriptor, or the jumbo frames device
option value when it is present. Previously only the descriptor
mtu was validated. 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>
---
v9:
- update commit message about gve_set_mtu to satisfy Sashiko
v6:
- update commit message to call out change in device 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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v9 01/12] gve: add struct gve_device_info to hold device properties
2026-09-30 19:04 ` [PATCH net-next v9 01/12] gve: add struct gve_device_info to hold device properties Harshitha Ramamurthy
@ 2026-10-02 10:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-10-02 10:06 UTC (permalink / raw)
To: hramamurthy
Cc: netdev, joshwash, andrew+netdev, davem, edumazet, kuba, pabeni,
ast, daniel, hawk, john.fastabend, sdf, jordanrhee, willemb,
nktgrg, maolson, thostet, jacob.e.keller, debarghyak, kees, bpf,
linux-kernel, przemyslaw.kitszel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] Dead and unused struct fields after the refactor.
- [Low] Stale comment in gve_probe().
- [Low] The commit message does not mention a behaviour change in
gve_set_buf_sizes().
--
gve: add struct gve_device_info to hold device properties
This patch adds struct gve_device_info to hold the properties negotiated
with the device. gve_adminq_describe_device() now fills that struct
instead of writing directly into gve_priv. The helpers that copy these
properties into priv and the netdev now run from gve_init_priv(), so the
upcoming MailboxQ mode can reuse them.
> 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;
[Severity: Low]
Are max_tx_queues and max_rx_queues meant to be used somewhere?
gve_fill_device_info() doesn't set them, and nothing reads them.
gve_set_num_queues() still takes its limits from priv->tx_cfg.max_queues
and priv->rx_cfg.max_queues. gve_probe() reads those from BAR0:
priv->tx_cfg.max_queues =
min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
If a later path such as MailboxQ fills device_info->max_*_queues, would
that value be ignored without any warning?
Every reader and writer of these gve_priv fields has also moved to
device_info, but the old declarations are still there:
struct gve_priv {
...
bool default_min_ring_size;
...
u16 default_num_queues; /* default num queues to set up */
...
};
Nothing assigns them any more, so they are always 0. The comment still
describes default_num_queues as if it holds a real value. Could these
fields be removed as part of this change?
> + u16 default_tx_ring_size;
[ ... ]
> 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
[ ... ]
> @@ -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;
[Severity: Low]
This isn't a bug, but NETIF_F_NTUPLE is now set in gve_init_priv(), so
gve_adminq_describe_device() no longer changes netdev features.
The comment above the netdev feature setup in gve_probe() still names
gve_adminq_describe_device as another place where features might be set.
Should it name gve_init_priv() instead?
[ ... ]
> 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
> @@ -2398,57 +2398,71 @@ static const struct xdp_metadata_ops gve_xdp_metadata_ops = {
[ ... ]
> +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;
> +}
[Severity: Low]
The commit message leaves out a behaviour change here. It says the only
change that isn't a refactor is the MTU validation.
Before this patch, gve_enable_supported_features() copied the
BUFFER_SIZES option into priv whenever the option was present, zeros
included:
priv->max_rx_buffer_size =
be16_to_cpu(dev_op_buffer_sizes->packet_buffer_size);
priv->header_buf_size =
be16_to_cpu(dev_op_buffer_sizes->header_buffer_size);
With the new nonzero guards, a device that sends packet_buffer_size == 0
now leaves priv->max_rx_buffer_size at the default that gve_probe() sets:
priv->max_rx_buffer_size = GVE_DEFAULT_RX_BUFFER_SIZE;
Before the patch it would have been 0. This value is the upper limit
used when choosing the DQO packet_buffer_size and when checking ethtool
rx-buf-len.
For header_buf_size, a negotiated 0 can no longer clear a nonzero value
already in priv. Today this code in gve_init_priv() only runs from
probe, where priv is zeroed, so nothing changes yet. It still doesn't
fit the goal of device_info being the one source of negotiated state.
Should the guards be dropped, or should the commit message mention this
change?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v9 02/12] gve: introduce control plane operations structure
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 01/12] gve: add struct gve_device_info to hold device properties Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 03/12] gve: introduce ctrl ops to set vectors and Qs Harshitha Ramamurthy
` (9 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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(®_bar->driver_version);
/* Get max queues to alloc etherdev */
max_tx_queues = ioread32be(®_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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 03/12] gve: introduce ctrl ops to set vectors and Qs
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 01/12] gve: add struct gve_device_info to hold device properties Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 02/12] gve: introduce control plane operations structure Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 04/12] gve: introduce gve_adminq_get_device_properties() Harshitha Ramamurthy
` (8 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 04/12] gve: introduce gve_adminq_get_device_properties()
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (2 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 03/12] gve: introduce ctrl ops to set vectors and Qs Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 05/12] gve: refactor gve_init_priv for reset path Harshitha Ramamurthy
` (7 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 unnecessary __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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 05/12] gve: refactor gve_init_priv for reset path
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (3 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 04/12] gve: introduce gve_adminq_get_device_properties() Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-10-02 10:06 ` netdev-bot+sashiko
2026-09-30 19:04 ` [PATCH net-next v9 06/12] gve: simplify reset logic Harshitha Ramamurthy
` (6 subsequent siblings)
11 siblings, 1 reply; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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(®_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(®_bar->driver_version);
/* Get max queues to alloc etherdev */
max_tx_queues = ioread32be(®_bar->max_tx_queues);
max_rx_queues = ioread32be(®_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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v9 05/12] gve: refactor gve_init_priv for reset path
2026-09-30 19:04 ` [PATCH net-next v9 05/12] gve: refactor gve_init_priv for reset path Harshitha Ramamurthy
@ 2026-10-02 10:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-10-02 10:06 UTC (permalink / raw)
To: hramamurthy
Cc: netdev, joshwash, andrew+netdev, davem, edumazet, kuba, pabeni,
ast, daniel, hawk, john.fastabend, sdf, jordanrhee, willemb,
nktgrg, maolson, thostet, jacob.e.keller, debarghyak, kees, bpf,
linux-kernel, przemyslaw.kitszel
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit message says "These changes are only code movement,
no functional change".
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a hard reset
leaks the AdminQ dma_pool and its buffer.
--
gve: refactor gve_init_priv for reset path
This patch splits gve_init_priv() so that gve_probe() now does the AdminQ
setup (the new gve_adminq_init()) and the device property negotiation
(gve_adminq_get_device_properties()). Device setup moves into a new
gve_setup_device() helper. gve_reset_recovery() now calls
gve_adminq_init(), gve_adminq_verify_driver_compatibility() and
gve_setup_device() directly, instead of gve_init_priv(priv, true).
> 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);
[Severity: Medium]
This isn't a bug introduced by this patch, but does a hard reset leak the
AdminQ dma_pool and its buffer here?
A hard reset is gve_reset(priv, false), or gve_reset() after gve_close()
fails. Either way it goes through:
gve_reset_and_teardown()
gve_trigger_reset()
gve_adminq_release()
gve_clear_admin_queue_ok(priv);
gve_teardown_priv_resources()
gve_adminq_free()
gve_adminq_free() then returns early, because ADMIN_QUEUE_OK is already
clear:
void gve_adminq_free(struct gve_priv *priv)
{
if (!gve_get_admin_queue_ok(priv))
return;
gve_adminq_release(priv);
dma_pool_free(priv->adminq_pool, priv->adminq, priv->adminq_bus_addr);
dma_pool_destroy(priv->adminq_pool);
...
}
So dma_pool_free() and dma_pool_destroy() never run. Then
gve_adminq_init() here calls gve_adminq_alloc(), which puts new
allocations in priv->adminq_pool and priv->adminq. The old pool and
buffer are lost.
The same thing seems to happen when gve_teardown_device_resources() calls
gve_trigger_reset() after a failed AdminQ command.
The baseline gve_init_priv(priv, true) path leaked in the same way. The
later "gve: simplify reset logic" patch in this series appears to fix it.
That patch makes gve_adminq_release() static and calls it only from
gve_adminq_free(). It also stops gve_adminq_release() from clearing
ADMIN_QUEUE_OK and removes gve_trigger_reset().
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Failed to alloc admin queue: err=%d\n", err);
> goto err;
> + }
> +
[ ... ]
> if (was_up) {
> err = gve_open(priv->dev);
> if (err)
> - goto err;
> + return err;
[Severity: Medium]
The commit message says:
These changes are only code movement, no functional change.
Is that accurate? The patch seems to change behaviour in a few places.
First, gve_adminq_init() writes the version string before it allocates
the AdminQ:
gve_adminq_write_version(®_bar->driver_version);
return gve_adminq_alloc(priv);
gve_reset_recovery() now calls this helper. That means the driver version
string is written to the BAR0 driver_version register on every reset,
resume and recovery. Before this patch, gve_write_version() ran only once,
from gve_probe(). gve_recover() still does this at the end of the series.
Second, the order of device accesses in gve_probe() changes. The version
write now comes after the max_tx_queues/max_rx_queues ioread32be() reads
and after alloc_etherdev_mqs(). AdminQ registration,
VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE now run before ctrl_ops is
set, before map_db_bar() and before alloc_ordered_workqueue().
Third, when gve_open() fails here, the old goto err path logged "Reset
failed! !!! DISABLING ALL QUEUES !!!" and called gve_turndown(). Now the
function just returns. Neither gve_reset() nor gve_resume() logs the
error, so a failed re-open after reset or resume leaves nothing in the log.
The only place this is mentioned is the v3 changelog below the ---
line, and that text is dropped when the patch is applied. The changelog
says "don't free device resources if gve_open() fails in the reset path".
The baseline err label did not free device resources either, though.
The later "gve: simplify reset logic" patch adds a "Failed to start
queues ... DISABLING ALL QUEUES" message in gve_recover(). At this commit,
though, the message is missing.
Could the commit message describe these changes, instead of calling the
patch only code movement?
> }
> return 0;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v9 06/12] gve: simplify reset logic
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (4 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 05/12] gve: refactor gve_init_priv for reset path Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-10-02 10:06 ` netdev-bot+sashiko
2026-09-30 19:04 ` [PATCH net-next v9 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences Harshitha Ramamurthy
` (5 subsequent siblings)
11 siblings, 1 reply; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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. Resets triggered by methods as part of the reset path
are implicitly ignored because gve_reset() now checks if there is a
reset in progress before proceeding.
2) Removing resets during the teardown portion of reset. This is partly
covered by removing recursive resets, but the primary goal in this
case is to allow the driver to complete teardown in a more direct
manner. Before this patch, gve_close() when called as part of
gve_shutdown, could end up triggering a hardware reset, then attempt
to close again. In such a case, destroying hardware queues would
inevitably fail, causing a loop. gve_close() is no longer called
directly in gve_reset() breaking any possibility of this loop.
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>
---
v9:
- use netif_device_detach() in gve_recover() error path to block
ethtool calls
- cancel the PTP worker before releasing device resources and call
gve_teardown_clock() only after the device has been reset.
- use gve_open() to bring queues back up in gve_recover() holding
onto the control plane on failure
- update commit message
v8:
- call gve_queues_stop() directly in gve_queues_start to handle XDP info
unregistration
- remove gve_queues_mem_remove() from gve_queues_start(); depend on
caller to call instead. This involves moving function definitions
around to avoid forward declaration.
- update commit description regarding teardown-related resets
- revert gve_mgmt_intr behavior
- handle TOCTOU when checking for reset
- record all configurations in priv before sending an AQ command that
can result in reset
v7:
- Return IRQ_HANDLED instead of IRQ_NONE in gve_mgmnt_intr()
- disable service task instead of stats report task for gve_probe() error
- cancel stats report in gve_free_stats_report()
- extract gve_teardown_control_plane_resouces() and gve_adminq_free() into
new function gve_reset_device() to ensure the device reset is triggered before
stopping queues
- don't call gve_queues_stop() in the gve_close() error path since the
gve_reset() takes care of that.
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 | 531 +++++++++---------
6 files changed, 298 insertions(+), 262 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..3ca0f8dba683 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -261,6 +261,7 @@ static void gve_free_stats_report(struct gve_priv *priv)
return;
timer_delete_sync(&priv->stats_report_timer);
+ cancel_work_sync(&priv->stats_report_task);
dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
priv->stats_report, priv->stats_report_bus);
priv->stats_report = NULL;
@@ -590,7 +591,80 @@ 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_tx_get_curr_alloc_cfg(struct gve_priv *priv,
+ struct gve_tx_alloc_rings_cfg *cfg)
+{
+ cfg->qcfg = &priv->tx_cfg;
+ cfg->raw_addressing = !gve_is_qpl(priv);
+ cfg->ring_size = priv->tx_desc_cnt;
+ cfg->pages_per_qpl = priv->tx_pages_per_qpl;
+ cfg->num_xdp_rings = cfg->qcfg->num_xdp_queues;
+ cfg->tx = priv->tx;
+}
+
+static void gve_rx_get_curr_alloc_cfg(struct gve_priv *priv,
+ struct gve_rx_alloc_rings_cfg *cfg)
+{
+ cfg->qcfg_rx = &priv->rx_cfg;
+ cfg->qcfg_tx = &priv->tx_cfg;
+ cfg->raw_addressing = !gve_is_qpl(priv);
+ cfg->enable_header_split = priv->header_split_enabled;
+ cfg->ring_size = priv->rx_desc_cnt;
+ cfg->pages_per_qpl = priv->rx_pages_per_qpl;
+ cfg->packet_buffer_size = priv->rx_cfg.packet_buffer_size;
+ cfg->rx = priv->rx;
+ cfg->xdp = !!cfg->qcfg_tx->num_xdp_queues;
+}
+
+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)
+{
+ gve_tx_get_curr_alloc_cfg(priv, tx_alloc_cfg);
+ gve_rx_get_curr_alloc_cfg(priv, rx_alloc_cfg);
+}
+
+static void gve_queues_mem_free(struct gve_priv *priv,
+ struct gve_tx_alloc_rings_cfg *tx_cfg,
+ struct gve_rx_alloc_rings_cfg *rx_cfg)
+{
+ if (gve_is_gqi(priv)) {
+ gve_tx_free_rings_gqi(priv, tx_cfg);
+ gve_rx_free_rings_gqi(priv, rx_cfg);
+ } else {
+ gve_tx_free_rings_dqo(priv, tx_cfg);
+ gve_rx_free_rings_dqo(priv, rx_cfg);
+ }
+}
+
+static void gve_queues_mem_remove(struct gve_priv *priv)
+{
+ struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
+ struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
+
+ gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+ gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+ priv->tx = NULL;
+ priv->rx = NULL;
+}
+
+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_teardown_clock(priv);
+ 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 +673,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 +718,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 +741,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 deconfigure_device;
}
err = gve_adminq_report_stats(priv, priv->stats_report_len,
@@ -658,67 +753,77 @@ static int gve_setup_device_resources(struct gve_priv *priv)
gve_set_device_resources_ok(priv);
return 0;
-abort_with_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;
+ if (priv->ptp)
+ ptp_cancel_worker_sync(priv->ptp->clock);
+
/* 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);
- }
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);
}
+/**
+ * gve_reset_device() - Reset the device
+ *
+ * @priv: Pointer to the GVE private device data structure.
+ *
+ * Once this returns, the device is guaranteed not to access any memory it
+ * shares with the driver, so it is safe for the caller to recycle it. Device
+ * commands in gve_teardown_control_plane_resources() can fail, in which case
+ * the hardware reset triggered by gve_adminq_free() is the only such
+ * guarantee.
+ */
+static void gve_reset_device(struct gve_priv *priv)
+{
+ gve_teardown_control_plane_resources(priv);
+ gve_adminq_free(priv);
+}
+
+static void gve_teardown_device(struct gve_priv *priv)
+{
+ gve_reset_device(priv);
+ /* Free any resources shared with the device only after we have a
+ * guarantee that the device will not try to access such resources.
+ */
+ gve_free_control_plane_resources(priv);
+ gve_queues_mem_remove(priv);
+}
+
static int gve_unregister_qpl(struct gve_priv *priv,
struct gve_queue_page_list *qpl)
{
@@ -916,17 +1021,6 @@ static void gve_init_sync_stats(struct gve_priv *priv)
u64_stats_init(&priv->rx[i].statss);
}
-static void gve_tx_get_curr_alloc_cfg(struct gve_priv *priv,
- struct gve_tx_alloc_rings_cfg *cfg)
-{
- cfg->qcfg = &priv->tx_cfg;
- cfg->raw_addressing = !gve_is_qpl(priv);
- cfg->ring_size = priv->tx_desc_cnt;
- cfg->pages_per_qpl = priv->tx_pages_per_qpl;
- cfg->num_xdp_rings = cfg->qcfg->num_xdp_queues;
- cfg->tx = priv->tx;
-}
-
static void gve_tx_stop_rings(struct gve_priv *priv, int num_rings)
{
int i;
@@ -1044,19 +1138,6 @@ static int gve_destroy_rings(struct gve_priv *priv)
return 0;
}
-static void gve_queues_mem_free(struct gve_priv *priv,
- struct gve_tx_alloc_rings_cfg *tx_cfg,
- struct gve_rx_alloc_rings_cfg *rx_cfg)
-{
- if (gve_is_gqi(priv)) {
- gve_tx_free_rings_gqi(priv, tx_cfg);
- gve_rx_free_rings_gqi(priv, rx_cfg);
- } else {
- gve_tx_free_rings_dqo(priv, tx_cfg);
- gve_rx_free_rings_dqo(priv, rx_cfg);
- }
-}
-
int gve_alloc_page(struct gve_priv *priv, struct device *dev,
struct page **page, dma_addr_t *dma,
enum dma_data_direction dir, gfp_t gfp_flags)
@@ -1157,8 +1238,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,37 +1348,16 @@ 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);
}
-static void gve_rx_get_curr_alloc_cfg(struct gve_priv *priv,
- struct gve_rx_alloc_rings_cfg *cfg)
-{
- cfg->qcfg_rx = &priv->rx_cfg;
- cfg->qcfg_tx = &priv->tx_cfg;
- cfg->raw_addressing = !gve_is_qpl(priv);
- cfg->enable_header_split = priv->header_split_enabled;
- cfg->ring_size = priv->rx_desc_cnt;
- cfg->pages_per_qpl = priv->rx_pages_per_qpl;
- cfg->packet_buffer_size = priv->rx_cfg.packet_buffer_size;
- cfg->rx = priv->rx;
- cfg->xdp = !!cfg->qcfg_tx->num_xdp_queues;
-}
-
-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)
-{
- gve_tx_get_curr_alloc_cfg(priv, tx_alloc_cfg);
- gve_rx_get_curr_alloc_cfg(priv, rx_alloc_cfg);
-}
-
static void gve_rx_start_ring(struct gve_priv *priv, int i)
{
if (gve_is_gqi(priv))
@@ -1335,15 +1393,16 @@ static void gve_rx_stop_rings(struct gve_priv *priv, int num_rings)
gve_rx_stop_ring(priv, i);
}
-static void gve_queues_mem_remove(struct gve_priv *priv)
+static void gve_queues_stop(struct gve_priv *priv)
{
- struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
- struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
+ gve_unreg_xdp_info(priv);
+ gve_drain_page_cache(priv);
- gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
- gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
- priv->tx = NULL;
- priv->rx = NULL;
+ timer_delete_sync(&priv->stats_report_timer);
+ 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);
}
/* The passed-in queue memory is stored into priv and the queues are made live.
@@ -1368,6 +1427,8 @@ static int gve_queues_start(struct gve_priv *priv,
priv->rx_desc_cnt = rx_alloc_cfg->ring_size;
priv->tx_pages_per_qpl = tx_alloc_cfg->pages_per_qpl;
priv->rx_pages_per_qpl = rx_alloc_cfg->pages_per_qpl;
+ priv->header_split_enabled = rx_alloc_cfg->enable_header_split;
+ priv->rx_cfg.packet_buffer_size = rx_alloc_cfg->packet_buffer_size;
gve_tx_start_rings(priv, gve_num_tx_queues(priv));
gve_rx_start_rings(priv, rx_alloc_cfg->qcfg_rx->num_queues);
@@ -1375,14 +1436,14 @@ static int gve_queues_start(struct gve_priv *priv,
err = netif_set_real_num_tx_queues(dev, priv->tx_cfg.num_queues);
if (err)
- goto stop_and_free_rings;
+ goto stop_rings;
err = netif_set_real_num_rx_queues(dev, priv->rx_cfg.num_queues);
if (err)
- goto stop_and_free_rings;
+ goto stop_rings;
err = gve_reg_xdp_info(priv, dev);
if (err)
- goto stop_and_free_rings;
+ goto stop_rings;
if (rx_alloc_cfg->reset_rss) {
err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
@@ -1394,9 +1455,6 @@ static int gve_queues_start(struct gve_priv *priv,
if (err)
goto reset;
- priv->header_split_enabled = rx_alloc_cfg->enable_header_split;
- priv->rx_cfg.packet_buffer_size = rx_alloc_cfg->packet_buffer_size;
-
err = gve_create_rings(priv);
if (err)
goto reset;
@@ -1415,16 +1473,14 @@ 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 */
- return err;
-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);
+ goto stop_rings;
+
+ /* Attempt to reset. If reset is successful, gve_queues_start was
+ * successful with the new config.
+ */
+ return gve_reset(priv, false);
+stop_rings:
+ gve_queues_stop(priv);
return err;
}
@@ -1435,70 +1491,55 @@ 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);
if (err)
return err;
- /* No need to free on error: ownership of resources is lost after
- * calling gve_queues_start.
- */
err = gve_queues_start(priv, &tx_alloc_cfg, &rx_alloc_cfg);
- if (err)
+ if (err) {
+ gve_queues_mem_remove(priv);
return err;
+ }
return 0;
}
-static int gve_queues_stop(struct gve_priv *priv)
+static int gve_close(struct net_device *dev)
{
+ struct gve_priv *priv = netdev_priv(dev);
int err;
- netif_carrier_off(priv->dev);
+ 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_turndown(priv);
- gve_drain_page_cache(priv);
+ gve_clear_device_rings_ok(priv);
err = gve_destroy_rings(priv);
if (err)
- goto err;
+ goto reset;
err = gve_unregister_qpls(priv);
if (err)
- goto err;
- gve_clear_device_rings_ok(priv);
+ goto reset;
}
- timer_delete_sync(&priv->stats_report_timer);
-
- gve_unreg_xdp_info(priv);
-
- gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
- gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
+ gve_queues_stop(priv);
+ gve_queues_mem_remove(priv);
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)
-{
- struct gve_priv *priv = netdev_priv(dev);
- int err;
-
- err = gve_queues_stop(priv);
- if (err)
- return err;
-
- gve_queues_mem_remove(priv);
- return 0;
+reset:
+ err = gve_reset(priv, true);
+ return err;
}
static void gve_handle_link_status(struct gve_priv *priv, bool link_status)
@@ -1833,9 +1874,7 @@ int gve_adjust_config(struct gve_priv *priv,
if (err) {
netif_err(priv, drv, priv->dev,
"Adjust config failed to start new queues, !!! DISABLING ALL QUEUES !!!\n");
- /* No need to free on error: ownership of resources is lost after
- * calling gve_queues_start.
- */
+ gve_queues_mem_remove(priv);
gve_turndown(priv);
return err;
}
@@ -2149,8 +2188,11 @@ static int gve_set_features(struct net_device *netdev,
}
if ((netdev->features & NETIF_F_NTUPLE) && !(features & NETIF_F_NTUPLE)) {
err = gve_flow_rules_reset(priv);
- if (err)
+ if (err) {
+ if (err == -ETIME)
+ gve_schedule_reset(priv);
goto revert_features;
+ }
}
return 0;
@@ -2235,7 +2277,8 @@ static void gve_handle_reset(struct gve_priv *priv)
if (gve_get_do_reset(priv)) {
rtnl_lock();
netdev_lock(priv->dev);
- gve_reset(priv, false);
+ if (gve_get_do_reset(priv))
+ gve_reset(priv, false);
netdev_unlock(priv->dev);
rtnl_unlock();
}
@@ -2421,25 +2464,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 +2549,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 +2557,79 @@ 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) {
+ /* On failure, hold on to the control plane to give a
+ * chance for the queues to be brought up later.
+ */
err = gve_open(priv->dev);
- if (err)
+ if (err) {
+ dev_err(&priv->pdev->dev,
+ "Failed to start queues: err=%d, !!! DISABLING ALL QUEUES !!!\n",
+ err);
return err;
+ }
}
+
+ /* undo any detach from an earlier failure */
+ netif_device_attach(priv->dev);
+
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: err=%d, detaching device\n",
+ err);
+ netif_device_detach(priv->dev);
+ 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);
}
- /* Set it all back up */
- err = gve_reset_recovery(priv, was_up);
+ disable_work(&priv->service_task);
+ gve_reset_device(priv);
+ gve_queues_stop(priv);
+ gve_queues_mem_remove(priv);
+ gve_free_control_plane_resources(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 +2956,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 +2964,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 +2972,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_sync(&priv->service_task);
+ gve_teardown_device(priv);
abort_with_wq:
destroy_workqueue(priv->gve_wq);
@@ -2976,7 +3006,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);
@@ -2991,17 +3022,16 @@ static void gve_shutdown(struct pci_dev *pdev)
struct gve_priv *priv = netdev_priv(netdev);
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);
+
+ /* detach here because gve_close() might attach in recovery */
+ netif_device_detach(netdev);
+ gve_teardown_device(priv);
netdev_unlock(netdev);
rtnl_unlock();
}
@@ -3013,16 +3043,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 +3067,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v9 06/12] gve: simplify reset logic
2026-09-30 19:04 ` [PATCH net-next v9 06/12] gve: simplify reset logic Harshitha Ramamurthy
@ 2026-10-02 10:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-10-02 10:06 UTC (permalink / raw)
To: hramamurthy
Cc: netdev, joshwash, andrew+netdev, davem, edumazet, kuba, pabeni,
ast, daniel, hawk, john.fastabend, sdf, jordanrhee, willemb,
nktgrg, maolson, thostet, jacob.e.keller, debarghyak, kees, bpf,
linux-kernel, przemyslaw.kitszel
Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.
Critical: 0 · High: 1 (3) · Medium: 2 (3) · Low: 5
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] If gve_open() fails inside gve_recover(priv, true), queue memory
that the device may still reference is freed and the device is never…
- [Medium] In gve_recover(), when setup_queues is true and gve_open()
fails, the function returns early and skips netif_device_attach(),…
- [Medium] On a running DQO RDA interface with XDP loaded,
gve_xsk_pool_enable() sets the xsk_pools bit, registers the pool and…
- [Low] gve_reset() now has a single graceful path.
- [Low] When gve_recover() fails (gve_adminq_init,
verify_driver_compatibility or gve_setup_device), it now calls…
- [Low] The patch fixes a real unbounded recursion in the baseline:
gve_adminq_configure_flow_rule() called gve_reset(priv, true) on…
- [Low] Item 2 of the commit message says that, before this patch,
gve_close() during gve_shutdown could trigger a hardware reset and…
- [Low] The comment above gve_queues_start() still says 'No memory is
allocated.
Pre-existing issues:
- [High] gve_adjust_config() allocates the new rings
(gve_queues_mem_alloc) before calling gve_close().
- [High] After a failed gve_recover() (teardown_device path),
priv->xsk_pools, priv->ntfy_blocks and priv->stats_report are NULL,…
- [High] gve_get_stats() (ndo_get_stats64) checks 'if (priv->rx)' and 'if
(priv->tx)' and then walks the ring arrays with no lock or RCU…
- [Medium] gve_reset() uses the non-sync
disable_work(&priv->service_task), which does not wait for a service…
- [Medium] gve_shutdown() and gve_suspend() sample was_up =
netif_running() before disable_work_sync() and before taking RTNL and…
- [Medium] gve_add_flow_rule(), gve_del_flow_rule() and the new NTUPLE-
disable branch in gve_set_features() start a reset only when the error…
--
gve: simplify reset logic
This commit reworks the gve reset path to remove recursive resets and
resets triggered during teardown. It splits control plane allocation from
device setup, and gve_reset() now tears down queues and the control plane
directly before calling gve_recover(). Shared memory is meant to be freed
only after the hardware reset done in gve_adminq_free().
> 2) Removing resets during the teardown portion of reset. This is partly
> covered by removing recursive resets, but the primary goal in this
> case is to allow the driver to complete teardown in a more direct
> manner. Before this patch, gve_close() when called as part of
> gve_shutdown, could end up triggering a hardware reset, then attempt
> to close again. In such a case, destroying hardware queues would
> inevitably fail, causing a loop. gve_close() is no longer called
> directly in gve_reset() breaking any possibility of this loop.
[Severity: Low]
Is this description of the old behaviour accurate? In the baseline,
gve_reset_and_teardown() calls gve_trigger_reset() first, and
gve_adminq_release() ends with:
gve_clear_device_rings_ok(priv);
gve_clear_device_resources_ok(priv);
gve_clear_admin_queue_ok(priv);
The second gve_close() -> gve_queues_stop() destroys rings only under
gve_get_device_rings_ok(). So it skips gve_destroy_rings() and
gve_unregister_qpls() entirely.
The loop that did exist was the recursive gve_reset() from
gve_adminq_configure_flow_rule() on -ETIME during teardown, via
gve_teardown_device_resources() -> gve_flow_rules_reset().
Could the commit message describe that mechanism instead?
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index a62cb7a921d07..901673d2e2648 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
[ ... ]
> @@ -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;
> }
[Severity: Low]
Removing this call fixes a real unbounded recursion. In the baseline,
gve_reset(priv, true) was called on -ETIME, and teardown then called
gve_flow_rules_reset() again with no reset-in-progress guard:
gve_reset()
gve_teardown_device_resources()
gve_flow_rules_reset()
gve_adminq_configure_flow_rule() <- -ETIME
gve_reset()
...
The patch also carries other fixes that the commit message doesn't
mention:
- cancel_work_sync(&priv->stats_report_task) in gve_free_stats_report()
- cancelling the PTP worker
- the NULL priv->rx check in gve_drain_page_cache()
Should these be split out, or at least get a Fixes: tag, rather than
going in under "simplify reset logic"?
> diff --git a/drivers/net/ethernet/google/gve/gve_flow_rule.c b/drivers/net/ethernet/google/gve/gve_flow_rule.c
> index 2c80cda28ef30..fae552f4ad6fb 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);
> + }
[Severity: Medium]
This is a pre-existing issue, which the patch moves and copies. Should this
also handle -ENOTRECOVERABLE?
gve_adminq_parse_err() returns -ETIME when the device completes a command
with a DEADLINE_EXCEEDED status. A real driver-side timeout in
gve_adminq_kick_and_wait() returns -ENOTRECOVERABLE instead, and that
triggers no reset.
Later gve_adminq_execute_cmd() calls then fail with -EINVAL through the
tail != head check. The admin queue stays unusable until something else
resets the device.
The same -ETIME-only check is used in gve_del_flow_rule() and in the new
NTUPLE disable branch in gve_set_features().
[ ... ]
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2fe280cf7e680..3ca0f8dba683a 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -590,7 +591,80 @@ static void gve_free_notify_blocks(struct gve_priv *priv)
[ ... ]
> +static void gve_queues_mem_remove(struct gve_priv *priv)
> +{
> + struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
> + struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
> +
> + gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
> + gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
> + priv->tx = NULL;
> + priv->rx = NULL;
> +}
[Severity: High]
This is a pre-existing issue. gve_get_stats() checks priv->rx and priv->tx
and then walks the ring arrays with no lock or RCU protection:
gve_get_stats() {
if (priv->rx) {
for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
do {
start = u64_stats_fetch_begin(&priv->rx[ring].statss);
...
}
dev_get_stats() callers such as /proc/net/dev hold neither rtnl nor the
netdev lock. Here gve_queues_mem_free() kvfree()s the arrays before
priv->tx and priv->rx are cleared, with no grace period.
Can a concurrent /proc/net/dev read dereference freed ring memory when
this runs from gve_reset(), gve_close() or gve_teardown_device()?
[ ... ]
> @@ -1335,15 +1393,16 @@ static void gve_rx_stop_rings(struct gve_priv *priv, int num_rings)
[ ... ]
> /* The passed-in queue memory is stored into priv and the queues are made live.
[Severity: Low]
This isn't a bug, but the rest of this comment still reads:
* No memory is allocated. Passed-in memory is freed on errors.
After this patch, the stop_rings path only calls gve_queues_stop().
gve_open() and gve_adjust_config() now call gve_queues_mem_remove()
themselves.
On the reset path, gve_reset() frees the passed-in rings and
gve_recover() -> gve_open() allocates new ones.
Should the comment be updated to match?
[ ... ]
> @@ -1415,16 +1473,14 @@ 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 */
> - return err;
> -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);
> + goto stop_rings;
> +
> + /* Attempt to reset. If reset is successful, gve_queues_start was
> + * successful with the new config.
> + */
> + return gve_reset(priv, false);
[Severity: Medium]
Before this patch, this path returned the original error. Now it returns
the result of gve_reset(), so a successful recovery is reported to the
caller as success. Can this break an AF_XDP zero-copy bind?
On a running DQO RDA interface with XDP loaded:
gve_xsk_pool_enable()
set_bit(qid, priv->xsk_pools)
gve_configure_rings_xdp()
gve_adjust_config()
gve_queues_start()
gve_create_rings() fails
return gve_reset(priv, false)
gve_free_control_plane_resources() <- frees xsk_pools
gve_recover()
gve_alloc_control_plane_resources() <- new, empty bitmap
gve_open() -> gve_reg_xdp_info() <- no XSK pool found
gve_xsk_pool_enable() then sees 0, and xp_assign_dev() marks the socket
zero-copy, even though the queue now uses an ordinary page pool.
Wouldn't the socket then silently receive no traffic?
> +stop_rings:
> + gve_queues_stop(priv);
> return err;
> }
>
> @@ -1435,70 +1491,55 @@ 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 is a pre-existing issue, and this check fences only gve_open().
After a failed gve_recover() takes the teardown_device path,
priv->xsk_pools, priv->ntfy_blocks and priv->stats_report are NULL, and
the admin queue dma_pool is destroyed. netif_running() usually stays true,
because gve_reset() was called from the service task or ethtool with the
interface up.
netif_device_detach() blocks ethtool and dev_open, but ndo_bpf and
ndo_set_features are still reachable. Could these then oops or use freed
memory?
AF_XDP zero-copy bind:
gve_xsk_pool_enable()
set_bit(qid, priv->xsk_pools) <- NULL bitmap
gve_xsk_pool_disable() does clear_bit() on the same NULL bitmap.
ethtool -K (GRO_HW) or XDP attach:
gve_set_features() / gve_set_xdp()
gve_adjust_config()
gve_close() <- succeeds, rings_ok is false
gve_queues_start()
gve_tx_start_rings() <- indexes NULL priv->ntfy_blocks
gve_register_qpls()
gve_adminq_execute_cmd() <- freed admin queue
Would a gve_get_device_resources_ok() check in gve_adjust_config() and in
the XSK pool paths help here?
> gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
>
> err = gve_queues_mem_alloc(priv, &tx_alloc_cfg, &rx_alloc_cfg);
> if (err)
> return err;
>
> - /* No need to free on error: ownership of resources is lost after
> - * calling gve_queues_start.
> - */
> err = gve_queues_start(priv, &tx_alloc_cfg, &rx_alloc_cfg);
> - if (err)
> + if (err) {
> + gve_queues_mem_remove(priv);
> return err;
> + }
[Severity: High]
Can this free memory that the device still references, without the
device ever being reset?
When gve_open() is called from gve_recover(priv, true), reset_in_progress
is set. A failure in gve_register_qpls() or gve_create_rings() inside
gve_queues_start() therefore goes from the reset label straight to
stop_rings:
reset:
if (gve_get_reset_in_progress(priv))
goto stop_rings;
Neither function rolls back on failure. gve_register_qpls() leaves the
earlier QPLs registered. gve_create_rings() may already have created all
TX queues, whose q_resources the device writes to ("This failure will
trigger a reset - no need to clean up").
gve_queues_mem_remove() here then unmaps and frees the QPL pages,
descriptor rings and q_resources.
gve_recover() then returns err and deliberately keeps the admin queue, so
gve_adminq_free() never resets the device. DEVICE_RINGS_OK was never set,
so a later gve_close() won't destroy those queues either.
That seems to conflict with the guarantee, in both the commit message and
the gve_reset_device() kdoc, that shared memory is freed only after the
device has been reset.
[ ... ]
> -static int gve_queues_stop(struct gve_priv *priv)
> +static int gve_close(struct net_device *dev)
> {
> + struct gve_priv *priv = netdev_priv(dev);
> int err;
>
> - netif_carrier_off(priv->dev);
> + 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_turndown(priv);
> - gve_drain_page_cache(priv);
> + gve_clear_device_rings_ok(priv);
> err = gve_destroy_rings(priv);
> if (err)
> - goto err;
> + goto reset;
[ ... ]
> +reset:
> + err = gve_reset(priv, true);
> + return err;
> }
[Severity: High]
This is a pre-existing issue, and the rewritten reset label keeps it.
gve_adjust_config() allocates the new rings with gve_queues_mem_alloc()
before calling gve_close(). On DQO RDA, gve_rx_create_page_pool() stores
a pointer into the notify block array in each new page pool:
.napi = &priv->ntfy_blocks[ntfy_id].napi,
Suppose gve_destroy_rings() or gve_unregister_qpls() fails here. Then
gve_reset(priv, true) runs gve_free_control_plane_resources() ->
gve_free_notify_blocks(), which kvfree()s priv->ntfy_blocks.
gve_recover() then allocates a new array.
If gve_reset() returns 0, gve_adjust_config() carries on with:
err = gve_queues_start(priv, tx_alloc_cfg, rx_alloc_cfg);
The pre-allocated rings passed in still have page pools pointing at the
freed ntfy_blocks. Can page_pool_napi_local(), or
page_pool_disable_direct_recycling() via gve_rx_stop_ring_dqo(), then
dereference freed memory?
If recovery fails instead, gve_adjust_config() calls
gve_queues_mem_free() on the same rings, and page_pool_destroy() sees the
same stale napi pointer.
This is reachable from gve_set_features(), gve_adjust_queues() or
gve_set_xdp() when an admin queue destroy command fails.
[ ... ]
> @@ -2545,62 +2557,79 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
[ ... ]
> + if (setup_queues) {
> + /* On failure, hold on to the control plane to give a
> + * chance for the queues to be brought up later.
> + */
> err = gve_open(priv->dev);
> - if (err)
> + if (err) {
> + dev_err(&priv->pdev->dev,
> + "Failed to start queues: err=%d, !!! DISABLING ALL QUEUES !!!\n",
> + err);
> return err;
> + }
> }
> +
> + /* undo any detach from an earlier failure */
> + netif_device_attach(priv->dev);
> +
> return 0;
[Severity: Medium]
If setup_queues is true and gve_open() fails, this returns before
netif_device_attach(). The comment says the control plane is kept so the
queues can be brought up later. But if an earlier gve_recover() failure
had detached the netdev, won't it stay detached?
For example:
gve_reset() -> gve_recover() fails -> netif_device_detach()
gve_suspend()
gve_resume()
gve_recover(priv, up_before_suspend = true)
gve_setup_device() succeeds
gve_open() fails
return err <- netif_device_attach() skipped
device_resources_ok is set at that point. But __dev_open() and the
ethtool ops return -ENODEV, so the queues can't be brought up without
rebinding the driver.
>
> -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: err=%d, detaching device\n",
> + err);
> + netif_device_detach(priv->dev);
> + gve_teardown_device(priv);
> return err;
> }
[Severity: Low]
When gve_recover() fails here, the netdev is detached and
gve_teardown_device() frees MSI-X, the management IRQ and the admin queue.
With the device not present, the ethtool ioctl and netlink paths return
-ENODEV, so gve_user_reset() can't be reached. __dev_open() refuses the
device as well. With the management IRQ gone, nothing can queue
service_task for a device-requested reset either.
Before this patch, a failed gve_reset_recovery() only freed the admin
queue and turned the queues down, so a later ethtool --reset could retry.
Is it intended that one transient failure in gve_adminq_init(),
gve_adminq_verify_driver_compatibility() or gve_setup_device() now leaves
the NIC unusable until the driver is rebound or a suspend/resume cycle
runs? The v9 changelog mentions this, but the commit message doesn't.
>
> -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);
> }
[Severity: Low]
gve_reset() now always takes the graceful path. With the interface up, it
sends the destroy-queue and unregister-QPL commands first. It then sends
the flow-rule reset, stats detach and deconfigure commands in
gve_teardown_control_plane_resources(). All of this happens before
gve_adminq_free() actually resets the device.
That now also covers resets requested through the service task: device
status RESET_MASK, TX timeout and RX errors. Before this patch,
gve_handle_reset() called gve_reset(priv, false), which released the
admin queue first and skipped every admin queue command.
On a device that has stopped processing the admin queue, won't the first
batch command stall for about two seconds while rtnl_lock and the netdev
lock are held?
gve_handle_reset()
gve_reset(priv, false)
gve_destroy_rings()
gve_adminq_destroy_tx_queues()
gve_adminq_kick_and_wait() <- times out
Later commands fail fast through the tail != head check in
gve_adminq_execute_cmd(), so the stall is limited to about one timeout.
Was dropping the reset-hardware-first ordering for device-requested
resets intended? The commit message doesn't mention it.
>
> - /* Set it all back up */
> - err = gve_reset_recovery(priv, was_up);
> + disable_work(&priv->service_task);
[Severity: Medium]
This is a pre-existing issue. disable_work() doesn't wait for a service
task that is already running.
gve_service_task() -> gve_handle_link_status() checks
gve_get_napi_enabled() and netif_carrier_ok() without rtnl or the netdev
lock, and may call netif_carrier_on().
If that runs at the same time as gve_turndown() in a gve_reset() triggered
by ethtool or a flow rule, can the carrier be turned back on while the
queues are disabled mid-reset? If the reset takes longer than
watchdog_timeo, this could also cause a spurious tx_timeout.
> + gve_reset_device(priv);
> + gve_queues_stop(priv);
> + gve_queues_mem_remove(priv);
> + gve_free_control_plane_resources(priv);
[ ... ]
> @@ -2991,17 +3022,16 @@ static void gve_shutdown(struct pci_dev *pdev)
> struct gve_priv *priv = netdev_priv(netdev);
> 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);
> +
> + /* detach here because gve_close() might attach in recovery */
> + netif_device_detach(netdev);
> + gve_teardown_device(priv);
[Severity: Medium]
This is a pre-existing issue. was_up is sampled before
disable_work_sync(), rtnl_lock() and netdev_lock().
If the interface is opened in that window, gve_close() is skipped. Then
gve_teardown_device() frees ntfy_blocks and the rings while NAPI is still
enabled. Could that lead to a use-after-free from NAPI?
The added disable_work_sync() makes this window slightly wider.
gve_suspend() follows the same pattern.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v9 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (5 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 06/12] gve: simplify reset logic Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-10-02 10:06 ` netdev-bot+sashiko
2026-09-30 19:04 ` [PATCH net-next v9 08/12] gve: split up notify block allocation and setup paths Harshitha Ramamurthy
` (4 subsequent siblings)
11 siblings, 1 reply; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 | 23 +++++++++++++++-----
4 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 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 3ca0f8dba683..156bee612ba6 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -707,7 +707,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,
@@ -722,7 +723,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);
@@ -744,12 +745,13 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
goto deconfigure_device;
}
- 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;
@@ -771,6 +773,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;
if (priv->ptp)
@@ -783,7 +786,8 @@ 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);
@@ -1820,6 +1824,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;
@@ -1835,15 +1840,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,
@@ -2484,6 +2491,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v9 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences
2026-09-30 19:04 ` [PATCH net-next v9 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences Harshitha Ramamurthy
@ 2026-10-02 10:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-10-02 10:06 UTC (permalink / raw)
To: hramamurthy
Cc: netdev, joshwash, andrew+netdev, davem, edumazet, kuba, pabeni,
ast, daniel, hawk, john.fastabend, sdf, jordanrhee, willemb,
nktgrg, maolson, thostet, jacob.e.keller, debarghyak, kees, bpf,
linux-kernel, przemyslaw.kitszel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] After this patch changes the prototype of
gve_adminq_get_ptype_map_dqo() from `(struct gve_priv *priv, struct…
--
gve: add gve_ctrl_ops for gve initialization/teardown sequences
This commit adds get_ptype_map, configure_rss, setup_stats_report and
reset_flow_rules callbacks to struct gve_ctrl_ops. The adminq
implementations are wired into gve_adminq_ops, and gve_main.c now calls
these operations through priv->ctrl_ops. gve_adminq_get_ptype_map_dqo()
now writes to priv->ptype_lut_dqo directly instead of taking the lut as a
parameter.
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> index fe1e8868cdfe2..5e51c060e2372 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;
^^^^
[Severity: Low]
This isn't a bug, but is this forward declaration still needed?
The removed struct gve_ptype_lut *ptype_lut parameter was the only user
of struct gve_ptype_lut in gve_adminq.h, so the declaration now has no
users. It is also still present at the end of the series, after
"gve: add link status/speed ctrl ops".
Could it be dropped in this patch, along with the parameter?
> -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);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v9 08/12] gve: split up notify block allocation and setup paths
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (6 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 09/12] gve: introduce new methods to handle IRQ doorbells Harshitha Ramamurthy
` (3 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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>
---
v8:
- gate mgmt irq vs ntfy block teardown on different conditions
- move gve_teardown_notify_blocks() to gve_teardown_device() from
gve_reset_device()
- don't set block->irq when tearing down notify blocks
- guard disable_irq() call in gve_remove_napi() on whether the irq was
requested
- teardown notify blocks as part of device reset
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 +++++++++++---------
drivers/net/ethernet/google/gve/gve_utils.c | 4 +-
3 files changed, 89 insertions(+), 72 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 156bee612ba6..01a271680d78 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -425,6 +425,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)
@@ -436,11 +454,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);
@@ -454,7 +470,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;
@@ -477,15 +493,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 *
@@ -493,15 +500,64 @@ 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->mgmt_irq_requested) {
+ free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
+ priv->mgmt_irq_requested = false;
+ }
+
+ 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_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);
@@ -519,9 +575,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;
@@ -535,61 +592,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_tx_get_curr_alloc_cfg(struct gve_priv *priv,
struct gve_tx_alloc_rings_cfg *cfg)
@@ -824,6 +832,7 @@ static void gve_teardown_device(struct gve_priv *priv)
/* Free any resources shared with the device only after we have a
* guarantee that the device will not try to access such resources.
*/
+ gve_teardown_notify_blocks(priv);
gve_free_control_plane_resources(priv);
gve_queues_mem_remove(priv);
}
@@ -2477,13 +2486,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 = {
@@ -2633,6 +2645,7 @@ int gve_reset(struct gve_priv *priv, bool skip_queue_setup)
gve_reset_device(priv);
gve_queues_stop(priv);
gve_queues_mem_remove(priv);
+ gve_teardown_notify_blocks(priv);
gve_free_control_plane_resources(priv);
enable_work(&priv->service_task);
diff --git a/drivers/net/ethernet/google/gve/gve_utils.c b/drivers/net/ethernet/google/gve/gve_utils.c
index b53b7fcdcdaf..caed38cd5f2b 100644
--- a/drivers/net/ethernet/google/gve/gve_utils.c
+++ b/drivers/net/ethernet/google/gve/gve_utils.c
@@ -119,6 +119,8 @@ void gve_remove_napi(struct gve_priv *priv, int ntfy_idx)
{
struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
- disable_irq(block->irq);
+ if (block->irq_requested)
+ disable_irq(block->irq);
+
netif_napi_del_locked(&block->napi);
}
--
2.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 09/12] gve: introduce new methods to handle IRQ doorbells
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (7 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 08/12] gve: split up notify block allocation and setup paths Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-10-02 10:06 ` netdev-bot+sashiko
2026-09-30 19:04 ` [PATCH net-next v9 10/12] gve: setup and teardown management interrupts Harshitha Ramamurthy
` (2 subsequent siblings)
11 siblings, 1 reply; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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. All behaviors related to the IRQ doorbell
indices will be managed by these new methods instead of occurring
directly in notify_block setup/teardown methods. Similarly, GQ ring
counters will be managed in `request_db_info`.
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 | 101 +++++++++----------
4 files changed, 99 insertions(+), 51 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 01a271680d78..d8fafe56f5b8 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)
{
@@ -428,15 +452,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);
@@ -493,24 +508,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;
@@ -530,13 +535,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_requested = false;
}
}
@@ -548,7 +554,7 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
int i;
int err;
- /* Setup Management Vector - the last vector */
+ /* Setup Management 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,
@@ -559,12 +565,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));
@@ -574,14 +579,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
@@ -598,7 +602,6 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
return err;
}
-
static void gve_tx_get_curr_alloc_cfg(struct gve_priv *priv,
struct gve_tx_alloc_rings_cfg *cfg)
{
@@ -665,9 +668,10 @@ static void gve_free_control_plane_resources(struct gve_priv *priv)
priv->ptype_lut_dqo = NULL;
gve_teardown_clock(priv);
- 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);
}
@@ -680,15 +684,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;
@@ -718,15 +725,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;
}
@@ -735,7 +736,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;
}
}
@@ -750,7 +751,7 @@ static int gve_setup_control_plane_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 deconfigure_device;
+ goto release_db_resources;
}
err = ops->setup_stats_report(priv, priv->stats_report_len,
@@ -763,8 +764,8 @@ static int gve_setup_control_plane_resources(struct gve_priv *priv)
gve_set_device_resources_ok(priv);
return 0;
-deconfigure_device:
- gve_adminq_deconfigure_device_resources(priv);
+release_db_resources:
+ ops->release_db_resources(priv);
return err;
}
@@ -799,11 +800,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
if (err)
dev_err(&priv->pdev->dev,
"Failed to detach stats report: err=%d\n", err);
- 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);
@@ -2507,6 +2504,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next v9 09/12] gve: introduce new methods to handle IRQ doorbells
2026-09-30 19:04 ` [PATCH net-next v9 09/12] gve: introduce new methods to handle IRQ doorbells Harshitha Ramamurthy
@ 2026-10-02 10:06 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-10-02 10:06 UTC (permalink / raw)
To: hramamurthy
Cc: netdev, joshwash, andrew+netdev, davem, edumazet, kuba, pabeni,
ast, daniel, hawk, john.fastabend, sdf, jordanrhee, willemb,
nktgrg, maolson, thostet, jacob.e.keller, debarghyak, kees, bpf,
linux-kernel, przemyslaw.kitszel
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message of "gve: introduce new methods to handle IRQ
doorbells" claims more than the code does and leaves out several…
--
gve: introduce new methods to handle IRQ doorbells
This commit adds request_db_info and release_db_resources callbacks to
struct gve_ctrl_ops. The AdminQ implementations issue the configure and
deconfigure device resources commands and set the per-block irq_db_index
pointers. Allocation of the irq_db_indices DMA buffer moves out of
gve_alloc_notify_blocks() into new helpers called from
gve_alloc_control_plane_resources().
> Admin Queue and Mailbox modes. All behaviors related to the IRQ doorbell
> indices will be managed by these new methods instead of occurring
> directly in notify_block setup/teardown methods.
[Severity: Low]
Is this description accurate for what the patch does?
gve_adminq_request_db_info() only sends the configure device resources
command, using the counter_array_bus and irq_db_indices_bus that are
already allocated. It then sets ntfy_blocks[i].irq_db_index.
gve_adminq_release_db_resources() only sends the deconfigure command.
The DMA buffer that holds the doorbell indices is allocated and freed by
two new static helpers in gve_main.c: gve_alloc_irq_db_indices() and
gve_free_irq_db_indices(). They are called from
gve_alloc_control_plane_resources() and gve_free_control_plane_resources(),
not from the new ops.
The next sentence says GQ ring counters will be managed in
request_db_info. However, the counter array is still allocated by
gve_alloc_counter_array() in gve_alloc_control_plane_resources().
The v3 changelog says this split was intentional. The "all behaviors"
wording looks like it is left over from the earlier design, even after the
v6 commit message update.
The commit message also leaves out a few other changes in the patch:
- gve_alloc_control_plane_resources() now allocates the counter array
after the notify blocks and stats report, followed by irq_db_indices.
gve_free_control_plane_resources() changes its free order to match.
- A new gve_ntfy_to_msix_idx() helper is now used for the msix_vectors[]
lookups in gve_setup_notify_blocks() and gve_teardown_notify_blocks().
The "Failed to receive msix vector" message now prints msix_idx.
- The "the last vector" and "the first n-1 vectors" comments are removed,
although mgmt_msix_idx is still num_ntfy_blks.
None of these seem to change behavior in AdminQ mode. Could the commit
message be updated to match the code and mention these changes?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930190428.1130007-1-hramamurthy%40google.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net-next v9 10/12] gve: setup and teardown management interrupts
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (8 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 09/12] gve: introduce new methods to handle IRQ doorbells Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 11/12] gve: add ctrl ops for queue operations Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 12/12] gve: add link status/speed ctrl ops Harshitha Ramamurthy
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 | 32 ++++++++++++++++++++
drivers/net/ethernet/google/gve/gve_adminq.h | 2 ++
drivers/net/ethernet/google/gve/gve_main.c | 26 +++++-----------
4 files changed, 46 insertions(+), 18 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..05b011417bf3 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1755,3 +1755,35 @@ 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;
+
+ 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 d8fafe56f5b8..63377bf2a0f8 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -291,14 +291,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;
-
- 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;
@@ -523,12 +515,10 @@ 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->mgmt_irq_requested) {
- free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
- priv->mgmt_irq_requested = false;
- }
+ ops->teardown_mgmt_irq(priv);
if (!priv->ntfy_blocks)
return;
@@ -549,21 +539,19 @@ static void gve_teardown_notify_blocks(struct gve_priv *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 */
- 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);
+ 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);
@@ -2506,6 +2494,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 11/12] gve: add ctrl ops for queue operations
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (9 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 10/12] gve: setup and teardown management interrupts Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
2026-09-30 19:04 ` [PATCH net-next v9 12/12] gve: add link status/speed ctrl ops Harshitha Ramamurthy
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 05b011417bf3..54f235ea27c1 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
@@ -1787,3 +1811,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 63377bf2a0f8..8d628c419972 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -944,33 +944,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
@@ -1114,26 +1097,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;
}
int gve_alloc_page(struct gve_priv *priv, struct device *dev,
@@ -2496,6 +2465,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next v9 12/12] gve: add link status/speed ctrl ops
2026-09-30 19:04 [PATCH net-next v9 00/12] gve: AdminQ mode related refactors Harshitha Ramamurthy
` (10 preceding siblings ...)
2026-09-30 19:04 ` [PATCH net-next v9 11/12] gve: add ctrl ops for queue operations Harshitha Ramamurthy
@ 2026-09-30 19:04 ` Harshitha Ramamurthy
11 siblings, 0 replies; 18+ messages in thread
From: Harshitha Ramamurthy @ 2026-09-30 19:04 UTC (permalink / raw)
To: netdev
Cc: joshwash, hramamurthy, andrew+netdev, davem, edumazet, kuba,
pabeni, ast, daniel, hawk, john.fastabend, sdf, jordanrhee,
willemb, nktgrg, maolson, thostet, jacob.e.keller, debarghyak,
kees, bpf, linux-kernel, przemyslaw.kitszel
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 54f235ea27c1..8b215fca9c1b 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -1837,3 +1837,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 8d628c419972..0b84ae4ef7ad 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1509,15 +1509,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 {
@@ -1544,7 +1547,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)) {
@@ -1571,8 +1573,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;
}
@@ -2003,11 +2005,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,
@@ -2335,12 +2337,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)
@@ -2467,6 +2471,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.56.0.rc1.315.gc6ed9934b7-goog
^ permalink raw reply [flat|nested] 18+ messages in thread