Re: [PATCH v5 3/4] PCI: Use pci_speed_string() for all PCI/PCI-X/PCIe strings

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Feb 29, 2020 at 05:10:38PM +0800, Yicong Yang wrote:
> On 2020/2/29 11:07, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > 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@hisilicon.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  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)";

Oh, right, thanks!