| Message ID | 0044d6cd05ad20ec3a6ec5a8a22b6ab652e251fe.1734257330.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix bwctrl boot hang | expand |
On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner wrote: > The Supported Link Speeds Vector in the Link Capabilities 2 Register > indicates the *supported* link speeds. The Max Link Speed field in > the Link Capabilities Register indicates the *maximum* of those speeds. > > pcie_get_supported_speeds() neglects to honor the Max Link Speed field > and will thus incorrectly deem higher speeds as supported. Fix it. > > One user-visible issue addressed here is an incorrect value in the sysfs > attribute "max_link_speed". Can confirm that I saw this effect on my affected Intel JHL7540 "Titan Ridge 2018" Thunderbolt controller. Feel free to add: Tested-by: Niklas Schnelle <niks@kernel.org>
On Sun, 15 Dec 2024 11:20:52 +0100 Lukas Wunner <lukas@wunner.de> wrote: > The Supported Link Speeds Vector in the Link Capabilities 2 Register > indicates the *supported* link speeds. The Max Link Speed field in > the Link Capabilities Register indicates the *maximum* of those speeds. > > pcie_get_supported_speeds() neglects to honor the Max Link Speed field > and will thus incorrectly deem higher speeds as supported. Fix it. > > One user-visible issue addressed here is an incorrect value in the sysfs > attribute "max_link_speed". > > But the main motivation is a boot hang reported by Niklas: Intel JHL7540 > "Titan Ridge 2018" Thunderbolt controllers supports 2.5-8 GT/s speeds, > but indicate 2.5 GT/s as maximum. Ilpo recalls seeing this on more > devices. It can be explained by the controller's Downstream Ports > supporting 8 GT/s if an Endpoint is attached, but limiting to 2.5 GT/s > if the port interfaces to a PCIe Adapter, in accordance with USB4 v2 > sec 11.2.1: > > "This section defines the functionality of an Internal PCIe Port that > interfaces to a PCIe Adapter. [...] > The Logical sub-block shall update the PCIe configuration registers > with the following characteristics: [...] > Max Link Speed field in the Link Capabilities Register set to 0001b > (data rate of 2.5 GT/s only). > Note: These settings do not represent actual throughput. Throughput > is implementation specific and based on the USB4 Fabric performance." > > The present commit is not sufficient on its own to fix Niklas' boot hang, > but it is a prerequisite. > > Fixes: d2bd39c0456b ("PCI: Store all PCIe Supported Link Speeds") > Reported-by: Niklas Schnelle <niks@kernel.org> > Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/ > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Makes sense as a hardening step, even without the oddity of thunderbolt. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Sun, 15 Dec 2024, Lukas Wunner wrote: > The Supported Link Speeds Vector in the Link Capabilities 2 Register > indicates the *supported* link speeds. The Max Link Speed field in > the Link Capabilities Register indicates the *maximum* of those speeds. > > pcie_get_supported_speeds() neglects to honor the Max Link Speed field > and will thus incorrectly deem higher speeds as supported. Fix it. > > One user-visible issue addressed here is an incorrect value in the sysfs > attribute "max_link_speed". > > But the main motivation is a boot hang reported by Niklas: Intel JHL7540 > "Titan Ridge 2018" Thunderbolt controllers supports 2.5-8 GT/s speeds, > but indicate 2.5 GT/s as maximum. Ilpo recalls seeing this on more > devices. It can be explained by the controller's Downstream Ports > supporting 8 GT/s if an Endpoint is attached, but limiting to 2.5 GT/s > if the port interfaces to a PCIe Adapter, in accordance with USB4 v2 > sec 11.2.1: > > "This section defines the functionality of an Internal PCIe Port that > interfaces to a PCIe Adapter. [...] > The Logical sub-block shall update the PCIe configuration registers > with the following characteristics: [...] > Max Link Speed field in the Link Capabilities Register set to 0001b > (data rate of 2.5 GT/s only). > Note: These settings do not represent actual throughput. Throughput > is implementation specific and based on the USB4 Fabric performance." > > The present commit is not sufficient on its own to fix Niklas' boot hang, > but it is a prerequisite. > > Fixes: d2bd39c0456b ("PCI: Store all PCIe Supported Link Speeds") > Reported-by: Niklas Schnelle <niks@kernel.org> > Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/ > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index ab0ef7b6c798..9f672399e688 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6247,6 +6247,10 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) > pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2); > speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS; > > + /* Ignore speeds higher than Max Link Speed */ > + speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, > + PCI_EXP_LNKCAP2_SLS_2_5GB); You pass a value instead of bit position to GENMASK() which is not correct way to use GENMASK(). You need to do either: ilog2(PCI_EXP_LNKCAP2_SLS_2_5GB) or __ffs(PCI_EXP_LNKCAP2_SLS) (Technically, also __ffs(PCI_EXP_LNKCAP2_SLS_2_5GB) would work). + Please check the correct header is included depending on which you pick. > + > /* PCIe r3.0-compliant */ > if (speeds) > return speeds; >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ab0ef7b6c798..9f672399e688 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6247,6 +6247,10 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev) pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2); speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS; + /* Ignore speeds higher than Max Link Speed */ + speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, + PCI_EXP_LNKCAP2_SLS_2_5GB); + /* PCIe r3.0-compliant */ if (speeds) return speeds;
The Supported Link Speeds Vector in the Link Capabilities 2 Register indicates the *supported* link speeds. The Max Link Speed field in the Link Capabilities Register indicates the *maximum* of those speeds. pcie_get_supported_speeds() neglects to honor the Max Link Speed field and will thus incorrectly deem higher speeds as supported. Fix it. One user-visible issue addressed here is an incorrect value in the sysfs attribute "max_link_speed". But the main motivation is a boot hang reported by Niklas: Intel JHL7540 "Titan Ridge 2018" Thunderbolt controllers supports 2.5-8 GT/s speeds, but indicate 2.5 GT/s as maximum. Ilpo recalls seeing this on more devices. It can be explained by the controller's Downstream Ports supporting 8 GT/s if an Endpoint is attached, but limiting to 2.5 GT/s if the port interfaces to a PCIe Adapter, in accordance with USB4 v2 sec 11.2.1: "This section defines the functionality of an Internal PCIe Port that interfaces to a PCIe Adapter. [...] The Logical sub-block shall update the PCIe configuration registers with the following characteristics: [...] Max Link Speed field in the Link Capabilities Register set to 0001b (data rate of 2.5 GT/s only). Note: These settings do not represent actual throughput. Throughput is implementation specific and based on the USB4 Fabric performance." The present commit is not sufficient on its own to fix Niklas' boot hang, but it is a prerequisite. Fixes: d2bd39c0456b ("PCI: Store all PCIe Supported Link Speeds") Reported-by: Niklas Schnelle <niks@kernel.org> Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/ Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/pci.c | 4 ++++ 1 file changed, 4 insertions(+)