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

[Koichiro Den]

On Fri, Sep 04, 2026 at 10:05:19AM -0500, Frank Li wrote:
> On Fri, Sep 04, 2026 at 03:53:35PM +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 the HOST.
> >
> > 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 this waiter on the HOST:
> >
> >   echo N > /sys/kernel/debug/ntb_tool/<H-device>/peer0/link_event
> >
> > While it is blocked, disable the link on the VHOST:
> >
> >   echo N > /sys/kernel/debug/ntb_tool/<V-device>/link
> >
> > Without this patch, the second command succeeds, but peer0/link on the
> > HOST remains Y and the waiter does not return.
> >
> > 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. Notify the other side only when this effective state
> > changes. Serialize the two paths because HOST commands run from delayed
> > work while the VHOST callbacks may run concurrently.
> >
> > With this patch, peer0/link on the HOST changes to N and the waiter
> > returns.
> >
> > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Note: This is a standalone fix, but it conflicts with the following
> > series:
> >
> >   [PATCH v2 0/3] PCI: endpoint: Support hardware-owned MSI-X table and PBA
> >   https://lore.kernel.org/r/20260830151948.3547577-1-den@xxxxxxxxxxxxx/
> >
> > That series still has pending issues and will need more work. I will base
> > v3 on this fix and make the new link-event IRQ honor the MSI/MSI-X
> > selection made by the HOST.
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 82 +++++++++++++++----
> >  1 file changed, 65 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index fba65abfb6b2..51200223f53c 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -41,6 +41,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/slab.h>
> >
> >  #include <linux/pci-ep-msi.h>
> > @@ -147,7 +148,10 @@ struct epf_ntb {
> >  	u16 vntb_pid;
> >  	u16 vntb_vid;
> >
> > -	bool linkup;
> > +	/* Serialize HOST and VHOST link state changes. */
> > +	struct mutex link_lock;
> > +	bool host_linkup;
> > +	bool vhost_linkup;
> >
> >  	/*
> >  	 * True when doorbells are interrupt-driven (MSI or embedded), false
> > @@ -178,24 +182,42 @@ static struct pci_epf_header epf_ntb_header = {
> >  	.interrupt_pin	= PCI_INTERRUPT_INTA,
> >  };
> >
> > +static void epf_ntb_update_link(struct epf_ntb *ntb)
> > +{
> > +	u16 link_status = READ_ONCE(ntb->reg->link_status);
> > +
> > +	/* The link is usable only after both sides have enabled it. */
> > +	if (ntb->host_linkup && ntb->vhost_linkup)
> > +		link_status |= LINK_STATUS_UP;
> > +	else
> > +		link_status &= ~LINK_STATUS_UP;
> > +
> > +	WRITE_ONCE(ntb->reg->link_status, link_status);
> > +}
> > +
> >  /**
> > - * 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
> > + * @link_up: true when the HOST has enabled the link
> >   *
> > - * Once NTB function in HOST invoke ntb_link_enable(),
> > - * this NTB function driver will trigger a link event to VHOST.
> > - *
> > - * Returns: Zero for success, or an error code in case of failure
> > + * Returns: Zero for success
> >   */
> >  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();
> > +		ntb_link_event(&ntb->ntb);
> > +	}
> >
> > -	ntb_link_event(&ntb->ntb);
> >  	return 0;
> >  }
> >
> > @@ -320,7 +342,6 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> >  		ctrl->command_status = COMMAND_STATUS_OK;
> >  		break;
> >  	case COMMAND_LINK_UP:
> > -		ntb->linkup = true;
> >  		ret = epf_ntb_link_up(ntb, true);
> >  		if (ret < 0)
> >  			ctrl->command_status = COMMAND_STATUS_ERROR;
> > @@ -328,7 +349,6 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> >  			ctrl->command_status = COMMAND_STATUS_OK;
> >  		goto reset_handler;
> >  	case COMMAND_LINK_DOWN:
> > -		ntb->linkup = false;
> >  		ret = epf_ntb_link_up(ntb, false);
> >  		if (ret < 0)
> >  			ctrl->command_status = COMMAND_STATUS_ERROR;
> > @@ -1456,11 +1476,34 @@ static int vntb_epf_peer_mw_get_addr(struct ntb_dev *ndev, int idx,
> >  	return 0;
> >  }
> >
> > +static int vntb_epf_set_link(struct epf_ntb *ntb, bool link_up)
> > +{
> > +	struct pci_epf *epf = ntb->epf;
> > +	bool notify;
> > +	int ret;
> > +
> > +	scoped_guard(mutex, &ntb->link_lock) {
> > +		notify = ntb->vhost_linkup != link_up && ntb->host_linkup;
> > +		ntb->vhost_linkup = link_up;
> > +		epf_ntb_update_link(ntb);
> > +	}
> > +
> > +	if (!notify)
> > +		return 0;
> > +
> > +	ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
> > +				PCI_IRQ_MSI, EPF_IRQ_LINK + 1);
> > +	if (ret)
> > +		dev_err(&epf->dev, "Failed to raise link event IRQ: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> >  static int vntb_epf_link_enable(struct ntb_dev *ntb,
> >  			enum ntb_speed max_speed,
> >  			enum ntb_width max_width)
> >  {
> > -	return 0;
> > +	return vntb_epf_set_link(ntb_ndev(ntb), true);
> >  }
> >
> >  static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
> > @@ -1620,7 +1663,7 @@ static u64 vntb_epf_link_is_up(struct ntb_dev *ndev,
> >  {
> >  	struct epf_ntb *ntb = ntb_ndev(ndev);
> >
> > -	return ntb->reg->link_status;
> > +	return READ_ONCE(ntb->reg->link_status);
> >  }
> >
> >  static int vntb_epf_db_clear_mask(struct ntb_dev *ndev, u64 db_bits)
> > @@ -1638,7 +1681,7 @@ static int vntb_epf_db_clear(struct ntb_dev *ndev, u64 db_bits)
> >
> >  static int vntb_epf_link_disable(struct ntb_dev *ntb)
> >  {
> > -	return 0;
> > +	return vntb_epf_set_link(ntb_ndev(ntb), false);
> >  }
> >
> >  static struct device *vntb_epf_get_dma_dev(struct ntb_dev *ndev)
> > @@ -1750,6 +1793,10 @@ static int epf_ntb_bind(struct pci_epf *epf)
> >  		goto err_bar_alloc;
> >  	}
> >
> > +	ntb->host_linkup = false;
> > +	ntb->vhost_linkup = false;
> > +	ntb->reg->link_status = 0;
> > +
> >  	ret = epf_ntb_epc_init(ntb);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to initialize EPC\n");
> > @@ -1833,6 +1880,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
> >  	epf->header = &epf_ntb_header;
> >  	ntb->epf = epf;
> >  	ntb->vbus_number = 0xff;
> > +	mutex_init(&ntb->link_lock);
> 
> devm_mutex_init();

Thanks for pointing that out. I will use it in v2.

Also, Sashiko's feedback seems valid. I have tested the updated patch with
ntb_tool again and confirmed that when the VHOST brings the link down, the
link_event waiters on both sides return. I will send v2 shortly.

Best regards,
Koichiro

> 
> others look good.
> 
> Frank
> >
> >  	INIT_WORK(&ntb->peer_db_work, vntb_epf_peer_db_work);
> >  	disable_work(&ntb->peer_db_work);
> > --
> > 2.51.0
> >