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

[Frank Li]

On Wed, Oct 14, 2026 at 04:14:48PM +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.
>
> 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>
> ---

Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

> Changes in v3:
>   - Rebase onto the latest pci/endpoint.
>   - Drop redundant dma_wmb(). (Frank)
>   - Retest using ntb_tool.
>   - Link to v2: https://lore.kernel.org/r/20260904162049.3432464-1-den@xxxxxxxxxxxxx/
>
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 90 +++++++++++++++----
>  1 file changed, 73 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 07ba338732d4..a11f9260ee86 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
> @@ -183,24 +187,54 @@ 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);
> +}
> +
> +static int epf_ntb_notify_link(struct ntb_dev *ndev)
> +{
> +	struct pci_epf *epf = ntb_ndev(ndev)->epf;
> +	int ret;
> +
> +	ntb_link_event(ndev);
> +
> +	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;
> +}
> +
>  /**
> - * 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)
> +		return epf_ntb_notify_link(ntb->ntb);
>
> -	ntb_link_event(ntb->ntb);
>  	return 0;
>  }
>
> @@ -325,7 +359,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;
> @@ -333,7 +366,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;
> @@ -1472,11 +1504,28 @@ static int vntb_epf_peer_mw_get_addr(struct ntb_dev *ndev, int idx,
>  	return 0;
>  }
>
> +static int vntb_epf_set_link(struct ntb_dev *ndev, bool link_up)
> +{
> +	struct epf_ntb *ntb = ntb_ndev(ndev);
> +	bool notify;
> +
> +	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;
> +
> +	return epf_ntb_notify_link(ndev);
> +}
> +
>  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, true);
>  }
>
>  static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
> @@ -1636,7 +1685,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)
> @@ -1654,7 +1703,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, false);
>  }
>
>  static struct device *vntb_epf_get_dma_dev(struct ntb_dev *ndev)
> @@ -1797,6 +1846,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");
> @@ -1882,7 +1935,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
>  {
>  	struct epf_ntb *ntb;
>  	struct device *dev;
> -	int i;
> +	int ret, i;
>
>  	dev = &epf->dev;
>
> @@ -1893,6 +1946,9 @@ static int epf_ntb_probe(struct pci_epf *epf,
>  	epf->header = &epf_ntb_header;
>  	ntb->epf = epf;
>  	ntb->vbus_number = 0xff;
> +	ret = devm_mutex_init(dev, &ntb->link_lock);
> +	if (ret)
> +		return ret;
>
>  	INIT_WORK(&ntb->peer_db_work, vntb_epf_peer_db_work);
>  	disable_work(&ntb->peer_db_work);
>
> base-commit: 5cdbd6d3a3a147fa9975e35f2602cbec9a72a932
> --
> 2.54.0
>