Re: [PATCH v3 06/15] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Mon, Sep 28, 2026 at 10:57:12AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2026-09-28 09:25, 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.
> > 
> > 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>
> 
> Thanks, I find smp_mb calls difficult to understand, but I think these
> are correct. I expect I ran into this problem a few times back when I
> was working on this code and had no idea the cause or how to fix it.
> 
> I have one minor suggestion below for the comment, other than that:
> 
> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>

Hi Logan, thanks for the review.

> 
> 
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 51d9e9969065..d290e5869c21 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -1101,6 +1101,12 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  	/* Publish the link only after every QP has been set up. */
> >  	atomic_set_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();
> > +
> 
> I don't find this comment all that easy to understand. Can we expand it
> a little? Maybe something like:
> 
>  Order the link_is_up store before the client_ready loads below, so
>  that this path or ntb_transport_link_up() is guaranteed to see the
>  other's flag. Pairs with smp_mb() in ntb_transport_link_up().

That's great. I think we can use it as-is.

I've gone through Sashiko's feedback on v3, and I don't think another respin is
needed for those points.

Jon, Dave, if v3 looks good to you, could you apply it with the comment updated
as Logan suggested? Of course, I'm happy to send v4 with that change if you'd
prefer.

Best regards,
Koichiro

> 
> 
> Thanks,
> 
> Logan
>