Re: [PATCH net-next v5 11/12] gve: add ctrl ops to for queue operations

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Unrelated, undocumented interface change: the patch drops the
  `const` qualifier from the first parameter of the inline helper…
- [Low] Commit message defects that will be permanently recorded in git
  history: the subject line reads 'gve: add ctrl ops to for queue…

--

gve: add ctrl ops to for queue operations

This adds create_queues and destroy_queues control ops to struct
gve_ctrl_ops, moves the existing per-queue create/destroy loops and their
logging into gve_adminq_create_queues() and gve_adminq_destroy_queues(),
and makes gve_create_rings()/gve_destroy_rings() call through the new ops.

> gve: add ctrl ops to for queue operations
>
> Add the next few control ops to
>
> - create queues
> - destroy queues

[Severity: Low]
This isn't a bug, but the subject line has a stray "to": "add ctrl ops to
for queue operations" reads better as "add ctrl ops for queue operations".

The body sentence "Add the next few control ops to" also ends without an
object before the bullet list.  Should it read something like "Add the next
few control ops to:"?

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_dqo.h b/drivers/net/ethernet/google/gve/gve_dqo.h
> index 5871f773f0c70..ce402020dff6a 100644
> --- a/drivers/net/ethernet/google/gve/gve_dqo.h
> +++ b/drivers/net/ethernet/google/gve/gve_dqo.h
> @@ -66,7 +66,7 @@ void gve_rx_write_doorbell_dqo(const struct gve_priv *priv, int queue_idx);
>  void gve_xdp_tx_flush_dqo(struct gve_priv *priv, u32 xdp_qid);
>  
>  static inline void
> -gve_tx_put_doorbell_dqo(const struct gve_priv *priv,
> +gve_tx_put_doorbell_dqo(struct gve_priv *priv,
>  			const struct gve_queue_resources *q_resources, u32 val)
>  {
>  	u64 index;

[Severity: Low]
What motivates dropping const from priv in gve_tx_put_doorbell_dqo()?  The
commit message says the change is "just moving existing code into ops, no
functional change", and this hunk isn't mentioned.

The body is unchanged and only reads from priv:

	index = be32_to_cpu(q_resources->db_index);
	iowrite32(val, &priv->db_bar2[index]);

All callers in gve_tx_dqo.c (gve_xdp_tx_flush_dqo(), gve_tx_dqo(),
gve_xsk_tx_dqo() and gve_xdp_xmit_dqo()) already pass a non-const
struct gve_priv *, and the signature is still the same at the tip of the
series, so nothing later in the series appears to need a mutable priv here.

It also leaves the neighbouring helpers in the same header inconsistent,
since they keep the const qualifier:

	void gve_rx_write_doorbell_dqo(const struct gve_priv *priv, int queue_idx);

	static inline void
	gve_write_irq_doorbell_dqo(const struct gve_priv *priv,
				   const struct gve_notify_block *block, u32 val)

Could this hunk be dropped, or the reason for it be spelled out in the
changelog?

[ ... ]

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