[PATCH v1 0/6] P2PDMA Cleanup

[PATCH v1 0/6] P2PDMA Cleanup

[Logan Gunthorpe]

Hi Bjorn,

This patch series consists of the P2PDMA cleanup and prep patches based
on feedback from  my P2PDMA mapping operations series (most recently
posted at [1]). I've reduced the recipient list of this series to those
that I thought would be interested or have provided the feedback that
inspired these patches.

Please consider taking these patches in the near term ahead of my mapping
ops series. These patches are largely cleanup and other minor fixes. The only
functional change is Patch 4 which adds a new warning that was suggested by
Don.

Patch 6 arguably isn't necessary yet as we don't care about sleeping
yet -- but it'd be a nice to have to reduce the number of prep patches for my
other series. However, if you don't want to take this patch now, I can
carry it in my other series.

I'm happy to make further fixes and update this series if anyone finds any
additional issues on review.

Thanks,

Logan

[1] https://lore.kernel.org/linux-block/20210513223203.5542-1-logang@deltatee.com/

--

Logan Gunthorpe (6):
  PCI/P2PDMA: Rename upstream_bridge_distance() and rework documentation
  PCI/P2PDMA: Use a buffer on the stack for collecting the acs list
  PCI/P2PDMA: Cleanup type for return value of calc_map_type_and_dist()
  PCI/P2PDMA: Print a warning if the host bridge is not in the whitelist
  PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take pagemap and device
  PCI/P2PDMA: Avoid pci_get_slot() which sleeps

 drivers/pci/p2pdma.c | 157 +++++++++++++++++++++++++------------------
 1 file changed, 92 insertions(+), 65 deletions(-)


base-commit: 614124bea77e452aa6df7a8714e8bc820b489922
--
2.20.1

[PATCH v1 1/6] PCI/P2PDMA: Rename upstream_bridge_distance() and rework documentation

[Logan Gunthorpe]

The function upstream_bridge_distance() has evolved such that it's name
is no longer entirely reflective of what the function does.

The function not only calculates the distance between two peers but also
calculates how the DMA addresses for those two peers should be mapped.

Thus, rename the function to calc_map_type_and_dist() and rework the
documentation to better describe the two pieces of information the
function returns.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 63 ++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 196382630363..6f90e9812f6e 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -354,7 +354,7 @@ static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
 }
 
 static enum pci_p2pdma_map_type
-__upstream_bridge_distance(struct pci_dev *provider, struct pci_dev *client,
+__calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 		int *dist, bool *acs_redirects, struct seq_buf *acs_list)
 {
 	struct pci_dev *a = provider, *b = client, *bb;
@@ -433,17 +433,18 @@ static unsigned long map_types_idx(struct pci_dev *client)
 }
 
 /*
- * Find the distance through the nearest common upstream bridge between
- * two PCI devices.
+ * Calculate the P2PDMA mapping type and distance between two PCI devices.
  *
- * If the two devices are the same device then 0 will be returned.
+ * If the two devices are the same device then PCI_P2PDMA_MAP_BUS_ADDR
+ * and a distance of 0 will be returned.
  *
  * If there are two virtual functions of the same device behind the same
- * bridge port then 2 will be returned (one step down to the PCIe switch,
- * then one step back to the same device).
+ * bridge port then PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 will be
+ * returned (one step down to the PCIe switch, then one step back to the
+ * same device).
  *
  * In the case where two devices are connected to the same PCIe switch, the
- * value 4 will be returned. This corresponds to the following PCI tree:
+ * distance of 4 will be returned. This corresponds to the following PCI tree:
  *
  *     -+  Root Port
  *      \+ Switch Upstream Port
@@ -454,31 +455,31 @@ static unsigned long map_types_idx(struct pci_dev *client)
  *
  * The distance is 4 because we traverse from Device A through the downstream
  * port of the switch, to the common upstream port, back up to the second
- * downstream port and then to Device B.
- *
- * Any two devices that cannot communicate using p2pdma will return
- * PCI_P2PDMA_MAP_NOT_SUPPORTED.
+ * downstream port and then to Device B. The mapping type returned will depend
+ * on the ACS redirection setting of the bridges along the path. If ACS
+ * redirect is set on any bridge port in the path then the TLPs will go through
+ * the host bridge. Otherwise PCI_P2PDMA_MAP_BUS_ADDR is returned.
  *
  * Any two devices that have a data path that goes through the host bridge
- * will consult a whitelist. If the host bridges are on the whitelist,
- * this function will return PCI_P2PDMA_MAP_THRU_HOST_BRIDGE.
- *
- * If either bridge is not on the whitelist this function returns
- * PCI_P2PDMA_MAP_NOT_SUPPORTED.
+ * will consult a whitelist. If the host bridge is in the whitelist,
+ * this function will return PCI_P2PDMA_MAP_THRU_HOST_BRIDGE with the
+ * distance set to the number of ports per above. If the device is not
+ * in the whitelist the type will be returned PCI_P2PDMA_MAP_NOT_SUPPORTED.
  *
- * If a bridge which has any ACS redirection bits set is in the path,
- * acs_redirects will be set to true. In this case, a list of all infringing
- * bridge addresses will be populated in acs_list (assuming it's non-null)
- * for printk purposes.
+ * If any ACS redirect bits are set, then the acs_redirects boolean will be
+ * set to true and their pci device name will be appended to the acs_list
+ * seq_buf. This seq_buf is used to print a warning informing the user
+ * how to disable ACS using a command line parameter.
+ * (See calc_map_type_and_dist_warn() below)
  */
 static enum pci_p2pdma_map_type
