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