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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mismatch between commit message, code comments, and actual implementation for `free_db_resources`.
--

--- Patch [12]: [PATCH net-next v3 12/15] gve: introduce new methods to handle IRQ doorbells ---
commit d17b52783c19c7cb12a008b8dab81756b1b45ee7
Author: Joshua Washington <[email protected]>

gve: introduce new methods to handle IRQ doorbells

This commit introduces new methods to struct gve_ctrl_ops for configuring
device resources like counter arrays and IRQ doorbell indices. It shifts
doorbell management and GQ ring counter setup from notify_block methods
into these new callbacks.

>   - Similar to above, move free logic out of
>     gve_adminq_free_db_resources() and rename all introduced methods
>     from *free_db_resources to *release_db_resources to reflect the
>     behavioral change.

[Severity: Medium]
This isn't a bug, but the changelog states the methods were renamed to
*release_db_resources. The code still uses the name *free_db_resources.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 8766ea62baf17..f3cfc9d0cdec6 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -836,6 +836,8 @@ 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
> + * @free_db_resources: Free DMA memory holding doorbell info (AdminQ only)

[Severity: Medium]
Is this comment still accurate? Since the logic to free DMA memory was moved
out of gve_adminq_free_db_resources(), it appears this method only
deconfigures device resources now, and the DMA memory is freed separately
by gve_free_irq_db_indices().

>   * @get_ptype_map: Learn packet type map from device and store it in @priv
>   * @configure_rss: Set up default RSS configuration

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index cb0e7005dd3ca..f1ac55cc5b10f 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

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

[Severity: Medium]
Should this function be named gve_adminq_release_db_resources() instead, to
match the intention stated in the changelog?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12