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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing wmb() with dma_wmb() breaks ordering between DMA doorbell writes and normal memory updates, leading to potential CQ overruns.
--

commit 3bd90cae934d1db51177852241805820453b9c02
Author: Li RongQing <lirongqing@baidu.com>

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

This commit replaces wmb() with dma_wmb() in various mlx5 completion queue
polling routines. The goal is to improve performance by using a lighter
barrier where only DMA coherent memory accesses are involved.

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

Does this accurately describe the code following the barrier?

While there might not be MMIO accesses, there are normal memory accesses
that follow, such as updates to consumer indices like sq->cc or pushing to
a freelist.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
> index 723f66a6bd634..17084d76209e8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
> @@ -259,7 +259,7 @@ static bool mlx5e_ptp_poll_ts_cq(struct mlx5e_cq *cq, int napi_budget)
>  	mlx5_cqwq_update_db_record(cqwq);
>  
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  
>  	while (metadata_buff_sz > 0)
>  		mlx5e_ptp_metadata_fifo_push(&ptpsq->metadata_freelist,

[Severity: High]
Is the DMA write to the CQ doorbell ordered before pushing to the
metadata_freelist in normal memory?

If dma_wmb() only guarantees ordering between two DMA memory accesses,
can the CPU reorder the normal memory write to execute before the wq->db
write?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> index 77ea51bfbaae5..ee0986bd5c18d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> @@ -817,7 +817,7 @@ bool mlx5e_poll_xdpsq_cq(struct mlx5e_cq *cq)
>  	mlx5_cqwq_update_db_record(&cq->wq);
>  
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  
>  	sq->cc = sqcc;

[Severity: High]
Does dma_wmb() guarantee that the DMA write to the CQ doorbell is ordered
before the normal memory write to sq->cc?

If it does not, this could lead to a CQ overrun where the transmitting CPU
observes the updated sq->cc, posts new WQEs, and rings the SQ doorbell
before the device observes the CQ space update. This could cause the device
to think the CQ is full, leading to dropped completions, leaked SKBs, or TX
timeouts.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index 0b5e600e4a6a9..14479da98f427 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -849,7 +849,7 @@ bool mlx5e_poll_tx_cq(struct mlx5e_cq *cq, int napi_budget)
>  	mlx5_cqwq_update_db_record(&cq->wq);
>  
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  
>  	sq->dma_fifo_cc = dma_fifo_cc;
>  	sq->cc = sqcc;

[Severity: High]
Will the normal memory write to sq->cc be properly ordered after the DMA
write to the CQ doorbell with only dma_wmb()?

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
> index 614cd57a40d74..8f7a89ae4ad8c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/aso.c
> @@ -421,7 +421,7 @@ int mlx5_aso_poll_cq(struct mlx5_aso *aso, bool with_data)
>  	mlx5_cqwq_update_db_record(&cq->wq);
>  
>  	/* ensure cq space is freed before enabling more cqes */
> -	wmb();
> +	dma_wmb();
>  
>  	if (with_data)
>  		aso->cc += MLX5_ASO_WQEBBS_DATA;

[Severity: High]
Could the normal memory write to aso->cc execute before the DMA write to
the CQ doorbell due to the relaxed guarantees of dma_wmb()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902023429.1717-1-lirongqing@baidu.com?part=1