Re: [PATCH net-next v8 07/12] gve: add gve_ctrl_ops for gve initialization/teardown sequences

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The kernel-doc for `struct gve_ctrl_ops` in
  drivers/net/ethernet/google/gve/gve.h says `@setup_stats_report` is…
- [Low] This patch changes the prototype to `int
  gve_adminq_get_ptype_map_dqo(struct gve_priv *priv);`, but…

--

gve: add gve_ctrl_ops for gve initialization/teardown sequences

This commit adds get_ptype_map, configure_rss, setup_stats_report and
reset_flow_rules callbacks to struct gve_ctrl_ops. The control plane
setup and teardown paths in gve_main.c now call them through
priv->ctrl_ops. gve_adminq_get_ptype_map_dqo() also changes: it now
writes into priv->ptype_lut_dqo directly and no longer takes a ptype_lut
argument.

> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 026d685ecaee3..e0583e8cd2cdb 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -833,12 +833,24 @@ 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.
> + * @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)
> + * @reset_flow_rules: Flush all flow rules from device
>   */
>  struct gve_ctrl_ops {
>  	int (*map_db_bar)(struct gve_priv *priv);
>  	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 (*get_ptype_map)(struct gve_priv *priv);
> +	int (*configure_rss)(struct gve_priv *priv,
> +			     struct ethtool_rxfh_param *param);
> +	int (*setup_stats_report)(struct gve_priv *priv,
> +				  u64 stats_report_len,
> +				  dma_addr_t stats_report_addr,
> +				  u64 interval_ms); /* AQ-specific */

[Severity: Low]
This isn't a bug, but is setup_stats_report meant to be optional for
back-ends other than AdminQ?

The kernel-doc says "(AdminQ only)" and the member has an "AQ-specific"
comment. Someone writing a future mailbox ops table could read that as
permission to leave the op NULL.

Both call sites use it without a NULL check, though. In
gve_setup_control_plane_resources():

	err = ops->setup_stats_report(priv, priv->stats_report_len,
				      priv->stats_report_bus,
				      GVE_STATS_REPORT_TIMER_PERIOD);

and in gve_teardown_control_plane_resources():

	err = ops->setup_stats_report(priv, 0, 0x0,
				      GVE_STATS_REPORT_TIMER_PERIOD);

A NULL call can't happen today. The only table, gve_adminq_ops, sets the
op to gve_adminq_report_stats(). Later in the series, release_db_resources
gets the same "(AdminQ only)" label and is also called without a NULL
check.

Could the kernel-doc say whether the op must always be provided, with
non-AdminQ back-ends supplying a stub? Or should the call sites check for
NULL?

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> index fe1e8868cdfe2..5e51c060e2372 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.h
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.h
> @@ -652,8 +652,7 @@ int gve_adminq_report_nic_ts(struct gve_priv *priv,
>  			     dma_addr_t nic_ts_report_addr);
>  
>  struct gve_ptype_lut;
> -int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
> -				 struct gve_ptype_lut *ptype_lut);
> +int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv);

[Severity: Low]
This isn't a bug, but now that the ptype_lut argument is gone, is the
"struct gve_ptype_lut;" forward declaration above still needed?

Nothing else in gve_adminq.h refers to that type. The declaration is also
still there at the end of the series.

[ ... ]

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