[PATCH 0/6] Improve link speed presentation process

[PATCH 0/6] Improve link speed presentation process

[Yicong Yang]

In this series:
1. Add 32 GT/s decoding in some macros as a complementary
2. Remove redundancy in speed presentation process and improve the codes.

Currently We use switch-case statements to acquire the speed
string according to the pci bus speed in current_link_speed_show()
and pcie_get_speed_cap(). It leads to redundant and when new
standard comes, we have to add cases in the related functions,
which is easy to omit at somewhere.

Abstract the judge statements out. Use macros and pci speed
arrays instead. Then only the macros and arrays need to be
extended when next generation comes.

Link:
https://lore.kernel.org/linux-pci/20200113211728.GA113776@google.com/
https://lore.kernel.org/linux-pci/20200114224909.GA19633@google.com/


Yicong Yang (6):
  PCI: add 32 GT/s decoding in some macros
  PCI: Make pci_bus_speed_strings[] public
  PCI: Add comments for link speed info arrays
  PCI: Improve and rename PCIE_SPEED2STR macro
  PCI: Add PCIE_LNKCAP2_SLS2SPEED macro
  PCI: Reduce redundancy in current_link_speed_show()

 drivers/pci/pci-sysfs.c | 26 ++++----------------------
 drivers/pci/pci.c       | 23 +++++++----------------
 drivers/pci/pci.h       | 22 +++++++++++++++-------
 drivers/pci/probe.c     | 37 +++++++++++++++++++++++++++++++++++++
 drivers/pci/slot.c      | 39 +++------------------------------------
 5 files changed, 66 insertions(+), 81 deletions(-)

--
2.8.1

[PATCH 1/6] PCI: add 32 GT/s decoding in some macros

[Yicong Yang]

Link speed 32.0 GT/s is supported in PCIe r5.0. Add in macro
PCIE_SPEED2STR and PCIE_SPEED2MBS_ENC to correctly decode.
This patch is a complementary to
commit de76cda215d5 ("PCI: Decode PCIe 32 GT/s link speed").

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/pci.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a0a53bd..a88c316 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -291,7 +291,8 @@ void pci_bus_put(struct pci_bus *bus);

 /* PCIe link information */
 #define PCIE_SPEED2STR(speed) \
