Hi Bjorn,
On 2020/2/29 11:07, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> Previously some PCI speed strings came from pci_speed_string(), some came
> from the PCIe-specific PCIE_SPEED2STR(), and some came from a PCIe-specific
> switch statement. These methods were inconsistent:
>
> pci_speed_string() PCIE_SPEED2STR() switch
> ------------------ ---------------- ------
> 33 MHz PCI
> ...
> 2.5 GT/s PCIe 2.5 GT/s 2.5 GT/s
> 5.0 GT/s PCIe 5 GT/s 5 GT/s
> 8.0 GT/s PCIe 8 GT/s 8 GT/s
> 16.0 GT/s PCIe 16 GT/s 16 GT/s
> 32.0 GT/s PCIe 32 GT/s 32 GT/s
>
> Standardize on pci_speed_string() as the single source of these strings.
>
> Note that this adds ".0" and "PCIe" to some messages, including sysfs
> "max_link_speed" files, a brcmstb "link up" message, and the link status
> dmesg logging, e.g.,
>
> nvme 0000:01:00.0: 16.000 Gb/s available PCIe bandwidth, limited by 5.0 GT/s PCIe x4 link at 0000:00:01.1 (capable of 31.504 Gb/s with 8.0 GT/s PCIe x4 link)
>
> I think it's better to standardize on a single version of the speed text.
> Previously we had strings like this:
>
> /sys/bus/pci/slots/0/cur_bus_speed: 8.0 GT/s PCIe
> /sys/bus/pci/slots/0/max_bus_speed: 8.0 GT/s PCIe
> /sys/devices/pci0000:00/0000:00:1c.0/current_link_speed: 8 GT/s
> /sys/devices/pci0000:00/0000:00:1c.0/max_link_speed: 8 GT/s
>
> This changes the latter two to match the slots files:
>
> /sys/devices/pci0000:00/0000:00:1c.0/current_link_speed: 8.0 GT/s PCIe
> /sys/devices/pci0000:00/0000:00:1c.0/max_link_speed: 8.0 GT/s PCIe
>
> Based-on-patch by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 3 +--
> drivers/pci/pci-sysfs.c | 27 +++++----------------------
> drivers/pci/pci.c | 6 +++---
> drivers/pci/pci.h | 9 ---------
> 4 files changed, 9 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d20aabc26273..41e88f1667bf 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -823,8 +823,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> lnksta = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKSTA);
> cls = FIELD_GET(PCI_EXP_LNKSTA_CLS, lnksta);
> nlw = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
> - dev_info(dev, "link up, %s x%u %s\n",
> - PCIE_SPEED2STR(cls + PCI_SPEED_133MHz_PCIX_533),
> + dev_info(dev, "link up, %s x%u %s\n", pci_speed_string(cls),
> nlw, ssc_good ? "(SSC)" : "(!SSC)");
Here comes the problem. cls is not a pci_bus_speed enumerate. The PCIe link speed decodes
from PCI_EXP_LNKSTA is from 0x000, we'll get the *wrong* string if passing cls directly to
pci_speed_string(). pcie_link_speed[](drivers/pci/probe.c, line 662) array should be used
here to do the conversion.
+ dev_info(dev, "link up, %s x%u %s\n", pci_speed_string(pcie_link_speed[cls]),
nlw, ssc_good ? "(SSC)" : "(!SSC)";
The other parts of the series are fine with me.
Regards,
Yicong Yang
>
> /* PCIe->SCB endian mode for BAR */
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766db0684..d123d1087061 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -156,7 +156,8 @@ 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_speed_string(pcie_get_speed_cap(pdev)));
> }
> static DEVICE_ATTR_RO(max_link_speed);
>
> @@ -175,33 +176,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_speed_string(speed));
> }
> static DEVICE_ATTR_RO(current_link_speed);
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d828ca835a98..421587badecf 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5872,14 +5872,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_speed_string(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_speed_string(speed), width,
> limiting_dev ? pci_name(limiting_dev) : "<unknown>",
> bw_cap / 1000, bw_cap % 1000,
> - PCIE_SPEED2STR(speed_cap), width_cap);
> + pci_speed_string(speed_cap), width_cap);
> }
>
> /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 809753b10fad..01f5d7f449a5 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -292,15 +292,6 @@ 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 */
> -#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")
> -
> /* PCIe speed to Mb/s reduced by encoding overhead */
> #define PCIE_SPEED2MBS_ENC(speed) \
> ((speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
