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

[netdev-bot+sashiko@kernel.org]

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.

> 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);
> +};

[ ... ]

> 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;
> +}

[ ... ]

> 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

[ ... ]

> @@ -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