-upstream_bridge_distance(struct pci_dev *provider, struct pci_dev *client,
+calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 		int *dist, bool *acs_redirects, struct seq_buf *acs_list)
 {
 	enum pci_p2pdma_map_type map_type;
 
-	map_type = __upstream_bridge_distance(provider, client, dist,
-					      acs_redirects, acs_list);
+	map_type = __calc_map_type_and_dist(provider, client, dist,
+					    acs_redirects, acs_list);
 
 	if (map_type == PCI_P2PDMA_MAP_THRU_HOST_BRIDGE) {
 		if (!cpu_supports_p2pdma() &&
@@ -494,8 +495,8 @@ upstream_bridge_distance(struct pci_dev *provider, struct pci_dev *client,
 }
 
 static enum pci_p2pdma_map_type
-upstream_bridge_distance_warn(struct pci_dev *provider, struct pci_dev *client,
-			      int *dist)
+calc_map_type_and_dist_warn(struct pci_dev *provider, struct pci_dev *client,
+			    int *dist)
 {
 	struct seq_buf acs_list;
 	bool acs_redirects;
@@ -505,8 +506,8 @@ upstream_bridge_distance_warn(struct pci_dev *provider, struct pci_dev *client,
 	if (!acs_list.buffer)
 		return -ENOMEM;
 
-	ret = upstream_bridge_distance(provider, client, dist, &acs_redirects,
-				       &acs_list);
+	ret = calc_map_type_and_dist(provider, client, dist, &acs_redirects,
+				     &acs_list);
 	if (acs_redirects) {
 		pci_warn(client, "ACS redirect is set between the client and provider (%s)\n",
 			 pci_name(provider));
@@ -565,11 +566,11 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 		}
 
 		if (verbose)
-			ret = upstream_bridge_distance_warn(provider,
-					pci_client, &distance);
+			ret = calc_map_type_and_dist_warn(provider, pci_client,
+							  &distance);
 		else
-			ret = upstream_bridge_distance(provider, pci_client,
-						       &distance, NULL, NULL);
+			ret = calc_map_type_and_dist(provider, pci_client,
+						     &distance, NULL, NULL);
 
 		pci_dev_put(pci_client);
 
-- 
2.20.1

[PATCH v1 2/6] PCI/P2PDMA: Use a buffer on the stack for collecting the acs list

[Logan Gunthorpe]

In order to call the calc_map_type_and_dist_warn() function from
a dma_map operation, the function must not sleep. The only reason
it sleeps is to allocate memory for the seq_buf to print a verbose
warning telling the user how to disable ACS for that path.

Instead of allocating the memory with kmalloc, allocate it on
the stack with a smaller buffer. A 128B buffer is enough to print
10 pci device names. A system with 10 bridge ports between two devices
that have ACS enabled would be unusually large, so this should
still be a reasonable limit.

This also allows cleaning up the awkward (and broken) return with
-ENOMEM which contradicts the return type and the caller was
not prepared for.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 6f90e9812f6e..3a5fb63c5f2c 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -500,11 +500,10 @@ calc_map_type_and_dist_warn(struct pci_dev *provider, struct pci_dev *client,
 {
 	struct seq_buf acs_list;
 	bool acs_redirects;
+	char buf[128];
 	int ret;
 
-	seq_buf_init(&acs_list, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE);
-	if (!acs_list.buffer)
-		return -ENOMEM;
+	seq_buf_init(&acs_list, buf, sizeof(buf));
 
 	ret = calc_map_type_and_dist(provider, client, dist, &acs_redirects,
 				     &acs_list);
@@ -522,8 +521,6 @@ calc_map_type_and_dist_warn(struct pci_dev *provider, struct pci_dev *client,
 			 pci_name(provider));
 	}
 
-	kfree(acs_list.buffer);
-
 	return ret;
 }
 
-- 
2.20.1

[PATCH v1 3/6] PCI/P2PDMA: Cleanup type for return value of calc_map_type_and_dist()

[Logan Gunthorpe]

Instead of using an int for the return value of this function use the
correct enum pci_p2pdma_map_type.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 3a5fb63c5f2c..09c864f193d2 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -544,11 +544,11 @@ calc_map_type_and_dist_warn(struct pci_dev *provider, struct pci_dev *client,
 int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 			     int num_clients, bool verbose)
 {
+	enum pci_p2pdma_map_type map;
 	bool not_supported = false;
 	struct pci_dev *pci_client;
 	int total_dist = 0;
-	int distance;
-	int i, ret;
+	int i, distance;
 
 	if (num_clients == 0)
 		return -1;
@@ -563,15 +563,15 @@ int pci_p2pdma_distance_many(struct pci_dev *provider, struct device **clients,
 		}
 
 		if (verbose)
-			ret = calc_map_type_and_dist_warn(provider, pci_client,
+			map = calc_map_type_and_dist_warn(provider, pci_client,
 							  &distance);
 		else
-			ret = calc_map_type_and_dist(provider, pci_client,
+			map = calc_map_type_and_dist(provider, pci_client,
 						     &distance, NULL, NULL);
 
 		pci_dev_put(pci_client);
 
-		if (ret == PCI_P2PDMA_MAP_NOT_SUPPORTED)
+		if (map == PCI_P2PDMA_MAP_NOT_SUPPORTED)
 			not_supported = true;
 
 		if (not_supported && !verbose)
-- 
2.20.1

[PATCH v1 4/6] PCI/P2PDMA: Print a warning if the host bridge is not in the whitelist

[Logan Gunthorpe]

If the host bridge is not in the whitelist print a warning in the
calc_map_type_and_dist_warn() path detailing the vendor and device IDs
that would need to be added to the whitelist.

Suggested-by: Don Dutile <ddutile@redhat.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 09c864f193d2..2de4a9e2da58 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -309,7 +309,7 @@ static const struct pci_p2pdma_whitelist_entry {
 };
 
 static bool __host_bridge_whitelist(struct pci_host_bridge *host,
-				    bool same_host_bridge)
+				    bool same_host_bridge, bool warn)
 {
 	struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
 	const struct pci_p2pdma_whitelist_entry *entry;
@@ -331,6 +331,10 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host,
 		return true;
 	}
 
+	if (warn)
+		pci_warn(root, "Host bridge not in P2PDMA whitelist: %04x:%04x\n",
+			 vendor, device);
+
 	return false;
 }
 
@@ -338,16 +342,17 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host,
  * If we can't find a common upstream bridge take a look at the root
  * complex and compare it to a whitelist of known good hardware.
  */
-static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
+static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b,
+				  bool warn)
 {
 	struct pci_host_bridge *host_a = pci_find_host_bridge(a->bus);
 	struct pci_host_bridge *host_b = pci_find_host_bridge(b->bus);
 
 	if (host_a == host_b)
-		return __host_bridge_whitelist(host_a, true);
+		return __host_bridge_whitelist(host_a, true, warn);
 
-	if (__host_bridge_whitelist(host_a, false) &&
-	    __host_bridge_whitelist(host_b, false))
+	if (__host_bridge_whitelist(host_a, false, warn) &&
+	    __host_bridge_whitelist(host_b, false, warn))
 		return true;
 
 	return false;
@@ -483,7 +488,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 
 	if (map_type == PCI_P2PDMA_MAP_THRU_HOST_BRIDGE) {
 		if (!cpu_supports_p2pdma() &&
-		    !host_bridge_whitelist(provider, client))
+		    !host_bridge_whitelist(provider, client, acs_redirects))
 			map_type = PCI_P2PDMA_MAP_NOT_SUPPORTED;
 	}
 
-- 
2.20.1

[PATCH v1 5/6] PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take pagemap and device

[Logan Gunthorpe]

All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a
struct device (of the client doing the DMA transfer). Thus move the
conversion to struct pci_devs for the provider and client into this
function.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 2de4a9e2da58..5dc1f9f62a93 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -814,12 +814,20 @@ void pci_p2pmem_publish(struct pci_dev *pdev, bool publish)
 }
 EXPORT_SYMBOL_GPL(pci_p2pmem_publish);
 
-static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct pci_dev *provider,
-						    struct pci_dev *client)
+static enum pci_p2pdma_map_type pci_p2pdma_map_type(struct dev_pagemap *pgmap,
+						    struct device *dev)
 {
+	struct pci_dev *provider = to_p2p_pgmap(pgmap)->provider;
+	struct pci_dev *client;
+
 	if (!provider->p2pdma)
 		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
 
+	if (!dev_is_pci(dev))
+		return PCI_P2PDMA_MAP_NOT_SUPPORTED;
+
+	client = to_pci_dev(dev);
+
 	return xa_to_value(xa_load(&provider->p2pdma->map_types,
 				   map_types_idx(client)));
 }
@@ -856,14 +864,8 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 {
 	struct pci_p2pdma_pagemap *p2p_pgmap =
 		to_p2p_pgmap(sg_page(sg)->pgmap);
-	struct pci_dev *client;
-
-	if (WARN_ON_ONCE(!dev_is_pci(dev)))
-		return 0;
 
-	client = to_pci_dev(dev);
-
-	switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) {
+	switch (pci_p2pdma_map_type(sg_page(sg)->pgmap, dev)) {
 	case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE:
 		return dma_map_sg_attrs(dev, sg, nents, dir, attrs);
 	case PCI_P2PDMA_MAP_BUS_ADDR:
@@ -887,17 +889,9 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_map_sg_attrs);
 void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
 		int nents, enum dma_data_direction dir, unsigned long attrs)
 {
-	struct pci_p2pdma_pagemap *p2p_pgmap =
-		to_p2p_pgmap(sg_page(sg)->pgmap);
 	enum pci_p2pdma_map_type map_type;
-	struct pci_dev *client;
-
-	if (WARN_ON_ONCE(!dev_is_pci(dev)))
-		return;
-
-	client = to_pci_dev(dev);
 
-	map_type = pci_p2pdma_map_type(p2p_pgmap->provider, client);
+	map_type = pci_p2pdma_map_type(sg_page(sg)->pgmap, dev);
 
 	if (map_type == PCI_P2PDMA_MAP_THRU_HOST_BRIDGE)
 		dma_unmap_sg_attrs(dev, sg, nents, dir, attrs);
-- 
2.20.1

[PATCH v1 6/6] PCI/P2PDMA: Avoid pci_get_slot() which sleeps

[Logan Gunthorpe]

In order to use upstream_bridge_distance_warn() from a dma_map function,
it must not sleep. However, pci_get_slot() takes the pci_bus_sem so it
might sleep.

In order to avoid this, try to get the host bridge's device from the first
element in the device list. It should be impossible for the host bridge's
device to go away while references are held on child devices, so the first
element should not be able to change and, thus, this should be safe.

Introduce a static function called pci_host_bridge_dev() to obtain the
host bridge's root device.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/p2pdma.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 5dc1f9f62a93..c7ecd0196102 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -308,10 +308,41 @@ static const struct pci_p2pdma_whitelist_entry {
 	{}
 };
 
+/*
+ * This lookup function tries to find the PCI device corresponding to a given
+ * host bridge.
+ *
+ * It assumes the host bridge device is the first PCI device in the
+ * bus->devices list and that the devfn is 00.0. These assumptions should hold
+ * for all the devices in the whitelist above.
+ *
+ * This function is equivalent to pci_get_slot(host->bus, 0), however it does
+ * not take the pci_bus_sem lock seeing __host_bridge_whitelist() must not
+ * sleep.
+ *
+ * For this to be safe, the caller should hold a reference to a device on the
+ * bridge, which should ensure the host_bridge device will not be freed
+ * or removed from the head of the devices list.
+ */
+static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
+{
+	struct pci_dev *root;
+
+	root = list_first_entry_or_null(&host->bus->devices,
+					struct pci_dev, bus_list);
+
+	if (!root)
+		return NULL;
+	if (root->devfn != PCI_DEVFN(0, 0))
+		return NULL;
+
+	return root;
+}
+
 static bool __host_bridge_whitelist(struct pci_host_bridge *host,
 				    bool same_host_bridge, bool warn)
 {
-	struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0));
+	struct pci_dev *root = pci_host_bridge_dev(host);
 	const struct pci_p2pdma_whitelist_entry *entry;
 	unsigned short vendor, device;
 
@@ -320,7 +351,6 @@ static bool __host_bridge_whitelist(struct pci_host_bridge *host,
 
 	vendor = root->vendor;
 	device = root->device;
-	pci_dev_put(root);
 
 	for (entry = pci_p2pdma_whitelist; entry->vendor; entry++) {
 		if (vendor != entry->vendor || device != entry->device)
-- 
2.20.1

Re: [PATCH v1 1/6] PCI/P2PDMA: Rename upstream_bridge_distance() and rework documentation

[Bjorn Helgaas]

On Thu, Jun 10, 2021 at 10:06:04AM -0600, Logan Gunthorpe wrote:
> The function upstream_bridge_distance() has evolved such that it's name
> is no longer entirely reflective of what the function does.
> 
> The function not only calculates the distance between two peers but also
> calculates how the DMA addresses for those two peers should be mapped.
> 
> Thus, rename the function to calc_map_type_and_dist() and rework the
> documentation to better describe the two pieces of information the
> function returns.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/p2pdma.c | 63 ++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 196382630363..6f90e9812f6e 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -354,7 +354,7 @@ static bool host_bridge_whitelist(struct pci_dev *a, struct pci_dev *b)
>  }
>  
>  static enum pci_p2pdma_map_type
> -__upstream_bridge_distance(struct pci_dev *provider, struct pci_dev *client,
> +__calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  		int *dist, bool *acs_redirects, struct seq_buf *acs_list)
>  {
>  	struct pci_dev *a = provider, *b = client, *bb;
> @@ -433,17 +433,18 @@ static unsigned long map_types_idx(struct pci_dev *client)
>  }
>  
>  /*
> - * Find the distance through the nearest common upstream bridge between
> - * two PCI devices.
> + * Calculate the P2PDMA mapping type and distance between two PCI devices.
>   *
> - * If the two devices are the same device then 0 will be returned.
> + * If the two devices are the same device then PCI_P2PDMA_MAP_BUS_ADDR
> + * and a distance of 0 will be returned.
>   *
>   * If there are two virtual functions of the same device behind the same
> - * bridge port then 2 will be returned (one step down to the PCIe switch,
> - * then one step back to the same device).
> + * bridge port then PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 will be
> + * returned (one step down to the PCIe switch, then one step back to the
> + * same device).

The new text is:

  If there are two virtual functions of the same device behind the same
  bridge port then PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 will be
  returned (one step down to the PCIe switch, then one step back to the
  same device).

I *think* this includes two functions of the same multi-function
device, or two virtual functions of the same device, right?  In both
cases, the two devices are obviously behind the same bridge port.

Is this usage of "down to the PCIe switch" the common usage in P2PDMA?
I normally think of going from an endpoint to a switch as being "up"
toward the CPU.  But PCIe made it all confusing by putting downstream
ports at the upstream end of links and vice versa.

We also have a bit of a mix in terminology between "bridge," "switch,"
"bridge port."  I'd probably write something like:

  If they are two functions of the same device behind the same bridge,
  return PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 (one hop up to
  the bridge, then one hop back down to another function of the same
  device).

No need to repost for this; just let me know what you think and I can
tweak accordingly.

Bjorn

Re: [PATCH v1 1/6] PCI/P2PDMA: Rename upstream_bridge_distance() and rework documentation

[Logan Gunthorpe]

On 2021-06-10 4:05 p.m., Bjorn Helgaas wrote:
> The new text is:
> 
>   If there are two virtual functions of the same device behind the same
>   bridge port then PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 will be
>   returned (one step down to the PCIe switch, then one step back to the
>   same device).
> 
> I *think* this includes two functions of the same multi-function
> device, or two virtual functions of the same device, right?  In both
> cases, the two devices are obviously behind the same bridge port.

Yes, that's correct, if it's the same device it must be behind the same
bridge port; so dropping the "behind the same bridge port" is a good idea.

> 
> Is this usage of "down to the PCIe switch" the common usage in P2PDMA?
> I normally think of going from an endpoint to a switch as being "up"
> toward the CPU.  But PCIe made it all confusing by putting downstream
> ports at the upstream end of links and vice versa.

Good point. I've been casually saying "down to", but you are right "up"
makes a lot more sense given the downstream/upstream terminology.

> We also have a bit of a mix in terminology between "bridge," "switch,"
> "bridge port."  I'd probably write something like:
> 
>   If they are two functions of the same device behind the same bridge,
>   return PCI_P2PDMA_MAP_BUS_ADDR and a distance of 2 (one hop up to
>   the bridge, then one hop back down to another function of the same
>   device).
> 
> No need to repost for this; just let me know what you think and I can
> tweak accordingly.

What you've written sounds good to me, but I might have just dropped the
"behind the same bridge" entirely given your feedback above.

Thanks!

Logan

Re: [PATCH v1 0/6] P2PDMA Cleanup

[Bjorn Helgaas]

On Thu, Jun 10, 2021 at 10:06:03AM -0600, Logan Gunthorpe wrote:
> Hi Bjorn,
> 
> This patch series consists of the P2PDMA cleanup and prep patches based
> on feedback from  my P2PDMA mapping operations series (most recently
> posted at [1]). I've reduced the recipient list of this series to those
> that I thought would be interested or have provided the feedback that
> inspired these patches.
> 
> Please consider taking these patches in the near term ahead of my mapping
> ops series. These patches are largely cleanup and other minor fixes. The only
> functional change is Patch 4 which adds a new warning that was suggested by
> Don.
> 
> Patch 6 arguably isn't necessary yet as we don't care about sleeping
> yet -- but it'd be a nice to have to reduce the number of prep patches for my
> other series. However, if you don't want to take this patch now, I can
> carry it in my other series.
> 
> I'm happy to make further fixes and update this series if anyone finds any
> additional issues on review.
> 
> Thanks,
> 
> Logan
> 
> [1] https://lore.kernel.org/linux-block/20210513223203.5542-1-logang@deltatee.com/
> 
> --
> 
> Logan Gunthorpe (6):
>   PCI/P2PDMA: Rename upstream_bridge_distance() and rework documentation
>   PCI/P2PDMA: Use a buffer on the stack for collecting the acs list
>   PCI/P2PDMA: Cleanup type for return value of calc_map_type_and_dist()
>   PCI/P2PDMA: Print a warning if the host bridge is not in the whitelist
>   PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take pagemap and device
>   PCI/P2PDMA: Avoid pci_get_slot() which sleeps
> 
>  drivers/pci/p2pdma.c | 157 +++++++++++++++++++++++++------------------
>  1 file changed, 92 insertions(+), 65 deletions(-)

Applied all 6 to pci/p2pdma for v5.14, thanks!

  6389d4374522 ("PCI/P2PDMA: Rename upstream_bridge_distance() and rework doc")
  e4ece59abd70 ("PCI/P2PDMA: Collect acs list in stack buffer to avoid sleeping")
  f9c125b9eb30 ("PCI/P2PDMA: Use correct calc_map_type_and_dist() return type")
  cf201bfe8cdc ("PCI/P2PDMA: Warn if host bridge not in whitelist")
  7e2faa1710c4 ("PCI/P2PDMA: Refactor pci_p2pdma_map_type()")
  3ec0c3ec2d92 ("PCI/P2PDMA: Avoid pci_get_slot(), which may sleep")