diff mbox series

[RFC,1/5] pci/p2p: add a function to test peer to peer capability

Message ID 20190129174728.6430-2-jglisse@redhat.com (mailing list archive)
State New, archived
Headers show
Series Device peer to peer (p2p) through vma | expand

Commit Message

Jerome Glisse Jan. 29, 2019, 5:47 p.m. UTC 
From: Jérôme Glisse <jglisse@redhat.com>

device_test_p2p() return true if two devices can peer to peer to
each other. We add a generic function as different inter-connect
can support peer to peer and we want to genericaly test this no
matter what the inter-connect might be. However this version only
support PCIE for now.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: iommu@lists.linux-foundation.org
---
 drivers/pci/p2pdma.c       | 27 +++++++++++++++++++++++++++
 include/linux/pci-p2pdma.h |  6 ++++++
 2 files changed, 33 insertions(+)

Comments

Logan Gunthorpe Jan. 29, 2019, 6:24 p.m. UTC | #1 
On 2019-01-29 10:47 a.m., jglisse@redhat.com wrote:
> +bool pci_test_p2p(struct device *devA, struct device *devB)
> +{
> +	struct pci_dev *pciA, *pciB;
> +	bool ret;
> +	int tmp;
> +
> +	/*
> +	 * For now we only support PCIE peer to peer but other inter-connect
> +	 * can be added.
> +	 */
> +	pciA = find_parent_pci_dev(devA);
> +	pciB = find_parent_pci_dev(devB);
> +	if (pciA == NULL || pciB == NULL) {
> +		ret = false;
> +		goto out;
> +	}
> +
> +	tmp = upstream_bridge_distance(pciA, pciB, NULL);
> +	ret = tmp < 0 ? false : true;
> +
> +out:
> +	pci_dev_put(pciB);
> +	pci_dev_put(pciA);
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(pci_test_p2p);

This function only ever returns false....

Logan
Greg KH Jan. 29, 2019, 7:44 p.m. UTC | #2 
On Tue, Jan 29, 2019 at 11:24:09AM -0700, Logan Gunthorpe wrote:
> 
> 
> On 2019-01-29 10:47 a.m., jglisse@redhat.com wrote:
> > +bool pci_test_p2p(struct device *devA, struct device *devB)
> > +{
> > +	struct pci_dev *pciA, *pciB;
> > +	bool ret;
> > +	int tmp;
> > +
> > +	/*
> > +	 * For now we only support PCIE peer to peer but other inter-connect
> > +	 * can be added.
> > +	 */
> > +	pciA = find_parent_pci_dev(devA);
> > +	pciB = find_parent_pci_dev(devB);
> > +	if (pciA == NULL || pciB == NULL) {
> > +		ret = false;
> > +		goto out;
> > +	}
> > +
> > +	tmp = upstream_bridge_distance(pciA, pciB, NULL);
> > +	ret = tmp < 0 ? false : true;
> > +
> > +out:
> > +	pci_dev_put(pciB);
> > +	pci_dev_put(pciA);
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_test_p2p);
> 
> This function only ever returns false....

I guess it was nevr actually tested :(

I feel really worried about passing random 'struct device' pointers into
the PCI layer.  Are we _sure_ it can handle this properly?

thanks,

greg k-h
Jerome Glisse Jan. 29, 2019, 7:53 p.m. UTC | #3 
On Tue, Jan 29, 2019 at 08:44:26PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 29, 2019 at 11:24:09AM -0700, Logan Gunthorpe wrote:
> > 
> > 
> > On 2019-01-29 10:47 a.m., jglisse@redhat.com wrote:
> > > +bool pci_test_p2p(struct device *devA, struct device *devB)
> > > +{
> > > +	struct pci_dev *pciA, *pciB;
> > > +	bool ret;
> > > +	int tmp;
> > > +
> > > +	/*
> > > +	 * For now we only support PCIE peer to peer but other inter-connect
> > > +	 * can be added.
> > > +	 */
> > > +	pciA = find_parent_pci_dev(devA);
> > > +	pciB = find_parent_pci_dev(devB);
> > > +	if (pciA == NULL || pciB == NULL) {
> > > +		ret = false;
> > > +		goto out;
> > > +	}
> > > +
> > > +	tmp = upstream_bridge_distance(pciA, pciB, NULL);
> > > +	ret = tmp < 0 ? false : true;
> > > +
> > > +out:
> > > +	pci_dev_put(pciB);
> > > +	pci_dev_put(pciA);
> > > +	return false;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_test_p2p);
> > 
> > This function only ever returns false....
> 
> I guess it was nevr actually tested :(
> 
> I feel really worried about passing random 'struct device' pointers into
> the PCI layer.  Are we _sure_ it can handle this properly?
> 

Oh yes i fixed it on the test rig and forgot to patch
my local git tree. My bad.

Cheers,
Jérôme
Alex Deucher Jan. 29, 2019, 7:56 p.m. UTC | #4 
On Tue, Jan 29, 2019 at 12:47 PM <jglisse@redhat.com> wrote:
>
> From: Jérôme Glisse <jglisse@redhat.com>
>
> device_test_p2p() return true if two devices can peer to peer to
> each other. We add a generic function as different inter-connect
> can support peer to peer and we want to genericaly test this no
> matter what the inter-connect might be. However this version only
> support PCIE for now.
>

What about something like these patches:
https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=4fab9ff69cb968183f717551441b475fabce6c1c
https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=f90b12d41c277335d08c9dab62433f27c0fadbe5
They are a bit more thorough.

Alex

> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Jason Gunthorpe <jgg@mellanox.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: iommu@lists.linux-foundation.org
> ---
>  drivers/pci/p2pdma.c       | 27 +++++++++++++++++++++++++++
>  include/linux/pci-p2pdma.h |  6 ++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index c52298d76e64..620ac60babb5 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -797,3 +797,30 @@ ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
>         return sprintf(page, "%s\n", pci_name(p2p_dev));
>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_enable_show);
> +
> +bool pci_test_p2p(struct device *devA, struct device *devB)
> +{
> +       struct pci_dev *pciA, *pciB;
> +       bool ret;
> +       int tmp;
> +
> +       /*
> +        * For now we only support PCIE peer to peer but other inter-connect
> +        * can be added.
> +        */
> +       pciA = find_parent_pci_dev(devA);
> +       pciB = find_parent_pci_dev(devB);
> +       if (pciA == NULL || pciB == NULL) {
> +               ret = false;
> +               goto out;
> +       }
> +
> +       tmp = upstream_bridge_distance(pciA, pciB, NULL);
> +       ret = tmp < 0 ? false : true;
> +
> +out:
> +       pci_dev_put(pciB);
> +       pci_dev_put(pciA);
> +       return false;
> +}
> +EXPORT_SYMBOL_GPL(pci_test_p2p);
> diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h
> index bca9bc3e5be7..7671cc499a08 100644
> --- a/include/linux/pci-p2pdma.h
> +++ b/include/linux/pci-p2pdma.h
> @@ -36,6 +36,7 @@ int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev,
>                             bool *use_p2pdma);
>  ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
>                                bool use_p2pdma);
> +bool pci_test_p2p(struct device *devA, struct device *devB);
>  #else /* CONFIG_PCI_P2PDMA */
>  static inline int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar,
>                 size_t size, u64 offset)
> @@ -97,6 +98,11 @@ static inline ssize_t pci_p2pdma_enable_show(char *page,
>  {
>         return sprintf(page, "none\n");
>  }
> +
> +static inline bool pci_test_p2p(struct device *devA, struct device *devB)
> +{
> +       return false;
> +}
>  #endif /* CONFIG_PCI_P2PDMA */
>
>
> --
> 2.17.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jerome Glisse Jan. 29, 2019, 8 p.m. UTC | #5 
On Tue, Jan 29, 2019 at 02:56:38PM -0500, Alex Deucher wrote:
> On Tue, Jan 29, 2019 at 12:47 PM <jglisse@redhat.com> wrote:
> >
> > From: Jérôme Glisse <jglisse@redhat.com>
> >
> > device_test_p2p() return true if two devices can peer to peer to
> > each other. We add a generic function as different inter-connect
> > can support peer to peer and we want to genericaly test this no
> > matter what the inter-connect might be. However this version only
> > support PCIE for now.
> >
> 
> What about something like these patches:
> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=4fab9ff69cb968183f717551441b475fabce6c1c
> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=f90b12d41c277335d08c9dab62433f27c0fadbe5
> They are a bit more thorough.

Yes it would be better, i forgot about those. I can include them
next time i post. Thank you for reminding me about those :)

