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

[Koichiro Den]

On Sat, Sep 12, 2026 at 03:04:21AM +0900, Koichiro Den wrote:
> 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.

Just for the record, smp_store_release/smp_load_acquire depicted above, instead
of WRITE_ONCE()/READ_ONCE(), is intentional. They are for MP ordering to publish
the QP setup, which is needed by an earlier patch:
https://lore.kernel.org/r/20260910040836.3792333-5-den@xxxxxxxxxxxxx/

Best regards,
Koichiro

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