Re: [PATCH net-next v6 04/12] gve: introduce gve_adminq_get_device_properties()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] gve_adminq_get_device_properties()
  (drivers/net/ethernet/google/gve/gve_adminq.c:1148) bundles…

Pre-existing issues:
- [Medium] gve_adminq_verify_driver_compatibility()
  (drivers/net/ethernet/google/gve/gve_adminq.c:1264, moved verbatim…

--

gve: introduce gve_adminq_get_device_properties()

This adds gve_adminq_get_device_properties(), a helper that runs the
VERIFY_DRIVER_COMPATIBILITY and DESCRIBE_DEVICE admin queue commands in
sequence, and moves the driver-info allocation and fill-in from
gve_main.c into gve_adminq_verify_driver_compatibility().  The commit
message describes it as pure code movement with no functional change.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 046155ae7d669..f420a8e1dd3df 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

> @@ -1143,6 +1145,27 @@ int gve_adminq_describe_device(struct gve_priv *priv)
>  	return err;
>  }
>  
> +int gve_adminq_get_device_properties(struct gve_priv *priv)
> +{
> +	int err;
> +
> +	err = gve_adminq_verify_driver_compatibility(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not verify driver compatibility: err=%d\n", err);
> +		return err;
> +	}
> +
> +	/* Get the initial information we need from the device */
> +	err = gve_adminq_describe_device(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not get device information: err=%d\n", err);
> +		return err;
> +	}
> +	return 0;
> +}

[Severity: Low]
Should this helper also carry the state handling that gve_init_priv()
currently performs around the same two commands?

gve_init_priv() in gve_main.c does this before calling
gve_adminq_describe_device():

	priv->num_registered_pages = 0;

	if (skip_describe_device)
		goto setup_device;

	device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
	/* Get the initial information we need from the device */
	err = gve_adminq_describe_device(priv);

The queue_format reset matters because gve_adminq_describe_device()
consumes the incoming value:

	} else if (device_info->queue_format == GVE_GQI_RDA_FORMAT) {
		dev_info(&priv->pdev->dev,
			 "Driver is running with GQI RDA queue format.\n");

A caller that invokes gve_adminq_get_device_properties() with a stale
non-zero device_info.queue_format would keep the previous format and end
up with supported_features_mask left at 0, silently disabling jumbo
frames, buffer sizes, modify-ring, flow steering, RSS and timestamps.

For the record, this does not appear to be reachable anywhere: at this
commit gve_adminq_get_device_properties() has no callers, only the
prototype in gve_adminq.h and the definition, and gve_init_priv() still
calls gve_adminq_describe_device() directly with the reset in place.  By
the end of the series the concern is resolved rather than left latent:
the single call site in gve_probe() does

	priv->device_info.queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
	err = gve_adminq_get_device_properties(priv);

skip_describe_device no longer exists in the driver, and
priv->num_registered_pages = 0 has moved into gve_setup_device().  Would
it be worth a note in the commit message that the surrounding state
handling stays with the caller?

> @@ -1205,20 +1228,53 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
>  	return gve_adminq_execute_cmd(priv, &cmd);
>  }
>  
> -int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> -					   u64 driver_info_len,
> -					   dma_addr_t driver_info_addr)
> +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv)
>  {
> +	struct gve_driver_info *driver_info;
>  	union gve_adminq_command cmd;
> +	dma_addr_t driver_info_bus;
> +	int err;
> +
> +	driver_info = dma_alloc_coherent(&priv->pdev->dev,
> +					 sizeof(struct gve_driver_info),
> +					 &driver_info_bus, GFP_KERNEL);
> +	if (!driver_info)
> +		return -ENOMEM;

[ ... ]

> -	return gve_adminq_execute_cmd(priv, &cmd);
> +	err = gve_adminq_execute_cmd(priv, &cmd);
> +
> +	/* It's ok if the device doesn't support this */
> +	if (err == -EOPNOTSUPP)
> +		err = 0;
> +
> +	dma_free_coherent(&priv->pdev->dev,
> +			  sizeof(struct gve_driver_info),
> +			  driver_info, driver_info_bus);
> +	return err;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but is it safe to hand
driver_info back to the DMA allocator when the admin queue timed out?

The identical sequence lived in gve_verify_driver_compatibility() in
gve_main.c before the move, so this is only being relocated here.

On the timeout path gve_adminq_kick_and_wait() has already rung the
doorbell and gives up without quiescing the queue:

	gve_adminq_kick_cmd(priv, head);
	if (!gve_adminq_wait_for_cmd(priv, head)) {
		dev_err(&priv->pdev->dev, "AQ commands timed out, need to reset AQ\n");
		priv->adminq_timeouts++;
		return -ENOTRECOVERABLE;
	}

The VERIFY_DRIVER_COMPATIBILITY descriptor still carries
driver_info_bus, so after gve_adminq_execute_cmd() returns
-ENOTRECOVERABLE the buffer is released while the device may still
fetch it.  Can the NIC read driver_info after dma_free_coherent() has
returned it to the allocator?

The same unconditional-free-after-execute pattern is used by
gve_adminq_register_page_list(), gve_adminq_execute_extended_cmd() and
gve_adminq_describe_device(), so if this is worth addressing it looks
like a separate change rather than something for this code movement.

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