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

[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] The new kernel-doc for `struct gve_ctrl_ops` understates the
  contract of the two new callbacks and mislabels one as optional-…

--

gve: introduce new methods to handle IRQ doorbells

This adds request_db_info and release_db_resources callbacks to struct
gve_ctrl_ops, moves the counter array and IRQ doorbell index DMA
allocations into their own helpers, and establishes the notify block to
IRQ doorbell index relationship inside the new ops instead of in the
notify block setup path.

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

[Severity: Low]
Does this kernel-doc match what the callbacks actually do?

gve_adminq_request_db_info() registers the event counter array with the
device in addition to the IRQ doorbell indices:

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

and gve_adminq_release_db_resources() tears that same registration down
via gve_adminq_deconfigure_device_resources(). This patch removes the only
other place the counter array was registered, from
gve_setup_control_plane_resources(), so the counter array semantics now
live entirely in these ops. The commit message itself says the ops
"encapsulate the configuration of device resources (counter arrays and IRQ
doorbell indices)", which the doc text does not mention.

Also, could the "(AdminQ only)" annotation be misread as "may be NULL"?
Both new callbacks are dereferenced unconditionally:

gve_setup_control_plane_resources()
	err = ops->request_db_info(priv);
	...
release_db_resources:
	ops->release_db_resources(priv);

gve_teardown_control_plane_resources()
	ops->release_db_resources(priv);

so every ops table has to populate them. Today only gve_adminq_ops exists
and it sets both, so nothing breaks, but for the Mailbox ops table this
series is preparing an implementer following the doc literally could set up
doorbells only and leave the counter array unregistered, or leave the
pointer NULL.

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

[ ... ]

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

[ ... ]

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