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.jarvi...@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
