Re: [PATCH net-next v3 10/15] gve: add gve_ctrl_ops for gve initialization/teardown sequences

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete abstraction of `configure_rss` leaves a direct dependency on AdminQ in the ethtool path.
--

--- Patch [10]: [PATCH net-next v3 10/15] gve: add gve_ctrl_ops for gve initialization/teardown sequences ---
commit fa76396ec69ce2dfc47469d3f87210437b9df3b5
Author: Joshua Washington <[email protected]>

gve: add gve_ctrl_ops for gve initialization/teardown sequences

Driver initialization and teardown involve a number of control plane
operations that need to be defined for gve_probe to operate in both
mailbox and adminq modes.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 578520e107930..c347661faac00 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -1809,6 +1814,7 @@ static int gve_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  
>  int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
>  {
> +	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
>  	struct gve_rss_config *rss_config = &priv->rss_config;
>  	struct ethtool_rxfh_param rxfh = {0};
>  	u16 i;
> @@ -1824,15 +1830,17 @@ int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
>  
>  	rxfh.hfunc = ETH_RSS_HASH_TOP;
>  
> -	return gve_adminq_configure_rss(priv, &rxfh);
> +	return ops->configure_rss(priv, &rxfh);
>  }

[Severity: Medium]
Since this abstracts the RSS configuration for non-adminq modes, does the
ethtool configuration path also need to be updated to use this new abstraction?

Currently, gve_set_rxfh() in drivers/net/ethernet/google/gve/gve_ethtool.c
still calls the adminq-specific function directly:

drivers/net/ethernet/google/gve/gve_ethtool.c:gve_set_rxfh() {
	...
	if (!priv->rss_key_size || !priv->rss_lut_size)
		return -EOPNOTSUPP;

	err = gve_adminq_configure_rss(priv, rxfh);
	if (err) {
		NL_SET_ERR_MSG_MOD(extack, "Fail to configure RSS config");
		return err;
	}
	...
}

Could this cause issues when configuring RSS via ethtool on a device using
a different backend, since it bypasses ops->configure_rss()?

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