Re: [PATCH net-next v5 02/12] gve: introduce control plane operations structure
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-07 00:03:56
Also in:
bpf, lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] New indirect control-plane op return value is discarded in gve_probe(). -- gve: introduce control plane operations structure This adds a gve_ctrl_ops structure to abstract control plane operations between the existing Adminq ABI and an upcoming Mailbox ABI, and moves the doorbell BAR map/unmap into the first two ops. Only the Adminq ops are populated by this patch, and gve_probe()/gve_remove() now call through priv->ctrl_ops.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h index 021adb9108dff..56148ea3cfbf5 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); +};
[ ... ]
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 512349c5517f4..08d8e49d563eb 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) +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; +}
[ ... ]
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 d3882de584e36..93b714e1d1307 100644 --- a/drivers/net/ethernet/google/gve/gve_main.c +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2937,19 +2934,27 @@ 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; + /* Set adminq ctrl ops */ + priv->ctrl_ops = &gve_adminq_ops; + + err = priv->ctrl_ops->map_db_bar(priv); + if (err) { + err = -ENOMEM;
^^^^ [Severity: Low] Is the err = -ENOMEM; assignment here intentional? The new op is declared as returning an int errno in struct gve_ctrl_ops, and gve_adminq_map_db_bar() already returns 0 or -ENOMEM and prints its own dev_err(), so the captured value is overwritten before it is ever used. With only the Adminq implementation present the resulting errno happens to be identical, so there is no change in behaviour today. Once the Mailbox op mentioned in the commit message is added, would an -EINVAL, -EIO, -ENODEV or -EPROBE_DEFER from map_db_bar() be reported to the PCI core as -ENOMEM instead? -EPROBE_DEFER in particular would turn a deferred probe into a hard failure. The other failure sites in gve_probe(), for example gve_init_priv() and register_netdev(), propagate err unchanged, so would just dropping the err = -ENOMEM; line match the surrounding convention?
+ 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;
}[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903215606.31633-1-hramamurthy%40google.com