Re: [PATCH 2/2 v2] PCI/P2PDMA: Modify p2p_dma_distance to detect P2P links

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 19 Sep 2024 01:13:44 -0700
Shivasharan S <shivasharan.srikanteshwara@broadcom.com> wrote:

> Update the p2p_dma_distance() to determine inter-switch P2P links existing
> between two switches and use this to calculate the DMA distance between
> two devices.
> 
> Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
> ---
>  drivers/pci/p2pdma.c       | 17 ++++++++++++++++-
>  drivers/pci/pcie/portdrv.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/portdrv.h |  2 ++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 4f47a13cb500..eed3b69e7293 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -21,6 +21,8 @@
>  #include <linux/seq_buf.h>
>  #include <linux/xarray.h>
>  
> +extern bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b);

That's nasty.  Include the header so you get a clean stub if
this support is not built in etc.

> +

> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index c940b4b242fd..2fe1598fc684 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -104,6 +104,40 @@ static bool pcie_port_is_p2p_supported(struct pci_dev *dev)
>  	return false;
>  }
>  
> +/**
> + * pcie_port_is_p2p_link_available: Determine if a P2P link is available
> + * between the two upstream bridges. The serial number of the two devices
> + * will be compared and if they are same then it is considered that the P2P
> + * link is available.
> + *
> + * Return value: true if inter switch P2P is available, return false otherwise.
> + */
> +bool pcie_port_is_p2p_link_available(struct pci_dev *a, struct pci_dev *b)
> +{
> +	u64 dsn_a, dsn_b;
> +
> +	/*
> +	 * Check if the devices support Inter switch P2P.
> +	 */

Single line comment syntax fine here.  However it's kind
of obvious, so I'd just drop the comment.


> +	if (!pcie_port_is_p2p_supported(a) ||
> +	    !pcie_port_is_p2p_supported(b))

Don't wrap this. I think it's under 80 chars anyway.

> +		return false;
> +
> +	dsn_a = pci_get_dsn(a);
> +	if (!dsn_a)
> +		return false;
If we assume that dsn is the only right way to detect this
(I'm fine with that for now) then we know the supported tests
above would only pass if this is true. Hence

return pci_get_dsn(a) == pci_get_dsn(b);

should be fine.

> +
> +	dsn_b = pci_get_dsn(b);
> +	if (!dsn_b)
> +		return false;
> +
> +	if (dsn_a == dsn_b)
> +		return true;

	return dsn_a == dsn_b;

> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(pcie_port_is_p2p_link_available);
> +
>  /*
>   * Traverse list of all PCI bridges and find devices that support Inter switch P2P
>   * and have the same serial number to create report the BDF over sysfs.