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@xxxxxxxxxxxxx>
> ---
>
> 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
>
