Re: [PATCH net-next 6/6] net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] Incomplete cleanup plus an incorrect commit-message premise.

--

net/mlx5: HWS, Remove unneeded WRITE_ONCE in post send

This drops the WRITE_ONCE(doorbell_cseg, NULL) statement and its comment
from hws_send_engine_post_ring() in the HWS send path, on the grounds that
the store targets a by-value parameter and therefore has no effect.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> index bddac018b44b8..8b70040eaca03 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/send.c
> @@ -111,9 +111,6 @@ static void hws_send_engine_post_ring(struct mlx5hws_send_ring_sq *sq,
>  	wmb();
>  
>  	mlx5_write64((__be32 *)doorbell_cseg, sq->uar_map);
> -
> -	/* Ensure doorbell is written on uar_page before poll_cq */
> -	WRITE_ONCE(doorbell_cseg, NULL);
>  }

[Severity: Low]
The changelog says:

    b) This code originated from another place (copy-paste bug),
    where there was actually a CQ polling right after this line,
    and the WRITE_ONCE was writing to another place.

Does that match the in-tree origin?  The only other place carrying this
exact comment and idiom is mlx5_aso_post_wqe() in
drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c:

	mlx5_write64((__be32 *)doorbell_cseg, aso->uar_map);

	/* Ensure doorbell is written on uar_page before poll_cq */
	WRITE_ONCE(doorbell_cseg, NULL);

and there doorbell_cseg is also a by-value parameter:

	void mlx5_aso_post_wqe(struct mlx5_aso *aso, bool with_data,
			       struct mlx5_wqe_ctrl_seg *doorbell_cseg)

so the store is dead at that site too, rather than "writing to another
place".  That line predates the HWS send path (it came in with commit
c491ded04325b9 in 2022).

Should aso.c get the same deletion in this patch?  Unlike the HWS site, the
aso.c one really is followed by CQ polling in every caller, for example
mlx5e_ipsec_aso_query():

	mlx5_aso_post_wqe(aso->aso, false, &wqe->ctrl);
	read_poll_timeout_atomic(mlx5_aso_poll_cq, ret, !ret, 10,
				 10 * USEC_PER_MSEC, false, aso->aso, false);

with the same pattern in mlx5e_tc_meter (en/tc/meter.c) and the macsec
paths, so the comment there documents an ordering guarantee the code never
implemented.  No runtime failure follows from it, since all of those
callers poll inside a read_poll_timeout*() retry loop and simply retry.

After this change, the two functionally identical doorbell-ring helpers in
the same driver no longer agree, and the construct the changelog calls
erroneous is still present at the site the changelog points to as its
origin.  Would it make sense to either remove it there as well, or, if some
real ordering requirement exists at that site, add an actual barrier
instead of a store to a local?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915111912.3926585-1-tariqt%40nvidia.com