[PATCH v6 0/5] PCI: Improve PCIe link status reporting

[PATCH v6 0/5] PCI: Improve PCIe link status reporting

[Bjorn Helgaas]

This is based on Tal's recent work to unify the approach for reporting PCIe
link speed/width and whether the device is being limited by a slower
upstream link.

The new pcie_print_link_status() interface appeared in v4.17-rc1; see
9e506a7b5147 ("PCI: Add pcie_print_link_status() to log link speed and
whether it's limited").

That's a good way to replace use of pcie_get_minimum_link(), which gives
misleading results when a path contains both a fast, narrow link and a
slow, wide link: it reports the equivalent of a slow, narrow link.

This series removes the remaining uses of pcie_get_minimum_link() and then
removes the interface itself.  I'd like to merge them all through the PCI
tree to make the removal easy.

This does change the dmesg reporting of link speeds, and in the ixgbe case,
it changes the reporting from KERN_WARN level to KERN_INFO.  If that's an
issue, let's talk about it.  I'm hoping the reduce code size, improved
functionality, and consistency across drivers is enough to make this
worthwhile.

---

Bjorn Helgaas (5):
      bnx2x: Report PCIe link properties with pcie_print_link_status()
      bnxt_en: Report PCIe link properties with pcie_print_link_status()
      cxgb4: Report PCIe link properties with pcie_print_link_status()
      ixgbe: Report PCIe link properties with pcie_print_link_status()
      PCI: Remove unused pcie_get_minimum_link()


 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   23 ++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.c        |   19 ------
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  |   75 ----------------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   47 --------------
 drivers/pci/pci.c                                |   43 -------------
 include/linux/pci.h                              |    2 -
 6 files changed, 9 insertions(+), 200 deletions(-)

[PATCH v6 1/5] bnx2x: Report PCIe link properties with pcie_print_link_status()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously the driver used pcie_get_minimum_link() to warn when the NIC
is in a slot that can't supply as much bandwidth as the NIC could use.

pcie_get_minimum_link() can be misleading because it finds the slowest link
and the narrowest link (which may be different links) without considering
the total bandwidth of each link.  For a path with a 16 GT/s x1 link and a
2.5 GT/s x16 link, it returns 2.5 GT/s x1, which corresponds to 250 MB/s of
bandwidth, not the true available bandwidth of about 1969 MB/s for a
16 GT/s x1 link.

Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.  This finds
the slowest link in the path to the device by computing the total bandwidth
of each link and compares that with the capabilities of the device.

The dmesg change is:

  - %s (%c%d) PCI-E x%d %s found at mem %lx, IRQ %d, node addr %pM
  + %s (%c%d) PCI-E found at mem %lx, IRQ %d, node addr %pM
  + %u.%03u Gb/s available PCIe bandwidth (%s x%d link)

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   23 ++++++----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index c766ae23bc74..5b1ed240bf18 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -13922,8 +13922,6 @@ static int bnx2x_init_one(struct pci_dev *pdev,
 {
 	struct net_device *dev = NULL;
 	struct bnx2x *bp;
-	enum pcie_link_width pcie_width;
-	enum pci_bus_speed pcie_speed;
 	int rc, max_non_def_sbs;
 	int rx_count, tx_count, rss_count, doorbell_size;
 	int max_cos_est;
@@ -14091,21 +14089,12 @@ static int bnx2x_init_one(struct pci_dev *pdev,
 		dev_addr_add(bp->dev, bp->fip_mac, NETDEV_HW_ADDR_T_SAN);
 		rtnl_unlock();
 	}
-	if (pcie_get_minimum_link(bp->pdev, &pcie_speed, &pcie_width) ||
-	    pcie_speed == PCI_SPEED_UNKNOWN ||
-	    pcie_width == PCIE_LNK_WIDTH_UNKNOWN)
-		BNX2X_DEV_INFO("Failed to determine PCI Express Bandwidth\n");
-	else
-		BNX2X_DEV_INFO(
-		       "%s (%c%d) PCI-E x%d %s found at mem %lx, IRQ %d, node addr %pM\n",
-		       board_info[ent->driver_data].name,
-		       (CHIP_REV(bp) >> 12) + 'A', (CHIP_METAL(bp) >> 4),
-		       pcie_width,
-		       pcie_speed == PCIE_SPEED_2_5GT ? "2.5GHz" :
-		       pcie_speed == PCIE_SPEED_5_0GT ? "5.0GHz" :
-		       pcie_speed == PCIE_SPEED_8_0GT ? "8.0GHz" :
-		       "Unknown",
-		       dev->base_addr, bp->pdev->irq, dev->dev_addr);
+	BNX2X_DEV_INFO(
+	       "%s (%c%d) PCI-E found at mem %lx, IRQ %d, node addr %pM\n",
+	       board_info[ent->driver_data].name,
+	       (CHIP_REV(bp) >> 12) + 'A', (CHIP_METAL(bp) >> 4),
+	       dev->base_addr, bp->pdev->irq, dev->dev_addr);
+	pcie_print_link_status(bp->pdev);
 
 	bnx2x_register_phc(bp);
 

[PATCH v6 2/5] bnxt_en: Report PCIe link properties with pcie_print_link_status()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously the driver used pcie_get_minimum_link() to warn when the NIC
is in a slot that can't supply as much bandwidth as the NIC could use.

pcie_get_minimum_link() can be misleading because it finds the slowest link
and the narrowest link (which may be different links) without considering
the total bandwidth of each link.  For a path with a 16 GT/s x1 link and a
2.5 GT/s x16 link, it returns 2.5 GT/s x1, which corresponds to 250 MB/s of
bandwidth, not the true available bandwidth of about 1969 MB/s for a
16 GT/s x1 link.

Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.  This finds
the slowest link in the path to the device by computing the total bandwidth
of each link and compares that with the capabilities of the device.

The dmesg change is:

  - PCIe: Speed %s Width x%d
  + %u.%03u Gb/s available PCIe bandwidth (%s x%d link)

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f83769d8047b..34fddb48fecc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -8621,22 +8621,6 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
 	return rc;
 }
 
-static void bnxt_parse_log_pcie_link(struct bnxt *bp)
-{
-	enum pcie_link_width width = PCIE_LNK_WIDTH_UNKNOWN;
-	enum pci_bus_speed speed = PCI_SPEED_UNKNOWN;
-
-	if (pcie_get_minimum_link(pci_physfn(bp->pdev), &speed, &width) ||
-	    speed == PCI_SPEED_UNKNOWN || width == PCIE_LNK_WIDTH_UNKNOWN)
-		netdev_info(bp->dev, "Failed to determine PCIe Link Info\n");
-	else
-		netdev_info(bp->dev, "PCIe: Speed %s Width x%d\n",
-			    speed == PCIE_SPEED_2_5GT ? "2.5GT/s" :
-			    speed == PCIE_SPEED_5_0GT ? "5.0GT/s" :
-			    speed == PCIE_SPEED_8_0GT ? "8.0GT/s" :
-			    "Unknown", width);
-}
-
 static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int version_printed;
@@ -8851,8 +8835,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netdev_info(dev, "%s found at mem %lx, node addr %pM\n",
 		    board_info[ent->driver_data].name,
 		    (long)pci_resource_start(pdev, 0), dev->dev_addr);
-
-	bnxt_parse_log_pcie_link(bp);
+	pcie_print_link_status(pdev);
 
 	return 0;
 

[PATCH v6 3/5] cxgb4: Report PCIe link properties with pcie_print_link_status()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously the driver used pcie_get_minimum_link() to warn when the NIC
is in a slot that can't supply as much bandwidth as the NIC could use.

pcie_get_minimum_link() can be misleading because it finds the slowest link
and the narrowest link (which may be different links) without considering
the total bandwidth of each link.  For a path with a 16 GT/s x1 link and a
2.5 GT/s x16 link, it returns 2.5 GT/s x1, which corresponds to 250 MB/s of
bandwidth, not the true available bandwidth of about 1969 MB/s for a
16 GT/s x1 link.

Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.  This finds
the slowest link in the path to the device by computing the total bandwidth
of each link and compares that with the capabilities of the device.

The dmesg change is:

  - PCIe link speed is %s, device supports %s
  - PCIe link width is x%d, device supports x%d
  + %u.%03u Gb/s available PCIe bandwidth (%s x%d link)

or, if the device is capable of better performance than is available in the
current slot:

  - A slot with more lanes and/or higher speed is suggested for optimal performance.
  + %u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c |   75 -----------------------
 1 file changed, 1 insertion(+), 74 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 24d2865b8806..7328f24ba1dd 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -5042,79 +5042,6 @@ static int init_rss(struct adapter *adap)
 	return 0;
 }
 
-static int cxgb4_get_pcie_dev_link_caps(struct adapter *adap,
-					enum pci_bus_speed *speed,
-					enum pcie_link_width *width)
-{
-	u32 lnkcap1, lnkcap2;
-	int err1, err2;
-
-#define  PCIE_MLW_CAP_SHIFT 4   /* start of MLW mask in link capabilities */
-
-	*speed = PCI_SPEED_UNKNOWN;
-	*width = PCIE_LNK_WIDTH_UNKNOWN;
-
-	err1 = pcie_capability_read_dword(adap->pdev, PCI_EXP_LNKCAP,
-					  &lnkcap1);
-	err2 = pcie_capability_read_dword(adap->pdev, PCI_EXP_LNKCAP2,
-					  &lnkcap2);
-	if (!err2 && lnkcap2) { /* PCIe r3.0-compliant */
-		if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
-			*speed = PCIE_SPEED_8_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
-			*speed = PCIE_SPEED_5_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
-			*speed = PCIE_SPEED_2_5GT;
-	}
-	if (!err1) {
-		*width = (lnkcap1 & PCI_EXP_LNKCAP_MLW) >> PCIE_MLW_CAP_SHIFT;
-		if (!lnkcap2) { /* pre-r3.0 */
-			if (lnkcap1 & PCI_EXP_LNKCAP_SLS_5_0GB)
-				*speed = PCIE_SPEED_5_0GT;
-			else if (lnkcap1 & PCI_EXP_LNKCAP_SLS_2_5GB)
-				*speed = PCIE_SPEED_2_5GT;
-		}
-	}
-
-	if (*speed == PCI_SPEED_UNKNOWN || *width == PCIE_LNK_WIDTH_UNKNOWN)
-		return err1 ? err1 : err2 ? err2 : -EINVAL;
-	return 0;
-}
-
-static void cxgb4_check_pcie_caps(struct adapter *adap)
-{
-	enum pcie_link_width width, width_cap;
-	enum pci_bus_speed speed, speed_cap;
-
-#define PCIE_SPEED_STR(speed) \
-	(speed == PCIE_SPEED_8_0GT ? "8.0GT/s" : \
-	 speed == PCIE_SPEED_5_0GT ? "5.0GT/s" : \
-	 speed == PCIE_SPEED_2_5GT ? "2.5GT/s" : \
-	 "Unknown")
-
-	if (cxgb4_get_pcie_dev_link_caps(adap, &speed_cap, &width_cap)) {
-		dev_warn(adap->pdev_dev,
-			 "Unable to determine PCIe device BW capabilities\n");
-		return;
-	}
-
-	if (pcie_get_minimum_link(adap->pdev, &speed, &width) ||
-	    speed == PCI_SPEED_UNKNOWN || width == PCIE_LNK_WIDTH_UNKNOWN) {
-		dev_warn(adap->pdev_dev,
-			 "Unable to determine PCI Express bandwidth.\n");
-		return;
-	}
-
-	dev_info(adap->pdev_dev, "PCIe link speed is %s, device supports %s\n",
-		 PCIE_SPEED_STR(speed), PCIE_SPEED_STR(speed_cap));
-	dev_info(adap->pdev_dev, "PCIe link width is x%d, device supports x%d\n",
-		 width, width_cap);
-	if (speed < speed_cap || width < width_cap)
-		dev_info(adap->pdev_dev,
-			 "A slot with more lanes and/or higher speed is "
-			 "suggested for optimal performance.\n");
-}
-
 /* Dump basic information about the adapter */
 static void print_adapter_info(struct adapter *adapter)
 {
@@ -5750,7 +5677,7 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	/* check for PCI Express bandwidth capabiltites */
-	cxgb4_check_pcie_caps(adapter);
+	pcie_print_link_status(pdev);
 
 	err = init_rss(adapter);
 	if (err)

[PATCH v6 4/5] ixgbe: Report PCIe link properties with pcie_print_link_status()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously the driver used pcie_get_minimum_link() to warn when the NIC
is in a slot that can't supply as much bandwidth as the NIC could use.

pcie_get_minimum_link() can be misleading because it finds the slowest link
and the narrowest link (which may be different links) without considering
the total bandwidth of each link.  For a path with a 16 GT/s x1 link and a
2.5 GT/s x16 link, it returns 2.5 GT/s x1, which corresponds to 250 MB/s of
bandwidth, not the true available bandwidth of about 1969 MB/s for a
16 GT/s x1 link.

Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.  This finds
the slowest link in the path to the device by computing the total bandwidth
of each link and compares that with the capabilities of the device.

The dmesg change is:

  - PCI Express bandwidth of %dGT/s available
  - (Speed:%s, Width: x%d, Encoding Loss:%s)
  + %u.%03u Gb/s available PCIe bandwidth (%s x%d link)

or, if the device is capable of better performance than is available in the
current slot:

  - This is not sufficient for optimal performance of this card.
  - For optimal performance, at least %dGT/s of bandwidth is required.
  - A slot with more lanes and/or higher speed is suggested.
  + %u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)

Note that the driver previously used dev_warn() to suggest using a
different slot, but pcie_print_link_status() uses dev_info() because if the
platform has no faster slot available, the user can't do anything about the
warning and may not want to be bothered with it.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   47 +------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index afadba99f7b8..8990285f6e12 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -270,9 +270,6 @@ static void ixgbe_check_minimum_link(struct ixgbe_adapter *adapter,
 				     int expected_gts)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
-	int max_gts = 0;
-	enum pci_bus_speed speed = PCI_SPEED_UNKNOWN;
-	enum pcie_link_width width = PCIE_LNK_WIDTH_UNKNOWN;
 	struct pci_dev *pdev;
 
 	/* Some devices are not connected over PCIe and thus do not negotiate
@@ -288,49 +285,7 @@ static void ixgbe_check_minimum_link(struct ixgbe_adapter *adapter,
 	else
 		pdev = adapter->pdev;
 
-	if (pcie_get_minimum_link(pdev, &speed, &width) ||
-	    speed == PCI_SPEED_UNKNOWN || width == PCIE_LNK_WIDTH_UNKNOWN) {
-		e_dev_warn("Unable to determine PCI Express bandwidth.\n");
-		return;
-	}
-
-	switch (speed) {
-	case PCIE_SPEED_2_5GT:
-		/* 8b/10b encoding reduces max throughput by 20% */
-		max_gts = 2 * width;
-		break;
-	case PCIE_SPEED_5_0GT:
-		/* 8b/10b encoding reduces max throughput by 20% */
-		max_gts = 4 * width;
-		break;
-	case PCIE_SPEED_8_0GT:
-		/* 128b/130b encoding reduces throughput by less than 2% */
-		max_gts = 8 * width;
-		break;
-	default:
-		e_dev_warn("Unable to determine PCI Express bandwidth.\n");
-		return;
-	}
-
-	e_dev_info("PCI Express bandwidth of %dGT/s available\n",
-		   max_gts);
-	e_dev_info("(Speed:%s, Width: x%d, Encoding Loss:%s)\n",
-		   (speed == PCIE_SPEED_8_0GT ? "8.0GT/s" :
-		    speed == PCIE_SPEED_5_0GT ? "5.0GT/s" :
-		    speed == PCIE_SPEED_2_5GT ? "2.5GT/s" :
-		    "Unknown"),
-		   width,
-		   (speed == PCIE_SPEED_2_5GT ? "20%" :
-		    speed == PCIE_SPEED_5_0GT ? "20%" :
-		    speed == PCIE_SPEED_8_0GT ? "<2%" :
-		    "Unknown"));
-
-	if (max_gts < expected_gts) {
-		e_dev_warn("This is not sufficient for optimal performance of this card.\n");
-		e_dev_warn("For optimal performance, at least %dGT/s of bandwidth is required.\n",
-			expected_gts);
-		e_dev_warn("A slot with more lanes and/or higher speed is suggested.\n");
-	}
+	pcie_print_link_status(pdev);
 }
 
 static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)

