PCI: Improve link speed presentation process

Currently We use switch-case statements to acquire the speed
string according to the pci bus speed in current_link_speed_show()
and pcie_get_speed_cap(). It leads to redundant and when new
standard comes, we have to add cases in the related functions,
which is easy to omit at somewhere.

Abstract the judge statements out. Use macros and pci speed
arrays instead. Then only the macros and arrays need to be
extended when next generation comes.

Link: https://lore.kernel.org/linux-pci/20200113211728.GA113776@google.com/
Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
Previously we get speed from sysfs likes "16.0 GT/s", etc. In this PATCH,
we get the speed string from pci_bus_speed_strings[], and it'll look like
"16.0 GT/s PCIe", etc. It makes no more affects and maybe make the information
more detailed.

 drivers/pci/pci-sysfs.c | 24 +++---------------------
 drivers/pci/pci.c       | 12 +-----------
 drivers/pci/pci.h       | 20 ++++++++++++++------
 drivers/pci/slot.c      |  2 +-
 4 files changed, 19 insertions(+), 39 deletions(-)

On Tue, Jan 14, 2020 at 04:11:34PM +0800, Yicong Yang wrote:
> Currently We use switch-case statements to acquire the speed
> string according to the pci bus speed in current_link_speed_show()
> and pcie_get_speed_cap(). It leads to redundant and when new
> standard comes, we have to add cases in the related functions,
> which is easy to omit at somewhere.
> 
> Abstract the judge statements out. Use macros and pci speed
> arrays instead. Then only the macros and arrays need to be
> extended when next generation comes.
> 
> Link: https://lore.kernel.org/linux-pci/20200113211728.GA113776@google.com/
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> Previously we get speed from sysfs likes "16.0 GT/s", etc. In this PATCH,
> we get the speed string from pci_bus_speed_strings[], and it'll look like
> "16.0 GT/s PCIe", etc. It makes no more affects and maybe make the information
> more detailed.

I like the direction this is heading a lot.

It'll be a little redundant in __pcie_print_link_status(), e.g.,

  - nvme 0000:01:00.0: 16.000 Gb/s available PCIe bandwidth, limited by 5 GT/s x4 link at 0000:00:01.1 (capable of 31.504 Gb/s with 8 GT/s x4 link)
  + nvme 0000:01:00.0: 16.000 Gb/s available PCIe bandwidth, limited by 5 GT/s PCIe x4 link at 0000:00:01.1 (capable of 31.504 Gb/s with 8 GT/s PCIe x4 link)

Prior to this patch, the strings containing "PCIe" are only in
bus_speed_read(), i.e., only for PCI slot "cur_bus_speed" and
"max_bus_speed" sysfs files for slots.  I'm not sure "PCIe" really
adds anything there -- I would think the question for a PCIe slot is
"how fast is it *and* how wide is it?"  That's what people need to
know when selecting a slot to plug a card into.

But I think we can defer the sysfs file question and keep all the
user-visible strings the same by removing "PCIe" from the
pci_bus_speed_strings[] and sprintf-ing it back in (when appropriate)
in bus_speed_read().

This patch both (a) removes a lot of redundancy and (b) adds 32 GT/s
in a couple places.  Those should be split: one patch should add 32
GT/s (this is probably just the first version you posted), and a
second patch should combine the strings.

That way the 32 GT/s change isn't buried in a big patch, and if the
string change ("16.0 GT/s" -> "16.0 GT/s PCIe") is in a patch by
itself that we can easily revert if it turns out to be a problem.

Side note: can you add a comment at the pcie_link_speed[] definition
about the index being the Current Link Speed field?  Maybe also a note
at the pcix_bus_speed[] definition (the index is the "Secondary Bus
Mode and Frequency" from the PCI-X Secondary Status Register in the
PCI-X Bridge Capability.

>  drivers/pci/pci-sysfs.c | 24 +++---------------------
>  drivers/pci/pci.c       | 12 +-----------
>  drivers/pci/pci.h       | 20 ++++++++++++++------
>  drivers/pci/slot.c      |  2 +-
>  4 files changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 7934129..8bcb136 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -175,33 +175,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 link_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";
> -	}
> +	link_speed = pcie_link_speed[linkstat & PCI_EXP_LNKSTA_CLS];
>  
> -	return sprintf(buf, "%s\n", speed);
> +	return sprintf(buf, "%s\n", PCIE_SPEED2STR(link_speed));
>  }
>  static DEVICE_ATTR_RO(current_link_speed);
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a97e257..ea72e6d8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5658,17 +5658,7 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
>  	 */
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
>  	if (lnkcap2) { /* PCIe r3.0-compliant */
> -		if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_32_0GB)
> -			return PCIE_SPEED_32_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB)
> -			return PCIE_SPEED_16_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
> -			return PCIE_SPEED_8_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
> -			return PCIE_SPEED_5_0GT;
> -		else if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
> -			return PCIE_SPEED_2_5GT;
> -		return PCI_SPEED_UNKNOWN;
> +		return PCIE_LNKCAP2_SLS2SPEED(lnkcap2);
>  	}

Remove the braces since there's only one line left.