Cheers,
Jérôme
Logan Gunthorpe Jan. 29, 2019, 8:24 p.m. UTC | #6 
On 2019-01-29 12:56 p.m., Alex Deucher wrote:
> On Tue, Jan 29, 2019 at 12:47 PM <jglisse@redhat.com> wrote:
>>
>> From: Jérôme Glisse <jglisse@redhat.com>
>>
>> device_test_p2p() return true if two devices can peer to peer to
>> each other. We add a generic function as different inter-connect
>> can support peer to peer and we want to genericaly test this no
>> matter what the inter-connect might be. However this version only
>> support PCIE for now.
>>
> 
> What about something like these patches:
> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=4fab9ff69cb968183f717551441b475fabce6c1c
> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=f90b12d41c277335d08c9dab62433f27c0fadbe5
> They are a bit more thorough.

Those new functions seem to have a lot of overlap with the code that is
already upstream in p2pdma.... Perhaps you should be improving the
p2pdma functions if they aren't suitable for what you want already
instead of creating new ones.

Logan
Logan Gunthorpe Jan. 29, 2019, 8:44 p.m. UTC | #7 
On 2019-01-29 12:44 p.m., Greg Kroah-Hartman wrote:
> On Tue, Jan 29, 2019 at 11:24:09AM -0700, Logan Gunthorpe wrote:
>>
>>
>> On 2019-01-29 10:47 a.m., jglisse@redhat.com wrote:
>>> +bool pci_test_p2p(struct device *devA, struct device *devB)
>>> +{
>>> +	struct pci_dev *pciA, *pciB;
>>> +	bool ret;
>>> +	int tmp;
>>> +
>>> +	/*
>>> +	 * For now we only support PCIE peer to peer but other inter-connect
>>> +	 * can be added.
>>> +	 */
>>> +	pciA = find_parent_pci_dev(devA);
>>> +	pciB = find_parent_pci_dev(devB);
>>> +	if (pciA == NULL || pciB == NULL) {
>>> +		ret = false;
>>> +		goto out;
>>> +	}
>>> +
>>> +	tmp = upstream_bridge_distance(pciA, pciB, NULL);
>>> +	ret = tmp < 0 ? false : true;
>>> +
>>> +out:
>>> +	pci_dev_put(pciB);
>>> +	pci_dev_put(pciA);
>>> +	return false;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_test_p2p);
>>
>> This function only ever returns false....
> 
> I guess it was nevr actually tested :(
> 
> I feel really worried about passing random 'struct device' pointers into
> the PCI layer.  Are we _sure_ it can handle this properly?

Yes, there are a couple of pci_p2pdma functions that take struct devices
directly simply because it's way more convenient for the caller. That's
what find_parent_pci_dev() takes care of (it returns false if the device
is not a PCI device). Whether that's appropriate here is hard to say
seeing we haven't seen any caller code.

Logan
Jerome Glisse Jan. 29, 2019, 9 p.m. UTC | #8 
On Tue, Jan 29, 2019 at 01:44:09PM -0700, Logan Gunthorpe wrote:
> 
> 
> On 2019-01-29 12:44 p.m., Greg Kroah-Hartman wrote:
> > On Tue, Jan 29, 2019 at 11:24:09AM -0700, Logan Gunthorpe wrote:
> >>
> >>
> >> On 2019-01-29 10:47 a.m., jglisse@redhat.com wrote:
> >>> +bool pci_test_p2p(struct device *devA, struct device *devB)
> >>> +{
> >>> +	struct pci_dev *pciA, *pciB;
> >>> +	bool ret;
> >>> +	int tmp;
> >>> +
> >>> +	/*
> >>> +	 * For now we only support PCIE peer to peer but other inter-connect
> >>> +	 * can be added.
> >>> +	 */
> >>> +	pciA = find_parent_pci_dev(devA);
> >>> +	pciB = find_parent_pci_dev(devB);
> >>> +	if (pciA == NULL || pciB == NULL) {
> >>> +		ret = false;
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	tmp = upstream_bridge_distance(pciA, pciB, NULL);
> >>> +	ret = tmp < 0 ? false : true;
> >>> +
> >>> +out:
> >>> +	pci_dev_put(pciB);
> >>> +	pci_dev_put(pciA);
> >>> +	return false;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(pci_test_p2p);
> >>
> >> This function only ever returns false....
> > 
> > I guess it was nevr actually tested :(
> > 
> > I feel really worried about passing random 'struct device' pointers into
> > the PCI layer.  Are we _sure_ it can handle this properly?
> 
> Yes, there are a couple of pci_p2pdma functions that take struct devices
> directly simply because it's way more convenient for the caller. That's
> what find_parent_pci_dev() takes care of (it returns false if the device
> is not a PCI device). Whether that's appropriate here is hard to say
> seeing we haven't seen any caller code.

Caller code as a reference (i already given that link in other part of
thread but just so that people don't have to follow all branches).

https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-p2p&id=401a567696eafb1d4faf7054ab0d7c3a16a5ef06

Cheers,
Jérôme
Alex Deucher Jan. 29, 2019, 9:28 p.m. UTC | #9 
On Tue, Jan 29, 2019 at 3:25 PM Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
>
> On 2019-01-29 12:56 p.m., Alex Deucher wrote:
> > On Tue, Jan 29, 2019 at 12:47 PM <jglisse@redhat.com> wrote:
> >>
> >> From: Jérôme Glisse <jglisse@redhat.com>
> >>
> >> device_test_p2p() return true if two devices can peer to peer to
> >> each other. We add a generic function as different inter-connect
> >> can support peer to peer and we want to genericaly test this no
> >> matter what the inter-connect might be. However this version only
> >> support PCIE for now.
> >>
> >
> > What about something like these patches:
> > https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=4fab9ff69cb968183f717551441b475fabce6c1c
> > https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=f90b12d41c277335d08c9dab62433f27c0fadbe5
> > They are a bit more thorough.
>
> Those new functions seem to have a lot of overlap with the code that is
> already upstream in p2pdma.... Perhaps you should be improving the
> p2pdma functions if they aren't suitable for what you want already
> instead of creating new ones.

Could be.  Those patches are pretty old.  They probably need to be
rebased on the latest upstream p2p stuff.

Alex
Christian König Jan. 30, 2019, 10:25 a.m. UTC | #10 
Am 29.01.19 um 21:24 schrieb Logan Gunthorpe:
>
> On 2019-01-29 12:56 p.m., Alex Deucher wrote:
>> On Tue, Jan 29, 2019 at 12:47 PM <jglisse@redhat.com> wrote:
>>> From: Jérôme Glisse <jglisse@redhat.com>
>>>
>>> device_test_p2p() return true if two devices can peer to peer to
>>> each other. We add a generic function as different inter-connect
>>> can support peer to peer and we want to genericaly test this no
>>> matter what the inter-connect might be. However this version only
>>> support PCIE for now.
>>>
>> What about something like these patches:
>> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=4fab9ff69cb968183f717551441b475fabce6c1c
>> https://cgit.freedesktop.org/~deathsimple/linux/commit/?h=p2p&id=f90b12d41c277335d08c9dab62433f27c0fadbe5
>> They are a bit more thorough.
> Those new functions seem to have a lot of overlap with the code that is
> already upstream in p2pdma.... Perhaps you should be improving the
> p2pdma functions if they aren't suitable for what you want already
> instead of creating new ones.

Yeah, well that's what I was suggesting for the very beginning :)

But completely agree the existing functions should be improved instead 
of adding new ones,
Christian.

>
> Logan
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index c52298d76e64..620ac60babb5 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -797,3 +797,30 @@  ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
 	return sprintf(page, "%s\n", pci_name(p2p_dev));
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_enable_show);
+
+bool pci_test_p2p(struct device *devA, struct device *devB)
+{
+	struct pci_dev *pciA, *pciB;
+	bool ret;
+	int tmp;
+
+	/*
+	 * For now we only support PCIE peer to peer but other inter-connect
+	 * can be added.
+	 */
+	pciA = find_parent_pci_dev(devA);
+	pciB = find_parent_pci_dev(devB);
+	if (pciA == NULL || pciB == NULL) {
+		ret = false;
+		goto out;
+	}
+
+	tmp = upstream_bridge_distance(pciA, pciB, NULL);
+	ret = tmp < 0 ? false : true;
+
+out:
+	pci_dev_put(pciB);
+	pci_dev_put(pciA);
+	return false;
+}
+EXPORT_SYMBOL_GPL(pci_test_p2p);
diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h
index bca9bc3e5be7..7671cc499a08 100644
--- a/include/linux/pci-p2pdma.h
+++ b/include/linux/pci-p2pdma.h
@@ -36,6 +36,7 @@  int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev,
 			    bool *use_p2pdma);
 ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev,
 			       bool use_p2pdma);
+bool pci_test_p2p(struct device *devA, struct device *devB);
 #else /* CONFIG_PCI_P2PDMA */
 static inline int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar,
 		size_t size, u64 offset)
@@ -97,6 +98,11 @@  static inline ssize_t pci_p2pdma_enable_show(char *page,
 {
 	return sprintf(page, "none\n");
 }
+
+static inline bool pci_test_p2p(struct device *devA, struct device *devB)
+{
+	return false;
+}
 #endif /* CONFIG_PCI_P2PDMA */