[PATCH v6 5/5] PCI: Remove unused pcie_get_minimum_link()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

In some cases pcie_get_minimum_link() returned misleading information
because it found the slowest link and the narrowest link without
considering the total bandwidth of the link.

For example, consider a path with these two links:

  - 16.0 GT/s  x1 link  (16.0 * 10^9 * 128 / 130) *  1 / 8 = 1969 MB/s
  -  2.5 GT/s x16 link  ( 2.5 * 10^9 *   8 /  10) * 16 / 8 = 4000 MB/s

The available bandwidth of the path is limited by the 16 GT/s link to about
1969 MB/s, but pcie_get_minimum_link() returned 2.5 GT/s x1, which
corresponds to only 250 MB/s.

Callers should use pcie_print_link_status() instead, or
pcie_bandwidth_available() if they need more detailed information.

Remove pcie_get_minimum_link() since there are no callers left.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   |   43 -------------------------------------------
 include/linux/pci.h |    2 --
 2 files changed, 45 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e597655a5643..4bafa817c40a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5069,49 +5069,6 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
 }
 EXPORT_SYMBOL(pcie_set_mps);
 
-/**
- * pcie_get_minimum_link - determine minimum link settings of a PCI device
- * @dev: PCI device to query
- * @speed: storage for minimum speed
- * @width: storage for minimum width
- *
- * This function will walk up the PCI device chain and determine the minimum
- * link width and speed of the device.
- */
-int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
-			  enum pcie_link_width *width)
-{
-	int ret;
-
-	*speed = PCI_SPEED_UNKNOWN;
-	*width = PCIE_LNK_WIDTH_UNKNOWN;
-
-	while (dev) {
-		u16 lnksta;
-		enum pci_bus_speed next_speed;
-		enum pcie_link_width next_width;
-
-		ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
-		if (ret)
-			return ret;
-
-		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
-		next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
-			PCI_EXP_LNKSTA_NLW_SHIFT;
-
-		if (next_speed < *speed)
-			*speed = next_speed;
-
-		if (next_width < *width)
-			*width = next_width;
-
-		dev = dev->bus->self;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL(pcie_get_minimum_link);
-
 /**
  * pcie_bandwidth_available - determine minimum link settings of a PCIe
  *			      device and its bandwidth limitation
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..230615620a4a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1079,8 +1079,6 @@ int pcie_get_readrq(struct pci_dev *dev);
 int pcie_set_readrq(struct pci_dev *dev, int rq);
 int pcie_get_mps(struct pci_dev *dev);
 int pcie_set_mps(struct pci_dev *dev, int mps);
-int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
-			  enum pcie_link_width *width);
 u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
 			     enum pci_bus_speed *speed,
 			     enum pcie_link_width *width);

RE: [PATCH v6 0/5] PCI: Improve PCIe link status reporting

[Keller, Jacob E]

> -----Original Message-----
> This does change the dmesg reporting of link speeds, and in the ixgbe case,
> it changes the reporting from KERN_WARN level to KERN_INFO.  If that's an
> issue, let's talk about it.  I'm hoping the reduce code size, improved
> functionality, and consistency across drivers is enough to make this
> worthwhile.
> 

I personally have no issue with this change, but I don't work on the ixgbe driver much anymore.

Thanks,
Jake

Re: [PATCH v6 5/5] PCI: Remove unused pcie_get_minimum_link()

[Bjorn Helgaas]

On Thu, May 03, 2018 at 03:00:43PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> In some cases pcie_get_minimum_link() returned misleading information
> because it found the slowest link and the narrowest link without
> considering the total bandwidth of the link.
> 
> For example, consider a path with these two links:
> 
>   - 16.0 GT/s  x1 link  (16.0 * 10^9 * 128 / 130) *  1 / 8 = 1969 MB/s
>   -  2.5 GT/s x16 link  ( 2.5 * 10^9 *   8 /  10) * 16 / 8 = 4000 MB/s
> 
> The available bandwidth of the path is limited by the 16 GT/s link to about
> 1969 MB/s, but pcie_get_minimum_link() returned 2.5 GT/s x1, which
> corresponds to only 250 MB/s.
> 
> Callers should use pcie_print_link_status() instead, or
> pcie_bandwidth_available() if they need more detailed information.
> 
> Remove pcie_get_minimum_link() since there are no callers left.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Hi Jeff,

I got your note that you applied this to dev-queue.  I assume that
means you also applied the preceding patches that removed all the
users.  I got a note about ixgbe, but not the others, so I'm just
double-checking.

> ---
>  drivers/pci/pci.c   |   43 -------------------------------------------
>  include/linux/pci.h |    2 --
>  2 files changed, 45 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e597655a5643..4bafa817c40a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5069,49 +5069,6 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
>  }
>  EXPORT_SYMBOL(pcie_set_mps);
>  
> -/**
> - * pcie_get_minimum_link - determine minimum link settings of a PCI device
> - * @dev: PCI device to query
> - * @speed: storage for minimum speed
> - * @width: storage for minimum width
> - *
> - * This function will walk up the PCI device chain and determine the minimum
> - * link width and speed of the device.
> - */
> -int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> -			  enum pcie_link_width *width)
> -{
> -	int ret;
> -
> -	*speed = PCI_SPEED_UNKNOWN;
> -	*width = PCIE_LNK_WIDTH_UNKNOWN;
> -
> -	while (dev) {
> -		u16 lnksta;
> -		enum pci_bus_speed next_speed;
> -		enum pcie_link_width next_width;
> -
> -		ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> -		if (ret)
> -			return ret;
> -
> -		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
> -		next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
> -			PCI_EXP_LNKSTA_NLW_SHIFT;
> -
> -		if (next_speed < *speed)
> -			*speed = next_speed;
> -
> -		if (next_width < *width)
> -			*width = next_width;
> -
> -		dev = dev->bus->self;
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(pcie_get_minimum_link);
> -
>  /**
>   * pcie_bandwidth_available - determine minimum link settings of a PCIe
>   *			      device and its bandwidth limitation
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 73178a2fcee0..230615620a4a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1079,8 +1079,6 @@ int pcie_get_readrq(struct pci_dev *dev);
>  int pcie_set_readrq(struct pci_dev *dev, int rq);
>  int pcie_get_mps(struct pci_dev *dev);
>  int pcie_set_mps(struct pci_dev *dev, int mps);
> -int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> -			  enum pcie_link_width *width);
>  u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
>  			     enum pci_bus_speed *speed,
>  			     enum pcie_link_width *width);
> 

Re: [PATCH v6 5/5] PCI: Remove unused pcie_get_minimum_link()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Thu, 2018-05-10 at 11:33 -0500, Bjorn Helgaas wrote:
> On Thu, May 03, 2018 at 03:00:43PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > In some cases pcie_get_minimum_link() returned misleading
> > information
> > because it found the slowest link and the narrowest link without
> > considering the total bandwidth of the link.
> > 
> > For example, consider a path with these two links:
> > 
> >    - 16.0 GT/s  x1 link  (16.0 * 10^9 * 128 / 130) *  1 / 8 = 1969
> > MB/s
> >    -  2.5 GT/s x16 link  ( 2.5 * 10^9 *   8 /  10) * 16 / 8 = 4000
> > MB/s
> > 
> > The available bandwidth of the path is limited by the 16 GT/s link
> > to about
> > 1969 MB/s, but pcie_get_minimum_link() returned 2.5 GT/s x1, which
> > corresponds to only 250 MB/s.
> > 
> > Callers should use pcie_print_link_status() instead, or
> > pcie_bandwidth_available() if they need more detailed information.
> > 
> > Remove pcie_get_minimum_link() since there are no callers left.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Hi Jeff,
> 
> I got your note that you applied this to dev-queue.  I assume that
> means you also applied the preceding patches that removed all the
> users.  I got a note about ixgbe, but not the others, so I'm just
> double-checking.

I did initially apply it, but realized that I would have to apply the
earlier patches as well, which did not pertain to the Intel wired LAN
drivers.  So I have removed this patch from queue and will only be
testing the ixgbe patch of the series, which Andrew has already tested
and responded to.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 4/5] ixgbe: Report PCIe link properties with pcie_print_link_status()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Thu, 2018-05-03 at 15:00 -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously the driver used pcie_get_minimum_link() to warn when the
> NIC
> is in a slot that can't supply as much bandwidth as the NIC could
> use.
> 
> pcie_get_minimum_link() can be misleading because it finds the
> slowest link
> and the narrowest link (which may be different links) without
> considering
> the total bandwidth of each link.  For a path with a 16 GT/s x1 link
> and a
> 2.5 GT/s x16 link, it returns 2.5 GT/s x1, which corresponds to 250
> MB/s of
> bandwidth, not the true available bandwidth of about 1969 MB/s for a
> 16 GT/s x1 link.
> 
> Use pcie_print_link_status() to report PCIe link speed and possible
> limitations instead of implementing this in the driver itself.  This
> finds
> the slowest link in the path to the device by computing the total
> bandwidth
> of each link and compares that with the capabilities of the device.
> 
> The dmesg change is:
> 
>   - PCI Express bandwidth of %dGT/s available
>   - (Speed:%s, Width: x%d, Encoding Loss:%s)
>   + %u.%03u Gb/s available PCIe bandwidth (%s x%d link)
> 
> or, if the device is capable of better performance than is available
> in the
> current slot:
> 
>   - This is not sufficient for optimal performance of this card.
>   - For optimal performance, at least %dGT/s of bandwidth is
> required.
>   - A slot with more lanes and/or higher speed is suggested.
>   + %u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at
> %s (capable of %u.%03u Gb/s with %s x%d link)
> 
> Note that the driver previously used dev_warn() to suggest using a
> different slot, but pcie_print_link_status() uses dev_info() because
> if the
> platform has no faster slot available, the user can't do anything
> about the
> warning and may not want to be bothered with it.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Since this is apart of a series, I am not planning to pick this up and
push to David Miller in my ixgbe updates.  This should remain in the
series so David can pick up the entire series at once.

> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   47 +------------
> ------------
>  1 file changed, 1 insertion(+), 46 deletions(-)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v6 0/5] PCI: Improve PCIe link status reporting

[Bjorn Helgaas]

On Thu, May 03, 2018 at 03:00:07PM -0500, Bjorn Helgaas wrote:
> This is based on Tal's recent work to unify the approach for reporting PCIe
> link speed/width and whether the device is being limited by a slower
> upstream link.
> 
> The new pcie_print_link_status() interface appeared in v4.17-rc1; see
> 9e506a7b5147 ("PCI: Add pcie_print_link_status() to log link speed and
> whether it's limited").
> 
> That's a good way to replace use of pcie_get_minimum_link(), which gives
> misleading results when a path contains both a fast, narrow link and a
> slow, wide link: it reports the equivalent of a slow, narrow link.
> 
> This series removes the remaining uses of pcie_get_minimum_link() and then
> removes the interface itself.  I'd like to merge them all through the PCI
> tree to make the removal easy.
> 
> This does change the dmesg reporting of link speeds, and in the ixgbe case,
> it changes the reporting from KERN_WARN level to KERN_INFO.  If that's an
> issue, let's talk about it.  I'm hoping the reduce code size, improved
> functionality, and consistency across drivers is enough to make this
> worthwhile.
> 
> ---
> 
> Bjorn Helgaas (5):
>       bnx2x: Report PCIe link properties with pcie_print_link_status()
>       bnxt_en: Report PCIe link properties with pcie_print_link_status()
>       cxgb4: Report PCIe link properties with pcie_print_link_status()
>       ixgbe: Report PCIe link properties with pcie_print_link_status()
>       PCI: Remove unused pcie_get_minimum_link()

Jeff has acked the ixgbe patch.

Any comments on the bnx2x, bnxt_en, or cxgb4 patches?

>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   23 ++-----
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c        |   19 ------
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  |   75 ----------------------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   47 --------------
>  drivers/pci/pci.c                                |   43 -------------
>  include/linux/pci.h                              |    2 -
>  6 files changed, 9 insertions(+), 200 deletions(-)

Re: [PATCH v6 0/5] PCI: Improve PCIe link status reporting

[Bjorn Helgaas]

[+to Davem]

On Thu, May 03, 2018 at 03:00:07PM -0500, Bjorn Helgaas wrote:
> This is based on Tal's recent work to unify the approach for reporting PCIe
> link speed/width and whether the device is being limited by a slower
> upstream link.
> 
> The new pcie_print_link_status() interface appeared in v4.17-rc1; see
> 9e506a7b5147 ("PCI: Add pcie_print_link_status() to log link speed and
> whether it's limited").
> 
> That's a good way to replace use of pcie_get_minimum_link(), which gives
> misleading results when a path contains both a fast, narrow link and a
> slow, wide link: it reports the equivalent of a slow, narrow link.
> 
> This series removes the remaining uses of pcie_get_minimum_link() and then
> removes the interface itself.  I'd like to merge them all through the PCI
> tree to make the removal easy.
> 
> This does change the dmesg reporting of link speeds, and in the ixgbe case,
> it changes the reporting from KERN_WARN level to KERN_INFO.  If that's an
> issue, let's talk about it.  I'm hoping the reduce code size, improved
> functionality, and consistency across drivers is enough to make this
> worthwhile.
> 
> ---
> 
> Bjorn Helgaas (5):
>       bnx2x: Report PCIe link properties with pcie_print_link_status()
>       bnxt_en: Report PCIe link properties with pcie_print_link_status()
>       cxgb4: Report PCIe link properties with pcie_print_link_status()
>       ixgbe: Report PCIe link properties with pcie_print_link_status()
>       PCI: Remove unused pcie_get_minimum_link()
> 
> 
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   23 ++-----
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c        |   19 ------
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  |   75 ----------------------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   47 --------------
>  drivers/pci/pci.c                                |   43 -------------
>  include/linux/pci.h                              |    2 -
>  6 files changed, 9 insertions(+), 200 deletions(-)

I applied all of these on pci/enumeration for v4.18.  If you'd rather take
them, Dave, let me know and I'll drop them.

I solicited more acks, but only heard from Jeff.

Re: [PATCH v6 0/5] PCI: Improve PCIe link status reporting

[Ganesh Goudar]

On Wednesday, May 05/23/18, 2018 at 16:46:51 -0500, Bjorn Helgaas wrote:
> [+to Davem]
> 
> On Thu, May 03, 2018 at 03:00:07PM -0500, Bjorn Helgaas wrote:
> > This is based on Tal's recent work to unify the approach for reporting PCIe
> > link speed/width and whether the device is being limited by a slower
> > upstream link.
> > 
> > The new pcie_print_link_status() interface appeared in v4.17-rc1; see
> > 9e506a7b5147 ("PCI: Add pcie_print_link_status() to log link speed and
> > whether it's limited").
> > 
> > That's a good way to replace use of pcie_get_minimum_link(), which gives
> > misleading results when a path contains both a fast, narrow link and a
> > slow, wide link: it reports the equivalent of a slow, narrow link.
> > 
> > This series removes the remaining uses of pcie_get_minimum_link() and then
> > removes the interface itself.  I'd like to merge them all through the PCI
> > tree to make the removal easy.
> > 
> > This does change the dmesg reporting of link speeds, and in the ixgbe case,
> > it changes the reporting from KERN_WARN level to KERN_INFO.  If that's an
> > issue, let's talk about it.  I'm hoping the reduce code size, improved
> > functionality, and consistency across drivers is enough to make this
> > worthwhile.
> > 
> > ---
> > 
> > Bjorn Helgaas (5):
> >       bnx2x: Report PCIe link properties with pcie_print_link_status()
> >       bnxt_en: Report PCIe link properties with pcie_print_link_status()
> >       cxgb4: Report PCIe link properties with pcie_print_link_status()
> >       ixgbe: Report PCIe link properties with pcie_print_link_status()
> >       PCI: Remove unused pcie_get_minimum_link()
> > 
> > 
> >  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   23 ++-----
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c        |   19 ------
> >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c  |   75 ----------------------
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   47 --------------
> >  drivers/pci/pci.c                                |   43 -------------
> >  include/linux/pci.h                              |    2 -
> >  6 files changed, 9 insertions(+), 200 deletions(-)
> 
> I applied all of these on pci/enumeration for v4.18.  If you'd rather take
> them, Dave, let me know and I'll drop them.
> 
> I solicited more acks, but only heard from Jeff.
Sorry for that, Thanks for cxgb4 changes.