| Message ID | 20250207161836.2755-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add support for logging Flit Mode TLPs (PCIe6) | expand |
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
On Fri, Feb 21, 2025 at 10:29:48AM -0500, Yazen Ghannam wrote: > 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." Updated locally. > > + 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? I suppose it doesn't need to be there, and these bandwidth lines are already pretty long (my fault, open to suggestions to shorten them), but I do think it's useful to have related info all on the same line. > +#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/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); } /** 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; } void pcie_update_link_speed(struct pci_bus *bus); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index b6536ed599c3..e6f11498a345 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -788,10 +788,11 @@ EXPORT_SYMBOL_GPL(pci_speed_string); void pcie_update_link_speed(struct pci_bus *bus) { struct pci_dev *bridge = bus->self; - u16 linksta; + u16 linksta, linksta2; pcie_capability_read_word(bridge, PCI_EXP_LNKSTA, &linksta); - __pcie_update_link_speed(bus, linksta); + pcie_capability_read_word(bridge, PCI_EXP_LNKSTA2, &linksta2); + __pcie_update_link_speed(bus, linksta, linksta2); } EXPORT_SYMBOL_GPL(pcie_update_link_speed); diff --git a/include/linux/pci.h b/include/linux/pci.h index 47b31ad724fa..9862f65d899d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -681,6 +681,7 @@ struct pci_bus { struct bin_attribute *legacy_mem; /* Legacy mem */ unsigned int is_added:1; unsigned int unsafe_warn:1; /* warned about RW1C config write */ + bool flit_mode; /* Link in Flit mode */ }; #define to_pci_bus(n) container_of(n, struct pci_bus, dev)
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 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(-)