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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>