-	((speed) == PCIE_SPEED_16_0GT ? "16 GT/s" : \
+	((speed) == PCIE_SPEED_32_0GT ? "32 GT/s" : \
+	 (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" : \
@@ -299,7 +300,8 @@ void pci_bus_put(struct pci_bus *bus);

 /* PCIe speed to Mb/s reduced by encoding overhead */
 #define PCIE_SPEED2MBS_ENC(speed) \
-	((speed) == PCIE_SPEED_16_0GT ? 16000*128/130 : \
+	((speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
+	 (speed) == PCIE_SPEED_16_0GT ? 16000*128/130 : \
 	 (speed) == PCIE_SPEED_8_0GT  ?  8000*128/130 : \
 	 (speed) == PCIE_SPEED_5_0GT  ?  5000*8/10 : \
 	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \
--
2.8.1

[PATCH 2/6] PCI: Make pci_bus_speed_strings[] public

[Yicong Yang]

pci_bus_speed_strings[] in slot.c defines universal speed information.
Make it public and move to probe.c so that we can use it. Remove "PCIe"
suffix of PCIe bus speed strings to reduce redundancy.

Use PCI_SPEED_UNKNOWN to judge the unknown speed condition in
bus_speed_read() in slot.c, as we cannot get array size from an external
array.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---

The reason why I don't add a boundary check is illustrated in Patch_4

 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
 drivers/pci/slot.c  | 35 +++--------------------------------
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a88c316..5fb1d76 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -9,6 +9,7 @@
 #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */

 extern const unsigned char pcie_link_speed[];
+extern const char *pci_bus_speed_strings[];
 extern bool pci_early_dump;

 bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb43..3c70b87 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -678,6 +678,35 @@ const unsigned char pcie_link_speed[] = {
 	PCI_SPEED_UNKNOWN		/* F */
 };

+/* these strings match up with the values in pci_bus_speed */
+const char *pci_bus_speed_strings[] = {
+	"33 MHz PCI",		/* 0x00 */
+	"66 MHz PCI",		/* 0x01 */
+	"66 MHz PCI-X",		/* 0x02 */
+	"100 MHz PCI-X",	/* 0x03 */
+	"133 MHz PCI-X",	/* 0x04 */
+	NULL,			/* 0x05 */
+	NULL,			/* 0x06 */
+	NULL,			/* 0x07 */
+	NULL,			/* 0x08 */
+	"66 MHz PCI-X 266",	/* 0x09 */
+	"100 MHz PCI-X 266",	/* 0x0a */
+	"133 MHz PCI-X 266",	/* 0x0b */
+	"Unknown AGP",		/* 0x0c */
+	"1x AGP",		/* 0x0d */
+	"2x AGP",		/* 0x0e */
+	"4x AGP",		/* 0x0f */
+	"8x AGP",		/* 0x10 */
+	"66 MHz PCI-X 533",	/* 0x11 */
+	"100 MHz PCI-X 533",	/* 0x12 */
+	"133 MHz PCI-X 533",	/* 0x13 */
+	"2.5 GT/s",	/* 0x14 */
+	"5.0 GT/s",	/* 0x15 */
+	"8.0 GT/s",	/* 0x16 */
+	"16.0 GT/s",	/* 0x17 */
+	"32.0 GT/s",	/* 0x18 */
+};
+
 void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
 {
 	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index ae4aa0e..140dafb 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -49,43 +49,14 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
 				slot->number);
 }

-/* these strings match up with the values in pci_bus_speed */
-static const char *pci_bus_speed_strings[] = {
-	"33 MHz PCI",		/* 0x00 */
-	"66 MHz PCI",		/* 0x01 */
-	"66 MHz PCI-X",		/* 0x02 */
-	"100 MHz PCI-X",	/* 0x03 */
-	"133 MHz PCI-X",	/* 0x04 */
-	NULL,			/* 0x05 */
-	NULL,			/* 0x06 */
-	NULL,			/* 0x07 */
-	NULL,			/* 0x08 */
-	"66 MHz PCI-X 266",	/* 0x09 */
-	"100 MHz PCI-X 266",	/* 0x0a */
-	"133 MHz PCI-X 266",	/* 0x0b */
-	"Unknown AGP",		/* 0x0c */
-	"1x AGP",		/* 0x0d */
-	"2x AGP",		/* 0x0e */
-	"4x AGP",		/* 0x0f */
-	"8x AGP",		/* 0x10 */
-	"66 MHz PCI-X 533",	/* 0x11 */
-	"100 MHz PCI-X 533",	/* 0x12 */
-	"133 MHz PCI-X 533",	/* 0x13 */
-	"2.5 GT/s PCIe",	/* 0x14 */
-	"5.0 GT/s PCIe",	/* 0x15 */
-	"8.0 GT/s PCIe",	/* 0x16 */
-	"16.0 GT/s PCIe",	/* 0x17 */
-	"32.0 GT/s PCIe",	/* 0x18 */
-};
-
 static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
 {
 	const char *speed_string;

-	if (speed < ARRAY_SIZE(pci_bus_speed_strings))
-		speed_string = pci_bus_speed_strings[speed];
-	else
+	if (speed == PCI_SPEED_UNKNOWN)
 		speed_string = "Unknown";
+	else
+		speed_string = pci_bus_speed_strings[speed];

 	return sprintf(buf, "%s\n", speed_string);
 }
--
2.8.1

[PATCH 3/6] PCI: Add comments for link speed info arrays

[Yicong Yang]

Add comments for pcix_bus_speed[] and pcie_link_speed[] arrays.
Indicating the capabilities which the information from.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/probe.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3c70b87..27e5e1e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -640,6 +640,10 @@ void pci_free_host_bridge(struct pci_host_bridge *bridge)
 }
 EXPORT_SYMBOL(pci_free_host_bridge);
 
+/**
+ * these indices represent secondary bus mode and
+ * frequency from  PCI_X_SSTATUS_FREQ
+ **/
 static const unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
 	PCI_SPEED_66MHz_PCIX,		/* 1 */
@@ -659,6 +663,10 @@ static const unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_133MHz_PCIX_533	/* F */
 };
 
+/**
+ * these indices represent PCIe link speed from
+ * PCI_EXP_LNKCAP, PCI_EXP_LNKSTA, PCI_EXP_LNKCAP2
+ **/
 const unsigned char pcie_link_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
 	PCIE_SPEED_2_5GT,		/* 1 */
-- 
2.8.1

[PATCH 4/6] PCI: Improve and rename PCIE_SPEED2STR macro

[Yicong Yang]

Use pci_bus_speed_strings[] array to refactor PCIE_SPEED2STR macro.
Rename PCIE_SPEED2STR with PCI_SPEED2STR as it's also used to
decode non-PCIe speeds. Modify bus_speed_read() and
__pcie_print_link_status() with PCI_SPEED2STR macro.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---

I don't add a boundary check in PCI_SPEED2STR macro because:
1. we cannot get the array size of an extern one using ARRAY_SIZE
2. It is the *speed* should be check valid or not when assigned,
rather than checking it here. Actually we do make it valid when
assigned, the speed is either a valid value or PCI_SPEED_UNKNOWN.
And it's ensured in pcie_get_speed_cap(), pci_set_bus_speed()
when probe, and pcie_link_speed[] array. Please check again for
sure.

 drivers/pci/pci-sysfs.c |  2 +-
 drivers/pci/pci.c       |  6 +++---
 drivers/pci/pci.h       | 10 +++-------
 drivers/pci/slot.c      | 10 +++-------
 4 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 13f766d..f4eafbc 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -156,7 +156,7 @@ static ssize_t max_link_speed_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);

-	return sprintf(buf, "%s\n", PCIE_SPEED2STR(pcie_get_speed_cap(pdev)));
+	return sprintf(buf, "%s\n", PCI_SPEED2STR(pcie_get_speed_cap(pdev)));
 }
 static DEVICE_ATTR_RO(max_link_speed);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e87196c..dce32ce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5868,14 +5868,14 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
 	if (bw_avail >= bw_cap && verbose)
 		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
 			 bw_cap / 1000, bw_cap % 1000,
-			 PCIE_SPEED2STR(speed_cap), width_cap);
+			 PCI_SPEED2STR(speed_cap), width_cap);
 	else if (bw_avail < bw_cap)
 		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
 			 bw_avail / 1000, bw_avail % 1000,
-			 PCIE_SPEED2STR(speed), width,
+			 PCI_SPEED2STR(speed), width,
 			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
 			 bw_cap / 1000, bw_cap % 1000,
-			 PCIE_SPEED2STR(speed_cap), width_cap);
+			 PCI_SPEED2STR(speed_cap), width_cap);
 }

 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5fb1d76..5e1f810 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -291,13 +291,9 @@ struct pci_bus *pci_bus_get(struct pci_bus *bus);
 void pci_bus_put(struct pci_bus *bus);

 /* PCIe link information */
-#define PCIE_SPEED2STR(speed) \
-	((speed) == PCIE_SPEED_32_0GT ? "32 GT/s" : \
-	 (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")
+#define PCI_SPEED2STR(speed) \
+	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
+	 pci_bus_speed_strings[speed])

 /* PCIe speed to Mb/s reduced by encoding overhead */
 #define PCIE_SPEED2MBS_ENC(speed) \
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 140dafb..871d598 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -51,14 +51,10 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)

 static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
 {
-	const char *speed_string;
+	if (speed <= PCI_SPEED_133MHz_PCIX_533)
+		return sprintf(buf, "%s\n", PCI_SPEED2STR(speed));

-	if (speed == PCI_SPEED_UNKNOWN)
-		speed_string = "Unknown";
-	else
-		speed_string = pci_bus_speed_strings[speed];
-
-	return sprintf(buf, "%s\n", speed_string);
+	return sprintf(buf, "%s PCIe\n", PCI_SPEED2STR(speed));
 }

 static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
--
2.8.1

[PATCH 5/6] PCI: Add PCIE_LNKCAP2_SLS2SPEED macro

[Yicong Yang]

Add PCIE_LNKCAP2_SLS2SPEED macro for transforming raw link cap 2
value to link speed. Use it in pcie_get_speed_cap() to reduce
redundancy. We'll not touch the functions when new link
speed comes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/pci.c | 17 ++++-------------
 drivers/pci/pci.h |  9 +++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index dce32ce..2ef4030 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5780,19 +5780,10 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
 	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
 	 */
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
-	if (lnkcap2) { /* PCIe r3.0-compliant */
-		if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB)
-			return PCIE_SPEED_32_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB)
-			return PCIE_SPEED_16_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
-			return PCIE_SPEED_8_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
-			return PCIE_SPEED_5_0GT;
-		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
-			return PCIE_SPEED_2_5GT;
-		return PCI_SPEED_UNKNOWN;
-	}
+
+	/* PCIe r3.0-compliant */
+	if (lnkcap2)
+		return PCIE_LNKCAP2_SLS2SPEED(lnkcap2);
 
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
 	if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5e1f810..3d988e9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -290,6 +290,15 @@ void pci_disable_bridge_window(struct pci_dev *dev);
 struct pci_bus *pci_bus_get(struct pci_bus *bus);
 void pci_bus_put(struct pci_bus *bus);
 
+/* PCIe link information from Link Capabilities 2 */
+#define PCIE_LNKCAP2_SLS2SPEED(lnkcap2) \
+	((lnkcap2) & PCI_EXP_LNKCAP2_SLS_32_0GB ? PCIE_SPEED_32_0GT : \
+	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_16_0GB ? PCIE_SPEED_16_0GT : \
+	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_8_0GB ? PCIE_SPEED_8_0GT : \
+	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_5_0GB ? PCIE_SPEED_5_0GT : \
+	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_2_5GB ? PCIE_SPEED_2_5GT : \
+	 PCI_SPEED_UNKNOWN)
+
 /* PCIe link information */
 #define PCI_SPEED2STR(speed) \
 	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
-- 
2.8.1

[PATCH 6/6] PCI: Reduce redundancy in current_link_speed_show()

[Yicong Yang]

Remove switch-case statements in current_link_speed_show(). Use
pcie_link_speed[] array to get link speed and PCI_SPEED2STR macro
to get link speed string.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/pci-sysfs.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index f4eafbc..eaece10 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -175,33 +175,15 @@ static ssize_t current_link_speed_show(struct device *dev,
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	u16 linkstat;
 	int err;
-	const char *speed;
+	enum pci_bus_speed speed;
 
 	err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
 	if (err)
 		return -EINVAL;
 
-	switch (linkstat & PCI_EXP_LNKSTA_CLS) {
-	case PCI_EXP_LNKSTA_CLS_32_0GB:
-		speed = "32 GT/s";
-		break;
-	case PCI_EXP_LNKSTA_CLS_16_0GB:
-		speed = "16 GT/s";
-		break;
-	case PCI_EXP_LNKSTA_CLS_8_0GB:
-		speed = "8 GT/s";
-		break;
-	case PCI_EXP_LNKSTA_CLS_5_0GB:
-		speed = "5 GT/s";
-		break;
-	case PCI_EXP_LNKSTA_CLS_2_5GB:
-		speed = "2.5 GT/s";
-		break;
-	default:
-		speed = "Unknown speed";
-	}
+	speed = pcie_link_speed[linkstat & PCI_EXP_LNKSTA_CLS];
 
-	return sprintf(buf, "%s\n", speed);
+	return sprintf(buf, "%s\n", PCI_SPEED2STR(speed));
 }
 static DEVICE_ATTR_RO(current_link_speed);
 
-- 
2.8.1

Re: [PATCH 0/6] Improve link speed presentation process

[Yicong Yang]

Hi Bjorn,

would you mind giving some comments to help me get these patches merged?

Thanks,
Yicong Yang

On 2020/1/15 17:04, Yicong Yang wrote:
> In this series:
> 1. Add 32 GT/s decoding in some macros as a complementary
> 2. Remove redundancy in speed presentation process and improve the codes.
>
> Currently We use switch-case statements to acquire the speed
> string according to the pci bus speed in current_link_speed_show()
> and pcie_get_speed_cap(). It leads to redundant and when new
> standard comes, we have to add cases in the related functions,
> which is easy to omit at somewhere.
>
> Abstract the judge statements out. Use macros and pci speed
> arrays instead. Then only the macros and arrays need to be
> extended when next generation comes.
>
> Link:
> https://lore.kernel.org/linux-pci/20200113211728.GA113776@google.com/
> https://lore.kernel.org/linux-pci/20200114224909.GA19633@google.com/
>
>
> Yicong Yang (6):
>   PCI: add 32 GT/s decoding in some macros
>   PCI: Make pci_bus_speed_strings[] public
>   PCI: Add comments for link speed info arrays
>   PCI: Improve and rename PCIE_SPEED2STR macro
>   PCI: Add PCIE_LNKCAP2_SLS2SPEED macro
>   PCI: Reduce redundancy in current_link_speed_show()
>
>  drivers/pci/pci-sysfs.c | 26 ++++----------------------
>  drivers/pci/pci.c       | 23 +++++++----------------
>  drivers/pci/pci.h       | 22 +++++++++++++++-------
>  drivers/pci/probe.c     | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/pci/slot.c      | 39 +++------------------------------------
>  5 files changed, 66 insertions(+), 81 deletions(-)
>
> --
> 2.8.1
>
>
> .
>

Re: [PATCH 2/6] PCI: Make pci_bus_speed_strings[] public

[Bjorn Helgaas]

On Wed, Jan 15, 2020 at 05:04:19PM +0800, Yicong Yang wrote:
> pci_bus_speed_strings[] in slot.c defines universal speed information.
> Make it public and move to probe.c so that we can use it. Remove "PCIe"
> suffix of PCIe bus speed strings to reduce redundancy.

This needs to say exactly where this change will be observed: /proc
file, /sys file, dmesg, etc.  I would prefer that an observable change
be in its own patch instead of being a by-product of a structural
change like this one.

> Use PCI_SPEED_UNKNOWN to judge the unknown speed condition in
> bus_speed_read() in slot.c, as we cannot get array size from an external
> array.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> 
> The reason why I don't add a boundary check is illustrated in Patch_4
> 
>  drivers/pci/pci.h   |  1 +
>  drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
>  drivers/pci/slot.c  | 35 +++--------------------------------
>  3 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index a88c316..5fb1d76 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -9,6 +9,7 @@
>  #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
> 
>  extern const unsigned char pcie_link_speed[];
> +extern const char *pci_bus_speed_strings[];
>  extern bool pci_early_dump;
> 
>  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb43..3c70b87 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -678,6 +678,35 @@ const unsigned char pcie_link_speed[] = {
>  	PCI_SPEED_UNKNOWN		/* F */
>  };
> 
> +/* these strings match up with the values in pci_bus_speed */
> +const char *pci_bus_speed_strings[] = {
> +	"33 MHz PCI",		/* 0x00 */
> +	"66 MHz PCI",		/* 0x01 */
> +	"66 MHz PCI-X",		/* 0x02 */
> +	"100 MHz PCI-X",	/* 0x03 */
> +	"133 MHz PCI-X",	/* 0x04 */
> +	NULL,			/* 0x05 */
> +	NULL,			/* 0x06 */
> +	NULL,			/* 0x07 */
> +	NULL,			/* 0x08 */
> +	"66 MHz PCI-X 266",	/* 0x09 */
> +	"100 MHz PCI-X 266",	/* 0x0a */
> +	"133 MHz PCI-X 266",	/* 0x0b */
> +	"Unknown AGP",		/* 0x0c */
> +	"1x AGP",		/* 0x0d */
> +	"2x AGP",		/* 0x0e */
> +	"4x AGP",		/* 0x0f */
> +	"8x AGP",		/* 0x10 */
> +	"66 MHz PCI-X 533",	/* 0x11 */
> +	"100 MHz PCI-X 533",	/* 0x12 */
> +	"133 MHz PCI-X 533",	/* 0x13 */
> +	"2.5 GT/s",	/* 0x14 */
> +	"5.0 GT/s",	/* 0x15 */
> +	"8.0 GT/s",	/* 0x16 */
> +	"16.0 GT/s",	/* 0x17 */
> +	"32.0 GT/s",	/* 0x18 */
> +};
> +
>  void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
>  {
>  	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index ae4aa0e..140dafb 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -49,43 +49,14 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>  				slot->number);
>  }
> 
> -/* these strings match up with the values in pci_bus_speed */
> -static const char *pci_bus_speed_strings[] = {
> -	"33 MHz PCI",		/* 0x00 */
> -	"66 MHz PCI",		/* 0x01 */
> -	"66 MHz PCI-X",		/* 0x02 */
> -	"100 MHz PCI-X",	/* 0x03 */
> -	"133 MHz PCI-X",	/* 0x04 */
> -	NULL,			/* 0x05 */
> -	NULL,			/* 0x06 */
> -	NULL,			/* 0x07 */
> -	NULL,			/* 0x08 */
> -	"66 MHz PCI-X 266",	/* 0x09 */
> -	"100 MHz PCI-X 266",	/* 0x0a */
> -	"133 MHz PCI-X 266",	/* 0x0b */
> -	"Unknown AGP",		/* 0x0c */
> -	"1x AGP",		/* 0x0d */
> -	"2x AGP",		/* 0x0e */
> -	"4x AGP",		/* 0x0f */
> -	"8x AGP",		/* 0x10 */
> -	"66 MHz PCI-X 533",	/* 0x11 */
> -	"100 MHz PCI-X 533",	/* 0x12 */
> -	"133 MHz PCI-X 533",	/* 0x13 */
> -	"2.5 GT/s PCIe",	/* 0x14 */
> -	"5.0 GT/s PCIe",	/* 0x15 */
> -	"8.0 GT/s PCIe",	/* 0x16 */
> -	"16.0 GT/s PCIe",	/* 0x17 */
> -	"32.0 GT/s PCIe",	/* 0x18 */
> -};
> -
>  static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>  {
>  	const char *speed_string;
> 
> -	if (speed < ARRAY_SIZE(pci_bus_speed_strings))
> -		speed_string = pci_bus_speed_strings[speed];
> -	else
> +	if (speed == PCI_SPEED_UNKNOWN)
>  		speed_string = "Unknown";
> +	else
> +		speed_string = pci_bus_speed_strings[speed];

This is a little bit problematic because previously we checked the
actual array index but here we don't, so we may not index past the end
of the array.

It's possible to specify the array size explicitly in the extern
declaration, and I *think* that might mean ARRAY_SIZE() would still
work.  It's not ideal to have to update the extern declaration when
adding speeds, but at least it's something that would be noticed by
even the most trivial testing.

>  	return sprintf(buf, "%s\n", speed_string);
>  }
> --
> 2.8.1
> 

Re: [PATCH 4/6] PCI: Improve and rename PCIE_SPEED2STR macro

[Bjorn Helgaas]

On Wed, Jan 15, 2020 at 05:04:21PM +0800, Yicong Yang wrote:
> Use pci_bus_speed_strings[] array to refactor PCIE_SPEED2STR macro.
> Rename PCIE_SPEED2STR with PCI_SPEED2STR as it's also used to
> decode non-PCIe speeds. Modify bus_speed_read() and
> __pcie_print_link_status() with PCI_SPEED2STR macro.

This looks like it should be split into two or three patches.

  - Rename
  - Fiddle with the "PCIe" suffix
  - Refactor with pci_bus_speed_strings[]

I don't know the right order for these.

> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> 
> I don't add a boundary check in PCI_SPEED2STR macro because:
> 1. we cannot get the array size of an extern one using ARRAY_SIZE
> 2. It is the *speed* should be check valid or not when assigned,
> rather than checking it here. Actually we do make it valid when
> assigned, the speed is either a valid value or PCI_SPEED_UNKNOWN.
> And it's ensured in pcie_get_speed_cap(), pci_set_bus_speed()
> when probe, and pcie_link_speed[] array. Please check again for
> sure.

I'm a little nervous about this because bus->cur_bus_speed and
bus->max_bus_speed are set several places where we can't validate them
(powerpc, s390, faraday_pci_probe(), cpqhpc_probe(), get_max_bus_speed
(ibmphp), shpc_get_max_bus_speed(), etc).

I don't think we can avoid out-of-bounds references by relying on all
those places to get it right, especially since some of these can read
speeds from hardware, so new hardware can give us a bus speed that old
software won't know about.

