Re: [PATCH net-next v4 01/10] NTB: ntb_transport: Order RX descriptor reads after completion

[Joe Damato]

On Mon, Sep 14, 2026 at 05:48:29PM +0900, Koichiro Den wrote:
> The peer writes payloads and descriptors into a DMA-coherent memory
> window. ntb_process_rxc() checks DESC_DONE_FLAG before consuming the
> descriptor and payload, but coherent memory alone does not order those
> reads on weakly ordered CPUs.
> 
> Read the completion word once and issue dma_rmb() after DONE is observed.
> Use the saved word for subsequent transport flag checks.
> 
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@xxxxxxxxxxxxxxx/
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> ---
> Changes in v4:
>   - No changes.
> 
>  drivers/ntb/ntb_transport.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f9caa1a653c5..74f4f8c1c7be 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1609,21 +1609,25 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  {
>  	struct ntb_payload_header *hdr;
>  	struct ntb_queue_entry *entry;
> +	unsigned int flags;
>  	void *offset;
>  
>  	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
>  	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);
>  
> -	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> -		qp->qp_num, hdr->ver, hdr->len, hdr->flags);
> -
> -	if (!(hdr->flags & DESC_DONE_FLAG)) {
> +	flags = READ_ONCE(hdr->flags);
> +	if (!(flags & DESC_DONE_FLAG)) {
>  		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");

I get why the dev_dbg line above was moved down, but i feel like if this case
is hit, you lose some of the debugability that the debug line intended?

idk maybe this dev_dbg should be something like

dev_dbg(blah->dev, "qp %d: done flag not set, flags %x\n", qp->qp_num, flags)

so that some of the debuggability is still preserved when this case is hit ?

>  		qp->rx_ring_empty++;
>  		return -EAGAIN;
>  	}
>  
> -	if (hdr->flags & LINK_DOWN_FLAG) {
> +	dma_rmb();
> +
> +	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> +		qp->qp_num, hdr->ver, hdr->len, flags);

I guess the above is a nit, so 

Reviewed-by: Joe Damato <joe@xxxxxxx>