>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3f6947e..90cacf6 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -9,6 +9,7 @@
>  #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
>  
>  extern const unsigned char pcie_link_speed[];
> +extern const char *pci_bus_speed_strings[];
>  extern bool pci_early_dump;
>  
>  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> @@ -286,17 +287,24 @@ 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 from Link Capabilities 2 */
> +#define PCIE_LNKCAP2_SLS2SPEED(mask) \
> +	((mask) & PCI_EXP_LNKCAP2_SLS_32_0GB ? PCIE_SPEED_32_0GT : \
> +	 (mask) & PCI_EXP_LNKCAP2_SLS_16_0GB ? PCIE_SPEED_16_0GT : \
> +	 (mask) & PCI_EXP_LNKCAP2_SLS_8_0GB ? PCIE_SPEED_8_0GT : \
> +	 (mask) & PCI_EXP_LNKCAP2_SLS_5_0GB ? PCIE_SPEED_5_0GT : \
> +	 (mask) & PCI_EXP_LNKCAP2_SLS_2_5GB ? PCIE_SPEED_2_5GT : \
> +	 PCI_SPEED_UNKNOWN)

The argument here is not really a "mask"; it's the entire LNKCAP2
value.  I'd call it "lnkcap2" so it's a hint about what callers should
pass.

>  /* PCIe link information */
>  #define PCIE_SPEED2STR(speed) \
> -	((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")
> +	((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
> +	 pci_bus_speed_strings[speed])

I think this will be a problem because pci_bus_speed_strings[] is
defined in slot.c, which is only built when CONFIG_SYSFS=y.  But
PCIE_SPEED2STR() is used by __pcie_print_link_status(), which can be
built without CONFIG_SYSFS=y.  Maybe a preliminary patch could move
pci_bus_speed_strings[] to probe.c, near the related pcix_bus_speed[]
and pcie_link_speed[]?

Also, bus_speed_read() contains what is basically an open-coded
PCIE_SPEED2STR(), except that bus_speed_read() does a bounds check.
I think we should maybe add that bounds check to PCIE_SPEED2STR() and
use it in bus_speed_read().

So I'm envisioning several patches here:

  - Add 32 GT/s to PCIE_SPEED2STR() and PCIE_SPEED2MBS_ENC() (your
    original patch).

  - Move pci_bus_speed_strings[] to probe.c.  No change at all except
    to become non-static.

  - Change PCIE_SPEED2STR() to use pci_bus_speed_strings[] and add
    bounds checking, remove "PCIe" from pci_bus_speed_strings[], use
    PCI_SPEED2STR() and add "PCIe" back in bus_speed_read().

  - Rename PCIE_SPEED2STR() to PCI_SPEED2STR().  Could be squashed
    with above, but keep it separate to start; I can trivially squash
    if it makes sense.

  - Add PCIE_LNKCAP2_SLS2SPEED() and use it in pcie_get_speed_cap().
    Separate patch because it's not related to the strings.

>  /* PCIe speed to Mb/s reduced by encoding overhead */
>  #define PCIE_SPEED2MBS_ENC(speed) \
> -	((speed) == PCIE_SPEED_16_0GT ? 16000*128/130 : \
> +	((speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
> +	 (speed) == PCIE_SPEED_16_0GT ? 16000*128/130 : \
>  	 (speed) == PCIE_SPEED_8_0GT  ?  8000*128/130 : \
>  	 (speed) == PCIE_SPEED_5_0GT  ?  5000*8/10 : \
>  	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index ae4aa0e..08a59ed 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -50,7 +50,7 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>  }
>  
>  /* these strings match up with the values in pci_bus_speed */
> -static const char *pci_bus_speed_strings[] = {
> +const char *pci_bus_speed_strings[] = {
>  	"33 MHz PCI",		/* 0x00 */
>  	"66 MHz PCI",		/* 0x01 */
>  	"66 MHz PCI-X",		/* 0x02 */
> -- 
> 2.8.1
>

Hi Yicong,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v5.5-rc6 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Yicong-Yang/PCI-Improve-link-speed-presentation-process/20200114-163536
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pci/pci-sysfs.o: In function `max_link_speed_show':
>> pci-sysfs.c:(.text+0x1e01): undefined reference to `pci_bus_speed_strings'
   pci-sysfs.c:(.text+0x1e10): undefined reference to `pci_bus_speed_strings'
   drivers/pci/pci-sysfs.o: In function `current_link_speed_show':
   pci-sysfs.c:(.text+0x2070): undefined reference to `pci_bus_speed_strings'
   pci-sysfs.c:(.text+0x2080): undefined reference to `pci_bus_speed_strings'

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Hi Bjorn, 
It seems like we have met the problem you indicated, that using pci_bus_speed_strings[] in
PCIE_BUS_SPEED2STR macro is only available when CONFIG_SYSFS=y.

I have moved the string array to probe.c in the new patch set according to the suggestions.
I hope it has not been buried in the mail list.
https://lore.kernel.org/linux-pci/1579079063-5668-1-git-send-email-yangyicong@hisilicon.com/

On 2020/1/16 15:45, kbuild test robot wrote:
> Hi Yicong,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on pci/next]
> [also build test ERROR on v5.5-rc6 next-20200110]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Yicong-Yang/PCI-Improve-link-speed-presentation-process/20200114-163536
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> config: ia64-allnoconfig (attached as .config)
> compiler: ia64-linux-gcc (GCC) 7.5.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.5.0 make.cross ARCH=ia64 
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/pci/pci-sysfs.o: In function `max_link_speed_show':
>>> pci-sysfs.c:(.text+0x1e01): undefined reference to `pci_bus_speed_strings'
>    pci-sysfs.c:(.text+0x1e10): undefined reference to `pci_bus_speed_strings'
>    drivers/pci/pci-sysfs.o: In function `current_link_speed_show':
>    pci-sysfs.c:(.text+0x2070): undefined reference to `pci_bus_speed_strings'
>    pci-sysfs.c:(.text+0x2080): undefined reference to `pci_bus_speed_strings'
>
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation