diff mbox series

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

Message ID 1579079063-5668-5-git-send-email-yangyicong@hisilicon.com
State New
Headers show
Series Improve link speed presentation process | expand

Commit Message

Yicong Yang Jan. 15, 2020, 9:04 a.m. UTC 
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(-)

--
2.8.1

Comments

Bjorn Helgaas Feb. 5, 2020, 6:50 p.m. UTC | #1 
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
>
Yicong Yang Feb. 6, 2020, 1:50 a.m. UTC | #2 
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
>>
> .
>
diff mbox series

Patch

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)