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

[Shivasharan Srikanteshwara]

On Tue, Sep 24, 2024 at 8:38 PM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Thu, 19 Sep 2024 01:13:44 -0700
> Shivasharan S <shivasharan.srikanteshwara@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >  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.
>
Will move this to the new header file that will be added.
> > +
>
> > 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.
>
>
Will do.

> > +     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.
>
Will do.

> > +             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.
>
Agreed. Will rework this as suggested and update in the next
patch.

> > +
> > +     dsn_b = pci_get_dsn(b);
> > +     if (!dsn_b)
> > +             return false;
> > +
> > +     if (dsn_a == dsn_b)
> > +             return true;
>
>         return dsn_a == dsn_b;
>
Above changes will take care of this as well.

> > +
> > +     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.
>
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature