This is mostly Tal's work to reduce code duplication in drivers and unify
the approach for reporting PCIe link speed/width and whether the device is
being limited by a slower upstream link.
This v5 series is based on Tal's v4 [1].
Changes since v4:
- Added patches to replace uses of pcie_get_minimum_link() in bnx2x,
bnxt_en, cxgb4, fm10k, and ixgbe. Note that this is a user-visible
change to the log messages, and in some cases changes dev_warn() to
dev_info(). I hope we can converge on something that works for
everybody, and it's OK if we need to tweak the text and/or level used
in pcie_print_link_status() to get there.
- Rebased on top of Jay Fang's patch that adds 16 GT/s decoding support.
- Changed pcie_get_speed_cap() and pcie_get_width_cap() to return the
values directly instead of returning both an error code and the value
via a reference parameter. I don't think the callers can really use
both the error and the value.
- Moved some declarations from linux/pci.h to drivers/pci/pci.h so
they're not visible outside the PCI subsystem. Also removed
corresponding EXPORT_SYMBOL()s. If we need these outside the PCI core,
we can export them again, but that's not needed yet.
- Reworked pcie_bandwidth_available() so it finds the uppermost limiting
device and returns width/speed info for that device (previously it
could return width from one device and speed from a different one).
The incremental diff between the v4 series (based on v4.17-rc1) and this v5
series (based on v4.17-rc1 + Jay Fang's patch) is attached. This diff
doesn't include the new patches to bnx2x, bnxt_en, cxgb4, fm10k, and ixgbe.
I don't have any of this hardware, so this is only compile-tested.
Bjorn
[1] https://lkml.kernel.org/r/1522394086-3555-1-git-send-email-talgi@mellanox.com
---
Bjorn Helgaas (6):
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()
fm10k: 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()
Tal Gilboa (8):
PCI: Add pcie_get_speed_cap() to find max supported link speed
PCI: Add pcie_get_width_cap() to find max supported link width
PCI: Add pcie_bandwidth_capable() to compute max supported link bandwidth
PCI: Add pcie_bandwidth_available() to compute bandwidth available to device
PCI: Add pcie_print_link_status() to log link speed and whether it's limited
net/mlx4_core: Report PCIe link properties with pcie_print_link_status()
net/mlx5: Report PCIe link properties with pcie_print_link_status()
net/mlx5e: Use pcie_bandwidth_available() to compute bandwidth
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/fm10k/fm10k_pci.c | 87 -----------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 47 ------
drivers/net/ethernet/mellanox/mlx4/main.c | 81 ----------
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 32 ----
drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 +
drivers/pci/pci-sysfs.c | 38 +----
drivers/pci/pci.c | 167 ++++++++++++++++++---
drivers/pci/pci.h | 20 +++
include/linux/pci.h | 6 +
12 files changed, 189 insertions(+), 410 deletions(-)
@@ -5103,193 +5103,169 @@ int pcie_set_mps(struct pci_dev *dev, int mps)}EXPORT_SYMBOL(pcie_set_mps);-/**-*pcie_get_minimum_link-determineminimumlinksettingsofaPCIdevice-*@dev:PCIdevicetoquery-*@speed:storageforminimumspeed-*@width:storageforminimumwidth-*-*Thisfunctionusepcie_bandwidth_available()fordeterminingtheminimum-*linkwidthandspeedofthedevice.Legacycodeiskeptforcompatibility.-*/-intpcie_get_minimum_link(structpci_dev*dev,enumpci_bus_speed*speed,-enumpcie_link_width*width)-{-intbw;--returnpcie_bandwidth_available(dev,speed,width,&bw,NULL);-}-EXPORT_SYMBOL(pcie_get_minimum_link);-/***pcie_bandwidth_available-determineminimumlinksettingsofaPCIe-deviceanditsbandwidthlimitation+*deviceanditsbandwidthlimitation*@dev:PCIdevicetoquery-*@speed:storageforminimumspeed-*@width:storageforminimumwidth-*@bw:storageforlinkbandwidth*@limiting_dev:storagefordevicecausingthebandwidthlimitation+*@speed:storageforspeedoflimitingdevice+*@width:storageforwidthoflimitingdevice*-*ThisfunctionwalksupthePCIdevicechainanddeterminestheminimumwidth,-*minimumspeedandavailablebandwidthofthedevice.+*WalkupthePCIdevicechainandfindthepointwheretheminimum+*bandwidthisavailable.Returnthebandwidthavailablethereand(if+*limiting_dev,speed,andwidthpointersaresupplied)informationabout+*thatpoint.*/-intpcie_bandwidth_available(structpci_dev*dev,enumpci_bus_speed*speed,-enumpcie_link_width*width,int*bw,-structpci_dev**limiting_dev)+u32pcie_bandwidth_available(structpci_dev*dev,structpci_dev**limiting_dev,+enumpci_bus_speed*speed,+enumpcie_link_width*width){-interr;+u16lnksta;+enumpci_bus_speednext_speed;+enumpcie_link_widthnext_width;+u32bw,next_bw;*speed=PCI_SPEED_UNKNOWN;*width=PCIE_LNK_WIDTH_UNKNOWN;-*bw=0;+bw=0;while(dev){-u16lnksta;-enumpci_bus_speednext_speed;-enumpcie_link_widthnext_width;--err=pcie_capability_read_word(dev,PCI_EXP_LNKSTA,&lnksta);-if(err)-returnerr;+pcie_capability_read_word(dev,PCI_EXP_LNKSTA,&lnksta);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;+next_bw=next_width*PCIE_SPEED2MBS_ENC(next_speed);/* Check if current device limits the total bandwidth */-if(!(*bw)||-(*bw>next_width*PCIE_SPEED2MBS_ENC(next_speed))){+if(!bw||next_bw<=bw){+bw=next_bw;+if(limiting_dev)*limiting_dev=dev;-*bw=next_width*PCIE_SPEED2MBS_ENC(next_speed);+if(speed)+*speed=next_speed;+if(width)+*width=next_width;}-dev=dev->bus->self;+dev=pci_upstream_bridge(dev);}-return0;+returnbw;}EXPORT_SYMBOL(pcie_bandwidth_available);/**-*pcie_get_speed_cap-queriesforthePCIdevice'slinkspeedcapability+*pcie_get_speed_cap-queryforthePCIdevice'slinkspeedcapability*@dev:PCIdevicetoquery-*@speed:storageforlinkspeed*-*ThisfunctionqueriesthePCIdevicespeedcapability.+*QuerythePCIdevicespeedcapability.Returnthemaximumlinkspeed+*supportedbythedevice.*/-intpcie_get_speed_cap(structpci_dev*dev,enumpci_bus_speed*speed)+enumpci_bus_speedpcie_get_speed_cap(structpci_dev*dev){-u32lnkcap;-interr1,err2;+u32lnkcap2,lnkcap;-*speed=PCI_SPEED_UNKNOWN;+/*+*PCIer4.0sec7.5.3.18recommendsusingtheSupportedLink+*SpeedsVectorinLinkCapabilities2whensupported,falling+*backtoMaxLinkSpeedinLinkCapabilitiesotherwise.+*/+pcie_capability_read_dword(dev,PCI_EXP_LNKCAP2,&lnkcap2);+if(lnkcap2){/* PCIe r3.0-compliant */+if(lnkcap2&PCI_EXP_LNKCAP2_SLS_16_0GB)+returnPCIE_SPEED_16_0GT;+elseif(lnkcap2&PCI_EXP_LNKCAP2_SLS_8_0GB)+returnPCIE_SPEED_8_0GT;+elseif(lnkcap2&PCI_EXP_LNKCAP2_SLS_5_0GB)+returnPCIE_SPEED_5_0GT;+elseif(lnkcap2&PCI_EXP_LNKCAP2_SLS_2_5GB)+returnPCIE_SPEED_2_5GT;+returnPCI_SPEED_UNKNOWN;+}-err1=pcie_capability_read_dword(dev,PCI_EXP_LNKCAP,-&lnkcap);-if(!err1&&lnkcap){-if(lnkcap&PCI_EXP_LNKCAP_SLS_8_0GB)-*speed=PCIE_SPEED_8_0GT;+pcie_capability_read_dword(dev,PCI_EXP_LNKCAP,&lnkcap);+if(lnkcap){+if(lnkcap&PCI_EXP_LNKCAP_SLS_16_0GB)+returnPCIE_SPEED_16_0GT;+elseif(lnkcap&PCI_EXP_LNKCAP_SLS_8_0GB)+returnPCIE_SPEED_8_0GT;elseif(lnkcap&PCI_EXP_LNKCAP_SLS_5_0GB)-*speed=PCIE_SPEED_5_0GT;+returnPCIE_SPEED_5_0GT;elseif(lnkcap&PCI_EXP_LNKCAP_SLS_2_5GB)-*speed=PCIE_SPEED_2_5GT;-return0;-}--err2=pcie_capability_read_dword(dev,PCI_EXP_LNKCAP2,-&lnkcap);-if(!err2&&lnkcap){/* PCIe r3.0-compliant */-if(lnkcap&PCI_EXP_LNKCAP2_SLS_8_0GB)-*speed=PCIE_SPEED_8_0GT;-elseif(lnkcap&PCI_EXP_LNKCAP2_SLS_5_0GB)-*speed=PCIE_SPEED_5_0GT;-elseif(lnkcap&PCI_EXP_LNKCAP2_SLS_2_5GB)-*speed=PCIE_SPEED_2_5GT;-return0;+returnPCIE_SPEED_2_5GT;}-returnerr1?err1:err2;+returnPCI_SPEED_UNKNOWN;}-EXPORT_SYMBOL(pcie_get_speed_cap);/**-*pcie_get_width_cap-queriesforthePCIdevice'slinkwidthcapability+*pcie_get_width_cap-queryforthePCIdevice'slinkwidthcapability*@dev:PCIdevicetoquery-*@width:storageforlinkwidth*-*ThisfunctionqueriesthePCIdevicewidthcapability.+*QuerythePCIdevicewidthcapability.Returnthemaximumlinkwidth+*supportedbythedevice.*/-intpcie_get_width_cap(structpci_dev*dev,enumpcie_link_width*width)+enumpcie_link_widthpcie_get_width_cap(structpci_dev*dev){u32lnkcap;-interr;--*width=PCIE_LNK_WIDTH_UNKNOWN;-err=pcie_capability_read_dword(dev,PCI_EXP_LNKCAP,&lnkcap);-if(!err&&lnkcap)-/* Shift start of width mask by 4 to get actual speed cap */-*width=(lnkcap&PCI_EXP_LNKCAP_MLW)>>4;+pcie_capability_read_dword(dev,PCI_EXP_LNKCAP,&lnkcap);+if(lnkcap)+return(lnkcap&PCI_EXP_LNKCAP_MLW)>>4;-returnerr;+returnPCIE_LNK_WIDTH_UNKNOWN;}-EXPORT_SYMBOL(pcie_get_width_cap);/**-*pcie_bandwidth_capable-CalculatesaPCIdevice'slinkbandwidthcapability+*pcie_bandwidth_capable-calculatesaPCIdevice'slinkbandwidthcapability*@dev:PCIdevice*@speed:storageforlinkspeed*@width:storageforlinkwidth*-*ThisfunctioncaculatesaPCIdevice'slinkbandwidthbyqueryingforits-*linkspeedandwidth,multiplyingthem,andapplyingencodingoverhead.+*CalculateaPCIdevice'slinkbandwidthbyqueryingforitslinkspeed+*andwidth,multiplyingthem,andapplyingencodingoverhead.*/-intpcie_bandwidth_capable(structpci_dev*dev,enumpci_bus_speed*speed,+u32pcie_bandwidth_capable(structpci_dev*dev,enumpci_bus_speed*speed,enumpcie_link_width*width){-pcie_get_speed_cap(dev,speed);-pcie_get_width_cap(dev,width);+*speed=pcie_get_speed_cap(dev);+*width=pcie_get_width_cap(dev);if(*speed==PCI_SPEED_UNKNOWN||*width==PCIE_LNK_WIDTH_UNKNOWN)return0;-return(*width)*PCIE_SPEED2MBS_ENC(*speed);+return*width*PCIE_SPEED2MBS_ENC(*speed);}-EXPORT_SYMBOL(pcie_bandwidth_capable);/**-*pcie_print_link_status-ReportsthePCIdevice'slinkspeedandwidth.+*pcie_print_link_status-ReportthePCIdevice'slinkspeedandwidth*@dev:PCIdevicetoquery*-*ThisfunctioncheckswhetherthePCIdevicecurrentspeedandwidthareequal-*tothemaximumPCIdevicecapabilities.+*Reporttheavailablebandwidthatthedevice.Ifthisislessthanthe+*deviceiscapableof,reportthedevice'smaximumpossiblebandwidthand+*theupstreamlinkthatlimitsitsperformancetolessthanthat.*/voidpcie_print_link_status(structpci_dev*dev){enumpcie_link_widthwidth,width_cap;-structpci_dev*limiting_dev=NULL;enumpci_bus_speedspeed,speed_cap;-intbw,bw_cap;+structpci_dev*limiting_dev=NULL;+u32bw_avail,bw_cap;bw_cap=pcie_bandwidth_capable(dev,&speed_cap,&width_cap);-pcie_bandwidth_available(dev,&speed,&width,&bw,&limiting_dev);+bw_avail=pcie_bandwidth_available(dev,&limiting_dev,&speed,&width);-if(bw>=bw_cap)+if(bw_avail>=bw_cap)pci_info(dev,"%d Mb/s available bandwidth (%s x%d link)\n",-bw,PCIE_SPEED2STR(speed),width);+bw_cap,PCIE_SPEED2STR(speed_cap),width_cap);else-pci_info(dev,"%d Mb/s available bandwidth (capable of %d Mb/s, %s x%d link)\n",-bw,bw_cap,PCIE_SPEED2STR(speed_cap),width_cap);-if(limiting_dev&&strcmp(pci_name(limiting_dev),pci_name(dev)))-pci_info(dev,"Bandwidth limited by device at %s\n",-pci_name(limiting_dev));+pci_info(dev,"%d Mb/s available bandwidth, limited by %s x%d link at %s (capable of %d Mb/s with %s x%d link)\n",+bw_avail,PCIE_SPEED2STR(speed),width,+limiting_dev?pci_name(limiting_dev):"<unknown>",+bw_cap,PCIE_SPEED2STR(speed_cap),width_cap);}EXPORT_SYMBOL(pcie_print_link_status);
@@ -253,6 +253,26 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx);voidpci_reassigndev_resource_alignment(structpci_dev*dev);voidpci_disable_bridge_window(structpci_dev*dev);+/* PCIe link information */+#define PCIE_SPEED2STR(speed) \+((speed)==PCIE_SPEED_16_0GT?"16 GT/s":\+(speed)==PCIE_SPEED_8_0GT?"8 GT/s":\+(speed)==PCIE_SPEED_5_0GT?"5 GT/s":\+(speed)==PCIE_SPEED_2_5GT?"2.5 GT/s":\+"Unknown speed")++/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */+#define PCIE_SPEED2MBS_ENC(speed) \+((speed)==PCIE_SPEED_8_0GT?7877:\+(speed)==PCIE_SPEED_5_0GT?4000:\+(speed)==PCIE_SPEED_2_5GT?2000:\+0)++enumpci_bus_speedpcie_get_speed_cap(structpci_dev*dev);+enumpcie_link_widthpcie_get_width_cap(structpci_dev*dev);+u32pcie_bandwidth_capable(structpci_dev*dev,enumpci_bus_speed*speed,+enumpcie_link_width*width);+/* Single Root I/O Virtualization */structpci_sriov{intpos;/* Capability position */
@@ -520,6 +520,7 @@#define PCI_EXP_LNKCAP_SLS_2_5GB 0x00000001 /* LNKCAP2 SLS Vector bit 0 */#define PCI_EXP_LNKCAP_SLS_5_0GB 0x00000002 /* LNKCAP2 SLS Vector bit 1 */#define PCI_EXP_LNKCAP_SLS_8_0GB 0x00000003 /* LNKCAP2 SLS Vector bit 2 */+#define PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */#define PCI_EXP_LNKCAP_MLW 0x000003f0 /* Maximum Link Width */#define PCI_EXP_LNKCAP_ASPMS 0x00000c00 /* ASPM Support */#define PCI_EXP_LNKCAP_L0SEL 0x00007000 /* L0s Exit Latency */
@@ -547,6 +548,7 @@#define PCI_EXP_LNKSTA_CLS_2_5GB 0x0001 /* Current Link Speed 2.5GT/s */#define PCI_EXP_LNKSTA_CLS_5_0GB 0x0002 /* Current Link Speed 5.0GT/s */#define PCI_EXP_LNKSTA_CLS_8_0GB 0x0003 /* Current Link Speed 8.0GT/s */+#define PCI_EXP_LNKSTA_CLS_16_0GB 0x0004 /* Current Link Speed 16.0GT/s */#define PCI_EXP_LNKSTA_NLW 0x03f0 /* Negotiated Link Width */#define PCI_EXP_LNKSTA_NLW_X1 0x0010 /* Current Link Width x1 */#define PCI_EXP_LNKSTA_NLW_X2 0x0020 /* Current Link Width x2 */
@@ -648,8 +650,9 @@#define PCI_CAP_EXP_RC_ENDPOINT_SIZEOF_V2 44 /* v2 endpoints without link end here */#define PCI_EXP_LNKCAP2 44 /* Link Capabilities 2 */#define PCI_EXP_LNKCAP2_SLS_2_5GB 0x00000002 /* Supported Speed 2.5GT/s */-#define PCI_EXP_LNKCAP2_SLS_5_0GB 0x00000004 /* Supported Speed 5.0GT/s */-#define PCI_EXP_LNKCAP2_SLS_8_0GB 0x00000008 /* Supported Speed 8.0GT/s */+#define PCI_EXP_LNKCAP2_SLS_5_0GB 0x00000004 /* Supported Speed 5GT/s */+#define PCI_EXP_LNKCAP2_SLS_8_0GB 0x00000008 /* Supported Speed 8GT/s */+#define PCI_EXP_LNKCAP2_SLS_16_0GB 0x00000010 /* Supported Speed 16GT/s */#define PCI_EXP_LNKCAP2_CROSSLINK 0x00000100 /* Crosslink supported */#define PCI_EXP_LNKCTL2 48 /* Link Control 2 */#define PCI_EXP_LNKSTA2 50 /* Link Status 2 */
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
From: Tal Gilboa <redacted>
Add pcie_bandwidth_available() to compute the bandwidth available to a
device. This may be limited by the device itself or by a slower upstream
link leading to the device.
The available bandwidth at each link along the path is computed as:
link_speed * link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Also return the device with the slowest link and the speed and width of
that link.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, leave pcie_get_minimum_link() alone for now, return
bw directly, use pci_upstream_bridge(), check "next_bw <= bw" to find
uppermost limiting device, return speed/width of the limiting device]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 3 +++
2 files changed, 57 insertions(+)
@@ -5146,6 +5146,60 @@ int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,}EXPORT_SYMBOL(pcie_get_minimum_link);+/**+*pcie_bandwidth_available-determineminimumlinksettingsofaPCIe+*deviceanditsbandwidthlimitation+*@dev:PCIdevicetoquery+*@limiting_dev:storagefordevicecausingthebandwidthlimitation+*@speed:storageforspeedoflimitingdevice+*@width:storageforwidthoflimitingdevice+*+*WalkupthePCIdevicechainandfindthepointwheretheminimum+*bandwidthisavailable.Returnthebandwidthavailablethereand(if+*limiting_dev,speed,andwidthpointersaresupplied)informationabout+*thatpoint.+*/+u32pcie_bandwidth_available(structpci_dev*dev,structpci_dev**limiting_dev,+enumpci_bus_speed*speed,+enumpcie_link_width*width)+{+u16lnksta;+enumpci_bus_speednext_speed;+enumpcie_link_widthnext_width;+u32bw,next_bw;++*speed=PCI_SPEED_UNKNOWN;+*width=PCIE_LNK_WIDTH_UNKNOWN;+bw=0;++while(dev){+pcie_capability_read_word(dev,PCI_EXP_LNKSTA,&lnksta);++next_speed=pcie_link_speed[lnksta&PCI_EXP_LNKSTA_CLS];+next_width=(lnksta&PCI_EXP_LNKSTA_NLW)>>+PCI_EXP_LNKSTA_NLW_SHIFT;++next_bw=next_width*PCIE_SPEED2MBS_ENC(next_speed);++/* Check if current device limits the total bandwidth */+if(!bw||next_bw<=bw){+bw=next_bw;++if(limiting_dev)+*limiting_dev=dev;+if(speed)+*speed=next_speed;+if(width)+*width=next_width;+}++dev=pci_upstream_bridge(dev);+}++returnbw;+}+EXPORT_SYMBOL(pcie_bandwidth_available);+/***pcie_get_speed_cap-queryforthePCIdevice'slinkspeedcapability*@dev:PCIdevicetoquery
From: Tal Gilboa <redacted>
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 30 insertions(+)
@@ -5283,6 +5283,35 @@ u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed,return*width*PCIE_SPEED2MBS_ENC(*speed);}+/**+*pcie_print_link_status-ReportthePCIdevice'slinkspeedandwidth+*@dev:PCIdevicetoquery+*+*Reporttheavailablebandwidthatthedevice.Ifthisislessthanthe+*deviceiscapableof,reportthedevice'smaximumpossiblebandwidthand+*theupstreamlinkthatlimitsitsperformancetolessthanthat.+*/+voidpcie_print_link_status(structpci_dev*dev)+{+enumpcie_link_widthwidth,width_cap;+enumpci_bus_speedspeed,speed_cap;+structpci_dev*limiting_dev=NULL;+u32bw_avail,bw_cap;++bw_cap=pcie_bandwidth_capable(dev,&speed_cap,&width_cap);+bw_avail=pcie_bandwidth_available(dev,&limiting_dev,&speed,&width);++if(bw_avail>=bw_cap)+pci_info(dev,"%d Mb/s available bandwidth (%s x%d link)\n",+bw_cap,PCIE_SPEED2STR(speed_cap),width_cap);+else+pci_info(dev,"%d Mb/s available bandwidth, limited by %s x%d link at %s (capable of %d Mb/s with %s x%d link)\n",+bw_avail,PCIE_SPEED2STR(speed),width,+limiting_dev?pci_name(limiting_dev):"<unknown>",+bw_cap,PCIE_SPEED2STR(speed_cap),width_cap);+}+EXPORT_SYMBOL(pcie_print_link_status);+/***pci_select_bars-MakeBARmaskfromthetypeofresource*@dev:thePCIdeviceforwhichBARmaskismade
From: Tal Gilboa <redacted>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Signed-off-by: Tal Gilboa <redacted>
Signed-off-by: Tariq Toukan <redacted>
[bhelgaas: changelog]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/net/ethernet/mellanox/mlx4/main.c | 81 -----------------------------
1 file changed, 1 insertion(+), 80 deletions(-)
@@ -623,85 +623,6 @@ static int mlx4_dev_cap(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)return0;}-staticintmlx4_get_pcie_dev_link_caps(structmlx4_dev*dev,-enumpci_bus_speed*speed,-enumpcie_link_width*width)-{-u32lnkcap1,lnkcap2;-interr1,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(dev->persist->pdev,PCI_EXP_LNKCAP,-&lnkcap1);-err2=pcie_capability_read_dword(dev->persist->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;-elseif(lnkcap2&PCI_EXP_LNKCAP2_SLS_5_0GB)-*speed=PCIE_SPEED_5_0GT;-elseif(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;-elseif(lnkcap1&PCI_EXP_LNKCAP_SLS_2_5GB)-*speed=PCIE_SPEED_2_5GT;-}-}--if(*speed==PCI_SPEED_UNKNOWN||*width==PCIE_LNK_WIDTH_UNKNOWN){-returnerr1?err1:-err2?err2:-EINVAL;-}-return0;-}--staticvoidmlx4_check_pcie_caps(structmlx4_dev*dev)-{-enumpcie_link_widthwidth,width_cap;-enumpci_bus_speedspeed,speed_cap;-interr;--#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")--err=mlx4_get_pcie_dev_link_caps(dev,&speed_cap,&width_cap);-if(err){-mlx4_warn(dev,-"Unable to determine PCIe device BW capabilities\n");-return;-}--err=pcie_get_minimum_link(dev->persist->pdev,&speed,&width);-if(err||speed==PCI_SPEED_UNKNOWN||-width==PCIE_LNK_WIDTH_UNKNOWN){-mlx4_warn(dev,-"Unable to determine PCI device chain minimum BW\n");-return;-}--if(width!=width_cap||speed!=speed_cap)-mlx4_warn(dev,-"PCIe BW is different than device's capability\n");--mlx4_info(dev,"PCIe link speed is %s, device supports %s\n",-PCIE_SPEED_STR(speed),PCIE_SPEED_STR(speed_cap));-mlx4_info(dev,"PCIe link width is x%d, device supports x%d\n",-width,width_cap);-return;-}-/*The function checks if there are live vf, return the num of them*/staticintmlx4_how_many_lives_vf(structmlx4_dev*dev){
@@ -3475,7 +3396,7 @@ static int mlx4_load_one(struct pci_dev *pdev, int pci_dev_data,*expressdevicecapabilitiesareunder-satisfiedbythebus.*/if(!mlx4_is_slave(dev))-mlx4_check_pcie_caps(dev);+pcie_print_link_status(dev->persist->pdev);/* In master functions, the communication channel must be initialized*afterobtainingitsaddressfromfw*/
@@ -1043,6 +1043,10 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv,dev_info(&pdev->dev,"firmware version: %d.%d.%d\n",fw_rev_maj(dev),fw_rev_min(dev),fw_rev_sub(dev));+/* Only PFs hold the relevant PCIe information for this query */+if(mlx5_core_is_pf(dev))+pcie_print_link_status(dev->pdev);+/* on load removing any previous indication of internal error, device is*up*/
From: Tal Gilboa <redacted>
Use the new pci_bandwidth_available() function to calculate maximum
available bandwidth through the PCI chain instead of computing it ourselves
with mlx5e_get_pci_bw().
This is used to detect when the device is capable of more bandwidth than is
available in the current slot. The driver may adjust compression settings
accordingly.
Note that pci_bandwidth_available() accounts for PCIe encoding overhead, so
it is more accurate than mlx5e_get_pci_bw() was.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: remove mlx5e_get_pci_bw() wrapper altogether]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 32 +--------------------
1 file changed, 1 insertion(+), 31 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 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(-)
@@ -13922,8 +13922,6 @@ static int bnx2x_init_one(struct pci_dev *pdev,{structnet_device*dev=NULL;structbnx2x*bp;-enumpcie_link_widthpcie_width;-enumpci_bus_speedpcie_speed;intrc,max_non_def_sbs;intrx_count,tx_count,rss_count,doorbell_size;intmax_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);
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
@@ -8469,22 +8469,6 @@ static int bnxt_init_mac_addr(struct bnxt *bp)returnrc;}-staticvoidbnxt_parse_log_pcie_link(structbnxt*bp)-{-enumpcie_link_widthwidth=PCIE_LNK_WIDTH_UNKNOWN;-enumpci_bus_speedspeed=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);-}-staticintbnxt_init_one(structpci_dev*pdev,conststructpci_device_id*ent){staticintversion_printed;
@@ -8694,8 +8678,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);return0;
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 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(-)
@@ -4762,79 +4762,6 @@ static int init_rss(struct adapter *adap)return0;}-staticintcxgb4_get_pcie_dev_link_caps(structadapter*adap,-enumpci_bus_speed*speed,-enumpcie_link_width*width)-{-u32lnkcap1,lnkcap2;-interr1,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;-elseif(lnkcap2&PCI_EXP_LNKCAP2_SLS_5_0GB)-*speed=PCIE_SPEED_5_0GT;-elseif(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;-elseif(lnkcap1&PCI_EXP_LNKCAP_SLS_2_5GB)-*speed=PCIE_SPEED_2_5GT;-}-}--if(*speed==PCI_SPEED_UNKNOWN||*width==PCIE_LNK_WIDTH_UNKNOWN)-returnerr1?err1:err2?err2:-EINVAL;-return0;-}--staticvoidcxgb4_check_pcie_caps(structadapter*adap)-{-enumpcie_link_widthwidth,width_cap;-enumpci_bus_speedspeed,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 */staticvoidprint_adapter_info(structadapter*adapter){
@@ -5466,7 +5393,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)
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 87 --------------------------
1 file changed, 1 insertion(+), 86 deletions(-)
@@ -2120,91 +2120,6 @@ static int fm10k_sw_init(struct fm10k_intfc *interface,return0;}-staticvoidfm10k_slot_warn(structfm10k_intfc*interface)-{-enumpcie_link_widthwidth=PCIE_LNK_WIDTH_UNKNOWN;-enumpci_bus_speedspeed=PCI_SPEED_UNKNOWN;-structfm10k_hw*hw=&interface->hw;-intmax_gts=0,expected_gts=0;--if(pcie_get_minimum_link(interface->pdev,&speed,&width)||-speed==PCI_SPEED_UNKNOWN||width==PCIE_LNK_WIDTH_UNKNOWN){-dev_warn(&interface->pdev->dev,-"Unable to determine PCI Express bandwidth.\n");-return;-}--switch(speed){-casePCIE_SPEED_2_5GT:-/* 8b/10b encoding reduces max throughput by 20% */-max_gts=2*width;-break;-casePCIE_SPEED_5_0GT:-/* 8b/10b encoding reduces max throughput by 20% */-max_gts=4*width;-break;-casePCIE_SPEED_8_0GT:-/* 128b/130b encoding has less than 2% impact on throughput */-max_gts=8*width;-break;-default:-dev_warn(&interface->pdev->dev,-"Unable to determine PCI Express bandwidth.\n");-return;-}--dev_info(&interface->pdev->dev,-"PCI Express bandwidth of %dGT/s available\n",-max_gts);-dev_info(&interface->pdev->dev,-"(Speed:%s, Width: x%d, Encoding Loss:%s, Payload:%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"),-hw->bus.width,-(speed==PCIE_SPEED_2_5GT?"20%":-speed==PCIE_SPEED_5_0GT?"20%":-speed==PCIE_SPEED_8_0GT?"<2%":-"Unknown"),-(hw->bus.payload==fm10k_bus_payload_128?"128B":-hw->bus.payload==fm10k_bus_payload_256?"256B":-hw->bus.payload==fm10k_bus_payload_512?"512B":-"Unknown"));--switch(hw->bus_caps.speed){-casefm10k_bus_speed_2500:-/* 8b/10b encoding reduces max throughput by 20% */-expected_gts=2*hw->bus_caps.width;-break;-casefm10k_bus_speed_5000:-/* 8b/10b encoding reduces max throughput by 20% */-expected_gts=4*hw->bus_caps.width;-break;-casefm10k_bus_speed_8000:-/* 128b/130b encoding has less than 2% impact on throughput */-expected_gts=8*hw->bus_caps.width;-break;-default:-dev_warn(&interface->pdev->dev,-"Unable to determine expected PCI Express bandwidth.\n");-return;-}--if(max_gts>=expected_gts)-return;--dev_warn(&interface->pdev->dev,-"This device requires %dGT/s of bandwidth for optimal performance.\n",-expected_gts);-dev_warn(&interface->pdev->dev,-"A %sslot with x%d lanes is suggested.\n",-(hw->bus_caps.speed==fm10k_bus_speed_2500?"2.5GT/s ":-hw->bus_caps.speed==fm10k_bus_speed_5000?"5.0GT/s ":-hw->bus_caps.speed==fm10k_bus_speed_8000?"8.0GT/s ":""),-hw->bus_caps.width);-}-/***fm10k_probe-DeviceInitializationRoutine*@pdev:PCIdeviceinformationstruct
@@ -2326,7 +2241,7 @@ static int fm10k_probe(struct pci_dev *pdev, const struct pci_device_id *ent)mod_timer(&interface->service_timer,(HZ*2)+jiffies);/* print warning for non-optimal configurations */-fm10k_slot_warn(interface);+pcie_print_link_status(interface->pdev);/* report MAC address for logging */dev_info(&pdev->dev,"%pM\n",netdev->dev_addr);
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 47 +------------------------
1 file changed, 1 insertion(+), 46 deletions(-)
@@ -270,9 +270,6 @@ static void ixgbe_check_minimum_link(struct ixgbe_adapter *adapter,intexpected_gts){structixgbe_hw*hw=&adapter->hw;-intmax_gts=0;-enumpci_bus_speedspeed=PCI_SPEED_UNKNOWN;-enumpcie_link_widthwidth=PCIE_LNK_WIDTH_UNKNOWN;structpci_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,elsepdev=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){-casePCIE_SPEED_2_5GT:-/* 8b/10b encoding reduces max throughput by 20% */-max_gts=2*width;-break;-casePCIE_SPEED_5_0GT:-/* 8b/10b encoding reduces max throughput by 20% */-max_gts=4*width;-break;-casePCIE_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);}staticvoidixgbe_service_event_schedule(structixgbe_adapter*adapter)
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, if the path
contained a 16 GT/s x1 link and a 2.5 GT/s x16 link,
pcie_get_minimum_link() returned 2.5 GT/s x1, which corresponds to 250 MB/s
of bandwidth, not the actual available bandwidth of about 2000 MB/s for a
16 GT/s x1 link.
Callers should use pcie_print_link_status() instead, or
pcie_bandwidth_available() if they need more detailed information.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 43 -------------------------------------------
include/linux/pci.h | 2 --
2 files changed, 45 deletions(-)
@@ -5103,49 +5103,6 @@ int pcie_set_mps(struct pci_dev *dev, int mps)}EXPORT_SYMBOL(pcie_set_mps);-/**-*pcie_get_minimum_link-determineminimumlinksettingsofaPCIdevice-*@dev:PCIdevicetoquery-*@speed:storageforminimumspeed-*@width:storageforminimumwidth-*-*ThisfunctionwillwalkupthePCIdevicechainanddeterminetheminimum-*linkwidthandspeedofthedevice.-*/-intpcie_get_minimum_link(structpci_dev*dev,enumpci_bus_speed*speed,-enumpcie_link_width*width)-{-intret;--*speed=PCI_SPEED_UNKNOWN;-*width=PCIE_LNK_WIDTH_UNKNOWN;--while(dev){-u16lnksta;-enumpci_bus_speednext_speed;-enumpcie_link_widthnext_width;--ret=pcie_capability_read_word(dev,PCI_EXP_LNKSTA,&lnksta);-if(ret)-returnret;--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;-}--return0;-}-EXPORT_SYMBOL(pcie_get_minimum_link);-/***pcie_bandwidth_available-determineminimumlinksettingsofaPCIe*deviceanditsbandwidthlimitation
From: Tal Gilboa <hidden> Date: 2018-04-01 20:39:08
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted hunk
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
From: Tal Gilboa <hidden> Date: 2018-04-01 20:42:01
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted hunk
From: Tal Gilboa <redacted>
Add pcie_bandwidth_available() to compute the bandwidth available to a
device. This may be limited by the device itself or by a slower upstream
link leading to the device.
The available bandwidth at each link along the path is computed as:
link_speed * link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Also return the device with the slowest link and the speed and width of
that link.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, leave pcie_get_minimum_link() alone for now, return
bw directly, use pci_upstream_bridge(), check "next_bw <= bw" to find
uppermost limiting device, return speed/width of the limiting device]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 3 +++
2 files changed, 57 insertions(+)
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -261,8 +261,17 @@ void pci_disable_bridge_window(struct pci_dev *dev);(speed)==PCIE_SPEED_2_5GT?"2.5 GT/s":\"Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
On Sun, Apr 01, 2018 at 11:41:42PM +0300, Tal Gilboa wrote:
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_available() to compute the bandwidth available to a
device. This may be limited by the device itself or by a slower upstream
link leading to the device.
The available bandwidth at each link along the path is computed as:
link_speed * link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Also return the device with the slowest link and the speed and width of
that link.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, leave pcie_get_minimum_link() alone for now, return
bw directly, use pci_upstream_bridge(), check "next_bw <= bw" to find
uppermost limiting device, return speed/width of the limiting device]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 3 +++
2 files changed, 57 insertions(+)
From: Tal Gilboa <hidden> Date: 2018-04-02 07:35:10
On 4/2/2018 3:40 AM, Bjorn Helgaas wrote:
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
quoted
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -261,8 +261,17 @@ void pci_disable_bridge_window(struct pci_dev *dev);(speed)==PCIE_SPEED_2_5GT?"2.5 GT/s":\"Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't added. Need to return 15754.
On Mon, Apr 02, 2018 at 10:34:58AM +0300, Tal Gilboa wrote:
On 4/2/2018 3:40 AM, Bjorn Helgaas wrote:
quoted
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
quoted
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -261,8 +261,17 @@ void pci_disable_bridge_window(struct pci_dev *dev);(speed)==PCIE_SPEED_2_5GT?"2.5 GT/s":\"Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't
added. Need to return 15754.
Oh, duh, of course! Sorry for being dense. What about the following?
I included the calculation as opposed to just the magic numbers to try
to make it clear how they're derived. This has the disadvantage of
truncating the result instead of rounding, but I doubt that's
significant in this context. If it is, we could use the magic numbers
and put the computation in a comment.
Another question: we currently deal in Mb/s, not MB/s. Mb/s has the
advantage of sort of corresponding to the GT/s numbers, but using MB/s
would have the advantage of smaller numbers that match the table here:
https://en.wikipedia.org/wiki/PCI_Express#History_and_revisions,
but I don't know what's most typical in user-facing situations.
What's better?
commit 946435491b35b7782157e9a4d1bd73071fba7709
Author: Tal Gilboa [off-list ref]
Date: Fri Mar 30 08:32:03 2018 -0500
PCI: Add pcie_bandwidth_capable() to compute max supported link bandwidth
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_width * max_link_speed * (1 - encoding_overhead)
2.5 and 5.0 GT/s links use 8b/10b encoding, which reduces the raw bandwidth
available by 20%; 8.0 GT/s and faster links use 128b/130b encoding, which
reduces it by about 1.5%.
The result is in Mb/s, i.e., megabits/second, of raw bandwidth.
Signed-off-by: Tal Gilboa [off-list ref]
[bhelgaas: add 16 GT/s, adjust for pcie_get_speed_cap() and
pcie_get_width_cap() signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas [off-list ref]
Reviewed-by: Tariq Toukan [off-list ref]
From: Tal Gilboa <hidden> Date: 2018-04-02 14:34:39
On 4/2/2018 5:05 PM, Bjorn Helgaas wrote:
On Mon, Apr 02, 2018 at 10:34:58AM +0300, Tal Gilboa wrote:
quoted
On 4/2/2018 3:40 AM, Bjorn Helgaas wrote:
quoted
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
quoted
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -261,8 +261,17 @@ void pci_disable_bridge_window(struct pci_dev *dev);(speed)==PCIE_SPEED_2_5GT?"2.5 GT/s":\"Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't
added. Need to return 15754.
Oh, duh, of course! Sorry for being dense. What about the following?
I included the calculation as opposed to just the magic numbers to try
to make it clear how they're derived. This has the disadvantage of
truncating the result instead of rounding, but I doubt that's
significant in this context. If it is, we could use the magic numbers
and put the computation in a comment.
We can always use DIV_ROUND_UP((speed * enc_nominator), enc_denominator). I think this is confusing and since this introduces a bandwidth limit I would prefer to give a wider limit than a wrong one, even it is by less than 1Mb/s. My vote is for leaving it as you wrote below.
Another question: we currently deal in Mb/s, not MB/s. Mb/s has the
advantage of sort of corresponding to the GT/s numbers, but using MB/s
would have the advantage of smaller numbers that match the table here:
https://en.wikipedia.org/wiki/PCI_Express#History_and_revisions,
but I don't know what's most typical in user-facing situations.
What's better?
I don't know what's better but for network devices we measure bandwidth in Gb/s, so presenting bandwidth in MB/s would mean additional calculations. The truth is I would have prefer to use Gb/s instead of Mb/s, but again, don't want to loss up to 1Gb/s.
quoted hunk
commit 946435491b35b7782157e9a4d1bd73071fba7709
Author: Tal Gilboa [off-list ref]
Date: Fri Mar 30 08:32:03 2018 -0500
PCI: Add pcie_bandwidth_capable() to compute max supported link bandwidth
Add pcie_bandwidth_capable() to compute the max link bandwidth supported by
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_width * max_link_speed * (1 - encoding_overhead)
2.5 and 5.0 GT/s links use 8b/10b encoding, which reduces the raw bandwidth
available by 20%; 8.0 GT/s and faster links use 128b/130b encoding, which
reduces it by about 1.5%.
The result is in Mb/s, i.e., megabits/second, of raw bandwidth.
Signed-off-by: Tal Gilboa [off-list ref]
[bhelgaas: add 16 GT/s, adjust for pcie_get_speed_cap() and
pcie_get_width_cap() signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas [off-list ref]
Reviewed-by: Tariq Toukan [off-list ref]
From: "Keller, Jacob E" <jacob.e.keller@intel.com> Date: 2018-04-02 15:56:11
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:06 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 12/14] fm10k: Report PCIe link properties with
pcie_print_link_status()
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
This comment is about what's being fixed, so it would have been easier to parse if it were written to more clearly indicate that we're removing (and not adding) this behavior.
Aside from the commit message (which I don't feel strongly enough needs a re-send of the patch) this looks good to me.
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Thanks Bjorn and Tal for fixing this!
From: "Keller, Jacob E" <jacob.e.keller@intel.com> Date: 2018-04-02 16:00:24
-----Original Message-----
From: Tal Gilboa [mailto:talgi@mellanox.com]
Sent: Monday, April 02, 2018 7:34 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 03/14] PCI: Add pcie_bandwidth_capable() to compute
max supported link bandwidth
On 4/2/2018 5:05 PM, Bjorn Helgaas wrote:
quoted
On Mon, Apr 02, 2018 at 10:34:58AM +0300, Tal Gilboa wrote:
quoted
On 4/2/2018 3:40 AM, Bjorn Helgaas wrote:
quoted
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
quoted
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth
supported by
quoted
quoted
quoted
quoted
quoted
a device, based on the max link speed and width, adjusted by the
encoding
quoted
quoted
quoted
quoted
quoted
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using
8b/10b
quoted
quoted
quoted
quoted
quoted
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
return PCIE_LNK_WIDTH_UNKNOWN; }+/**+ * pcie_bandwidth_capable - calculates a PCI device's link bandwidth
capability
quoted
quoted
quoted
quoted
quoted
+ * @dev: PCI device+ * @speed: storage for link speed+ * @width: storage for link width+ *+ * Calculate a PCI device's link bandwidth by querying for its link speed+ * and width, multiplying them, and applying encoding overhead.+ */+u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed
+ return 0;++ return *width * PCIE_SPEED2MBS_ENC(*speed);+}+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
(speed) == PCIE_SPEED_2_5GT ? "2.5 GT/s" : \ "Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for
gen3 */
quoted
quoted
quoted
quoted
quoted
+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't
added. Need to return 15754.
Oh, duh, of course! Sorry for being dense. What about the following?
I included the calculation as opposed to just the magic numbers to try
to make it clear how they're derived. This has the disadvantage of
truncating the result instead of rounding, but I doubt that's
significant in this context. If it is, we could use the magic numbers
and put the computation in a comment.
We can always use DIV_ROUND_UP((speed * enc_nominator),
enc_denominator). I think this is confusing and since this introduces a
bandwidth limit I would prefer to give a wider limit than a wrong one,
even it is by less than 1Mb/s. My vote is for leaving it as you wrote below.
quoted
Another question: we currently deal in Mb/s, not MB/s. Mb/s has the
advantage of sort of corresponding to the GT/s numbers, but using MB/s
would have the advantage of smaller numbers that match the table here:
https://en.wikipedia.org/wiki/PCI_Express#History_and_revisions,
but I don't know what's most typical in user-facing situations.
What's better?
I don't know what's better but for network devices we measure bandwidth
in Gb/s, so presenting bandwidth in MB/s would mean additional
calculations. The truth is I would have prefer to use Gb/s instead of
Mb/s, but again, don't want to loss up to 1Gb/s.
I prefer this version with the calculation in line since it makes the derivation clear. Keeping them in Mb/s makes it easier to convert to Gb/s, which is what most people would expect.
Thanks,
Jake
quoted
commit 946435491b35b7782157e9a4d1bd73071fba7709
Author: Tal Gilboa [off-list ref]
Date: Fri Mar 30 08:32:03 2018 -0500
PCI: Add pcie_bandwidth_capable() to compute max supported link
bandwidth
quoted
Add pcie_bandwidth_capable() to compute the max link bandwidth
supported by
quoted
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_width * max_link_speed * (1 - encoding_overhead)
2.5 and 5.0 GT/s links use 8b/10b encoding, which reduces the raw
bandwidth
quoted
available by 20%; 8.0 GT/s and faster links use 128b/130b encoding, which
reduces it by about 1.5%.
The result is in Mb/s, i.e., megabits/second, of raw bandwidth.
Signed-off-by: Tal Gilboa [off-list ref]
[bhelgaas: add 16 GT/s, adjust for pcie_get_speed_cap() and
pcie_get_width_cap() signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas [off-list ref]
Reviewed-by: Tariq Toukan [off-list ref]
return PCIE_LNK_WIDTH_UNKNOWN; }+/**+ * pcie_bandwidth_capable - calculate a PCI device's link bandwidth capability+ * @dev: PCI device+ * @speed: storage for link speed+ * @width: storage for link width+ *+ * Calculate a PCI device's link bandwidth by querying for its link speed+ * and width, multiplying them, and applying encoding overhead. The result+ * is in Mb/s, i.e., megabits/second of raw bandwidth.+ */+u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed
+ return 0;++ return *width * PCIE_SPEED2MBS_ENC(*speed);+}+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
From: "Keller, Jacob E" <jacob.e.keller@intel.com> Date: 2018-04-02 16:25:20
quoted hunk
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:05 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed and
whether it's limited
From: Tal Gilboa <redacted>
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 30 insertions(+)
enum pci_bus_speed *speed,
return *width * PCIE_SPEED2MBS_ENC(*speed);
}
+/**
+ * pcie_print_link_status - Report the PCI device's link speed and width
+ * @dev: PCI device to query
+ *
+ * Report the available bandwidth at the device. If this is less than the
+ * device is capable of, report the device's maximum possible bandwidth and
+ * the upstream link that limits its performance to less than that.
+ */
+void pcie_print_link_status(struct pci_dev *dev)
+{
+ enum pcie_link_width width, width_cap;
+ enum pci_bus_speed speed, speed_cap;
+ struct pci_dev *limiting_dev = NULL;
+ u32 bw_avail, bw_cap;
+
+ bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
+ bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed,
&width);
+
+ if (bw_avail >= bw_cap)
+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+ else
+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d
link at %s (capable of %d Mb/s with %s x%d link)\n",
+ bw_avail, PCIE_SPEED2STR(speed), width,
+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+}
Personally, I would make thic last one a pci_warn() to indicate it at a higher log level, but I'm ok with the wording, and if consensus is that this should be at info, I'm ok with that.
Thanks,
Jake
quoted hunk
+EXPORT_SYMBOL(pcie_print_link_status);+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
On Mon, Apr 02, 2018 at 04:00:16PM +0000, Keller, Jacob E wrote:
quoted
-----Original Message-----
From: Tal Gilboa [mailto:talgi@mellanox.com]
Sent: Monday, April 02, 2018 7:34 AM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 03/14] PCI: Add pcie_bandwidth_capable() to compute
max supported link bandwidth
On 4/2/2018 5:05 PM, Bjorn Helgaas wrote:
quoted
On Mon, Apr 02, 2018 at 10:34:58AM +0300, Tal Gilboa wrote:
quoted
On 4/2/2018 3:40 AM, Bjorn Helgaas wrote:
quoted
On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote:
quoted
On 3/31/2018 12:05 AM, Bjorn Helgaas wrote:
quoted
From: Tal Gilboa <redacted>
Add pcie_bandwidth_capable() to compute the max link bandwidth
supported by
quoted
quoted
quoted
quoted
quoted
a device, based on the max link speed and width, adjusted by the
encoding
quoted
quoted
quoted
quoted
quoted
overhead.
The maximum bandwidth of the link is computed as:
max_link_speed * max_link_width * (1 - encoding_overhead)
The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using
8b/10b
quoted
quoted
quoted
quoted
quoted
encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b
encoding.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap()
signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Tariq Toukan <redacted>
---
drivers/pci/pci.c | 21 +++++++++++++++++++++
drivers/pci/pci.h | 9 +++++++++
2 files changed, 30 insertions(+)
return PCIE_LNK_WIDTH_UNKNOWN; }+/**+ * pcie_bandwidth_capable - calculates a PCI device's link bandwidth
capability
quoted
quoted
quoted
quoted
quoted
+ * @dev: PCI device+ * @speed: storage for link speed+ * @width: storage for link width+ *+ * Calculate a PCI device's link bandwidth by querying for its link speed+ * and width, multiplying them, and applying encoding overhead.+ */+u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed
+ return 0;++ return *width * PCIE_SPEED2MBS_ENC(*speed);+}+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
(speed) == PCIE_SPEED_2_5GT ? "2.5 GT/s" : \ "Unknown speed")+/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for
gen3 */
quoted
quoted
quoted
quoted
quoted
+#define PCIE_SPEED2MBS_ENC(speed) \
Missing gen4.
I made it "gen3+". I think that's accurate, isn't it? The spec
doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2
says rates of 8 GT/s or higher (which I think includes gen3 and gen4)
use 128b/130b encoding.
I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't
added. Need to return 15754.
Oh, duh, of course! Sorry for being dense. What about the following?
I included the calculation as opposed to just the magic numbers to try
to make it clear how they're derived. This has the disadvantage of
truncating the result instead of rounding, but I doubt that's
significant in this context. If it is, we could use the magic numbers
and put the computation in a comment.
We can always use DIV_ROUND_UP((speed * enc_nominator),
enc_denominator). I think this is confusing and since this introduces a
bandwidth limit I would prefer to give a wider limit than a wrong one,
even it is by less than 1Mb/s. My vote is for leaving it as you wrote below.
quoted
Another question: we currently deal in Mb/s, not MB/s. Mb/s has the
advantage of sort of corresponding to the GT/s numbers, but using MB/s
would have the advantage of smaller numbers that match the table here:
https://en.wikipedia.org/wiki/PCI_Express#History_and_revisions,
but I don't know what's most typical in user-facing situations.
What's better?
I don't know what's better but for network devices we measure bandwidth
in Gb/s, so presenting bandwidth in MB/s would mean additional
calculations. The truth is I would have prefer to use Gb/s instead of
Mb/s, but again, don't want to loss up to 1Gb/s.
I prefer this version with the calculation in line since it makes
the derivation clear. Keeping them in Mb/s makes it easier to
convert to Gb/s, which is what most people would expect.
OK, let's keep this patch as-is since returning Mb/s means we
don't have to worry about floating point, and it sounds like we
agree the truncation isn't a big deal.
I'll post a proposal to convert to Gb/s when printing.
quoted
quoted
commit 946435491b35b7782157e9a4d1bd73071fba7709
Author: Tal Gilboa [off-list ref]
Date: Fri Mar 30 08:32:03 2018 -0500
PCI: Add pcie_bandwidth_capable() to compute max supported link
bandwidth
quoted
Add pcie_bandwidth_capable() to compute the max link bandwidth
supported by
quoted
a device, based on the max link speed and width, adjusted by the encoding
overhead.
The maximum bandwidth of the link is computed as:
max_link_width * max_link_speed * (1 - encoding_overhead)
2.5 and 5.0 GT/s links use 8b/10b encoding, which reduces the raw
bandwidth
quoted
available by 20%; 8.0 GT/s and faster links use 128b/130b encoding, which
reduces it by about 1.5%.
The result is in Mb/s, i.e., megabits/second, of raw bandwidth.
Signed-off-by: Tal Gilboa [off-list ref]
[bhelgaas: add 16 GT/s, adjust for pcie_get_speed_cap() and
pcie_get_width_cap() signatures, don't export outside drivers/pci]
Signed-off-by: Bjorn Helgaas [off-list ref]
Reviewed-by: Tariq Toukan [off-list ref]
return PCIE_LNK_WIDTH_UNKNOWN; }+/**+ * pcie_bandwidth_capable - calculate a PCI device's link bandwidth capability+ * @dev: PCI device+ * @speed: storage for link speed+ * @width: storage for link width+ *+ * Calculate a PCI device's link bandwidth by querying for its link speed+ * and width, multiplying them, and applying encoding overhead. The result+ * is in Mb/s, i.e., megabits/second of raw bandwidth.+ */+u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed
+ return 0;++ return *width * PCIE_SPEED2MBS_ENC(*speed);+}+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
On Mon, Apr 02, 2018 at 04:25:17PM +0000, Keller, Jacob E wrote:
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:05 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed and
whether it's limited
From: Tal Gilboa <redacted>
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 30 insertions(+)
enum pci_bus_speed *speed,
return *width * PCIE_SPEED2MBS_ENC(*speed);
}
+/**
+ * pcie_print_link_status - Report the PCI device's link speed and width
+ * @dev: PCI device to query
+ *
+ * Report the available bandwidth at the device. If this is less than the
+ * device is capable of, report the device's maximum possible bandwidth and
+ * the upstream link that limits its performance to less than that.
+ */
+void pcie_print_link_status(struct pci_dev *dev)
+{
+ enum pcie_link_width width, width_cap;
+ enum pci_bus_speed speed, speed_cap;
+ struct pci_dev *limiting_dev = NULL;
+ u32 bw_avail, bw_cap;
+
+ bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
+ bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed,
&width);
+
+ if (bw_avail >= bw_cap)
+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+ else
+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d
link at %s (capable of %d Mb/s with %s x%d link)\n",
+ bw_avail, PCIE_SPEED2STR(speed), width,
+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+}
Personally, I would make thic last one a pci_warn() to indicate it at a
higher log level, but I'm ok with the wording, and if consensus is that
this should be at info, I'm ok with that.
Tal's original patch did have a pci_warn() here, and we went back and
forth a bit. They get bug reports when a device doesn't perform as
expected, which argues for pci_warn(). But they also got feedback
saying warnings are a bit too much, which argues for pci_info() [1]
I don't have a really strong opinion either way. I have a slight
preference for info because the user may not be able to do anything
about it (there may not be a faster slot available), and I think
distros are usually configured so a warning interrupts the smooth
graphical boot.
It looks like mlx4, fm10k, and ixgbe currently use warnings, while
bnx2x, bnxt_en, and cxgb4 use info. It's a tie so far :)
[1] https://lkml.kernel.org/r/e47f3628-b56c-4d0a-f18b-5ffaf261ccc0@mellanox.com
Here's a proposal for printing the bandwidth as "x.xxx Gb/s":
commit ad370f38c1b5e9b8bb941eaed84ebb676c4bdaa4
Author: Tal Gilboa [off-list ref]
Date: Fri Mar 30 08:56:47 2018 -0500
PCI: Add pcie_print_link_status() to log link speed and whether it's limited
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa [off-list ref]
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited, print bandwidth in Gb/s]
Signed-off-by: Bjorn Helgaas [off-list ref]
@@ -5287,6 +5287,38 @@ u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed, return *width * PCIE_SPEED2MBS_ENC(*speed); }+/**+ * pcie_print_link_status - Report the PCI device's link speed and width+ * @dev: PCI device to query+ *+ * Report the available bandwidth at the device. If this is less than the+ * device is capable of, report the device's maximum possible bandwidth and+ * the upstream link that limits its performance to less than that.+ */+void pcie_print_link_status(struct pci_dev *dev)+{+ enum pcie_link_width width, width_cap;+ enum pci_bus_speed speed, speed_cap;+ struct pci_dev *limiting_dev = NULL;+ u32 bw_avail, bw_cap;++ bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);+ bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width);++ if (bw_avail >= bw_cap)+ pci_info(dev, "%u.%03u Gb/s available bandwidth (%s x%d link)\n",+ bw_cap / 1000, bw_cap % 1000,+ PCIE_SPEED2STR(speed_cap), width_cap);+ else+ pci_info(dev, "%u.%03u Gb/s available bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",+ bw_avail / 1000, bw_avail % 1000,+ PCIE_SPEED2STR(speed), width,+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",+ bw_cap / 1000, bw_cap % 1000,+ PCIE_SPEED2STR(speed_cap), width_cap);+}+EXPORT_SYMBOL(pcie_print_link_status);+ /** * pci_select_bars - Make BAR mask from the type of resource * @dev: the PCI device for which BAR mask is made
From: "Keller, Jacob E" <jacob.e.keller@intel.com> Date: 2018-04-02 20:25:06
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Monday, April 02, 2018 12:58 PM
To: Keller, Jacob E <jacob.e.keller@intel.com>
Cc: Tal Gilboa <redacted>; Tariq Toukan <redacted>; Ariel
Elior [off-list ref]; Ganesh Goudar [off-list ref];
Kirsher, Jeffrey T [off-list ref]; everest-linux-l2@cavium.com;
intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed
and whether it's limited
On Mon, Apr 02, 2018 at 04:25:17PM +0000, Keller, Jacob E wrote:
quoted
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:05 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed
and
quoted
quoted
whether it's limited
From: Tal Gilboa <redacted>
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 30 insertions(+)
enum pci_bus_speed *speed,
return *width * PCIE_SPEED2MBS_ENC(*speed);
}
+/**
+ * pcie_print_link_status - Report the PCI device's link speed and width
+ * @dev: PCI device to query
+ *
+ * Report the available bandwidth at the device. If this is less than the
+ * device is capable of, report the device's maximum possible bandwidth and
+ * the upstream link that limits its performance to less than that.
+ */
+void pcie_print_link_status(struct pci_dev *dev)
+{
+ enum pcie_link_width width, width_cap;
+ enum pci_bus_speed speed, speed_cap;
+ struct pci_dev *limiting_dev = NULL;
+ u32 bw_avail, bw_cap;
+
+ bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
+ bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed,
&width);
+
+ if (bw_avail >= bw_cap)
+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+ else
+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d
link at %s (capable of %d Mb/s with %s x%d link)\n",
+ bw_avail, PCIE_SPEED2STR(speed), width,
+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+}
Personally, I would make thic last one a pci_warn() to indicate it at a
higher log level, but I'm ok with the wording, and if consensus is that
this should be at info, I'm ok with that.
Tal's original patch did have a pci_warn() here, and we went back and
forth a bit. They get bug reports when a device doesn't perform as
expected, which argues for pci_warn(). But they also got feedback
saying warnings are a bit too much, which argues for pci_info() [1]
I don't have a really strong opinion either way. I have a slight
preference for info because the user may not be able to do anything
about it (there may not be a faster slot available), and I think
distros are usually configured so a warning interrupts the smooth
graphical boot.
It looks like mlx4, fm10k, and ixgbe currently use warnings, while
bnx2x, bnxt_en, and cxgb4 use info. It's a tie so far :)
[1] https://lkml.kernel.org/r/e47f3628-b56c-4d0a-f18b-
5ffaf261ccc0@mellanox.com
With that information, I'm fine with the proposal to display this as only an info. The message is still printed and can be used for debugging purposes, and I think that's really enough.
Here's a proposal for printing the bandwidth as "x.xxx Gb/s":
On Mon, Apr 02, 2018 at 03:56:06PM +0000, Keller, Jacob E wrote:
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:06 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 12/14] fm10k: Report PCIe link properties with
pcie_print_link_status()
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information because
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
This comment is about what's being fixed, so it would have been easier to
parse if it were written to more clearly indicate that we're removing
(and not adding) this behavior.
Good point. Is this any better?
fm10k: Report PCIe link properties with pcie_print_link_status()
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.
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.
From: "Keller, Jacob E" <jacob.e.keller@intel.com> Date: 2018-04-02 20:36:14
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Bjorn Helgaas
Sent: Monday, April 02, 2018 1:32 PM
To: Keller, Jacob E <jacob.e.keller@intel.com>
Cc: Tal Gilboa <redacted>; Tariq Toukan <redacted>; Ariel
Elior [off-list ref]; Ganesh Goudar [off-list ref];
Kirsher, Jeffrey T [off-list ref]; everest-linux-l2@cavium.com;
intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 12/14] fm10k: Report PCIe link properties with
pcie_print_link_status()
On Mon, Apr 02, 2018 at 03:56:06PM +0000, Keller, Jacob E wrote:
quoted
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:06 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 12/14] fm10k: Report PCIe link properties with
pcie_print_link_status()
From: Bjorn Helgaas <bhelgaas@google.com>
Use pcie_print_link_status() to report PCIe link speed and possible
limitations instead of implementing this in the driver itself.
Note that pcie_get_minimum_link() can return misleading information
because
quoted
quoted
it finds the slowest link and the narrowest link without considering the
total bandwidth of the link. If the path contains a 16 GT/s x1 link and a
2.5 GT/s x16 link, pcie_get_minimum_link() returns 2.5 GT/s x1, which
corresponds to 250 MB/s of bandwidth, not the actual available bandwidth of
about 2000 MB/s for a 16 GT/s x1 link.
This comment is about what's being fixed, so it would have been easier to
parse if it were written to more clearly indicate that we're removing
(and not adding) this behavior.
Good point. Is this any better?
fm10k: Report PCIe link properties with pcie_print_link_status()
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.
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.
From: Tal Gilboa <hidden> Date: 2018-04-02 21:09:19
On 4/2/2018 11:25 PM, Keller, Jacob E wrote:
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Monday, April 02, 2018 12:58 PM
To: Keller, Jacob E <jacob.e.keller@intel.com>
Cc: Tal Gilboa <redacted>; Tariq Toukan <redacted>; Ariel
Elior [off-list ref]; Ganesh Goudar [off-list ref];
Kirsher, Jeffrey T [off-list ref]; everest-linux-l2@cavium.com;
intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-pci@vger.kernel.org
Subject: Re: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed
and whether it's limited
On Mon, Apr 02, 2018 at 04:25:17PM +0000, Keller, Jacob E wrote:
quoted
quoted
-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org]
Sent: Friday, March 30, 2018 2:05 PM
To: Tal Gilboa <redacted>
Cc: Tariq Toukan <redacted>; Keller, Jacob E
[off-list ref]; Ariel Elior [off-list ref]; Ganesh
Goudar [off-list ref]; Kirsher, Jeffrey T
[off-list ref]; everest-linux-l2@cavium.com; intel-wired-
lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-pci@vger.kernel.org
Subject: [PATCH v5 05/14] PCI: Add pcie_print_link_status() to log link speed
and
quoted
quoted
whether it's limited
From: Tal Gilboa <redacted>
Add pcie_print_link_status(). This logs the current settings of the link
(speed, width, and total available bandwidth).
If the device is capable of more bandwidth but is limited by a slower
upstream link, we include information about the link that limits the
device's performance.
The user may be able to move the device to a different slot for better
performance.
This provides a unified method for all PCI devices to report status and
issues, instead of each device reporting in a different way, using
different code.
Signed-off-by: Tal Gilboa <redacted>
[bhelgaas: changelog, reword log messages, print device capabilities when
not limited]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 30 insertions(+)
enum pci_bus_speed *speed,
return *width * PCIE_SPEED2MBS_ENC(*speed);
}
+/**
+ * pcie_print_link_status - Report the PCI device's link speed and width
+ * @dev: PCI device to query
+ *
+ * Report the available bandwidth at the device. If this is less than the
+ * device is capable of, report the device's maximum possible bandwidth and
+ * the upstream link that limits its performance to less than that.
+ */
+void pcie_print_link_status(struct pci_dev *dev)
+{
+ enum pcie_link_width width, width_cap;
+ enum pci_bus_speed speed, speed_cap;
+ struct pci_dev *limiting_dev = NULL;
+ u32 bw_avail, bw_cap;
+
+ bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
+ bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed,
&width);
+
+ if (bw_avail >= bw_cap)
+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+ else
+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d
link at %s (capable of %d Mb/s with %s x%d link)\n",
+ bw_avail, PCIE_SPEED2STR(speed), width,
+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",
+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
+}
Personally, I would make thic last one a pci_warn() to indicate it at a
higher log level, but I'm ok with the wording, and if consensus is that
this should be at info, I'm ok with that.
Tal's original patch did have a pci_warn() here, and we went back and
forth a bit. They get bug reports when a device doesn't perform as
expected, which argues for pci_warn(). But they also got feedback
saying warnings are a bit too much, which argues for pci_info() [1]
I don't have a really strong opinion either way. I have a slight
preference for info because the user may not be able to do anything
about it (there may not be a faster slot available), and I think
distros are usually configured so a warning interrupts the smooth
graphical boot.
It looks like mlx4, fm10k, and ixgbe currently use warnings, while
bnx2x, bnxt_en, and cxgb4 use info. It's a tie so far :)
[1] https://lkml.kernel.org/r/e47f3628-b56c-4d0a-f18b-
5ffaf261ccc0@mellanox.com
With that information, I'm fine with the proposal to display this as only an info. The message is still printed and can be used for debugging purposes, and I think that's really enough.
quoted
Here's a proposal for printing the bandwidth as "x.xxx Gb/s":
Should this be "(speed * x ) / y" instead? wouldn't they calculate
128/130 and truncate that to zero before multiplying by the speed? Or
are compilers smart enough to do this the other way to avoid the
losses?
Thanks,
Jake
Should this be "(speed * x ) / y" instead? wouldn't they calculate
128/130 and truncate that to zero before multiplying by the speed? Or
are compilers smart enough to do this the other way to avoid the
losses?
Should this be "(speed * x ) / y" instead? wouldn't they calculate
128/130 and truncate that to zero before multiplying by the speed? Or
are compilers smart enough to do this the other way to avoid the
losses?
From: Jakub Kicinski <hidden> Date: 2018-04-13 04:33:00
On Fri, 30 Mar 2018 16:05:18 -0500, Bjorn Helgaas wrote:
quoted hunk
+ if (bw_avail >= bw_cap)+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);+ else+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d link at %s (capable of %d Mb/s with %s x%d link)\n",+ bw_avail, PCIE_SPEED2STR(speed), width,+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
I was just looking at using this new function to print PCIe BW for a
NIC, but I'm slightly worried that there is nothing in the message that
says PCIe... For a NIC some people may interpret the bandwidth as NIC
bandwidth:
[ 39.839989] nfp 0000:04:00.0: Netronome Flow Processor NFP4000/NFP6000 PCIe Card Probe
[ 39.848943] nfp 0000:04:00.0: 63.008 Gb/s available bandwidth (8 GT/s x8 link)
[ 39.857146] nfp 0000:04:00.0: RESERVED BARs: 0.0: General/MSI-X SRAM, 0.1: PCIe XPB/MSI-X PBA, 0.4: Explicit0, 0.5: Explicit1, fre4
It's not a 63Gbps NIC... I'm sorry if this was discussed before and I
didn't find it. Would it make sense to add the "PCIe: " prefix to the
message like bnx2x used to do? Like:
nfp 0000:04:00.0: PCIe: 63.008 Gb/s available bandwidth (8 GT/s x8 link)
Sorry for a very late comment.
On Thu, Apr 12, 2018 at 09:32:49PM -0700, Jakub Kicinski wrote:
On Fri, 30 Mar 2018 16:05:18 -0500, Bjorn Helgaas wrote:
quoted
+ if (bw_avail >= bw_cap)+ pci_info(dev, "%d Mb/s available bandwidth (%s x%d link)\n",+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);+ else+ pci_info(dev, "%d Mb/s available bandwidth, limited by %s x%d link at %s (capable of %d Mb/s with %s x%d link)\n",+ bw_avail, PCIE_SPEED2STR(speed), width,+ limiting_dev ? pci_name(limiting_dev) : "<unknown>",+ bw_cap, PCIE_SPEED2STR(speed_cap), width_cap);
I was just looking at using this new function to print PCIe BW for a
NIC, but I'm slightly worried that there is nothing in the message that
says PCIe... For a NIC some people may interpret the bandwidth as NIC
bandwidth:
[ 39.839989] nfp 0000:04:00.0: Netronome Flow Processor NFP4000/NFP6000 PCIe Card Probe
[ 39.848943] nfp 0000:04:00.0: 63.008 Gb/s available bandwidth (8 GT/s x8 link)
[ 39.857146] nfp 0000:04:00.0: RESERVED BARs: 0.0: General/MSI-X SRAM, 0.1: PCIe XPB/MSI-X PBA, 0.4: Explicit0, 0.5: Explicit1, fre4
It's not a 63Gbps NIC... I'm sorry if this was discussed before and I
didn't find it. Would it make sense to add the "PCIe: " prefix to the
message like bnx2x used to do? Like:
nfp 0000:04:00.0: PCIe: 63.008 Gb/s available bandwidth (8 GT/s x8 link)
I agree, that does look potentially confusing. How about this:
nfp 0000:04:00.0: 63.008 Gb/s available PCIe bandwidth (8 GT/s x8 link)
I did have to look twice at this before I remembered that we're
printing Gb/s (not GB/s). Most of the references I found on the web
use GB/s when talking about total PCIe bandwidth.
But either way I think it's definitely worth mentioning PCIe
explicitly.
I was just looking at using this new function to print PCIe BW for a
NIC, but I'm slightly worried that there is nothing in the message that
says PCIe... For a NIC some people may interpret the bandwidth as NIC
bandwidth:
[ 39.839989] nfp 0000:04:00.0: Netronome Flow Processor NFP4000/NFP6000
It's not a 63Gbps NIC... I'm sorry if this was discussed before and I
didn't find it. Would it make sense to add the "PCIe: " prefix to the
message like bnx2x used to do? Like:
nfp 0000:04:00.0: PCIe: 63.008 Gb/s available bandwidth (8 GT/s x8 link)
I agree, that does look potentially confusing. How about this:
nfp 0000:04:00.0: 63.008 Gb/s available PCIe bandwidth (8 GT/s x8 link)
I did have to look twice at this before I remembered that we're
printing Gb/s (not GB/s). Most of the references I found on the web
use GB/s when talking about total PCIe bandwidth.
But either way I think it's definitely worth mentioning PCIe
explicitly.
I also agree printing PCIe explicitly is good.
Thanks,
Jake