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

[Koichiro Den]

On Fri, Sep 18, 2026 at 05:36:43PM -0700, Joe Damato wrote:
> 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");

Hi Joe, thank you for taking the time to review this series.

> 
> 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 ?

That makes sense. I'll follow your suggestion in the next revision. I think I
need to respin anyway.

Best regards,
Koichiro

> 
> >  		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>