Re: [PATCH v2 1/2] PCI: Track Flit Mode Status & print it with link status

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Fri, Feb 07, 2025 at 06:18:35PM +0200, Ilpo Järvinen wrote:
> PCIe r6.0 added Flit mode that mainly alters HW behavior but some OS
> visible changes are also because of it. The OS visible changes include

The first sentence reads oddly. Maybe a slight change?

"...but there are some OS visible changes because of it."

> differences in the layout of some capabilities and interpretation of
> the TLP headers (in diagnostics situations).
> 
> To be able to determine which mode the PCIe link is using, store the
> Flit Mode Status (PCIe r6.1 sec 7.5.3.20) information in addition to
> the link speed into struct pci_bus in pcie_update_link_speed().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c |  5 +++--
>  drivers/pci/pci.c                | 12 ++++++++----
>  drivers/pci/pci.h                |  3 ++-
>  drivers/pci/probe.c              |  5 +++--
>  include/linux/pci.h              |  1 +
>  5 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bb5a8d9f03ad..10130ac9f979 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -292,7 +292,7 @@ int pciehp_check_link_status(struct controller *ctrl)
>  {
>  	struct pci_dev *pdev = ctrl_dev(ctrl);
>  	bool found;
> -	u16 lnk_status;
> +	u16 lnk_status, linksta2;
>  
>  	if (!pcie_wait_for_link(pdev, true)) {
>  		ctrl_info(ctrl, "Slot(%s): No link\n", slot_name(ctrl));
> @@ -319,7 +319,8 @@ int pciehp_check_link_status(struct controller *ctrl)
>  		return -1;
>  	}
>  
> -	__pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
> +	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA2, &linksta2);
> +	__pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status, linksta2);
>  
>  	if (!found) {
>  		ctrl_info(ctrl, "Slot(%s): No device found\n",
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..313c66863752 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6190,21 +6190,25 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
>  	enum pci_bus_speed speed, speed_cap;
>  	struct pci_dev *limiting_dev = NULL;
>  	u32 bw_avail, bw_cap;
> +	char *flit_mode = "";
>  
>  	bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
>  	bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width);
>  
> +	if (dev->bus && dev->bus->flit_mode)
> +		flit_mode = ", in Flit mode";
> +
>  	if (bw_avail >= bw_cap && verbose)
> -		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
> +		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)%s\n",
>  			 bw_cap / 1000, bw_cap % 1000,
> -			 pci_speed_string(speed_cap), width_cap);
> +			 pci_speed_string(speed_cap), width_cap, flit_mode);
>  	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",
> +		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)%s\n",
>  			 bw_avail / 1000, bw_avail % 1000,
>  			 pci_speed_string(speed), width,
>  			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
>  			 bw_cap / 1000, bw_cap % 1000,
> -			 pci_speed_string(speed_cap), width_cap);
> +			 pci_speed_string(speed_cap), width_cap, flit_mode);

Does the "Flit mode" message *need* to go into these lines? Could it be
its own message?

 +#include <linux/string_choices.h>

 @@ -6190,21 +6190,25 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
  	enum pci_bus_speed speed, speed_cap;
  	struct pci_dev *limiting_dev = NULL;
  	u32 bw_avail, bw_cap;
  
  	bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
  	bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width);
  
 +	if (dev->bus)
 +		pci_info(dev, "Flit mode: %s\n", str_enabled_disabled(dev->bus->flit_mode);
 +
  	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,
 			 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,
  			 pci_speed_string(speed), width,
  			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
  			 bw_cap / 1000, bw_cap % 1000,
 			 pci_speed_string(speed_cap), width_cap);

>  }
>  
>  /**
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..9c6a4a980678 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -406,9 +406,10 @@ const char *pci_speed_string(enum pci_bus_speed speed);
>  void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
>  void pcie_report_downtraining(struct pci_dev *dev);
>  
> -static inline void __pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
> +static inline void __pcie_update_link_speed(struct pci_bus *bus, u16 linksta, u16 linksta2)
>  {
>  	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
> +	bus->flit_mode = linksta2 & PCI_EXP_LNKSTA2_FLIT;

Can we align on the '='?

Thanks,
Yazen