Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > worker is completing setup on another CPU. Concurrent transport setup
> > and a client link-up request can both read the other's flag as false and
> > leave QP link work unqueued. The QP then stays down until another link
> > event or client link-up request.
> >
> > This is the store-buffering pattern described in
> > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> >
> > Add a full barrier between the store and load on each side, and
> > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> >
> > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@xxxxxxxxxxxxxxx/
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >   - New patch (Sashiko)
> >
> >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 75d4a2e021f4..1332d53bcfe7 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  	/* Publish the link only after every QP has been set up. */
> >  	smp_store_release(&nt->link_is_up, true);
> >
> > +	/*
> > +	 * Prevent both sides from missing each other's flag. Pairs with
> > +	 * the barrier in ntb_transport_link_up().
> > +	 */
> > +	smp_mb();
> > +
> >  	for (i = 0; i < nt->qp_count; i++) {
> >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> >
> > -		if (qp->client_ready)
> > +		if (READ_ONCE(qp->client_ready))
> 
> I think it'd better change to use atomic variable for client_ready to avoid
> manual handle smp sync.

AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
those operations are unordered.
So to illustrate this in litmus test like form:

   # L = nt->link_is_up
   # R = qp->client_ready
   # Both initially false

   Transport setup                  Client link-up
   ---------------                  --------------
    smp_store_release(&L, true);     WRITE_ONCE(R, true);
    smp_mb(); /* added */            smp_mb(); /* added */
    r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);

   Both reads return false?
    Before: allowed
    After:  forbidden

I might be misunderstanding your suggestion though. If you have something
different in mind, please let me know.

Thanks for the review.
Koichiro

> 
> Frank
> 
> >  			ntb_transport_schedule_qp_link(qp, 0);
> >  	}
> >
> > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> >  	if (!qp)
> >  		return;
> >
> > -	qp->client_ready = true;
> > +	WRITE_ONCE(qp->client_ready, true);
> > +
> > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > +	smp_mb();
> >
> >  	ntb_transport_schedule_qp_link(qp, 0);
> >  }
> > --
> > 2.51.0
> >