>  drivers/pci/pci-sysfs.c |  2 +-
>  drivers/pci/pci.c       |  6 +++---
>  drivers/pci/pci.h       | 10 +++-------
>  drivers/pci/slot.c      | 10 +++-------
>  4 files changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766d..f4eafbc 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -156,7 +156,7 @@ static ssize_t max_link_speed_show(struct device *dev,
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> 
> -	return sprintf(buf, "%s\n", PCIE_SPEED2STR(pcie_get_speed_cap(pdev)));
> +	return sprintf(buf, "%s\n", PCI_SPEED2STR(pcie_get_speed_cap(pdev)));
>  }
>  static DEVICE_ATTR_RO(max_link_speed);
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e87196c..dce32ce 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5868,14 +5868,14 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
>  	if (bw_avail >= bw_cap && verbose)
>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
>  			 bw_cap / 1000, bw_cap % 1000,
> -			 PCIE_SPEED2STR(speed_cap), width_cap);
> +			 PCI_SPEED2STR(speed_cap), width_cap);
>  	else if (bw_avail < bw_cap)
>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
>  			 bw_avail / 1000, bw_avail % 1000,
> -			 PCIE_SPEED2STR(speed), width,
> +			 PCI_SPEED2STR(speed), width,
>  			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
>  			 bw_cap / 1000, bw_cap % 1000,
> -			 PCIE_SPEED2STR(speed_cap), width_cap);
> +			 PCI_SPEED2STR(speed_cap), width_cap);
>  }
> 
>  /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5fb1d76..5e1f810 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -291,13 +291,9 @@ struct pci_bus *pci_bus_get(struct pci_bus *bus);
>  void pci_bus_put(struct pci_bus *bus);
> 
>  /* PCIe link information */
> -#define PCIE_SPEED2STR(speed) \
> -	((speed) == PCIE_SPEED_32_0GT ? "32 GT/s" : \
> -	 (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")
> +#define PCI_SPEED2STR(speed) \
> +	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
> +	 pci_bus_speed_strings[speed])
> 
>  /* PCIe speed to Mb/s reduced by encoding overhead */
>  #define PCIE_SPEED2MBS_ENC(speed) \
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 140dafb..871d598 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -51,14 +51,10 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
> 
>  static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>  {
> -	const char *speed_string;
> +	if (speed <= PCI_SPEED_133MHz_PCIX_533)
> +		return sprintf(buf, "%s\n", PCI_SPEED2STR(speed));
> 
> -	if (speed == PCI_SPEED_UNKNOWN)
> -		speed_string = "Unknown";
> -	else
> -		speed_string = pci_bus_speed_strings[speed];
> -
> -	return sprintf(buf, "%s\n", speed_string);
> +	return sprintf(buf, "%s PCIe\n", PCI_SPEED2STR(speed));
>  }
> 
>  static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
> --
> 2.8.1
> 

Re: [PATCH 5/6] PCI: Add PCIE_LNKCAP2_SLS2SPEED macro

[Bjorn Helgaas]

On Wed, Jan 15, 2020 at 05:04:22PM +0800, Yicong Yang wrote:
> Add PCIE_LNKCAP2_SLS2SPEED macro for transforming raw link cap 2
> value to link speed. Use it in pcie_get_speed_cap() to reduce
> redundancy. We'll not touch the functions when new link
> speed comes.

The patch seems OK to me, but I don't see where it reduces redundancy.
There was one copy of "lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB" before,
and there's one copy after.  It's just moved from pci.c to pci.h.
Or am I missing something?

> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/pci.c | 17 ++++-------------
>  drivers/pci/pci.h |  9 +++++++++
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index dce32ce..2ef4030 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5780,19 +5780,10 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
>  	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
>  	 */
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
> -	if (lnkcap2) { /* PCIe r3.0-compliant */
> -		if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB)
> -			return PCIE_SPEED_32_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB)
> -			return PCIE_SPEED_16_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
> -			return PCIE_SPEED_8_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
> -			return PCIE_SPEED_5_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
> -			return PCIE_SPEED_2_5GT;
> -		return PCI_SPEED_UNKNOWN;
> -	}
> +
> +	/* PCIe r3.0-compliant */
> +	if (lnkcap2)
> +		return PCIE_LNKCAP2_SLS2SPEED(lnkcap2);
>  
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>  	if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5e1f810..3d988e9 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -290,6 +290,15 @@ void pci_disable_bridge_window(struct pci_dev *dev);
>  struct pci_bus *pci_bus_get(struct pci_bus *bus);
>  void pci_bus_put(struct pci_bus *bus);
>  
> +/* PCIe link information from Link Capabilities 2 */
> +#define PCIE_LNKCAP2_SLS2SPEED(lnkcap2) \
> +	((lnkcap2) & PCI_EXP_LNKCAP2_SLS_32_0GB ? PCIE_SPEED_32_0GT : \
> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_16_0GB ? PCIE_SPEED_16_0GT : \
> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_8_0GB ? PCIE_SPEED_8_0GT : \
> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_5_0GB ? PCIE_SPEED_5_0GT : \
> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_2_5GB ? PCIE_SPEED_2_5GT : \
> +	 PCI_SPEED_UNKNOWN)
> +
>  /* PCIe link information */
>  #define PCI_SPEED2STR(speed) \
>  	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
> -- 
> 2.8.1
> 

Re: [PATCH 2/6] PCI: Make pci_bus_speed_strings[] public

[Yicong Yang]

On 2020/2/6 2:35, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2020 at 05:04:19PM +0800, Yicong Yang wrote:
>> pci_bus_speed_strings[] in slot.c defines universal speed information.
>> Make it public and move to probe.c so that we can use it. Remove "PCIe"
>> suffix of PCIe bus speed strings to reduce redundancy.
> This needs to say exactly where this change will be observed: /proc
> file, /sys file, dmesg, etc.  I would prefer that an observable change
> be in its own patch instead of being a by-product of a structural
> change like this one.

I'll split this patch from this series and send it individually before the series.
And make detailed description in the next version.


