Re: [PATCH 01/16] NTB: ntb_transport: Abort link setup on QP MW allocation failure

On Mon, Aug 10, 2026 at 01:41:59PM -0500, Frank Li wrote:
> On Tue, Aug 11, 2026 at 01:51:20AM +0900, Koichiro Den wrote:
> > ntb_transport_setup_qp_mw() can fail while growing a QP's RX entry pool,
> > but the link worker ignores that error. The worker can then publish a QP
> > whose memory-window state is only partly initialized, and later work can
> > use stale or incomplete pointers.
> >
> > Set up every QP memory window before publishing the transport link. On
> > failure, clear the QP pointers before releasing its MW backing and leave
> > the link down.
> >
> > Fixes: a754a8fcaf38 ("NTB: allocate number transport entries depending on size of ring size")
> > Signed-off-by: Koichiro Den <[email protected]>
> > ---
> >  drivers/ntb/ntb_transport.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index f59f926d4bfa..3efc50955253 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -1084,14 +1084,19 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  			goto out1;
> >  	}
> >
> > -	nt->link_is_up = true;
> > +	nt->link_is_up = false;
> > +	for (i = 0; i < nt->qp_count; i++) {
> > +		rc = ntb_transport_setup_qp_mw(nt, i);
> > +		if (rc)
> > +			goto out1;
> > +		ntb_transport_setup_qp_peer_msi(nt, i);
> > +	}
> >
> > +	/* Publish the link only after every QP has been set up. */
> > +	nt->link_is_up = true;
> 
> Not sure if need WRITE_ONCE() or other memory barrier to make sure
> ntb_transport_setup_qp_mw() and ntb_transport_setup_qp_peer_msi() actually
> complete before set this flag.

In that sense, I think we need a compiler barrier + WRITE_ONCE to avoid
use_msi=true <-> nt->link_is_up=true reordering in case dev_info becomes no-op.

But if we want this nt->link_is_up read/write more robust, here I would choose
smp_store_release(&nt->link_is_up, true), paired with smp_load_acquire() in
ntb_transport_link_up(), because another CPU can possibly observe
link_is_up=true there and queue the work.

I'm not sure this compiler or memory barrier issue should be folded into this
small patch, but if preferred, I'll do so.

Thanks for the review,
Koichiro

> 
> Frank
> 
> >  	for (i = 0; i < nt->qp_count; i++) {
> >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> >
> > -		ntb_transport_setup_qp_mw(nt, i);
> > -		ntb_transport_setup_qp_peer_msi(nt, i);
> > -
> >  		if (qp->client_ready)
> >  			schedule_delayed_work(&qp->link_work, 0);
> >  	}
> > @@ -1099,6 +1104,13 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  	return;
> >
> >  out1:
> > +	for (i = 0; i < nt->qp_count; i++) {
> > +		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> > +
> > +		qp->rx_buff = NULL;
> > +		qp->remote_rx_info = NULL;
> > +	}
> > +
> >  	for (i = 0; i < nt->mw_count; i++)
> >  		ntb_free_mw(nt, i);
> >
> > --
> > 2.51.0
> >