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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] Undisclosed changes in the commit message: besides the two new
  `gve_ctrl_ops` callbacks, this patch (a) adds a brand-new no-op inline…
- [Low] The new error log in `gve_setup_control_plane_resources()` —
  `dev_err(&priv->pdev->dev, "Failed to get db info");` at…
- [Low] The kernel-doc added for `struct gve_ctrl_ops` understates the
  contract of the two new callbacks.

--

gve: introduce new methods to handle IRQ doorbells

This patch adds request_db_info and release_db_resources callbacks to
struct gve_ctrl_ops so that the device-resource configuration can differ
between Admin Queue and Mailbox modes. It also moves the association of
IRQ doorbell indices with notify blocks into the new callbacks.

A few questions on the changes below.

> 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 describe the full contract of the two callbacks?

The only implementation also registers and revokes the event counter
array, not just the doorbell index array:

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

  gve_adminq_release_db_resources()
    gve_adminq_deconfigure_device_resources(priv);

The commit message itself says the ops "encapsulate the configuration of
device resources (counter arrays and IRQ doorbell indices)", which
matches the code but not the doc.

This matters because the same patch moves gve_free_counter_array() into
gve_free_control_plane_resources(), so the counter-array coherent buffer
is now freed on the strength of release_db_resources() having told the
device to stop using it. Should the doc mention the counter array so a
future mailbox implementation does not skip that part and leave the
device DMAing into freed memory?

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

[ ... ]

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

[Severity: Low]
Could the commit message say something about this helper? It is a new
identity function whose priv argument is unused, and it is wired into
both gve_setup_notify_blocks() and gve_teardown_notify_blocks() in this
patch, but the changelog only mentions the two new gve_ctrl_ops members.

As written it encodes the AdminQ assumption that the management vector
is the last one (priv->mgmt_msix_idx == priv->num_ntfy_blks), which is
not stated anywhere. Would a short comment help?

The same applies to the resource movement further down: the
irq_db_indices DMA buffer is moved out of gve_alloc_notify_blocks() /
gve_free_notify_blocks() into new helpers, and the alloc/free order in
gve_alloc_control_plane_resources() / gve_free_control_plane_resources()
changes. The only note about that is in the v3 changelog below the ---
line, which does not end up in git history.

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

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index d2f974a0cc877..84efcdea6be96 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;
> +}
> +

[ ... ]

> @@ -611,9 +614,10 @@ static void gve_free_control_plane_resources(struct gve_priv *priv)
>  	kvfree(priv->ptype_lut_dqo);
>  	priv->ptype_lut_dqo = NULL;
>  
> -	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);
>  }
> @@ -626,15 +630,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;
>  
> @@ -664,15 +671,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;
>  	}

[Severity: Low]
Is the missing "\n" here intentional? Without a terminating newline the
printk record stays open for continuation, so unrelated output can be
appended to this line. The message it replaces, and the next dev_err()
in this same function, both terminate and include the error code:

	dev_err(&priv->pdev->dev,
		"Failed to get ptype map: err=%d\n", err);

The message also looks redundant, since the only implementation,
gve_adminq_request_db_info(), already logs the same failure with the
error code before returning -ENXIO.

>  
> @@ -681,7 +682,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;
>  		}
>  	}

[ ... ]

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