Re: [PATCH net-next, v3] net/mlx5: Use dma_wmb() for completion queue doorbell updates

[netdev-bot+sashiko]

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

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

- [Medium] Commit message justification is factually wrong for at least
  one converted site, and the change's correctness silently depends on…

--

net/mlx5: Use dma_wmb() for completion queue doorbell updates

This converts the wmb() that follows mlx5_cqwq_update_db_record() into
dma_wmb() at six CQ polling sites in the mlx5 driver.  The reasoning given
is that only DMA coherent memory is written around the barrier, so the
lighter DMA barrier is sufficient.  The wmb() in wc.c is intentionally left
alone.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> index 1f6bde5d76265..1341874ee58a3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c
> @@ -384,7 +384,7 @@ static inline void mlx5_fpga_conn_cqes(struct mlx5_fpga_conn *conn,
>  
>  	mlx5_fpga_dbg(conn->fdev, "Re-arming CQ with cc# %u\n", conn->cq.wq.cc);
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  	mlx5_fpga_conn_arm_cq(conn);
>  }

[Severity: Medium]
The commit message says:

    Since only DMA coherent memory accesses are involved (no MMIO accesses
    follow), we can safely use dma_wmb() instead of wmb().

Is that accurate for mlx5_fpga_conn_cqes()?  The statement right after the
converted barrier is mlx5_fpga_conn_arm_cq(), which reaches an MMIO write:

drivers/net/ethernet/mellanox/mlx5/core/fpga/conn.c:
static void mlx5_fpga_conn_arm_cq(struct mlx5_fpga_conn *conn)
{
	mlx5_cq_arm(&conn->cq.mcq, MLX5_CQ_DB_REQ_NOT,
		    conn->fdev->conn_res.uar->map, conn->cq.wq.cc);
}

include/linux/mlx5/cq.h:mlx5_cq_arm() {
	*cq->arm_db = cpu_to_be32(sn << 28 | cmd | ci);

	/* Make sure that the doorbell record in host memory is
	 * written before ringing the doorbell via PCI MMIO.
	 */
	wmb();
	...
	mlx5_write64(doorbell, uar_page + MLX5_CQ_DOORBELL);
}

and mlx5_write64() uses __raw_writeq()/__raw_writel(), which carry no
implicit barriers.

The same holds indirectly for the other four converted sites,
mlx5e_poll_rx_cq(), mlx5e_poll_tx_cq(), mlx5e_poll_xdpsq_cq() and
mlx5e_ptp_poll_ts_cq(): the NAPI caller in en_txrx.c arms the CQ over MMIO
shortly after the poll returns:

drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h:
static inline void mlx5e_cq_arm(struct mlx5e_cq *cq)
{
	mlx5_cq_arm(&cq->mcq, MLX5_CQ_DB_REQ_NOT, cq->uar->map, cq->wq.cc);
}

dma_wmb() is strictly weaker than wmb() on some architectures (arm64
dmb(oshst) versus dsb(st)), and by itself it does not order the coherent
memory consumer-index store against a later MMIO write.  So is the change
relying on the wmb() inside mlx5_cq_arm() to provide that ordering?

If so, could the changelog state that instead, and could the code comment
be updated from "ensure cq space is freed before enabling more cqes" to
mention that the MMIO ordering comes from mlx5_cq_arm()?  Otherwise a later
change that relaxes the wmb() in mlx5_cq_arm(), or a new arm path without
one, would quietly lose the CQ consumer-index versus arm-doorbell ordering
with nothing in the history pointing at it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902023429.1717-1-lirongqing%40baidu.com