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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The driver fails to mutually notify the local NTB client or HOST of effective link state changes, potentially causing clients waiting for link events to hang.

Pre-existing issues:
- [High] Missing endianness conversions for PCIe shared memory.
--

commit ca462c18f2f26ec109681df8452341853193267f
Author: Koichiro Den <den@valinux.co.jp>
PCI: endpoint: pci-epf-vntb: Track link state from both sides

This patch tracks the link state of both HOST and VHOST sides of a virtual
NTB device separately. It reports the effective link as up only when both
sides enable it and notifies the other side when this effective state changes.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index c3caec927d748..8af3dcdd893c4 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but the patch propagates a
pre-existing structural flaw by adding more native endian accesses to
ntb->reg->link_status.

Will this cause the endpoint function driver to read and write corrupted link
status over the PCIe bus on big-endian architectures? The PCIe bus is
inherently little-endian, so it seems this needs cpu_to_le16() or
le16_to_cpu() conversions to avoid breaking device functionality globally on
big-endian endpoint architectures.

> +
> +	/* 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);
> +	}

[Severity: High]
Does this code miss raising an IRQ to notify the HOST?

When the HOST changes the effective state via epf_ntb_link_up(), this notifies
the VHOST but fails to raise an IRQ for the HOST. Could this cause NTB clients
waiting for a link event to hang indefinitely because the event is never
delivered to the side that initiated the change?

>  
> -	ntb_link_event(&ntb->ntb);
>  	return 0;
>  }

[ ... ]

> @@ -1455,11 +1475,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);

[Severity: High]
Does this code miss calling ntb_link_event(&ntb->ntb) to notify the local
VHOST?

When the VHOST changes the effective link state via vntb_epf_set_link(), the
driver raises an IRQ for the HOST but fails to notify the local VHOST clients.
Similar to epf_ntb_link_up(), could this cause NTB clients blocking on a link
event to hang indefinitely?

> +	if (ret)
> +		dev_err(&epf->dev, "Failed to raise link event IRQ: %d\n", ret);
> +
> +	return ret;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904065335.3059625-1-den@valinux.co.jp?part=1