>> Use PCI_SPEED_UNKNOWN to judge the unknown speed condition in
>> bus_speed_read() in slot.c, as we cannot get array size from an external
>> array.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>
>> The reason why I don't add a boundary check is illustrated in Patch_4
>>
>>  drivers/pci/pci.h   |  1 +
>>  drivers/pci/probe.c | 29 +++++++++++++++++++++++++++++
>>  drivers/pci/slot.c  | 35 +++--------------------------------
>>  3 files changed, 33 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index a88c316..5fb1d76 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -9,6 +9,7 @@
>>  #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
>>
>>  extern const unsigned char pcie_link_speed[];
>> +extern const char *pci_bus_speed_strings[];
>>  extern bool pci_early_dump;
>>
>>  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 512cb43..3c70b87 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -678,6 +678,35 @@ const unsigned char pcie_link_speed[] = {
>>  	PCI_SPEED_UNKNOWN		/* F */
>>  };
>>
>> +/* these strings match up with the values in pci_bus_speed */
>> +const char *pci_bus_speed_strings[] = {
>> +	"33 MHz PCI",		/* 0x00 */
>> +	"66 MHz PCI",		/* 0x01 */
>> +	"66 MHz PCI-X",		/* 0x02 */
>> +	"100 MHz PCI-X",	/* 0x03 */
>> +	"133 MHz PCI-X",	/* 0x04 */
>> +	NULL,			/* 0x05 */
>> +	NULL,			/* 0x06 */
>> +	NULL,			/* 0x07 */
>> +	NULL,			/* 0x08 */
>> +	"66 MHz PCI-X 266",	/* 0x09 */
>> +	"100 MHz PCI-X 266",	/* 0x0a */
>> +	"133 MHz PCI-X 266",	/* 0x0b */
>> +	"Unknown AGP",		/* 0x0c */
>> +	"1x AGP",		/* 0x0d */
>> +	"2x AGP",		/* 0x0e */
>> +	"4x AGP",		/* 0x0f */
>> +	"8x AGP",		/* 0x10 */
>> +	"66 MHz PCI-X 533",	/* 0x11 */
>> +	"100 MHz PCI-X 533",	/* 0x12 */
>> +	"133 MHz PCI-X 533",	/* 0x13 */
>> +	"2.5 GT/s",	/* 0x14 */
>> +	"5.0 GT/s",	/* 0x15 */
>> +	"8.0 GT/s",	/* 0x16 */
>> +	"16.0 GT/s",	/* 0x17 */
>> +	"32.0 GT/s",	/* 0x18 */
>> +};
>> +
>>  void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
>>  {
>>  	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index ae4aa0e..140dafb 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -49,43 +49,14 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>>  				slot->number);
>>  }
>>
>> -/* these strings match up with the values in pci_bus_speed */
>> -static const char *pci_bus_speed_strings[] = {
>> -	"33 MHz PCI",		/* 0x00 */
>> -	"66 MHz PCI",		/* 0x01 */
>> -	"66 MHz PCI-X",		/* 0x02 */
>> -	"100 MHz PCI-X",	/* 0x03 */
>> -	"133 MHz PCI-X",	/* 0x04 */
>> -	NULL,			/* 0x05 */
>> -	NULL,			/* 0x06 */
>> -	NULL,			/* 0x07 */
>> -	NULL,			/* 0x08 */
>> -	"66 MHz PCI-X 266",	/* 0x09 */
>> -	"100 MHz PCI-X 266",	/* 0x0a */
>> -	"133 MHz PCI-X 266",	/* 0x0b */
>> -	"Unknown AGP",		/* 0x0c */
>> -	"1x AGP",		/* 0x0d */
>> -	"2x AGP",		/* 0x0e */
>> -	"4x AGP",		/* 0x0f */
>> -	"8x AGP",		/* 0x10 */
>> -	"66 MHz PCI-X 533",	/* 0x11 */
>> -	"100 MHz PCI-X 533",	/* 0x12 */
>> -	"133 MHz PCI-X 533",	/* 0x13 */
>> -	"2.5 GT/s PCIe",	/* 0x14 */
>> -	"5.0 GT/s PCIe",	/* 0x15 */
>> -	"8.0 GT/s PCIe",	/* 0x16 */
>> -	"16.0 GT/s PCIe",	/* 0x17 */
>> -	"32.0 GT/s PCIe",	/* 0x18 */
>> -};
>> -
>>  static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>>  {
>>  	const char *speed_string;
>>
>> -	if (speed < ARRAY_SIZE(pci_bus_speed_strings))
>> -		speed_string = pci_bus_speed_strings[speed];
>> -	else
>> +	if (speed == PCI_SPEED_UNKNOWN)
>>  		speed_string = "Unknown";
>> +	else
>> +		speed_string = pci_bus_speed_strings[speed];
> This is a little bit problematic because previously we checked the
> actual array index but here we don't, so we may not index past the end
> of the array.
>
> It's possible to specify the array size explicitly in the extern
> declaration, and I *think* that might mean ARRAY_SIZE() would still
> work.  It's not ideal to have to update the extern declaration when
> adding speeds, but at least it's something that would be noticed by
> even the most trivial testing.

Maybe it's possible to add an array size valuable near the bus speed
string array and make it external. Then we can reference it here.

Thanks,
Yang

>>  	return sprintf(buf, "%s\n", speed_string);
>>  }
>> --
>> 2.8.1
>>
> .
>

Re: [PATCH 4/6] PCI: Improve and rename PCIE_SPEED2STR macro

[Yicong Yang]

On 2020/2/6 2:50, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2020 at 05:04:21PM +0800, Yicong Yang wrote:
>> Use pci_bus_speed_strings[] array to refactor PCIE_SPEED2STR macro.
>> Rename PCIE_SPEED2STR with PCI_SPEED2STR as it's also used to
>> decode non-PCIe speeds. Modify bus_speed_read() and
>> __pcie_print_link_status() with PCI_SPEED2STR macro.
> This looks like it should be split into two or three patches.
>
>   - Rename
>   - Fiddle with the "PCIe" suffix
>   - Refactor with pci_bus_speed_strings[]
>
> I don't know the right order for these.

I'll split this patch in next version.


>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>
>> I don't add a boundary check in PCI_SPEED2STR macro because:
>> 1. we cannot get the array size of an extern one using ARRAY_SIZE
>> 2. It is the *speed* should be check valid or not when assigned,
>> rather than checking it here. Actually we do make it valid when
>> assigned, the speed is either a valid value or PCI_SPEED_UNKNOWN.
>> And it's ensured in pcie_get_speed_cap(), pci_set_bus_speed()
>> when probe, and pcie_link_speed[] array. Please check again for
>> sure.
> I'm a little nervous about this because bus->cur_bus_speed and
> bus->max_bus_speed are set several places where we can't validate them
> (powerpc, s390, faraday_pci_probe(), cpqhpc_probe(), get_max_bus_speed
> (ibmphp), shpc_get_max_bus_speed(), etc).

Seems I didn't consider such situation. I'll add the boundary check in
next version.

Thanks,
Yang


