Re: [PATCH v2] PCI: endpoint: pci-epf-vntb: Track link state from both sides

[Koichiro Den]

On Tue, Oct 13, 2026 at 11:56:01AM -0500, Frank Li wrote:
> On Sat, Sep 05, 2026 at 01:20:49AM +0900, Koichiro Den wrote:
> > The control-region link status is currently updated only by
> > COMMAND_LINK_UP and COMMAND_LINK_DOWN from the HOST. The virtual
> > NTB link callbacks are empty. Consequently, the HOST can see the link as
> > up before the VHOST has enabled it, and ntb_link_disable() on the VHOST
> > leaves LINK_STATUS_UP set without notifying either NTB client.
> >
> > ntb_netdev can hide this device-level state bug because ntb_transport uses
> > a separate per-QP LINK_DOWN_FLAG message. Use ntb_tool to observe the
> > device link state directly:
> >
> > After bringing both sides up, start one waiter on each side:
> >
> >   HOST# echo N > /sys/kernel/debug/ntb_tool/<H-device>/peer0/link_event
> >   VHOST# echo N > /sys/kernel/debug/ntb_tool/<V-device>/peer0/link_event
> >
> > While both waiters are blocked, disable the link on the VHOST:
> >
> >   VHOST# echo N > /sys/kernel/debug/ntb_tool/<V-device>/link
> >
> > Without this patch, the link-disable command succeeds, but peer0/link
> > remains Y on both sides and neither waiter returns.
> >
> > Track HOST and VHOST enablement separately and report the effective link
> > as up only when both sides have enabled it, as pci-epf-ntb does for its
> > two physical hosts. Both NTB clients observe this effective state, so
> > notify both sides only when it changes, including the side whose request
> > completes the transition. Serialize the two paths because HOST commands
> > run from delayed work while the VHOST callbacks may run concurrently.
> >
> > Order a link-status change caused by a HOST command before publishing
> > command completion.
> >
> > With this patch, peer0/link changes to N on both sides and both waiters
> > return.
> >
> > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Notify both NTB clients when the effective link state changes. (Sashiko)
> >   - Use devm_mutex_init(). (Frank)
> >   - Update the commit message + reproducer accordingly.
> >
> ...
> >  /**
> > - * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
> > + * epf_ntb_link_up() - Update the HOST link state
> >   * @ntb: NTB device that facilitates communication between HOST and VHOST
> > - * @link_up: true or false indicating Link is UP or Down
> > - *
> > - * Once NTB function in HOST invoke ntb_link_enable(),
> > - * this NTB function driver will trigger a link event to VHOST.
> > + * @link_up: true when the HOST has enabled the link
> >   *
> >   * Returns: Zero for success, or an error code in case of failure
> >   */
> >  static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
> >  {
> > -	if (link_up)
> > -		ntb->reg->link_status |= LINK_STATUS_UP;
> > -	else
> > -		ntb->reg->link_status &= ~LINK_STATUS_UP;
> > +	bool notify;
> > +
> > +	scoped_guard(mutex, &ntb->link_lock) {
> > +		notify = ntb->host_linkup != link_up && ntb->vhost_linkup;
> > +		ntb->host_linkup = link_up;
> > +		epf_ntb_update_link(ntb);
> > +	}
> > +
> > +	if (notify) {
> > +		/* Publish link status before completing the HOST command. */
> > +		dma_wmb();
> 
> why need dma_wmb() here?

This is my fault.
Thanks for catching this. This is actually left over from v1, where the
HOST-command path did not raise an IRQ back to HOST (from VHOST).
I'll drop this. Let me send v3.

> suppose there are writel() at
> pci_epc_raise_irq(), or barrier should be inside pci_epc_raise_irq().

That sounds reasonable. (Even if some drivers do not use a non-relaxed writel(),
the ordering between the status update and the IRQ should still be handled by
the EPC core or controller drivers, not by consumer like vNTB. In hindsight,
handling it here would unnecessarily spaghettify the code.)

Thanks for the review!
Koichiro Den

> 
> Frank