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

[Frank Li]

On Wed, Aug 12, 2026 at 11:15:37AM +0900, Koichiro Den wrote:
> 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 <den@xxxxxxxxxxxxx>
> > > ---
> > >  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.

It should be seperate patch to fix it.

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