>
> I don't think we can avoid out-of-bounds references by relying on all
> those places to get it right, especially since some of these can read
> speeds from hardware, so new hardware can give us a bus speed that old
> software won't know about.
>
>>  drivers/pci/pci-sysfs.c |  2 +-
>>  drivers/pci/pci.c       |  6 +++---
>>  drivers/pci/pci.h       | 10 +++-------
>>  drivers/pci/slot.c      | 10 +++-------
>>  4 files changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 13f766d..f4eafbc 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -156,7 +156,7 @@ static ssize_t max_link_speed_show(struct device *dev,
>>  {
>>  	struct pci_dev *pdev = to_pci_dev(dev);
>>
>> -	return sprintf(buf, "%s\n", PCIE_SPEED2STR(pcie_get_speed_cap(pdev)));
>> +	return sprintf(buf, "%s\n", PCI_SPEED2STR(pcie_get_speed_cap(pdev)));
>>  }
>>  static DEVICE_ATTR_RO(max_link_speed);
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e87196c..dce32ce 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5868,14 +5868,14 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
>>  	if (bw_avail >= bw_cap && verbose)
>>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
>>  			 bw_cap / 1000, bw_cap % 1000,
>> -			 PCIE_SPEED2STR(speed_cap), width_cap);
>> +			 PCI_SPEED2STR(speed_cap), width_cap);
>>  	else if (bw_avail < bw_cap)
>>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
>>  			 bw_avail / 1000, bw_avail % 1000,
>> -			 PCIE_SPEED2STR(speed), width,
>> +			 PCI_SPEED2STR(speed), width,
>>  			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
>>  			 bw_cap / 1000, bw_cap % 1000,
>> -			 PCIE_SPEED2STR(speed_cap), width_cap);
>> +			 PCI_SPEED2STR(speed_cap), width_cap);
>>  }
>>
>>  /**
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 5fb1d76..5e1f810 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -291,13 +291,9 @@ struct pci_bus *pci_bus_get(struct pci_bus *bus);
>>  void pci_bus_put(struct pci_bus *bus);
>>
>>  /* PCIe link information */
>> -#define PCIE_SPEED2STR(speed) \
>> -	((speed) == PCIE_SPEED_32_0GT ? "32 GT/s" : \
>> -	 (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")
>> +#define PCI_SPEED2STR(speed) \
>> +	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
>> +	 pci_bus_speed_strings[speed])
>>
>>  /* PCIe speed to Mb/s reduced by encoding overhead */
>>  #define PCIE_SPEED2MBS_ENC(speed) \
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index 140dafb..871d598 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -51,14 +51,10 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>>
>>  static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>>  {
>> -	const char *speed_string;
>> +	if (speed <= PCI_SPEED_133MHz_PCIX_533)
>> +		return sprintf(buf, "%s\n", PCI_SPEED2STR(speed));
>>
>> -	if (speed == PCI_SPEED_UNKNOWN)
>> -		speed_string = "Unknown";
>> -	else
>> -		speed_string = pci_bus_speed_strings[speed];
>> -
>> -	return sprintf(buf, "%s\n", speed_string);
>> +	return sprintf(buf, "%s PCIe\n", PCI_SPEED2STR(speed));
>>  }
>>
>>  static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
>> --
>> 2.8.1
>>
> .
>

Re: [PATCH 5/6] PCI: Add PCIE_LNKCAP2_SLS2SPEED macro

[Yicong Yang]

On 2020/2/6 2:54, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2020 at 05:04:22PM +0800, Yicong Yang wrote:
>> Add PCIE_LNKCAP2_SLS2SPEED macro for transforming raw link cap 2
>> value to link speed. Use it in pcie_get_speed_cap() to reduce
>> redundancy. We'll not touch the functions when new link
>> speed comes.
> The patch seems OK to me, but I don't see where it reduces redundancy.
> There was one copy of "lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB" before,
> and there's one copy after.  It's just moved from pci.c to pci.h.
> Or am I missing something?

Seems I used improper description here. I'll correct it in next version.

Thanks,
Yang


>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/pci/pci.c | 17 ++++-------------
>>  drivers/pci/pci.h |  9 +++++++++
>>  2 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index dce32ce..2ef4030 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5780,19 +5780,10 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
>>  	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
>>  	 */
>>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
>> -	if (lnkcap2) { /* PCIe r3.0-compliant */
>> -		if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB)
>> -			return PCIE_SPEED_32_0GT;
>> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB)
>> -			return PCIE_SPEED_16_0GT;
>> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
>> -			return PCIE_SPEED_8_0GT;
>> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
>> -			return PCIE_SPEED_5_0GT;
>> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
>> -			return PCIE_SPEED_2_5GT;
>> -		return PCI_SPEED_UNKNOWN;
>> -	}
>> +
>> +	/* PCIe r3.0-compliant */
>> +	if (lnkcap2)
>> +		return PCIE_LNKCAP2_SLS2SPEED(lnkcap2);
>>  
>>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>>  	if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 5e1f810..3d988e9 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -290,6 +290,15 @@ void pci_disable_bridge_window(struct pci_dev *dev);
>>  struct pci_bus *pci_bus_get(struct pci_bus *bus);
>>  void pci_bus_put(struct pci_bus *bus);
>>  
>> +/* PCIe link information from Link Capabilities 2 */
>> +#define PCIE_LNKCAP2_SLS2SPEED(lnkcap2) \
>> +	((lnkcap2) & PCI_EXP_LNKCAP2_SLS_32_0GB ? PCIE_SPEED_32_0GT : \
>> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_16_0GB ? PCIE_SPEED_16_0GT : \
>> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_8_0GB ? PCIE_SPEED_8_0GT : \
>> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_5_0GB ? PCIE_SPEED_5_0GT : \
>> +	 (lnkcap2) & PCI_EXP_LNKCAP2_SLS_2_5GB ? PCIE_SPEED_2_5GT : \
>> +	 PCI_SPEED_UNKNOWN)
>> +
>>  /* PCIe link information */
>>  #define PCI_SPEED2STR(speed) \
>>  	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
>> -- 
>> 2.8.1
>>
> .
>